!!!FEATURE: Add notion of correlation and causation Id #157

bwaidelich · 2017-10-12T15:40:03Z

This is a new approach to introduce the notion of optional
correlation/causation ids to events.

Usage:

$eventWithCorrelationId = new EventWithMetadata($originalEvent, ['correlationId' => $id]);
$eventWithCausationId = new EventWithMetadata($originalEvent, ['causationId' => $id]);

This also reworks the EventStreamFilterInterface to be easier to extend/implement
and adds a new implementation to filter for the correlationId (as preparation for event-sourced
Process Managers).

Breaking Change:

This is a breaking change because it changes the EventStreamFilterInterface and because
it moves the EventWithMetadata class to a Decorator sub namespace.
Besides, the schema of the doctrine based event-store is updated, adding the
two columns correlationid and causationid.
To update the db accordingly, just run

./flow eventstore:setupall

again.

Resolves: #6

This is a new approach to introduce the notion of optional correlation/causation ids to events. Usage: $eventWithCorrelationId = new EventWithMetadata($originalEvent, ['correlationId' => $id]); $eventWithCausationId = new EventWithMetadata($originalEvent, ['causationId' => $id]); This also reworks the `EventStreamFilterInterface` to be easier to extend/implement and adds a new implementation to filter for the `correlationId` (as preparation for event-sourced Process Managers). Breaking Change: This is a breaking change because it changes the `EventStreamFilterInterface`. Besides, the schema of the doctrine based event-store is updated, adding the two columns `correlationid` and `causationid`. To update the db accordingly, just run ./flow eventstore:setupall again. Resolves: neos#6

bwaidelich · 2017-10-12T15:41:03Z

Classes/Event/EventWithMetadata.php

+        } else {
+            $this->event = $event;
+            $this->metadata = $metadata;
+        }


I sneaked in this improvement as follow-up to #156, so that EventWithMetadata can actually be nested

bwaidelich · 2017-10-12T15:41:48Z

Classes/EventStore/EventStreamFilterInterface.php

+     *
+     * @return array
+     */
+    public function getFilterValues(): array;


Changed this so that we don't have to implement/update loads of empty methods when creating new dedicated filters

I'm unsure about this. I see the hassle of the empty methods, but with this we forfeit all of the type safety and implementation clarity for some additional flexibility/conciseness. Also, the actual filtering method now is much more technical due to the array_key_exists checks with the constants.

Can we maybe just bring down the hassle with an AllEventsStreamFilter implementation, so that concrete filters only need to extend that and override the specifc methods that they want to filter on?

Just to clarify: I see the problem with the current implementation not so much in the hassle of implementing methods but in the lack of extensibility.
To implement a new filter we currently have to change the interface which will break every implementation.

we forfeit all of the type safety

Yes, there's slightly reduced type safety but I considered it a pragmatic solution as this won't be used in highlevel userland code anyways.

the actual filtering method now is much more technical

is that a problem? That happens only within the EventStore implementation, which is very technical anyways.

Can we maybe just bring down the hassle with an AllEventsStreamFilter

I would really like to avoid abstract base classes to solve this and a QOM/DSL would be overkill IMO.
What I could imagine is a purely interface-based approach like

interface FiltersStreamNameInterface { public function getStreamName(): string } interface FiltersStreamNamePrefixInterface { public function getStreamNamePrefix(): string } // ...

but is that really worth it?

To implement a new filter we currently have to change the interface which will break every implementation.

Fair point, but my take would be, that there is an upper boundary on what the EventFilter interface should contain, because there's only so much that makes (generically) sense to filter events by. Anything on top can still be solved with an untyped "additionalFilterProperties(): array" method, but the basic filters should be expressive and type safe.

is that a problem? That happens only within the EventStore implementation, which is very technical anyways.

You're right, I just prefer to to read something like if ($foo->hasSomething()) over if (array_key_exists($foo, FooClass::SOMETHING)) any time, even inside technical code.

I would really like to avoid abstract base classes

