-
-
Notifications
You must be signed in to change notification settings - Fork 514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Port Sample/EventStoreDB/Simple/ECommerce to F#+Equinox #84
Port Sample/EventStoreDB/Simple/ECommerce to F#+Equinox #84
Conversation
21d40a2
to
772071e
Compare
Do I assume right, that instead, you'd like to show how to resolve conflicts using Equinox decider? I don't think that's a bad idea to pass versions, it's just different 😉 If my assumption is right, then I'm of course fine with that. There is no sample on conflict resolution yet in this repo, so it'd be valuable to have it.
Fine by me. It's a valid approach, of course needs a bit of explanation for newbies about the assumptions. (Anton Stöckl wrote a nice article on it recently: https://www.eventstore.com/blog/live-projections-for-read-models-with-event-sourcing-and-cqrs)
Best if it shows only active carts. But of course, the list endpoint can be delivered later on. I'm trying to show in my samples the typical flow of the application. It doesn't have to be production-grade, but it's worth if people have at least basic application scenarios solved. Thus, I'm also showing how to implement list endpoint, as for me, when I was learning Event Sourcing, it was mind-boggling.
Fine by me. Samples doesn't have to be 1:1, so it's fine to change the details to show some different aspects. |
member _.InitializeCart([<FromBody>] request : InitializeShoppingCartRequest) : Async<IActionResult> = async { | ||
if obj.ReferenceEquals(null, request) then nameof request |> nullArg | ||
|
||
// TODO this should be resolved via an idempotent lookup in the Client's list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QUESTION: Why do we need to do lookup here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mean literally here. I should have put it more completely in the comment, but IMO it's more realistic to model something closer to a real world app, i.e.
- client keeps (only) a client id as a cookie or similar
- initial shopping is anonymous, and that clientid can be used to look it up. When you want to add to cart, you can add with just the client id, or if you have the current valid cartid, fine, pass that.
- then, when you log in, the real identity is known, and normally uou merge the anon cart into a concrete cart -- which may or may not be initialized or closed, but you dont care
- when you log in, you the supply the clientid that belongs to the account back to the user
- you don't want to produce random orphan cartids as the very last thing you want soemone to do is take something out of a cart or forget they had it
- clients don't really care about the initializatiion (and forcing an extra roundtrip or flag when somone just wants to add to cart does not make sense either)
This implies two things:
- an indirection via clientid when accessing a cart
- the job of an add to cart function is to add it to the current cart, or start a new cart, but really the clientid is the main thing, and forcing initialization busywork roundtrips on clients does not help anyone
- the job of a confirm cart function is however relative to a particular cart - i.e. you do need some sort of protocol beyond the customer only ever passing a clientid
How flowery you want to make it is up to you of course, but the bottom line is that this call is not and can not be idempotent and is prone to creating lots of orphan carts that are zero use to anyone, and that's rarely a good design
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oskardudycz I've extended the TODO comment and added a reference to this in the overview. Can you resolve this comment if and when you feel that covers it?
By the same token, if you have time to to and from on PMing the features, we could extend the logic to have a client maintain a cart history and have the commands insert into the current cart. Bonus points if there's going to be >1 sample in the repo doing the same thing...
p.s. I really like that you included logging and metrics by default. That's the area that I should also improve in other samples. |
Regarding Metrics, there's very good metrics for both Equinox and Propulsion via a Grafana dashboard on Prometheus in jet/dotnet-templates#100 |
If someone hits add twice in quick succession from two tabs, I want something predictable to happen. Either:
This also overlaps with what you do when you merge carts - do you add the quantities, pick the max, or something else
That's not a bad article in terms of walking the tradeoffs - not sure I like the term "Live" as its pretty overloaded and/or meaningless. I have used the term
So you sort by IsActive DESC, DateCreated DESC and page it ? Do you make it searchable? based on what fields? do confirmed ones immediately fall out? I have trouble engaging with this requirement as it seems very hypothetical to be paging through them given how dynamic it gets at any scale. The design of the table depends on whether you want to be able to search by skuid, total price or other properties etc. If there was a clearer spec, I'd be looking to map it down to something document shaped and less than 1MB. Right, all that said, it does make sense to do some kind of indexing based on projecting from the events to illustrate the point. In the interests of providing something useful, perhaps I could implement two HTTP feeds which allows one to walk:
I could include other summary information in the feed, or just the ids. The rough pattern is the List* things in https://github.com/jet/dotnet-templates/tree/master/equinox-patterns/Domain However I'd be concerned that having this sample do something completely different to the other samples makes it all a bit meangless and/or confusing as you need to read lots of code in many languages to be able to begin to orient yourself
I worked on a system that priced the full cart dynamically each time it was rendered (it did save the prices so changes can be flagged and confirmed). On reflection doing this feels wrong; similar to the above thing, coming up with some random scenario and then having this sample be different to all the others without it being meaningful is probably just confusing. I think its valuable to illustrate the basic mechanisms for how to have the pricing be managed outside of the mainline logic of the cart itself though 🤔 |
cbbe4c2
to
70b393e
Compare
@@ -36,8 +36,8 @@ public PricedProductItem MergeWith(PricedProductItem productItem) | |||
throw new ArgumentException("Product unit prices do not match."); | |||
|
|||
return new PricedProductItem( | |||
new ProductItem(ProductId, productItem.Quantity + productItem.Quantity), | |||
new ProductItem(ProductId, Quantity + productItem.Quantity), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oskardudycz Feel free to cherrypick this bugfix I noticed in porting
@@ -26,11 +26,10 @@ public record PricedProductItemRequest( | |||
); | |||
|
|||
public record RemoveProductRequest( | |||
Guid? ShoppingCartId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oskardudycz Feel free to cherrypick - this is redundant
420e299
to
49e7df7
Compare
76ce151
to
1c22086
Compare
4c1cc9a
to
9e9d873
Compare
(WIP, not compiling as Propulsion bits not changed yet)
18f0eb2
to
9e13479
Compare
…g that during rebase
bfcbc06
to
ce74302
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ports
Sample/EventStoreDB/Simple/ECommerce/ShoppingCarts
to a) F# b) EquinoxThere are some variations introduced as part of the port:
PricedProductItem
type melts away (you can see it there in the initial impl in the earlier commits though), and we have atype Item
inmodule Fold
as well as a separate one one inmodule Details
cartId
in all events 'pattern' - for me the extraneous stuff like that should not be redundantly specified on every eventShoppingCartsController.Get
reads the write model. This is frequently done in Equinox asIPricingCalculator
has two differences from the C# versionProductId -> Async<decimal>
- the interface doesn't achieve anythingTransactAsync
instead ofTransact
in that instance)ConfirmedIngester
Maintains a paged, deduplicated list of Cart summaries for allConfirmed
cartsReactor
program that listens to the CosmosDB ChangeFeed / EventStoreDB $all subscription:ECommerce.Reactor.ConfirmedHandler
drives this in response toShoppingCart.Events.Confirmed
eventsECommerce.Reactor.ShoppingCartSummaryHandler
stashes summaries of Cart state into Cosmos asRollingState
documentsConfirmedFeed
endpoint surfaces theConfirmedEpoch
s as a feed that can be consumed over HTTP (e.g. viaPropulsion.Feed
)version
when rendering a cart, and only permitting updates if you started from the current version, the Commands vary slightly from the original in that they are defined in such a way that they can be applied idempotently.FeedConsumer
console app consumesConfirmedFeed
🤔 ... except what should it do with the info?Aside from completing the above, there are some more things to be resolved before this can be merged. We can decide what to do here on the basis of making a call whether to err on the side of:
i. aligning with, and remaining in alignment with the API spec of the source sample
ii. making the impl more 'real world' in nature by cleaning/extending/complicating the semantics compared with the C# base version
Debates to be resolved:
InitializeCart
API is not representative of a real world system. See my comments here and as such it'd be nice to replace it with a more realistic demonstration of how one would deal with logged in / not yet logged in mode, and how one would have the APIs handle an add regardless of whether the cartId has been rolled over to a new one since the page got loaded.