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

Event is mutable #20

Open
beachwalker opened this issue Mar 9, 2016 · 1 comment
Open

Event is mutable #20

beachwalker opened this issue Mar 9, 2016 · 1 comment

Comments

@beachwalker
Copy link

This makes Event a mutable type.

public class Event : Message
{
    public int Version;
}

I think Commands and Events should be immutable instead.

@EnricoMassone
Copy link

EnricoMassone commented Jun 3, 2019

Hi @beachwalker ,

I know that this issue is quite old, but I would like to contribute.

The reason for which the Event class is mutable is related to the need of assigning the right version to the event itself, when the event is appended to the event stream. This is done right after the optimistic concurrency check in the EventStore class, see here at line 62.

Two subjects somewhat related to this are the issue #7 and the issue #31 that I recently opened in order to better understand what is the semantic of the Version property.

If my interpretation of the Version property is fine (see #31 for details), we could submit a pull request in order to fix both this issue and #7 . This is the top level plan:

  • fix the issue AggregateRoot version never updated #7 so that each time an event is applied to the aggregate state the Version property is incremented by 1 (it must be clear how to initialize the counter of the aggregate version. I guess that in Greg's view the aggregate version should be zero after applying the first event. Anyway, it's just a matter of preference, we should only agree on a convention).
  • change the Event base class so that the Version property becomes read only. We could add a protected constructor requesting the value for Version as its sole parameter
  • change the code which raises an event inside the aggregate, so that the value for Event.Version is passed inside the event constructor as this.Version + 1 (the current aggregate version plus one).
  • the optimistic concurrency check in the EventStore class will stay the same, the only change is removing the code at line 62 in the EventStore class (because the Event.Version property won't be writable anymore).

I'm quite confident that assigning the Version to an event in advance (before performing the optimistic concurrency check) won't break anything on the correctness side: if there are concurrency issues the events won't be appended to the event stream so the Version assigned to them is not relevant. Otherwise, in case of a successful optimistic concurrency check, the event Version assigned this way is exactly the same as the one assigned by the currently existing code (because the command is performed against the most recent aggregate version available and, when the events are saved, the aggregate version in the event store is not changed because the optimistic concurrency check was successful).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants