From 8936a973010bde24ceff21fdee084b25e50cc227 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Thu, 31 Oct 2024 16:08:07 +0100 Subject: [PATCH 1/3] TASK: Improve pruning test to assert a replay actually works --- .../Workspaces/PruneContentStreams.feature | 82 +++++++++++++++++-- 1 file changed, 75 insertions(+), 7 deletions(-) diff --git a/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/Workspaces/PruneContentStreams.feature b/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/Workspaces/PruneContentStreams.feature index c64ec997a14..b1930a667c6 100644 --- a/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/Workspaces/PruneContentStreams.feature +++ b/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/Workspaces/PruneContentStreams.feature @@ -6,6 +6,10 @@ Feature: If content streams are not in use anymore by the workspace, they can be Given using no content dimensions And using the following node types: """yaml + 'Neos.ContentRepository.Testing:Content': + properties: + text: + type: string """ And using identifier "default", I define a content repository And I am in content repository "default" @@ -118,24 +122,47 @@ Feature: If content streams are not in use anymore by the workspace, they can be | workspaceName | "review" | | baseWorkspaceName | "live" | | newContentStreamId | "review-cs-identifier" | + When the command CreateNodeAggregateWithNode is executed with payload: + | Key | Value | + | workspaceName | "review" | + | nodeAggregateId | "nody-mc-nodeface" | + | nodeTypeName | "Neos.ContentRepository.Testing:Content" | + | originDimensionSpacePoint | {} | + | parentNodeAggregateId | "root-node" | + | initialPropertyValues | {"text": "Review Initial"} | And the command CreateWorkspace is executed with payload: | Key | Value | | workspaceName | "user-test" | | baseWorkspaceName | "review" | | newContentStreamId | "user-cs-identifier" | + When the command SetNodeProperties is executed with payload: + | Key | Value | + | workspaceName | "review" | + | nodeAggregateId | "nody-mc-nodeface" | + | propertyValues | {"text": "Review Edited"} | + + When the command CreateNodeAggregateWithNode is executed with payload: + | Key | Value | + | workspaceName | "live" | + | nodeAggregateId | "nodimus-secondus" | + | nodeTypeName | "Neos.ContentRepository.Testing:Content" | + | originDimensionSpacePoint | {} | + | parentNodeAggregateId | "root-node" | + | initialPropertyValues | {"text": "Live WS"} | + + Then workspaces user-test,review have status OUTDATED + # now, we rebase the "review" workspace, effectively marking the "review-cs-identifier" content stream as no longer in use. # however, we are not allowed to drop the content stream from the event store yet, because the "user-cs-identifier" is based # on the (no-longer-in-direct-use) review-cs-identifier. When the command RebaseWorkspace is executed with payload: - | Key | Value | - | workspaceName | "review" | - | rebaseErrorHandlingStrategy | "force" | + | Key | Value | + | workspaceName | "review" | + | rebasedContentStreamId | "review-cs-rebased" | - And I prune removed content streams from the event stream - - # the events should still exist - Then I expect exactly 3 events to be published on stream "ContentStream:review-cs-identifier" + Then workspace review has status UP_TO_DATE + Then workspace user-test has status OUTDATED Then I expect the content stream pruner status output: """ @@ -143,3 +170,44 @@ Feature: If content streams are not in use anymore by the workspace, they can be Okay. No pruneable streams in the event stream """ + + And I prune removed content streams from the event stream + + # the events should still exist but not the projected content stream + Then I expect the content stream "review-cs-identifier" to not exist + Then I expect exactly 5 events to be published on stream "ContentStream:review-cs-identifier" + + And I replay the content graph + + Then workspaces review has status UP_TO_DATE + Then workspaces user-test has status OUTDATED + + When I am in workspace "user-test" and dimension space point {} + Then I expect node aggregate identifier "nody-mc-nodeface" to lead to node user-cs-identifier;nody-mc-nodeface;{} + And I expect this node to have the following properties: + | Key | Value | + | text | "Review Initial" | + # because we didnt rebase the user workspace and are based on the old review ws, the live node doesnt exist here + Then I expect node aggregate identifier "nodimus-secondus" to lead to no node + + When I am in workspace "review" and dimension space point {} + Then I expect node aggregate identifier "nody-mc-nodeface" to lead to node review-cs-rebased;nody-mc-nodeface;{} + And I expect this node to have the following properties: + | Key | Value | + | text | "Review Edited" | + Then I expect node aggregate identifier "nodimus-secondus" to lead to node review-cs-rebased;nodimus-secondus;{} + And I expect this node to have the following properties: + | Key | Value | + | text | "Live WS" | + + # content stream is still writeable + When the command SetNodeProperties is executed with payload: + | Key | Value | + | workspaceName | "review" | + | nodeAggregateId | "nody-mc-nodeface" | + | propertyValues | {"text": "Review after replay"} | + When I am in workspace "review" and dimension space point {} + Then I expect node aggregate identifier "nody-mc-nodeface" to lead to node review-cs-rebased;nody-mc-nodeface;{} + And I expect this node to have the following properties: + | Key | Value | + | text | "Review after replay" | \ No newline at end of file From b85904c815ae8e3cc4f3bb79e0e0fec8a0da06e8 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Thu, 31 Oct 2024 17:51:25 +0100 Subject: [PATCH 2/3] TASK: Fully document and test behaviour that `findWorkspaceByName` is null after prune and partial replay see https://neos-project.slack.com/archives/C04PYL8H3/p1730383249323669 --- .../src/ContentGraphReadModelAdapter.php | 7 ++- .../Workspaces/PruneContentStreams.feature | 41 ++++++++++++++++- .../Feature/WorkspaceCommandHandler.php | 15 ------ .../Features/Bootstrap/CRTestSuiteTrait.php | 46 ++++++++++++++++++- ...ricCommandExecutionAndEventPublication.php | 14 ++++++ 5 files changed, 104 insertions(+), 19 deletions(-) diff --git a/Neos.ContentGraph.DoctrineDbalAdapter/src/ContentGraphReadModelAdapter.php b/Neos.ContentGraph.DoctrineDbalAdapter/src/ContentGraphReadModelAdapter.php index 7e5e2051a5f..c6efb6f3ae7 100644 --- a/Neos.ContentGraph.DoctrineDbalAdapter/src/ContentGraphReadModelAdapter.php +++ b/Neos.ContentGraph.DoctrineDbalAdapter/src/ContentGraphReadModelAdapter.php @@ -161,6 +161,7 @@ private function getBasicWorkspaceQuery(): QueryBuilder return $queryBuilder ->select('ws.name, ws.baseWorkspaceName, ws.currentContentStreamId, cs.hasChanges, cs.sourceContentStreamVersion = scs.version as upToDateWithBase') ->from($this->tableNames->workspace(), 'ws') + // through this join we enforce that the `currentContentStreamId` actually exists in the content stream table ->join('ws', $this->tableNames->contentStream(), 'cs', 'cs.id = ws.currentcontentstreamid') ->leftJoin('cs', $this->tableNames->contentStream(), 'scs', 'scs.id = cs.sourceContentStreamId'); } @@ -170,6 +171,8 @@ private function getBasicWorkspaceQuery(): QueryBuilder */ private static function workspaceFromDatabaseRow(array $row): Workspace { + $name = WorkspaceName::fromString($row['name']); + $currentContentStreamId = ContentStreamId::fromString($row['currentContentStreamId']); $baseWorkspaceName = $row['baseWorkspaceName'] !== null ? WorkspaceName::fromString($row['baseWorkspaceName']) : null; if ($baseWorkspaceName === null) { @@ -184,9 +187,9 @@ private static function workspaceFromDatabaseRow(array $row): Workspace } return Workspace::create( - WorkspaceName::fromString($row['name']), + $name, $baseWorkspaceName, - ContentStreamId::fromString($row['currentContentStreamId']), + $currentContentStreamId, $status, $baseWorkspaceName === null ? false diff --git a/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/Workspaces/PruneContentStreams.feature b/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/Workspaces/PruneContentStreams.feature index b1930a667c6..69bf8f3f8ac 100644 --- a/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/Workspaces/PruneContentStreams.feature +++ b/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/Workspaces/PruneContentStreams.feature @@ -210,4 +210,43 @@ Feature: If content streams are not in use anymore by the workspace, they can be Then I expect node aggregate identifier "nody-mc-nodeface" to lead to node review-cs-rebased;nody-mc-nodeface;{} And I expect this node to have the following properties: | Key | Value | - | text | "Review after replay" | \ No newline at end of file + | text | "Review after replay" | + + Scenario: Pruning removed content streams and replaying will lead to workspaces without content stream (and the workspace not fetch able) + When the command CreateWorkspace is executed with payload: + | Key | Value | + | workspaceName | "user-test" | + | baseWorkspaceName | "live" | + | newContentStreamId | "user-cs-identifier" | + + When the command RebaseWorkspace is executed with payload: + | Key | Value | + | workspaceName | "user-test" | + | rebasedContentStreamId | "user-cs-identifier-rebased" | + | rebaseErrorHandlingStrategy | "force" | + + Then I expect the content stream "user-cs-identifier" to not exist + And I prune removed content streams from the event stream + Then I expect exactly 0 events to be published on stream "ContentStream:user-cs-identifier" + + Then I expect the highest sequence number to be 8 + # replay before the rebase, when the workspaces content stream does not exist + And I replay the content graph until 7 + + Then I expect the workspace "user-test" to not exist + Then I expect the following workspaces to exist: + | name | base | status | content stream | publishable changes | + | "live" | null | "UP_TO_DATE" | "cs-identifier" | false | + + When I am in workspace "user-test" and dimension space point {} + # FIXME maybe getContentGraph should already throw an exception if the content stream does not exist? + Then I expect node aggregate identifier "root-node" to lead to no node + + And I replay the content graph + Then I expect the following workspaces to exist: + | name | base | status | content stream | publishable changes | + | "live" | null | "UP_TO_DATE" | "cs-identifier" | false | + | "user-test" | "live" | "UP_TO_DATE" | "user-cs-identifier-rebased" | false | + + When I am in workspace "user-test" and dimension space point {} + Then I expect node aggregate identifier "root-node" to lead to node user-cs-identifier-rebased;root-node;{} diff --git a/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php b/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php index 20143536272..92f8ac9e7ba 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php +++ b/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php @@ -183,10 +183,6 @@ private function handlePublishWorkspace( // no-op return; } - - if (!$commandHandlingDependencies->contentStreamExists($workspace->currentContentStreamId)) { - throw new \RuntimeException('Cannot publish nodes on a workspace with a stateless content stream', 1729711258); - } $this->requireContentStreamToNotBeClosed($baseWorkspace->currentContentStreamId, $commandHandlingDependencies); $baseContentStreamVersion = $commandHandlingDependencies->getContentStreamVersion($baseWorkspace->currentContentStreamId); @@ -336,9 +332,6 @@ private function handleRebaseWorkspace( ): \Generator { $workspace = $this->requireWorkspace($command->workspaceName, $commandHandlingDependencies); $baseWorkspace = $this->requireBaseWorkspace($workspace, $commandHandlingDependencies); - if (!$commandHandlingDependencies->contentStreamExists($workspace->currentContentStreamId)) { - throw new \RuntimeException('Cannot rebase a workspace with a stateless content stream', 1711718314); - } if ( $workspace->status === WorkspaceStatus::UP_TO_DATE @@ -440,10 +433,6 @@ private function handlePublishIndividualNodesFromWorkspace( return; } - // todo check that fetching workspace throws if there is no content stream id for it - if (!$commandHandlingDependencies->contentStreamExists($workspace->currentContentStreamId)) { - throw new \RuntimeException('Cannot publish nodes on a workspace with a stateless content stream', 1710410114); - } $this->requireContentStreamToNotBeClosed($baseWorkspace->currentContentStreamId, $commandHandlingDependencies); $baseContentStreamVersion = $commandHandlingDependencies->getContentStreamVersion($baseWorkspace->currentContentStreamId); @@ -576,10 +565,6 @@ private function handleDiscardIndividualNodesFromWorkspace( return; } - if (!$commandHandlingDependencies->contentStreamExists($workspace->currentContentStreamId)) { - throw new \RuntimeException('Cannot discard nodes on a workspace with a stateless content stream', 1710408112); - } - yield $this->closeContentStream( $workspace->currentContentStreamId, $commandHandlingDependencies diff --git a/Neos.ContentRepository.TestSuite/Classes/Behavior/Features/Bootstrap/CRTestSuiteTrait.php b/Neos.ContentRepository.TestSuite/Classes/Behavior/Features/Bootstrap/CRTestSuiteTrait.php index 18fddc56f68..ffc643bf985 100644 --- a/Neos.ContentRepository.TestSuite/Classes/Behavior/Features/Bootstrap/CRTestSuiteTrait.php +++ b/Neos.ContentRepository.TestSuite/Classes/Behavior/Features/Bootstrap/CRTestSuiteTrait.php @@ -17,6 +17,7 @@ use Behat\Behat\Hook\Scope\BeforeScenarioScope; use Behat\Gherkin\Node\PyStringNode; use Behat\Gherkin\Node\TableNode; +use Neos\ContentGraph\DoctrineDbalAdapter\DoctrineDbalContentGraphProjection; use Neos\ContentRepository\Core\Factory\ContentRepositoryServiceFactoryDependencies; use Neos\ContentRepository\Core\Factory\ContentRepositoryServiceFactoryInterface; use Neos\ContentRepository\Core\Factory\ContentRepositoryServiceInterface; @@ -139,12 +140,40 @@ public function iExpectTheContentStreamToExist(string $rawContentStreamId): void /** * @Then /^I expect the content stream "([^"]*)" to not exist$/ */ - public function iExpectTheContentStreamToNotExist(string $rawContentStreamId, string $not = ''): void + public function iExpectTheContentStreamToNotExist(string $rawContentStreamId): void { $contentStream = $this->currentContentRepository->findContentStreamById(ContentStreamId::fromString($rawContentStreamId)); Assert::assertNull($contentStream, sprintf('Content stream "%s" was not expected to exist, but it does', $rawContentStreamId)); } + /** + * @Then /^I expect the workspace "([^"]*)" to not exist$/ + */ + public function iExpectTheWorkspaceToNotExist(string $rawWorkspaceName): void + { + $workspaceByName = $this->currentContentRepository->findWorkspaceByName(WorkspaceName::fromString($rawWorkspaceName)); + Assert::assertNull($workspaceByName, sprintf('Workspace "%s" was not expected to exist, but it does', $rawWorkspaceName)); + } + + /** + * @Then I expect the following workspaces to exist: + */ + public function iExpectTheFollowingWorkspaces(TableNode $payloadTable): void + { + $actualComparableHash = []; + $workspaces = $this->currentContentRepository->findWorkspaces(); + foreach ($workspaces as $workspace) { + $actualComparableHash[] = array_map(json_encode(...), [ + 'name' => $workspace->workspaceName, + 'base' => $workspace->baseWorkspaceName, + 'status' => $workspace->status, + 'content stream' => $workspace->currentContentStreamId, + 'publishable changes' => false, // todo https://github.com/neos/neos-development-collection/pull/5332 + ]); + } + Assert::assertSame($payloadTable->getHash(), $actualComparableHash); + } + /** * @Then /^workspace(?:s)? ([^"]*) ha(?:s|ve) status ([^"]*)$/ */ @@ -285,6 +314,21 @@ public function iReplayTheProjection(string $projectionName): void $this->currentContentRepository->catchUpProjection($projectionName, CatchUpOptions::create()); } + /** + * @When I replay the content graph + * @When I replay the content graph until :maximumSequenceNumber + */ + public function iReplayTheContentGraphProjection(?int $maximumSequenceNumber = null): void + { + // fixme allow to specify `ContentGraphProjectionInterface` here instead of adapter class + $this->currentContentRepository->resetProjectionState(DoctrineDbalContentGraphProjection::class); + $catchupOptions = CatchUpOptions::create(); + if ($maximumSequenceNumber) { + $catchupOptions = $catchupOptions->with(maximumSequenceNumber: $maximumSequenceNumber); + } + $this->currentContentRepository->catchUpProjection(DoctrineDbalContentGraphProjection::class, $catchupOptions); + } + protected function deserializeProperties(array $properties): PropertyValuesToWrite { return PropertyValuesToWrite::fromArray( diff --git a/Neos.ContentRepository.TestSuite/Classes/Behavior/Features/Bootstrap/GenericCommandExecutionAndEventPublication.php b/Neos.ContentRepository.TestSuite/Classes/Behavior/Features/Bootstrap/GenericCommandExecutionAndEventPublication.php index ffaf8f77780..2355e215c3c 100644 --- a/Neos.ContentRepository.TestSuite/Classes/Behavior/Features/Bootstrap/GenericCommandExecutionAndEventPublication.php +++ b/Neos.ContentRepository.TestSuite/Classes/Behavior/Features/Bootstrap/GenericCommandExecutionAndEventPublication.php @@ -213,6 +213,20 @@ public function iExpectExactlyEventToBePublishedOnStreamWithPrefix(int $numberOf Assert::assertEquals($numberOfEvents, count($this->currentEventStreamAsArray), 'Number of events did not match'); } + + /** + * @Then /^I expect the highest sequence number to be (\d+)$/ + * @param int $numberOfEvents + * @param string $streamName + */ + public function iExpectTheHighestSequnceNumberToBe(int $highestSequenceNumber) + { + $streamName = VirtualStreamName::all(); + $stream = iterator_to_array($this->getEventStore()->load($streamName)->backwards()->limit(1), false); + Assert::assertEquals($highestSequenceNumber, ($stream[0] ?? null)?->sequenceNumber->value, 'Sequence number did not match'); + } + + /** * @Then /^event at index (\d+) is of type "([^"]*)" with payload:/ * @param int $eventNumber From 1d98e054768ca296b1242a3fa4956ef752d7a7e8 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Thu, 31 Oct 2024 17:53:09 +0100 Subject: [PATCH 3/3] TASK: Simplify `requireContentStream` by removing one sql query --- .../Classes/CommandHandler/CommandHandlingDependencies.php | 6 +++++- .../Classes/Feature/Common/ConstraintChecks.php | 7 ------- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/Neos.ContentRepository.Core/Classes/CommandHandler/CommandHandlingDependencies.php b/Neos.ContentRepository.Core/Classes/CommandHandler/CommandHandlingDependencies.php index 9e726cacea1..5d46265127a 100644 --- a/Neos.ContentRepository.Core/Classes/CommandHandler/CommandHandlingDependencies.php +++ b/Neos.ContentRepository.Core/Classes/CommandHandler/CommandHandlingDependencies.php @@ -16,6 +16,7 @@ use Neos\ContentRepository\Core\Projection\ContentGraph\ContentGraphInterface; use Neos\ContentRepository\Core\Projection\ContentGraph\ContentGraphReadModelInterface; +use Neos\ContentRepository\Core\SharedModel\Exception\ContentStreamDoesNotExistYet; use Neos\ContentRepository\Core\SharedModel\Exception\WorkspaceDoesNotExist; use Neos\ContentRepository\Core\SharedModel\Workspace\ContentStreamId; use Neos\ContentRepository\Core\SharedModel\Workspace\Workspace; @@ -52,7 +53,10 @@ public function isContentStreamClosed(ContentStreamId $contentStreamId): bool { $contentStream = $this->contentGraphReadModel->findContentStreamById($contentStreamId); if ($contentStream === null) { - throw new \InvalidArgumentException(sprintf('Failed to find content stream with id "%s"', $contentStreamId->value), 1729863973); + throw new ContentStreamDoesNotExistYet( + 'Content stream "' . $contentStreamId->value . '" does not exist.', + 1521386692 + ); } return $contentStream->isClosed; } diff --git a/Neos.ContentRepository.Core/Classes/Feature/Common/ConstraintChecks.php b/Neos.ContentRepository.Core/Classes/Feature/Common/ConstraintChecks.php index 2be9688f065..fe63ebf09b1 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/Common/ConstraintChecks.php +++ b/Neos.ContentRepository.Core/Classes/Feature/Common/ConstraintChecks.php @@ -81,13 +81,6 @@ protected function requireContentStream( CommandHandlingDependencies $commandHandlingDependencies ): ContentStreamId { $contentStreamId = $commandHandlingDependencies->getContentGraph($workspaceName)->getContentStreamId(); - if (!$commandHandlingDependencies->contentStreamExists($contentStreamId)) { - throw new ContentStreamDoesNotExistYet( - 'Content stream for "' . $workspaceName->value . '" does not exist yet.', - 1521386692 - ); - } - if ($commandHandlingDependencies->isContentStreamClosed($contentStreamId)) { throw new ContentStreamIsClosed( 'Content stream "' . $contentStreamId->value . '" is closed.',