Doesn't need to be abstract even. Think of it like this: Any (custom) filter is a specialization of an AllEventsStreamFilter. It is a real is-a relationship, so inheritance is a plausible modelling pattern.

What I could imagine is a purely interface-based approach like

TBH I would even prefer that over a single array typed filter specification interface. But it would lead to lots of if ($foo instanceof Bar) checks - PHP finally needs some real pattern matching soon :/

bwaidelich · 2017-10-12T15:42:05Z

Classes/EventStore/Storage/Doctrine/DoctrineEventStorage.php

@@ -109,7 +110,7 @@ public function load(EventStreamFilterInterface $filter): EventStream
        $this->applyEventStreamFilter($query, $filter);

        $streamIterator = new DoctrineStreamIterator($query);
-        return new EventStream($streamIterator, $this->getStreamVersion($filter));
+        return new EventStream($streamIterator);


The second argument was not used

bwaidelich · 2017-10-12T15:43:06Z

Classes/EventStore/Storage/Doctrine/DoctrineEventStorage.php

-        $result->addNotice(new Notice($this->connection->getDriver()->getName(), null, [], 'Driver'));
-        $result->addNotice(new Notice($this->connection->getUsername(), null, [], 'Username'));
+        $result->addNotice(new Notice((string)$this->connection->getHost(), null, [], 'Host'));
+        $result->addNotice(new Notice((string)$this->connection->getPort(), null, [], 'Port'));


Without this cast, an exception is thrown if the port is not set because of the recently introduced type hints on the Notice class

albe · 2017-10-16T11:38:19Z

Classes/EventStore/EventStreamFilterInterface.php

+     *
+     * @return array
+     */
+    public function getFilterValues(): array;


I'm unsure about this. I see the hassle of the empty methods, but with this we forfeit all of the type safety and implementation clarity for some additional flexibility/conciseness. Also, the actual filtering method now is much more technical due to the array_key_exists checks with the constants.

Can we maybe just bring down the hassle with an AllEventsStreamFilter implementation, so that concrete filters only need to extend that and override the specifc methods that they want to filter on?

albe · 2017-10-16T11:44:47Z

Classes/EventStore/Storage/Doctrine/DoctrineEventStorage.php

+     *
+     * @return Schema
+     */
+    private function createEventStoreSchema()


Any reason to have the schema moved from the dedicated file? I found it better to contain the schema specification in an own file

There used to be more functionality in the Schema folder and in this file. Now there's only one static method left that is invoked in a single place.
What's the advantage of a separate file for this one method?

Less "visual debt" :P Nah, really, I just prefer to have different concerns separated in different locations. Schema is a different concern than how the EventStorage is implemented IMO.

Personally, I like the new way. Maybe it's just a matter of taste.

albe · 2017-10-16T11:46:12Z

Tests/Unit/Event/EventWithMetadataTest.php

+    /**
+     * @test
+     */
+    public function eventIsUnwrappedWhenNestingEventsWithMetadata()


+1 for test coverage of sneaked in feature ;)

robertlemke

I'll test this with my project now …

robertlemke · 2017-10-27T05:49:48Z

Classes/EventStore/Storage/Doctrine/DoctrineEventStorage.php

