BUGFIX: Fix and test tethered node structure adjustment #4969

mhsdesign · 2024-03-30T10:58:55Z

I found out that this line

neos-development-collection/Neos.ContentRepository.Core/Classes/Feature/Common/TetheredNodeInternals.php

Line 128 in 1fcdec4

} elseif (count($childNodeAggregates) === 1) {

is totally untested. Thus i wanted to write a test.

This pr also adds causation ids and meta data for changes made by structure adjustments to easier debug them. See also #3887

Related: #4966
Related: #4832 (comment)

Upgrade instructions

Review instructions

Checklist

Code follows the PSR-2 coding style
Tests have been created, run and adjusted as needed
The PR is created against the lowest maintained branch
Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
Reviewer - The first section explains the change briefly for change-logs
Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

mhsdesign · 2024-03-30T11:00:20Z

Currently fails with

RuntimeException: Failed to update and commit highest applied sequence number for subscriber "Neos\ContentGraph\DoctrineDbalAdapter\DoctrineDbalContentGraphProjection". Please run Neos\ContentRepository\Core\Infrastructure\DbalCheckpointStorage::setUp() in /Users/marchenryschultz/Code/core/neos-manufacture-90/Packages/Neos/Neos.ContentRepository.Core/Classes/Infrastructure/DbalCheckpointStorage.php:125

this is due to the blocking in $this->eventPersister->publishEvents. If we dont catchup everything is fine. So a projection problem?

…eAggregate ... The behavior should be fully equivalent

mhsdesign · 2024-03-31T15:16:56Z

Neos.ContentRepository.Core/Classes/Feature/Common/TetheredNodeInternals.php

-                break;
-            }
-            /** @var Node $childNodeSource Node aggregates are never empty */
+            $occupiedDimensionSpacePoints = $childNodeAggregate->occupiedDimensionSpacePoints->getPoints();


idk if we should guard here with this check ... the added tests will pass either way ;)

if (count($occupiedDimensionSpacePoints) !== 1) { throw new \RuntimeException('TODO: The ChildNodeAggregate occupies multiple origins.', 1711897662); }

I don't see an issue with the child node aggregate occupying multiple DSPs, that's totally legit

nezaniel

Needs to be synced with 9.0, looks otherwise fine so far

nezaniel · 2024-04-27T07:45:59Z

Neos.ContentRepository.Core/Classes/Feature/Common/TetheredNodeInternals.php

-                break;
-            }
-            /** @var Node $childNodeSource Node aggregates are never empty */
+            $occupiedDimensionSpacePoints = $childNodeAggregate->occupiedDimensionSpacePoints->getPoints();


I don't see an issue with the child node aggregate occupying multiple DSPs, that's totally legit

…redNodeStructureAdjustment

mhsdesign · 2024-04-27T08:56:10Z

✅

mhsdesign · 2024-04-27T08:58:08Z

Neos.ContentRepository.Core/Classes/EventStore/EventsToPublish.php

@@ -33,4 +35,31 @@ public static function empty(): self
            ExpectedVersion::ANY()
        );
    }
+
+    public function withCausationOfFirstEventAndAdditionalMetaData(EventMetadata $metadata): self


that might be something were i guess only @bwaidelich can say if this is a good idea.

I use this utility to tie all events together and add a description to the first event as to the why.

