Skip to content
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

Open
wants to merge 48 commits into
base: main
Choose a base branch
from

Conversation

bartelink
Copy link
Contributor

@bartelink bartelink commented Nov 21, 2021

This ports Sample/EventStoreDB/Simple/ECommerce/ShoppingCarts to a) F# b) Equinox

There are some variations introduced as part of the port:

  • In general, idiomatic Equinox modules tries to encapsulate the contracts and the logic within a single scope - as a result the PricedProductItem type melts away (you can see it there in the initial impl in the earlier commits though), and we have a type Item in module Fold as well as a separate one one in module Details
  • I have not followed the cartId in all events 'pattern' - for me the extraneous stuff like that should not be redundantly specified on every event
  • ShoppingCartsController.Get reads the write model. This is frequently done in Equinox as
    • the caching will dampen the cost of reading from the write model on both EventStoreDB and Cosmos DB
    • on Cosmos DB, there's no perf advantage to having a separate read model store as long as you're not hitting the max RU costs so flattening the state into that would not buy much in terms of efficiently
  • IPricingCalculator has two differences from the C# version
    • Instead of using an interface, we just use a function signature of ProductId -> Async<decimal> - the interface doesn't achieve anything
    • As can be seen from the signature, the calculation is deemed to require an Async function (i.e. Task in C# - this is something I've seen in real life, and also serves as a good example of how one uses TransactAsync instead of Transact in that instance)
  • ConfirmedIngester Maintains a paged, deduplicated list of Cart summaries for all Confirmed carts
  • There's a Reactor program that listens to the CosmosDB ChangeFeed / EventStoreDB $all subscription:
    • ECommerce.Reactor.ConfirmedHandler drives this in response to ShoppingCart.Events.Confirmed events
    • ECommerce.Reactor.ShoppingCartSummaryHandler stashes summaries of Cart state into Cosmos as RollingState documents
  • The ConfirmedFeed endpoint surfaces the ConfirmedEpochs as a feed that can be consumed over HTTP (e.g. via Propulsion.Feed)
  • Rather than passing out the 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.
  • The FeedConsumer console app consumes ConfirmedFeed 🤔 ... 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:

  • I feel that the current 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.
  • A pageable carts list API seems reasonable up to a point, but begs lots of questions as to how you manage that over time - it would just be full of dead carts and you'd thus have to come up with a reasonable ordering that would align with something someone managing a store might really do

@oskardudycz
Copy link
Owner

oskardudycz commented Nov 25, 2021

@bartelink

In general I would not lean on passing versions out and then back into APIs; while this is implementable, I don't think its worth demonstrating on the basis that it's a bad idea

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.

In general with Equinox at small scale, a GET endpoint can be serviced directly from the aggregate - i.e. I would not have a denormalized summary view table - however that may warrant some discussion (code implements it by reading the write store through the cache atm)

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)

A pageable carts list API seems reasonable up to a point, but begs lots of questions as to how you manage that over time - it would just be full of dead carts and you'd thus have to come up with a reasonable ordering that would align with something someone managing a store might really do

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.

I think making the pricing calculator be a Task might be a good exercise, both because I've seen such a thing, and because it's relatively easy to do the way I've structured it

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
Copy link
Owner

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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...

@oskardudycz
Copy link
Owner

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.

@bartelink
Copy link
Contributor Author

Regarding Metrics, there's very good metrics for both Equinox and Propulsion via a Grafana dashboard on Prometheus in jet/dotnet-templates#100
There's some wiring missing in Equinox.EventStore, i.e. an Equinox.EventStore.Prometheus but it can be really useful to be able to break traffic down by category or reads vs writes and/or latency heatmaps etc

@bartelink
Copy link
Contributor Author

bartelink commented Nov 25, 2021

In general I would not lean on passing versions out and then back into APIs; while this is implementable, I don't think its worth demonstrating on the basis that it's a bad idea

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.

If someone hits add twice in quick succession from two tabs, I want something predictable to happen. Either:

  • each request has a requestid in it and we dedup attempts based on whether that particular request succeeded before
  • a request is idempotent because it specifies the quantity - i.e. the semantics are add or change quantity and the quantity is the desired total number of items in the cart

This also overlaps with what you do when you merge carts - do you add the quantities, pick the max, or something else

In general with Equinox at small scale, a GET endpoint can be serviced directly from the aggregate - i.e. I would not have a denormalized summary view table - however that may warrant some discussion (code implements it by reading the write store through the cache atm)

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)

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 Synchronous Query in https://github.com/jet/equinox/blob/master/DOCUMENTATION.md#glossary as it is more explicit and unambiguous, but its not exactly pithy and I've never heard anyone else use the term.

A pageable carts list API seems reasonable up to a point, but begs lots of questions as to how you manage that over time - it would just be full of dead carts and you'd thus have to come up with a reasonable ordering that would align with something someone managing a store might really do

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.

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:

  • a feed of all cartids ever
  • a feed of confirmed cartids ever

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 think making the pricing calculator be a Task might be a good exercise, both because I've seen such a thing, and because it's relatively easy to do the way I've structured it

Fine by me. Samples doesn't have to be 1:1, so it's fine to change the details to show some different aspects.

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 🤔

@@ -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),
Copy link
Contributor Author

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,
Copy link
Contributor Author

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

@bartelink bartelink marked this pull request as ready for review December 18, 2021 15:54
@bartelink bartelink changed the title Equinox WIP Port Sample/EventStoreDB/Simple/ECommerce to F#+Equinox Dec 18, 2021
@bartelink bartelink force-pushed the bartelink/add-equinox branch 3 times, most recently from 420e299 to 49e7df7 Compare December 30, 2021 17:28
@bartelink bartelink force-pushed the bartelink/add-equinox branch 2 times, most recently from 76ce151 to 1c22086 Compare March 3, 2022 09:17
@bartelink bartelink force-pushed the bartelink/add-equinox branch 2 times, most recently from 94786ec to 7626601 Compare May 23, 2022 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants