mookid8000 / Cirqus

:x: d60 event sourcing + CQRS framework
MIT License
233 stars 39 forks source link

Remove TOwner from DomainEvent<TOwner> #54

Open asgerhallas opened 9 years ago

asgerhallas commented 9 years ago

After @mhertis implemented the ability to emit events from subclasses of a root, the term owner is now ambiguous: if the event is emitted from a subclass the Owner metadata is actually the type of the emitter, not TOwner.

Thus now TOwner only serves one purpose: to prevent emitting events, that belong to another root hierarchy. And I'm not sure if that adds any value. The id is what the emitted event belongs to, and I could easily emit the same event from two different types of roots (or even from a command) without that posing a problem - we always use the id when retrieving it again and will never get events that does not belong to that id.

I think removing TOwner from DomainEvent<TOwner> will:

  1. Make it more obvious that TOwner is not always the same as Metadata.Owner.
  2. Remove some gotchas from the testing tools, by making it more explicit about who's emitting events.
  3. Make it ready for emitting events from commands.

And if one really needs the check for certain events only being emitted from a certain root (or it's subtypes) then that is easily implemented as a decorator on the UnitOfWork.

What do you think? :)

mookid8000 commented 9 years ago

Not because I disagree (completely) - but MetadataKeys.Owner is the concrete type emitting the event - see RealUnitOfWork and is used to figure out which concrete type to hydrate, which basically allows you to

 var aggregateRoot = commandContext.Load<AggregateRoot>(someId);

and have aggregateRoot be an instance of the real, inherited root type

asgerhallas commented 9 years ago

True, but TOwner of DomainEvent<TOwner> is not that type any longer...

asgerhallas commented 9 years ago

I can see that github has removed all my brackets... two secs, I think that'll clear it up :)

asgerhallas commented 9 years ago

So let me put another way, why have a generic DomainEvent<TOwner> class when

  1. the meaning of TOwner is not same as the metadata key Owner (it was before, but isn't now)
  2. it does have any other value, except a run time type check that can be implemented otherwise if needed
  3. it prevents us from emitting events from commands in the future
mookid8000 commented 9 years ago

ok I agree - we should probably make DomainEvent the event base class... should we keep DomainEvent<TRoot> around? or should it be something that you create yourself in your projects in order to be able to e.g. implement ISubscribeTo<DomainEvent<SomeParticularRoot>> in order to have all events of some particular root dispatched to a view?

asgerhallas commented 9 years ago

My first thought is the latter. I think it is confusing if for example - in the current implementation - you try to do:

ISubscribeTo<DomainEvent<SubClassOfSomeParticularRoot>>

... and don't get any events. I think it would be straight forward to just implement custom filtering logic in the handler, where I can then do:

if (domainEvent.GetOwner() == typeof(SubClassOfSomeParticularRoot)) ...

... or make your own infrastructure around it, if it's something you do a lot.

asgerhallas commented 9 years ago

"... and don't get any events" - at least that is what I think it does, if I'm not missing something :)

asgerhallas commented 9 years ago

As for perfomance, that would result in views being loaded, that are not loaded today, and that would of course have an impact, but it could be implemented by having a static accessible filter like:

public class View : IFilter<OnlyRootsOfThisParticularTypeFilter> {}

public class OnlyRootsOfThisParticularTypeFilter 
{
     bool Handles(Type eventType) {}
}

It may even be possible to support filters that operates on metadata only to save the deserialization of event body (and affected views) before absolutely neccessary.

mookid8000 commented 9 years ago

No need to IFilter anything - you can always filter by having a ViewLocator that doesn't return any IDs for some particular types of events.

asgerhallas commented 9 years ago

Ah yes, I see. I kind of thought the ViewLocators were supposed to more generally applicable, but that may be because I have never needed anything more advanced so far :)

mhertis commented 9 years ago

DomainEvent<TRoot> will still be useful (at least for us :) as we heavily use it in views as @mookid8000 pointed out.

asgerhallas commented 9 years ago

@mhertis ok, I think we should make it easy to still do that then. What would you think of a ISubscribeToEventsFrom<T> interface that - as opposed to now - will handle subclasses of aggregate roots. And it will do so based on the emitting type.

Today if you have ISubscribeTo<DomainEvent<SomeRoot>> it won't handle events of type DomainEvent<SubclassOfSomeRoot> nor the other way around. I think that's a potential source of confusion and errors.

I will spend tomorrow trying to implement this, so please let me know if it's totally useless :)

mookid8000 commented 9 years ago

Today if you have ISubscribeTo> it won't handle events of type DomainEvent nor the other way around. I think that's a potential source of confusion and errors.

I don't understand what you mean by this - if a view implements ISubscribeTo<DomainEvent>, it will get each and every event that gets emitted.

If it implements ISubscribeTo<DomainEvent<SomeRoot>> it will get each and every event emitted by that root.

Moreover, if the view implements ISubscribeTo<MyFunkyEventBase>, the view will get all events derived off of MyFunkyEventBase.

Generally, view managers will receive events that are polymorphically compatible with the implemented subscriber interfaces.

asgerhallas commented 9 years ago

I've updated my post with correct markup, hope that makes more sense now :)

asgerhallas commented 9 years ago

It's the same argument I made a few posts earlier, just a different wording :)

sdebruyn commented 9 years ago

I agree with @asgerhallas. I am building an API with Cirqus and a DomainEvent without the generics would simplify things a lot.

Example:

I have an abstract class Animal and subclasses Dog and Cat. My Animal has a ScratchEvent because both cats and dogs can scratch. Dog has a BarkEvent and Cat has a MeowEvent. I have a CatsView and a DogsView. The ScratchEvent should be delivered to both views, but the BarkEvent should only be delivered to DogsView and the MeowEvent should only be delivered to CatsView.

With the current implementation I define a lot of interfaces to get all events in every view. E.g. IGetViewIdsFor<DomainEvent<Cat>> is not enough.