It's a good idea in general (thus #3887) but we have to get this right:
The first event is not really the cause for the other events but the command. Our commands don't have an id yet. We could actually change that since we persist the commands.
But instead we should IMO focus on the correlation id (so that all events of one transaction are inter-connected).
The causation id does not have to be a UUID – we could e.g. add some speaking prefix, for example the command type (see #3887 (comment))

okay thanks so i understand all should share one common correlationId (CorrelationId::fromString(UuidFactory::create()).

But additionally i want to add somewhere to the (first?) event (as part of metadata?) which structure adjustment caused the creation of this event.

So that the events read like:

Name correlation id metadata

NodeWasCreated 123 {structureAdjustment: 'Content Stream: %s; Dimension Space Point: %s, Node Aggregate: %s --- The tethered child node "bar" is missing.'}

SomethingLol 123

at first i tried to insert the rendered structure adjustment message in there but that wont work because its to long and special chars.

I see, that makes sense.
What about turning this method into

/** * @param \Closure(EventInterface|DecoratedEvent): array<mixed> $callback **/ public function mapEvents(\Closure $callback): array { return new self( $this->streamName, $this->events->map($callback), $this->expectedVersion ); }

and then in the calling side below do sth. along the lines of

$isFirstEvent = true; $enrichedEventsToPublish = $eventsToPublish->mapEvents(function (EventInterface|DecoratedEvent) use (&$isFirstEvent, $correlationId) { $metadata = null; if ($isFirstEvent) { $metadata = $event instanceof DecoratedEvent && $event->metadata !== null ? $event->metadata->value : []; $metadata['structureAdjustment'] = mb_strimwidth($adjustment->render() , 0, 250, '…'); $isFirstEvent = false; } return DecoratedEvent::create( event: $event, correlationId: $correlationId, metadata: $metadata, ) );

ok, this is is not much cleaner. But it avoids turning iterators to arrays vice versa and is a bit less dependent to inner workings.

~~btw: you can probably omit the correlation id since we should add that within ContentRepository::handle() with #3887~~ nevermind, in this case that's not possible

&$isFirstEvent :D no okay fine with me ;)

"along the lines of" – there's probably more bugs in the dummy code above.

BTW: DecoratedEvent and EventMetadata is not easy to be used.. We could extend that (in the neos/eventstore package) a bit so that we could do

$metadata->with('newKey', $newValue);

so that we could simplify the above to sth like:

$decoratedEvent = DecoratedEvent::create( event: $event, correlationId: $correlationId, ); if ($isFirstEvent) { $decoratedEvent = $decoratedEvent->withAddedMetadata('structureAdjustment', mb_strimwidth($adjustment->render() , 0, 250, '…')); $isFirstEvent = false; } return $decoratedEvent;

bwaidelich

Just some initial thoughts on correlation and causation ids, happy to discuss this further!

bwaidelich · 2024-04-27T12:44:45Z

Neos.ContentRepository.Core/Classes/EventStore/EventsToPublish.php

@@ -33,4 +35,31 @@ public static function empty(): self
            ExpectedVersion::ANY()
        );
    }
+
+    public function withCausationOfFirstEventAndAdditionalMetaData(EventMetadata $metadata): self


