Aggregate may change identity during history load

[thomaseyde]

Problem

The way the aggregate assumes its identity when event are loaded from history may cause it to change identity.

We discovered this when we had the bright idea on raising events from one aggregate on behalf of an other. Yes, that's the total improper thing to do, but there are no safe guards against it.

When the aggregate is in this state, one of two things happens:

1. All changes are saved to the wrong event stream
2. Or you get a concurrency exception you can't escape from, because the other event stream is on a different version.

The problem is complex:

• There are two ways to give the event an identity: Either you do it yourself, or the aggregate will do it if it's missing
• There are also two ways to give the aggregate an identity: You do it on creation, or when history is loaded
• When the aggregate gives the event an identity, it only checks for a missing id, not that the given is correct.

Possible fixes

The aggregate shouldn't accept incorrect event ids

This should be easy. And shouldn't break existing contracts.

On save: Set empty event ids to the aggregate's id. Ensure existing ids are the same as the aggregate id. Throw otherwise.

On load: Ensure all event ids are the same. Throw otherwise.

Design the aggregate for immutable events

This would be a breaking change.

There are many who argue for immutable events. In that case, the event id should be set at creation time, as it's not possible to change it later.

In this case, the IEvent.Id property should be read only. The aggregate should still ensure valid event ids.

[gautema]

Hi.

That's a nifty problem you got there. I haven't thought of or heard of anyone doing anything like that, so there is no safe guard for those kind of things.

As to solution, I agree that events ideally should be immutable, but I decided at some point that it was easier to just set a missing id from the framework than to enforce that the people set it. I'm not necessarily against changing that, but I feel that it at least requirers some thought.

Throwing when ids are wrong sounds like a good idea. If you want to make a pull request, you are very welcome, if not I can take a look in a free moment and see if I can do it my self.

[gautema]

I had the time, so I fixed it. I will update the nuget a bit later today.

[gautema]

After testing the new release I realised the extra checking gives it at least 10% lower performance. This part of the code is very hot and I will have to consider if the speed degradation is actually worth it. It's released in release 0.29 for now.

[thomaseyde]

That was fast!

Strange though, that an extra check should yield a 10% speed degradation. Would that be against an in-memory repository? If so, do you think the degradation is noticeable when network I/O is added to the mix?

On Wed, Jul 17, 2019 at 1:17 PM Gaute Magnussen notifications@github.com wrote:

After testing the new release I realised the extra checking gives it at least 10% lower performance. This part of the code is very hot and I will have to consider if the speed degradation is actually worth it. It's released in release 0.29 for now.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/gautema/CQRSlite/issues/87?email_source=notifications&email_token=AAD3IM4GOXKCGUJY2UTQNFTP735UHA5CNFSM4IEMY7U2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2D3UEI#issuecomment-512211473, or mute the thread https://github.com/notifications/unsubscribe-auth/AAD3IM2CY55U6XYE4ZMYXRTP735UHANCNFSM4IEMY7UQ .

[gautema]

10% degradation is against the framework isolated. So there’s probably not noticeable in real life. Still it’s in the the middle of a really hot path of the code, so one extra check on every event written or read is a bit of extra work.