search

Eventuous / eventuous

Event Sourcing library for .NET
https://eventuous.dev
Apache License 2.0
453 stars 73 forks source link

`StreamName.For<T>()` is prepending type name #74

Closed thomaseyde closed 2 years ago

thomaseyde commented 2 years ago

Why is StreamName.For<T>() prepending the type name? Why is it depending on T at all?

I have at least two problems with this:

  1. My event store already have established stream ids. Eventuous is unable to read these without me implementing an adapter and hack the incoming stream name.
  2. Renaming an aggregate will disconnect it from existing streams.
alexeyzimarev commented 2 years ago

What your stream ids look like?

alexeyzimarev commented 2 years ago

About "Why is it depending on T at all?" - it's because Eventuous is built on the opinion that since a stream is also a transaction boundary, one stream should match one aggregate. Therefore, the stream name is built from the aggregate type and its identity. It's a good default, but it has a limitation about separating, for example, different bounded contexts that have the same aggregate names (and, probably, the same ids) by the bounded context namespace. I had a discussion about it with colleagues just this week.

If you have some samples of what you have in your event store or any suggestion about how to figure out the stream name given the aggregate type and id, I would love to hear that.

And yes, I agree that renaming the aggregate type will break things, and it's not good.

thomaseyde commented 2 years ago

In my opinion stream names shares the same problem as event names. Which you solved with explicit mapping.

One approach is to not tie stream name to aggregate. Another one is to map aggregate types the same way as event types.

But why do you need this at all? We have to provide the stream name/aggregate id in the application service anyway.

Our stream ids look like: /inventory/customer-id/ad/members

A third option would be to provide us a hook where we can define our own conventions.

alexeyzimarev commented 2 years ago

I think the default behaviour of the command service is okay. You just need to extract the id, otherwise, you'd need to interpolate the string for the stream id all the time. Also, making it work in a way that you will return the full stream id would make this https://github.com/Eventuous/eventuous/issues/39 impossible.

Detaching stream name from aggregate type is a good idea, I fully agree. If we all agree on that, I can make it work.

thomaseyde commented 2 years ago

Agreed 🙂. Most important to me is to use Eventuous on top of my existing streams. Right now I hacked the stream name in my own adapter for SqlStreamStore. But it’s ugly.

alexeyzimarev commented 2 years ago

Btw concerning SqlStreamStore, how do you feel using it as it's no longer maintained?

alexeyzimarev commented 2 years ago

@thomaseyde have a look at #78, I think it will solve the issue you have. Not all overloads of the app service command handler registrations are in place, but it should give you the idea.

alexeyzimarev commented 2 years ago

I believe the change in #78 is enough

alexeyzimarev commented 2 years ago

@thomaseyde I think specifying the stream name calculation function for each command is an overkill.

My idea is that the identity record can have more properties, then the getId function can populate these properties for each command, and then you need only one id -> stream name function defined per identity type.

https://github.com/Eventuous/eventuous/blob/93cbb3b1f62432052793f8ba63fdf3df8f925393/src/Core/test/Eventuous.Tests/StoringEventsWithCustomStream.cs#L10-L15

thomaseyde commented 2 years ago

Thinking more about this: StreamName and AggregateId are different concerns.

I should be able to identify my aggregate however I like without having to think about persistence. The aggregate should be persistence agnostic.

The stream name, however, is all about persistence and need to contain all information needed.

The ApplicationService then should provide a strategy to map AggregateId to StreamName. This could be a function T -> StreamName.

If I have a common command interface for my aggregate, I could:

StreamName name = StreamNameFrom<IMyCommand>(c => /**/)

If I don't, I could use an object overload and pattern matching:

StreamName name = StreamNameFrom(c => c switch { /**/ }).

Which may be what is suggested earlier here or in a related issue?

alexeyzimarev commented 2 years ago

The ApplicationService then should provide a strategy to map AggregateId to StreamName. This could be a function T -> StreamName.

It's right in the code I have shown in the previous comment. I merged my PR, so I want to close this now. If you get any feedback, please feel free to provide some.

alexeyzimarev commented 2 years ago

Docs: https://eventuous.dev/docs/persistence/aggregate-stream/#stream-name