It's a good idea in general (thus #3887) but we have to get this right:
The first event is not really the cause for the other events but the command. Our commands don't have an id yet. We could actually change that since we persist the commands.
But instead we should IMO focus on the correlation id (so that all events of one transaction are inter-connected).
The causation id does not have to be a UUID – we could e.g. add some speaking prefix, for example the command type (see #3887 (comment))

bwaidelich · 2024-04-27T12:50:41Z

Neos.ContentRepository.StructureAdjustment/src/StructureAdjustmentService.php

+            $enrichedEventsToPublish = $eventsToPublish->withCausationOfFirstEventAndAdditionalMetaData(
+                EventMetadata::fromArray([
+                    'structureAdjustment' => mb_strimwidth($adjustment->render() , 0, 250, '…')
+                ])
+            );


IMO this should look something like:

Suggested change

$enrichedEventsToPublish = $eventsToPublish->withCausationOfFirstEventAndAdditionalMetaData(

EventMetadata::fromArray([

'structureAdjustment' => mb_strimwidth($adjustment->render() , 0, 250, '…')

])

);

$enrichedEventsToPublish = $eventsToPublish->withCorrelationId(CorrelationId::fromString(UuidFactory::create());

bwaidelich · 2024-04-27T12:54:59Z

Neos.ContentRepository.Core/Classes/EventStore/EventsToPublish.php

+        /** @var list<EventInterface|DecoratedEvent> $restEvents */
+        $restEvents = iterator_to_array($this->events);
+        if (empty($restEvents)) {
+            return $this;
+        }
+        $firstEvent = array_shift($restEvents);
+
+        if ($firstEvent instanceof DecoratedEvent && $firstEvent->eventMetadata) {
+            $metadata = EventMetadata::fromArray(array_merge($firstEvent->eventMetadata->value, $metadata->value));
+        }
+
+        $decoratedFirstEvent = DecoratedEvent::create($firstEvent, eventId: Event\EventId::create(), metadata: $metadata);
+
+        $decoratedRestEvents = [];
+        foreach ($restEvents as $event) {
+            $decoratedRestEvents[] = DecoratedEvent::create($event, causationId: $decoratedFirstEvent->eventId);
+        }
+
+        return new EventsToPublish(
+            $this->streamName,
+            Events::fromArray([$decoratedFirstEvent, ...$decoratedRestEvents]),
+            $this->expectedVersion
+        );


That's quite a lot of complexity. I don't really think that we need to handle the first event differently from the others in this case (see suggestion below) so maybe we can simplify this to a one-liner like:

return new self( $this->streamName, $this->events->map(fn (Event $event) => DecoratedEvent::create( event: $event, correlationId: $correlationId, ), $this->expectedVersion, );

mhsdesign added 2 commits March 30, 2024 11:57

TASK: neos#3887 set causationId and meta data for structure adjustments

754af80

TASK: Add test for createEventsForMissingTetheredNode

5d01d4d

github-actions bot added Task 9.0 labels Mar 30, 2024

Bernhard Schmitt and others added 3 commits March 30, 2024 12:35

vary the correct node when adding missing tethered children

26737c2

TASK: Adjust test to contain exact event expectations

973b70a

TASK: Prefer working with occupiedDsp instead arbitrary node from Nod…

3fde3e6

…eAggregate ... The behavior should be fully equivalent

mhsdesign commented Mar 31, 2024

View reviewed changes

TASK: Make phpstan happy (as easter present)

93f8111

mhsdesign requested a review from nezaniel April 2, 2024 13:29

This was referenced Apr 2, 2024

TASK: Document NodeAggregate #4832

Open

BUGFIX: Catch not completely covered node aggregates #4966

Merged

mhsdesign changed the title ~~TASK: Write test for tethered node structure adjustment~~ BUGFIX: Fix and test tethered node structure adjustment Apr 3, 2024

github-actions bot added the Bug label Apr 3, 2024

nezaniel reviewed Apr 27, 2024

View reviewed changes

Merge remote-tracking branch 'origin/9.0' into task/writeTestForTethe…

1e97147

…redNodeStructureAdjustment

mhsdesign requested a review from nezaniel April 27, 2024 08:56

mhsdesign commented Apr 27, 2024

View reviewed changes

bwaidelich reviewed Apr 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUGFIX: Fix and test tethered node structure adjustment #4969

BUGFIX: Fix and test tethered node structure adjustment #4969

mhsdesign commented Mar 30, 2024 •

edited

mhsdesign commented Mar 30, 2024

mhsdesign Mar 31, 2024

nezaniel Apr 27, 2024

nezaniel left a comment

nezaniel Apr 27, 2024

mhsdesign commented Apr 27, 2024

mhsdesign Apr 27, 2024

bwaidelich Apr 27, 2024 •

edited

mhsdesign Apr 27, 2024

bwaidelich Apr 27, 2024 •

edited

bwaidelich Apr 27, 2024 •

edited

mhsdesign Apr 29, 2024

bwaidelich Apr 29, 2024

bwaidelich left a comment

bwaidelich Apr 27, 2024 •

edited

bwaidelich Apr 27, 2024

bwaidelich Apr 27, 2024

Name	correlation id	metadata
NodeWasCreated	123	`{structureAdjustment: 'Content Stream: %s; Dimension Space Point: %s, Node Aggregate: %s --- The tethered child node "bar" is missing.'}`
SomethingLol	123

BUGFIX: Fix and test tethered node structure adjustment #4969

Are you sure you want to change the base?

BUGFIX: Fix and test tethered node structure adjustment #4969

Conversation

mhsdesign commented Mar 30, 2024 • edited

mhsdesign commented Mar 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nezaniel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsdesign commented Apr 27, 2024

Choose a reason for hiding this comment

bwaidelich Apr 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwaidelich Apr 27, 2024 • edited

Choose a reason for hiding this comment

bwaidelich Apr 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwaidelich left a comment

Choose a reason for hiding this comment

bwaidelich Apr 27, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsdesign commented Mar 30, 2024 •

edited

bwaidelich Apr 27, 2024 •

edited

bwaidelich Apr 27, 2024 •

edited

bwaidelich Apr 27, 2024 •

edited

bwaidelich Apr 27, 2024 •

edited