-
-
Notifications
You must be signed in to change notification settings - Fork 221
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
BUGFIX: Fix and test tethered node structure adjustment #4969
base: 9.0
Are you sure you want to change the base?
Changes from all commits
71ed856
bf4ed47
d1af19b
3abb86e
b3d6f7e
14e9e63
07f429a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
|
@@ -33,4 +35,31 @@ public static function empty(): self | |
ExpectedVersion::ANY() | ||
); | ||
} | ||
|
||
public function withCausationOfFirstEventAndAdditionalMetaData(EventMetadata $metadata): self | ||
{ | ||
/** @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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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,
); |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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);
} There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 | ||||||||||||||
{ | ||||||||||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO this should look something like:
Suggested change
|
||||||||||||||
|
||||||||||||||
$this->eventPersister->publishEvents($enrichedEventsToPublish); | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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))
There was a problem hiding this comment.
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:
{structureAdjustment: 'Content Stream: %s; Dimension Space Point: %s, Node Aggregate: %s --- The tethered child node "bar" is missing.'}
at first i tried to insert the rendered structure adjustment message in there but that wont work because its to long and special chars.
There was a problem hiding this comment.
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
and then in the calling side below do sth. along the lines of
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.
There was a problem hiding this comment.
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 withinnevermind, in this case that's not possibleContentRepository::handle()
with #3887There was a problem hiding this comment.
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 ;)There was a problem hiding this comment.
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
andEventMetadata
is not easy to be used.. We could extend that (in theneos/eventstore
package) a bit so that we could doso that we could simplify the above to sth like: