From 00b48955b1cc127099d476f97417435c584c4182 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Wed, 18 Dec 2024 14:41:31 +0100 Subject: [PATCH 1/5] TASK: Add note why we dont handle `WorkspaceWasPublished` --- .../Classes/AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php | 1 + 1 file changed, 1 insertion(+) diff --git a/Neos.Neos/Classes/AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php b/Neos.Neos/Classes/AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php index 6c9c7ab8c8..0341460e44 100644 --- a/Neos.Neos/Classes/AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php +++ b/Neos.Neos/Classes/AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php @@ -73,6 +73,7 @@ public function onAfterEvent(EventInterface $eventInstance, EventEnvelope $event } } + // Note that we don't need to update the index for WorkspaceWasPublished, as updateNode will be invoked already with the published node and then clean up its previous usages in nested workspaces match ($eventInstance::class) { NodeAggregateWithNodeWasCreated::class => $this->updateNode($eventInstance->getWorkspaceName(), $eventInstance->nodeAggregateId, $eventInstance->originDimensionSpacePoint->toDimensionSpacePoint()), NodePeerVariantWasCreated::class => $this->updateNode($eventInstance->getWorkspaceName(), $eventInstance->nodeAggregateId, $eventInstance->peerOrigin->toDimensionSpacePoint()), From 915138321e06a2e84e0ff25e7a29a72b61f5835a Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Wed, 18 Dec 2024 14:50:25 +0100 Subject: [PATCH 2/5] FEATURE: Indicate if a rebase had conflicts --- .../W6-WorkspaceRebasing/01-BasicFeatures.feature | 8 ++++++++ .../03-RebasingWithConflictingChanges.feature | 8 ++++++++ .../Classes/Feature/WorkspaceCommandHandler.php | 2 ++ .../Feature/WorkspaceRebase/Event/WorkspaceWasRebased.php | 6 ++++++ 4 files changed, 24 insertions(+) diff --git a/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/W6-WorkspaceRebasing/01-BasicFeatures.feature b/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/W6-WorkspaceRebasing/01-BasicFeatures.feature index e080cf4d6b..c72aa1f5c6 100644 --- a/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/W6-WorkspaceRebasing/01-BasicFeatures.feature +++ b/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/W6-WorkspaceRebasing/01-BasicFeatures.feature @@ -62,6 +62,14 @@ Feature: Rebasing with no conflict | rebaseErrorHandlingStrategy | "force" | Then I expect the content stream "user-cs-identifier" to not exist + Then I expect exactly 2 events to be published on stream with prefix "Workspace:user-test" + And event at index 1 is of type "WorkspaceWasRebased" with payload: + | Key | Expected | + | workspaceName | "user-test" | + | newContentStreamId | "user-cs-rebased" | + | previousContentStreamId | "user-cs-identifier" | + | hadConflicts | false | + When I am in workspace "user-test" and dimension space point {} Then I expect node aggregate identifier "sir-david-nodenborough" to lead to node user-cs-rebased;sir-david-nodenborough;{} diff --git a/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/W6-WorkspaceRebasing/03-RebasingWithConflictingChanges.feature b/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/W6-WorkspaceRebasing/03-RebasingWithConflictingChanges.feature index 7f30b4c68a..a12eb8316c 100644 --- a/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/W6-WorkspaceRebasing/03-RebasingWithConflictingChanges.feature +++ b/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/W6-WorkspaceRebasing/03-RebasingWithConflictingChanges.feature @@ -179,6 +179,14 @@ Feature: Workspace rebasing - conflicting changes | rebasedContentStreamId | "user-cs-identifier-rebased" | | rebaseErrorHandlingStrategy | "force" | + Then I expect exactly 2 events to be published on stream with prefix "Workspace:user-ws" + And event at index 1 is of type "WorkspaceWasRebased" with payload: + | Key | Expected | + | workspaceName | "user-ws" | + | newContentStreamId | "user-cs-identifier-rebased" | + | previousContentStreamId | "user-cs-identifier" | + | hadConflicts | true | + Then I expect the content stream "user-cs-identifier" to not exist Then I expect the content stream "user-cs-identifier-rebased" to exist diff --git a/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php b/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php index abb65c4c82..e7262f7b6e 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php +++ b/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php @@ -288,6 +288,7 @@ private function rebaseWorkspaceWithoutChanges( $workspace->workspaceName, $newContentStreamId, $workspace->currentContentStreamId, + hadConflicts: false ), ), ExpectedVersion::ANY() @@ -404,6 +405,7 @@ static function ($handle) use ($rebaseableCommands): void { $command->workspaceName, $command->rebasedContentStreamId, $workspace->currentContentStreamId, + hadConflicts: $commandSimulator->hasConflicts() ), ), ExpectedVersion::ANY() diff --git a/Neos.ContentRepository.Core/Classes/Feature/WorkspaceRebase/Event/WorkspaceWasRebased.php b/Neos.ContentRepository.Core/Classes/Feature/WorkspaceRebase/Event/WorkspaceWasRebased.php index 263f9e418c..4156c1c08b 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/WorkspaceRebase/Event/WorkspaceWasRebased.php +++ b/Neos.ContentRepository.Core/Classes/Feature/WorkspaceRebase/Event/WorkspaceWasRebased.php @@ -16,6 +16,7 @@ use Neos\ContentRepository\Core\EventStore\EventInterface; use Neos\ContentRepository\Core\Feature\Common\EmbedsWorkspaceName; +use Neos\ContentRepository\Core\Feature\WorkspaceRebase\Dto\RebaseErrorHandlingStrategy; use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId; use Neos\ContentRepository\Core\SharedModel\Workspace\WorkspaceName; @@ -34,6 +35,10 @@ public function __construct( * The old content stream ID (which is not active anymore now) */ public ContentStreamId $previousContentStreamId, + /** + * Indicates if all events in the workspace were kept or if failing changes were discarded {@see RebaseErrorHandlingStrategy::STRATEGY_FORCE} + */ + public bool $hadConflicts ) { } @@ -48,6 +53,7 @@ public static function fromArray(array $values): self WorkspaceName::fromString($values['workspaceName']), ContentStreamId::fromString($values['newContentStreamId']), ContentStreamId::fromString($values['previousContentStreamId']), + $values['hadConflicts'] ?? false ); } From e7d185c46fdcc785cb4f47b4739b86acaab5eb6c Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Wed, 18 Dec 2024 14:54:34 +0100 Subject: [PATCH 3/5] BUGFIX: Asset usage prevent orphaned asset usage entries in case of a rebase with conflicts --- .../Feature/WorkspaceRebase/Event/WorkspaceWasRebased.php | 2 +- .../AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/Neos.ContentRepository.Core/Classes/Feature/WorkspaceRebase/Event/WorkspaceWasRebased.php b/Neos.ContentRepository.Core/Classes/Feature/WorkspaceRebase/Event/WorkspaceWasRebased.php index 4156c1c08b..ade0ecd749 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/WorkspaceRebase/Event/WorkspaceWasRebased.php +++ b/Neos.ContentRepository.Core/Classes/Feature/WorkspaceRebase/Event/WorkspaceWasRebased.php @@ -36,7 +36,7 @@ public function __construct( */ public ContentStreamId $previousContentStreamId, /** - * Indicates if all events in the workspace were kept or if failing changes were discarded {@see RebaseErrorHandlingStrategy::STRATEGY_FORCE} + * Indicates if failing changes were discarded during a forced rebase {@see RebaseErrorHandlingStrategy::STRATEGY_FORCE} or if all events in the workspace were kept. */ public bool $hadConflicts ) { diff --git a/Neos.Neos/Classes/AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php b/Neos.Neos/Classes/AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php index 0341460e44..b7277445c6 100644 --- a/Neos.Neos/Classes/AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php +++ b/Neos.Neos/Classes/AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php @@ -16,6 +16,7 @@ use Neos\ContentRepository\Core\Feature\NodeVariation\Event\NodePeerVariantWasCreated; use Neos\ContentRepository\Core\Feature\NodeVariation\Event\NodeSpecializationVariantWasCreated; use Neos\ContentRepository\Core\Feature\WorkspacePublication\Event\WorkspaceWasDiscarded; +use Neos\ContentRepository\Core\Feature\WorkspaceRebase\Event\WorkspaceWasRebased; use Neos\ContentRepository\Core\Projection\CatchUpHook\CatchUpHookInterface; use Neos\ContentRepository\Core\Projection\ContentGraph\ContentGraphReadModelInterface; use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\FindDescendantNodesFilter; @@ -73,6 +74,12 @@ public function onAfterEvent(EventInterface $eventInstance, EventEnvelope $event } } + if ($eventInstance instanceof WorkspaceWasRebased && $eventInstance->hadConflicts) { + // because we don't know which changes were discarded in a conflict, we discard all changes and will build up the index on succeeding calls (with the kept reapplied events) + $this->discardWorkspace($eventInstance->getWorkspaceName()); + return; + } + // Note that we don't need to update the index for WorkspaceWasPublished, as updateNode will be invoked already with the published node and then clean up its previous usages in nested workspaces match ($eventInstance::class) { NodeAggregateWithNodeWasCreated::class => $this->updateNode($eventInstance->getWorkspaceName(), $eventInstance->nodeAggregateId, $eventInstance->originDimensionSpacePoint->toDimensionSpacePoint()), From 1c4f9416788404815fcdaaacc7bccd18ca206c54 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Thu, 9 Jan 2025 11:27:35 +0100 Subject: [PATCH 4/5] TASK: Rename to `hasSkippedEvents` and store originally conflicting sequence numbers in force-rebase event Storing the skipped sequence numbers should ease debugging rather than having 'just' a boolean indicator. The public api does not need to know about that thus the getter. --- .../01-BasicFeatures.feature | 2 +- .../03-RebasingWithConflictingChanges.feature | 2 +- .../Feature/WorkspaceCommandHandler.php | 6 +++-- .../WorkspaceRebase/ConflictingEvents.php | 10 ++++++++ .../Event/WorkspaceWasRebased.php | 24 +++++++++++++++---- .../CatchUpHook/AssetUsageCatchUpHook.php | 2 +- 6 files changed, 37 insertions(+), 9 deletions(-) diff --git a/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/W6-WorkspaceRebasing/01-BasicFeatures.feature b/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/W6-WorkspaceRebasing/01-BasicFeatures.feature index c72aa1f5c6..0e75f159f1 100644 --- a/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/W6-WorkspaceRebasing/01-BasicFeatures.feature +++ b/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/W6-WorkspaceRebasing/01-BasicFeatures.feature @@ -68,7 +68,7 @@ Feature: Rebasing with no conflict | workspaceName | "user-test" | | newContentStreamId | "user-cs-rebased" | | previousContentStreamId | "user-cs-identifier" | - | hadConflicts | false | + | skippedEvents | [] | When I am in workspace "user-test" and dimension space point {} Then I expect node aggregate identifier "sir-david-nodenborough" to lead to node user-cs-rebased;sir-david-nodenborough;{} diff --git a/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/W6-WorkspaceRebasing/03-RebasingWithConflictingChanges.feature b/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/W6-WorkspaceRebasing/03-RebasingWithConflictingChanges.feature index a12eb8316c..92cae60618 100644 --- a/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/W6-WorkspaceRebasing/03-RebasingWithConflictingChanges.feature +++ b/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/W6-WorkspaceRebasing/03-RebasingWithConflictingChanges.feature @@ -185,7 +185,7 @@ Feature: Workspace rebasing - conflicting changes | workspaceName | "user-ws" | | newContentStreamId | "user-cs-identifier-rebased" | | previousContentStreamId | "user-cs-identifier" | - | hadConflicts | true | + | skippedEvents | [12,14] | Then I expect the content stream "user-cs-identifier" to not exist Then I expect the content stream "user-cs-identifier-rebased" to exist diff --git a/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php b/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php index e7262f7b6e..354f1691e7 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php +++ b/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php @@ -49,6 +49,7 @@ use Neos\ContentRepository\Core\Feature\WorkspacePublication\Event\WorkspaceWasDiscarded; use Neos\ContentRepository\Core\Feature\WorkspacePublication\Event\WorkspaceWasPublished; use Neos\ContentRepository\Core\Feature\WorkspaceRebase\Command\RebaseWorkspace; +use Neos\ContentRepository\Core\Feature\WorkspaceRebase\ConflictingEvent; use Neos\ContentRepository\Core\Feature\WorkspaceRebase\Dto\RebaseErrorHandlingStrategy; use Neos\ContentRepository\Core\Feature\WorkspaceRebase\Event\WorkspaceWasRebased; use Neos\ContentRepository\Core\Feature\WorkspaceRebase\Exception\PartialWorkspaceRebaseFailed; @@ -288,7 +289,7 @@ private function rebaseWorkspaceWithoutChanges( $workspace->workspaceName, $newContentStreamId, $workspace->currentContentStreamId, - hadConflicts: false + skippedEvents: [] ), ), ExpectedVersion::ANY() @@ -405,7 +406,8 @@ static function ($handle) use ($rebaseableCommands): void { $command->workspaceName, $command->rebasedContentStreamId, $workspace->currentContentStreamId, - hadConflicts: $commandSimulator->hasConflicts() + skippedEvents: $commandSimulator->getConflictingEvents() + ->map(fn (ConflictingEvent $conflictingEvent) => $conflictingEvent->getSequenceNumber()) ), ), ExpectedVersion::ANY() diff --git a/Neos.ContentRepository.Core/Classes/Feature/WorkspaceRebase/ConflictingEvents.php b/Neos.ContentRepository.Core/Classes/Feature/WorkspaceRebase/ConflictingEvents.php index 1b30aa6100..d60be60acb 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/WorkspaceRebase/ConflictingEvents.php +++ b/Neos.ContentRepository.Core/Classes/Feature/WorkspaceRebase/ConflictingEvents.php @@ -55,4 +55,14 @@ public function count(): int { return count($this->items); } + + /** + * @template T + * @param \Closure(ConflictingEvent): T $callback + * @return list + */ + public function map(\Closure $callback): array + { + return array_map($callback, $this->items); + } } diff --git a/Neos.ContentRepository.Core/Classes/Feature/WorkspaceRebase/Event/WorkspaceWasRebased.php b/Neos.ContentRepository.Core/Classes/Feature/WorkspaceRebase/Event/WorkspaceWasRebased.php index ade0ecd749..c38d7ab364 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/WorkspaceRebase/Event/WorkspaceWasRebased.php +++ b/Neos.ContentRepository.Core/Classes/Feature/WorkspaceRebase/Event/WorkspaceWasRebased.php @@ -19,6 +19,7 @@ use Neos\ContentRepository\Core\Feature\WorkspaceRebase\Dto\RebaseErrorHandlingStrategy; use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId; use Neos\ContentRepository\Core\SharedModel\Workspace\WorkspaceName; +use Neos\EventStore\Model\Event\SequenceNumber; /** * @api events are the persistence-API of the content repository @@ -36,9 +37,10 @@ public function __construct( */ public ContentStreamId $previousContentStreamId, /** - * Indicates if failing changes were discarded during a forced rebase {@see RebaseErrorHandlingStrategy::STRATEGY_FORCE} or if all events in the workspace were kept. + * @var list + * @internal actually conflicting event's sequence numbers: only for debugging & testing please use {@see hasSkippedEvents()} which is API instead. */ - public bool $hadConflicts + public array $skippedEvents ) { } @@ -47,18 +49,32 @@ public function getWorkspaceName(): WorkspaceName return $this->workspaceName; } + /** + * Indicates if failing changes were discarded during a forced rebase {@see RebaseErrorHandlingStrategy::STRATEGY_FORCE} or if all events in the workspace were kept. + */ + public function hasSkippedEvents(): bool + { + return $this->skippedEvents !== []; + } + public static function fromArray(array $values): self { return new self( WorkspaceName::fromString($values['workspaceName']), ContentStreamId::fromString($values['newContentStreamId']), ContentStreamId::fromString($values['previousContentStreamId']), - $values['hadConflicts'] ?? false + array_map(SequenceNumber::fromInteger(...), $values['skippedEvents'] ?? []) ); } public function jsonSerialize(): array { - return get_object_vars($this); + return [ + 'workspaceName' => $this->workspaceName, + 'newContentStreamId' => $this->newContentStreamId, + 'previousContentStreamId' => $this->previousContentStreamId, + // todo SequenceNumber is NOT jsonSerializeAble!!! + 'skippedEvents' => array_map(fn (SequenceNumber $sequenceNumber) => $sequenceNumber->value, $this->skippedEvents) + ]; } } diff --git a/Neos.Neos/Classes/AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php b/Neos.Neos/Classes/AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php index b7277445c6..61a115440b 100644 --- a/Neos.Neos/Classes/AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php +++ b/Neos.Neos/Classes/AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php @@ -74,7 +74,7 @@ public function onAfterEvent(EventInterface $eventInstance, EventEnvelope $event } } - if ($eventInstance instanceof WorkspaceWasRebased && $eventInstance->hadConflicts) { + if ($eventInstance instanceof WorkspaceWasRebased && $eventInstance->hasSkippedEvents()) { // because we don't know which changes were discarded in a conflict, we discard all changes and will build up the index on succeeding calls (with the kept reapplied events) $this->discardWorkspace($eventInstance->getWorkspaceName()); return; From 19cb5c2132468110f7670aae3e953ad175ac4652 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Thu, 9 Jan 2025 11:36:11 +0100 Subject: [PATCH 5/5] TASK: Inline condition into match call --- .../AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/Neos.Neos/Classes/AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php b/Neos.Neos/Classes/AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php index 61a115440b..e84c82b9bf 100644 --- a/Neos.Neos/Classes/AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php +++ b/Neos.Neos/Classes/AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php @@ -74,12 +74,6 @@ public function onAfterEvent(EventInterface $eventInstance, EventEnvelope $event } } - if ($eventInstance instanceof WorkspaceWasRebased && $eventInstance->hasSkippedEvents()) { - // because we don't know which changes were discarded in a conflict, we discard all changes and will build up the index on succeeding calls (with the kept reapplied events) - $this->discardWorkspace($eventInstance->getWorkspaceName()); - return; - } - // Note that we don't need to update the index for WorkspaceWasPublished, as updateNode will be invoked already with the published node and then clean up its previous usages in nested workspaces match ($eventInstance::class) { NodeAggregateWithNodeWasCreated::class => $this->updateNode($eventInstance->getWorkspaceName(), $eventInstance->nodeAggregateId, $eventInstance->originDimensionSpacePoint->toDimensionSpacePoint()), @@ -89,6 +83,8 @@ public function onAfterEvent(EventInterface $eventInstance, EventEnvelope $event NodePropertiesWereSet::class => $this->updateNode($eventInstance->getWorkspaceName(), $eventInstance->nodeAggregateId, $eventInstance->originDimensionSpacePoint->toDimensionSpacePoint()), WorkspaceWasDiscarded::class => $this->discardWorkspace($eventInstance->getWorkspaceName()), DimensionSpacePointWasMoved::class => $this->updateDimensionSpacePoint($eventInstance->getWorkspaceName(), $eventInstance->source, $eventInstance->target), + // because we don't know which changes were discarded in a conflict, we discard all changes and will build up the index on succeeding calls (with the kept reapplied events) + WorkspaceWasRebased::class => $eventInstance->hasSkippedEvents() && $this->discardWorkspace($eventInstance->getWorkspaceName()), default => null }; }