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.

Sign up for GitHub

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

Jump to bottom

BUGFIX: Fix and test tethered node structure adjustment #4969

Draft
wants to merge 7 commits into
base: 9.0
Choose a base branch
from
Draft

BUGFIX: Fix and test tethered node structure adjustment #4969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
71ed856
TASK: #3887 set causationId and meta data for structure adjustments
mhsdesign Mar 30, 2024
bf4ed47
TASK: Add test for `createEventsForMissingTetheredNode`
mhsdesign Mar 30, 2024
d1af19b
vary the correct node when adding missing tethered children
Mar 30, 2024
3abb86e
TASK: Adjust test to contain exact event expectations
mhsdesign Mar 31, 2024
b3d6f7e
TASK: Prefer working with occupiedDsp instead arbitrary node from Nod…
mhsdesign Mar 31, 2024
14e9e63
TASK: Make phpstan happy (as easter present)
mhsdesign Mar 31, 2024
07f429a
TASK: Adjust tests to `peerSucceedingSiblings` change
mhsdesign May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 35 additions & 4 deletions ...ository.BehavioralTests/Tests/Behavior/Features/StructureAdjustment/TetheredNodes.feature
View file
Open in desktop
Expand Up @@ -39,10 +39,10 @@ Feature: Tethered Nodes integrity violations
And the graph projection is fully up to date
And I am in workspace "live"
And the command CreateRootNodeAggregateWithNode is executed with payload:
| Key | Value |
| nodeAggregateId | "lady-eleonode-rootford" |
| nodeTypeName | "Neos.ContentRepository:Root" |
| tetheredDescendantNodeAggregateIds | {"originally-tethered-node": "originode-tetherton"} |
| Key | Value |
| nodeAggregateId | "lady-eleonode-rootford" |
| nodeTypeName | "Neos.ContentRepository:Root" |
| tetheredDescendantNodeAggregateIds | {"originally-tethered-node": "originode-tetherton", "originally-tethered-node/tethered-leaf": "originode-tetherton-leaf"} |
# We have to add another node since root nodes have no dimension space points and thus cannot be varied
# Node /document
And the event NodeAggregateWithNodeWasCreated was published with payload:
Expand Down Expand Up @@ -229,3 +229,34 @@ Feature: Tethered Nodes integrity violations
| Type | nodeAggregateId |
| TETHERED_NODE_TYPE_WRONG | nodewyn-tetherton |

Scenario: Tethered child node is missing in new dimension space point
We specialize/generalize/... the other tethered Node
When I change the content dimensions in content repository "default" to:
| Identifier | Values | Generalizations |
| market | DE, CH | CH->DE |
| language | en, de, gsw, new | gsw->de->en, new |
And the command UpdateRootNodeAggregateDimensions is executed with payload:
| Key | Value |
| nodeAggregateId | "lady-eleonode-rootford" |
And the graph projection is fully up to date
And I expect exactly 8 events to be published on stream "ContentStream:cs-identifier"

Then I expect the following structure adjustments for type "Neos.ContentRepository:Root":
| Type | nodeAggregateId |
| TETHERED_NODE_MISSING | lady-eleonode-rootford |
When I adjust the node structure for node type "Neos.ContentRepository:Root"
Then I expect exactly 10 events to be published on stream "ContentStream:cs-identifier"
And event at index 8 is of type "NodePeerVariantWasCreated" with payload:
| Key | Expected |
| contentStreamId | "cs-identifier" |
| nodeAggregateId | "originode-tetherton" |
| sourceOrigin | {"market":"DE","language":"en"} |
| peerOrigin | {"market":"DE","language":"new"} |
| peerSucceedingSiblings | [{"dimensionSpacePoint":{"market":"DE","language":"new"},"nodeAggregateId":null},{"dimensionSpacePoint":{"market":"CH","language":"new"},"nodeAggregateId":null}] |
And event at index 9 is of type "NodePeerVariantWasCreated" with payload:
| Key | Expected |
| contentStreamId | "cs-identifier" |
| nodeAggregateId | "originode-tetherton-leaf" |
| sourceOrigin | {"market":"DE","language":"en"} |
| peerOrigin | {"market":"DE","language":"new"} |
| peerSucceedingSiblings | [{"dimensionSpacePoint":{"market":"DE","language":"new"},"nodeAggregateId":null},{"dimensionSpacePoint":{"market":"CH","language":"new"},"nodeAggregateId":null}] |
29 changes: 29 additions & 0 deletions Neos.ContentRepository.Core/Classes/EventStore/EventsToPublish.php
View file
Open in desktop
Expand Up @@ -7,6 +7,8 @@
use Neos\ContentRepository\Core\CommandHandler\CommandHandlerInterface;
use Neos\ContentRepository\Core\ContentRepository;
use Neos\EventStore\EventStoreInterface;
use Neos\EventStore\Model\Event;
use Neos\EventStore\Model\Event\EventMetadata;
use Neos\EventStore\Model\Event\StreamName;
use Neos\EventStore\Model\EventStream\ExpectedVersion;

