Improvements #14
Improvements #14
Conversation
prolic
commented
Jan 22, 2017
•
prolic
commented
- resolves: Version vs. event no problem with snapshots #13
- use variadics for aggregate result
- several improvements
|
Changes Unknown when pulling 38f3bc3 on improvements into ** on master**. |
|
Changes Unknown when pulling d6b333b on improvements into ** on master**. |
|
Changes Unknown when pulling d6b333b on improvements into ** on master**. |
| * ] | ||
| * $message is expected to be an instance of Prooph\Common\Messaging\Message | ||
| */ | ||
| function buildCommandDispatcher( | ||
| callable $eventStoreFactory, | ||
| callable $snapshotStoreFactory, | ||
| array $commandMap, | ||
| AggregateDefiniton $aggregateDefiniton, |
There was a problem hiding this comment.
with this change you can no longer handle commands for different aggregates. Why is it removed from the map?
There was a problem hiding this comment.
I thought like "microservice" - only one aggregate type per microservice is allowed.
There was a problem hiding this comment.
but I can also revert this change, if you think it's not good.
There was a problem hiding this comment.
Yeah, I think this is a good default, but I can image that you have maybe two aggregates in one microservice. It is a trade off between independence of a microservice versus sharing some code like value objects.
There was a problem hiding this comment.
damn ok, lets try it with one aggregate per microservice.
There was a problem hiding this comment.
arg, did not see your comment. revert +1
|
|
||
| public function __construct(string $id) | ||
| // this is only required when using one stream per aggregate type | ||
| ->withMetadataMatch('_aggregate_version', Operator::EQUALS(), $aggregateVersion); |
There was a problem hiding this comment.
This will not work! see comment below.
| ): Iterator { | ||
| $eventStore = $eventStoreFactory(); | ||
|
|
||
| if ($eventStore->hasStream($streamName)) { | ||
| return $eventStore->load($streamName, $fromVersion, null, $metadataMatcher)->streamEvents(); | ||
| return $eventStore->load($streamName, 1, null, $metadataMatcher)->streamEvents(); |
There was a problem hiding this comment.
Here is the problem (see comment above). You specify an equal match for the aggregate_version, so only one event will match, not all newer events.
|
Changes Unknown when pulling 5e70698 on improvements into ** on master**. |
| * ] | ||
| * $message is expected to be an instance of Prooph\Common\Messaging\Message | ||
| */ | ||
| function buildCommandDispatcher( | ||
| callable $eventStoreFactory, | ||
| callable $snapshotStoreFactory, | ||
| array $commandMap, | ||
| AggregateDefiniton $aggregateDefiniton, |
There was a problem hiding this comment.
arg, did not see your comment. revert +1
| RegisterUser::class => function (array $state, Message $message) use (&$factories): AggregateResult { | ||
| return User\registerUser($state, $message, $factories['emailGuard']()); | ||
| }, | ||
| ChangeUserName::class => User\changeUserName, | ||
| ]; | ||
|
|
||
| $dispatch = Kernel\buildCommandDispatcher( | ||
| $factories['eventStore'], | ||
| $factories['snapshotStore'], |
There was a problem hiding this comment.
can we make snapshotting optional? Like it is for standard event-sourcing
There was a problem hiding this comment.
use InMemorySnapshotStore, no?
|
Changes Unknown when pulling 2074b3c on improvements into ** on master**. |
| @@ -43,9 +43,9 @@ function changeUserName(array $state, ChangeUserName $command): AggregateResult | |||
| throw new InvalidArgumentException('Username too short'); | |||
| } | |||
|
|
|||
| $event = new UserNameWasChanged($command->payload()); | |||
| $event = new UserNameWasChanged($command->payload(), ['_aggregate_version' => $state['version'] + 1]); | |||
There was a problem hiding this comment.
we should move this to the apply method, otherwise you can easily forget to set the version as metadata and break snapshotting
There was a problem hiding this comment.
apply would be wrong. maybe a helper function to create events which requires the version as argument?
There was a problem hiding this comment.
There is no such thing as $event->withAddedPayloadKey($key, $value)
| ++$state['version']; | ||
|
|
||
| return $state; | ||
| return array_merge($state, $event->payload(), ['activated' => false, 'blocked_reason' => 'duplicate email', 'version' => ++$state['version']]); |
There was a problem hiding this comment.
++$state['version'] That is a problem, because you can use the apply method only once for the same events. When you replay and state already has a version set, this will increment the version which is wrong. Replay again and version is incremented again. This is a side-effect and very dangerous.
There was a problem hiding this comment.
Note that we have two distinct methods in the event sourcing aggreate, one for apply and one for replay. Maybe we need this distinction here, too? Or we find a functional way of solving the problem ;)
|
updated |
|
Changes Unknown when pulling bd7b7c8 on improvements into ** on master**. |
|
Changes Unknown when pulling 05daa45 on improvements into ** on master**. |