+            $schemaDiff = (new Comparator())->compare($fromSchema, $this->createEventStoreSchema());
+            $statements = $schemaDiff->toSaveSql($this->connection->getDatabasePlatform());
+            if ($statements !== []) {
+                $result->addWarning(new Warning('The schama of table %s is not up-to-date', null, [$this->eventTableName], 'Table schema'));


a typo ("schama")

robertlemke · 2017-10-27T05:51:10Z

Classes/EventStore/Storage/Doctrine/DoctrineEventStorage.php

+     *
+     * @return Schema
+     */
+    private function createEventStoreSchema()


Personally, I like the new way. Maybe it's just a matter of taste.

robertlemke

This is a good step ahead, however, I have the following concerns / thoughts:

can't we call it "correlationIdentifier" instead of "correlationId"? It really sticks out from all my other code where I have value objects called "somethingIdentifier" …
it's a bit weird that the correlation identifier and causation identifier appear both in metadata and their own fields
in general it doesn't feel like first-class support for these identifiers yet, because the way to provide it is so implicit. It's easy to produce bugs because just mistyping it ("corelationId") won't give you any errors

Should we discuss this further or is it too much discussion for this feature?

bwaidelich · 2017-10-27T07:57:11Z

can't we call it "correlationIdentifier" instead of "correlationId"?

Yes of course. I find somethingId just easier to comprehend, but you're right that we went the "identifier" way everywhere else, so good point!

correlation identifier and causation identifier appear both in metadata and their own fields

Conceptually they are part of the event metadata (and that's how it is in all other implementations I saw). In the doctrine store we just put them into a separate column additionally in order to make it possible to look up events by the correlation id. It's an implementation detail if you will.

in general it doesn't feel like first-class support for these identifiers yet

That's true and maybe we can work around this with more explicit decorators and value objects for the different ids:

$eventWithCorrelationId = new EventWithCorrelationId($originalEvent, $correlationId);
$eventWithCorrelationAndCausationId = new EventWithCausationId($eventWithCorrelationId, $causationId);

and maybe even additional interfaces so you can just implement support for those ids in your domain events..
does that make sense?

* Move `EventWithMetadata` to `Decorator` sub namespace * Introduce `EventWithMetadataInterface` * Add `EventWithCorrelationIdentifier`/`EventWithCausationIdentifier` decorators * Rename "id" to "identifier" where applicable

robertlemke

That looks great and I just tested it - it works, too.

bwaidelich requested review from hlubek, robertlemke and albe October 12, 2017 15:40

bwaidelich commented Oct 12, 2017

View reviewed changes

bwaidelich mentioned this pull request Oct 12, 2017

Applied events log starts at zero #141

Open

albe reviewed Oct 16, 2017

View reviewed changes

TASK: Cosmetic tweaks to satisfy StyleCI checks

00f6046

robertlemke reviewed Oct 27, 2017

View reviewed changes

Fix a typo in a warning message

0b351c0

robertlemke reviewed Oct 27, 2017

View reviewed changes

robertlemke added 2 commits October 27, 2017 09:46

Fix error in EventStreamNotFound exception message

d7110ea

Provide getFilterValue() method for easier handling

9bc9907

Bastian Waidelich and others added 3 commits October 27, 2017 14:39

Merge branch 'master' into 6-event-correlation-and-causation-id

5288e76

TASK: Cosmetic StyleCi fixes

3b93df2

robertlemke approved these changes Oct 30, 2017

View reviewed changes

robertlemke merged commit 4680470 into neos:master Oct 30, 2017

robertlemke deleted the 6-event-correlation-and-causation-id branch October 30, 2017 09:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

!!!FEATURE: Add notion of correlation and causation Id #157

!!!FEATURE: Add notion of correlation and causation Id #157

bwaidelich commented Oct 12, 2017 •

edited

bwaidelich Oct 12, 2017

bwaidelich Oct 12, 2017

albe Oct 16, 2017

bwaidelich Oct 17, 2017

albe Oct 17, 2017

bwaidelich Oct 12, 2017

bwaidelich Oct 12, 2017

albe Oct 16, 2017

albe Oct 16, 2017

bwaidelich Oct 17, 2017

albe Oct 17, 2017

robertlemke Oct 27, 2017

albe Oct 16, 2017

robertlemke left a comment

robertlemke Oct 27, 2017

robertlemke Oct 27, 2017

robertlemke left a comment

bwaidelich commented Oct 27, 2017

robertlemke left a comment

!!!FEATURE: Add notion of correlation and causation Id #157

!!!FEATURE: Add notion of correlation and causation Id #157

Conversation

bwaidelich commented Oct 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertlemke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertlemke left a comment

Choose a reason for hiding this comment

bwaidelich commented Oct 27, 2017

robertlemke left a comment

Choose a reason for hiding this comment

bwaidelich commented Oct 12, 2017 •

edited