Expand All @@ -33,4 +35,31 @@ public static function empty(): self
ExpectedVersion::ANY()
);
}

public function withCausationOfFirstEventAndAdditionalMetaData(EventMetadata $metadata): self
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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;

{
/** @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
);
Comment on lines +41 to +63
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
);

}
}
19 changes: 9 additions & 10 deletions Neos.ContentRepository.Core/Classes/Feature/Common/TetheredNodeInternals.php
View file
Open in desktop
Expand Up @@ -125,23 +125,22 @@ protected function createEventsForMissingTetheredNode(
} else {
if (!$childNodeAggregate->classification->isTethered()) {
throw new \RuntimeException(
'We found a child node aggregate through the given node path; but it is not tethered.'
'TODO: We found a child node aggregate through the given node path; but it is not tethered.'
. ' We do not support re-tethering yet'
. ' (as this case should happen very rarely as far as we think).'
. ' (as this case should happen very rarely as far as we think).',
1711897665
);
}

$childNodeSource = null;
foreach ($childNodeAggregate->getNodes() as $node) {
$childNodeSource = $node;
break;
}
/** @var Node $childNodeSource Node aggregates are never empty */
$occupiedDimensionSpacePoints = $childNodeAggregate->occupiedDimensionSpacePoints->getPoints();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assert($occupiedDimensionSpacePoints !== []);
$arbitraryOccupiedDimensionSpacePoint = array_shift($occupiedDimensionSpacePoints);

return $this->createEventsForVariations(
$contentGraph,
$childNodeSource->originDimensionSpacePoint,
$arbitraryOccupiedDimensionSpacePoint,
$originDimensionSpacePoint,
$parentNodeAggregate
$childNodeAggregate
);
}
}
Expand Down
10 changes: 9 additions & 1 deletion Neos.ContentRepository.StructureAdjustment/src/StructureAdjustmentService.php
View file
Open in desktop
Expand Up @@ -20,6 +20,7 @@
use Neos\ContentRepository\StructureAdjustment\Adjustment\StructureAdjustment;
use Neos\ContentRepository\StructureAdjustment\Adjustment\TetheredNodeAdjustments;
use Neos\ContentRepository\StructureAdjustment\Adjustment\UnknownNodeTypeAdjustment;
use Neos\EventStore\Model\Event\EventMetadata;

class StructureAdjustmentService implements ContentRepositoryServiceInterface
{
Expand Down Expand Up @@ -95,7 +96,14 @@ public function fixError(StructureAdjustment $adjustment): void
$remediation = $adjustment->remediation;
$eventsToPublish = $remediation();
assert($eventsToPublish instanceof EventsToPublish);
$this->eventPersister->publishEvents($eventsToPublish)->block();

$enrichedEventsToPublish = $eventsToPublish->withCausationOfFirstEventAndAdditionalMetaData(
EventMetadata::fromArray([
'structureAdjustment' => mb_strimwidth($adjustment->render() , 0, 250, '…')
])
);
Comment on lines +100 to +104
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());


$this->eventPersister->publishEvents($enrichedEventsToPublish);
}
}
}