From ed9efe32bb17df081029923c8379af90f0e8ae92 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Sun, 10 Nov 2024 20:14:16 +0100 Subject: [PATCH] BUGFIX: Copy node respect tethered nodes correctly #5350 In case a leaf node that is tethered is copied, we un-tether it. A migration `flow migrateevents:migratecopytetherednode` fixes this for previous cases. In case a tethered node is attempted to be copied which has tethered childnodes determined by the parent nodetype we fail. This is not possible. What we dont do yet is determine this correctly and there are false positives: > we assume here that the child node is tethered because the grandparent specifies that. > this is not always fully correct and we could loosen the constraint by checking the node type schema to correctly determine this, we have to evaluate the node type schema which is not available currently. --- ...ode_ConstraintChecks_TetheredNodes.feature | 51 ++++++++++++++++++ .../CopyNode_TetheredNodes.feature | 38 +++++++++++-- .../Dto/NodeSubtreeSnapshot.php | 23 +++++--- .../TetheredNodesCannotBePartiallyCopied.php | 26 +++++++++ .../Bootstrap/Features/NodeCopying.php | 14 +++++ .../MigrateEventsCommandController.php | 16 ++++++ .../Classes/Service/EventMigrationService.php | 54 ++++++++++++++++++- 7 files changed, 211 insertions(+), 11 deletions(-) create mode 100644 Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/NodeCopying/CopyNode_ConstraintChecks_TetheredNodes.feature create mode 100644 Neos.ContentRepository.Core/Classes/SharedModel/Exception/TetheredNodesCannotBePartiallyCopied.php diff --git a/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/NodeCopying/CopyNode_ConstraintChecks_TetheredNodes.feature b/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/NodeCopying/CopyNode_ConstraintChecks_TetheredNodes.feature new file mode 100644 index 00000000000..b6b0ddd3264 --- /dev/null +++ b/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/NodeCopying/CopyNode_ConstraintChecks_TetheredNodes.feature @@ -0,0 +1,51 @@ +@contentrepository @adapters=DoctrineDBAL +Feature: Copy nodes (without dimensions) + + Background: + Given using no content dimensions + And using the following node types: + """yaml + 'Neos.ContentRepository.Testing:Tethered': [] + 'Neos.ContentRepository.Testing:TetheredDocument': + childNodes: + tethered: + type: 'Neos.ContentRepository.Testing:Tethered' + 'Neos.ContentRepository.Testing:Document': + childNodes: + tethered-document: + type: 'Neos.ContentRepository.Testing:TetheredDocument' + 'Neos.ContentRepository.Testing:DocumentWithoutTetheredChildren': [] + """ + And using identifier "default", I define a content repository + And I am in content repository "default" + And I am user identified by "initiating-user-identifier" + And the command CreateRootWorkspace is executed with payload: + | Key | Value | + | workspaceName | "live" | + | newContentStreamId | "cs-identifier" | + And I am in workspace "live" and dimension space point {} + And the command CreateRootNodeAggregateWithNode is executed with payload: + | Key | Value | + | nodeAggregateId | "lady-eleonode-rootford" | + | nodeTypeName | "Neos.ContentRepository:Root" | + And the following CreateNodeAggregateWithNode commands are executed: + | nodeAggregateId | parentNodeAggregateId | nodeTypeName | tetheredDescendantNodeAggregateIds | + | node-mc-nodeface | lady-eleonode-rootford | Neos.ContentRepository.Testing:DocumentWithoutTetheredChildren | {} | + | node-wan-kenody | lady-eleonode-rootford | Neos.ContentRepository.Testing:Document | {"tethered-document": "nodewyn-tetherton", "tethered-document/tethered": "nodimer-tetherton"} | + + Scenario: Coping fails if the leaf of a nested tethered node is attempted to be copied + And I expect the node aggregate "nodewyn-tetherton" to exist + And I expect this node aggregate to be classified as "tethered" + + And I expect the node aggregate "nodimer-tetherton" to exist + And I expect this node aggregate to be classified as "tethered" + + When the command CopyNodesRecursively is executed with payload and exceptions are caught: + | Key | Value | + | sourceDimensionSpacePoint | {} | + | sourceNodeAggregateId | "nodewyn-tetherton" | + | targetDimensionSpacePoint | {} | + | targetParentNodeAggregateId | "node-mc-nodeface" | + | nodeAggregateIdMapping | {"nodewyn-tetherton": "nodewyn-tetherton-copy", "nodimer-tetherton": "nodimer-tetherton-copy"} | + + Then the last command should have thrown an exception of type "TetheredNodesCannotBePartiallyCopied" diff --git a/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/NodeCopying/CopyNode_TetheredNodes.feature b/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/NodeCopying/CopyNode_TetheredNodes.feature index d669d174e1a..35e05df0cec 100644 --- a/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/NodeCopying/CopyNode_TetheredNodes.feature +++ b/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/NodeCopying/CopyNode_TetheredNodes.feature @@ -24,10 +24,10 @@ Feature: Copy nodes with tethered nodes | nodeTypeName | "Neos.ContentRepository:Root" | When the following CreateNodeAggregateWithNode commands are executed: - | nodeAggregateId | parentNodeAggregateId | nodeTypeName | tetheredDescendantNodeAggregateIds | - | sir-david-nodenborough | lady-eleonode-rootford | Neos.ContentRepository.Testing:Document | {} | - | nody-mc-nodeface | sir-david-nodenborough | Neos.ContentRepository.Testing:Document | | - | sir-nodeward-nodington-iii | lady-eleonode-rootford | Neos.ContentRepository.Testing:DocumentWithTethered | {"tethered": "nodewyn-tetherton"} | + | nodeAggregateId | parentNodeAggregateId | nodeTypeName | tetheredDescendantNodeAggregateIds | + | sir-david-nodenborough | lady-eleonode-rootford | Neos.ContentRepository.Testing:Document | {} | + | nody-mc-nodeface | sir-david-nodenborough | Neos.ContentRepository.Testing:Document | | + | sir-nodeward-nodington-i | lady-eleonode-rootford | Neos.ContentRepository.Testing:DocumentWithTethered | {"tethered": "nodewyn-tetherton"} | Scenario: Coping a tethered node turns it into a regular node And I expect the node aggregate "nodewyn-tetherton" to exist @@ -50,3 +50,33 @@ Feature: Copy nodes with tethered nodes And I expect this node aggregate to disable dimension space points [] And I expect this node aggregate to have no child node aggregates And I expect this node aggregate to have the parent node aggregates ["sir-david-nodenborough"] + + Scenario: Coping a node with tethered node keeps the child node tethered + And I expect the node aggregate "nodewyn-tetherton" to exist + And I expect this node aggregate to be classified as "tethered" + + When the command CopyNodesRecursively is executed with payload: + | Key | Value | + | sourceDimensionSpacePoint | {} | + | sourceNodeAggregateId | "sir-nodeward-nodington-i" | + | targetDimensionSpacePoint | {} | + | targetParentNodeAggregateId | "sir-david-nodenborough" | + | nodeAggregateIdMapping | {"sir-nodeward-nodington-i": "sir-nodeward-nodington-ii", "nodewyn-tetherton": "nodewyn-tetherton-copy"} | + + And I expect the node aggregate "sir-nodeward-nodington-ii" to exist + And I expect this node aggregate to be classified as "regular" + And I expect this node aggregate to be unnamed + And I expect this node aggregate to be of type "Neos.ContentRepository.Testing:DocumentWithTethered" + And I expect this node aggregate to occupy dimension space points [[]] + And I expect this node aggregate to disable dimension space points [] + And I expect this node aggregate to have the child node aggregates ["nodewyn-tetherton-copy"] + And I expect this node aggregate to have the parent node aggregates ["sir-david-nodenborough"] + + And I expect the node aggregate "nodewyn-tetherton-copy" to exist + And I expect this node aggregate to be classified as "tethered" + And I expect this node aggregate to be named "tethered" + And I expect this node aggregate to be of type "Neos.ContentRepository.Testing:Document" + And I expect this node aggregate to occupy dimension space points [[]] + And I expect this node aggregate to disable dimension space points [] + And I expect this node aggregate to have no child node aggregates + And I expect this node aggregate to have the parent node aggregates ["sir-nodeward-nodington-ii"] diff --git a/Neos.ContentRepository.Core/Classes/Feature/NodeDuplication/Dto/NodeSubtreeSnapshot.php b/Neos.ContentRepository.Core/Classes/Feature/NodeDuplication/Dto/NodeSubtreeSnapshot.php index 1a724f5dbef..e3cdf134a1d 100644 --- a/Neos.ContentRepository.Core/Classes/Feature/NodeDuplication/Dto/NodeSubtreeSnapshot.php +++ b/Neos.ContentRepository.Core/Classes/Feature/NodeDuplication/Dto/NodeSubtreeSnapshot.php @@ -14,6 +14,7 @@ use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\FindReferencesFilter; use Neos\ContentRepository\Core\Projection\ContentGraph\Node; use Neos\ContentRepository\Core\Projection\ContentGraph\References; +use Neos\ContentRepository\Core\SharedModel\Exception\TetheredNodesCannotBePartiallyCopied; use Neos\ContentRepository\Core\SharedModel\Node\NodeAggregateClassification; use Neos\ContentRepository\Core\SharedModel\Node\NodeAggregateId; use Neos\ContentRepository\Core\SharedModel\Node\NodeName; @@ -49,24 +50,34 @@ private function __construct( public static function fromSubgraphAndStartNode(ContentSubgraphInterface $subgraph, Node $sourceNode): self { - $childNodes = []; + return self::createSnapshotRecursively($subgraph, $sourceNode, true); + } + + private static function createSnapshotRecursively(ContentSubgraphInterface $subgraph, Node $sourceNode, bool $firstLevel): self + { + $childNodeSnapshots = []; foreach ( $subgraph->findChildNodes($sourceNode->aggregateId, FindChildNodesFilter::create()) as $sourceChildNode ) { - $childNodes[] = self::fromSubgraphAndStartNode($subgraph, $sourceChildNode); + if ($firstLevel && $sourceNode->classification->isTethered() && $sourceChildNode->classification->isTethered()) { + // we assume here that the child node is tethered because the grandparent specifies that. + // this is not always fully correct and we could loosen the constraint by checking the node type schema + throw new TetheredNodesCannotBePartiallyCopied(sprintf('Cannot copy tethered node %s because child node %s is also tethered. Only standalone tethered nodes can be copied.', $sourceNode->aggregateId->value, $sourceChildNode->aggregateId->value), 1731264887); + } + + $childNodeSnapshots[] = self::createSnapshotRecursively($subgraph, $sourceChildNode, false); } - $properties = $sourceNode->properties; return new self( $sourceNode->aggregateId, $sourceNode->nodeTypeName, $sourceNode->name, - $sourceNode->classification, - $properties->serialized(), + $firstLevel ? NodeAggregateClassification::CLASSIFICATION_REGULAR : $sourceNode->classification, + $sourceNode->properties->serialized(), self::serializeProjectedReferences( $subgraph->findReferences($sourceNode->aggregateId, FindReferencesFilter::create()) ), - $childNodes + $childNodeSnapshots ); } diff --git a/Neos.ContentRepository.Core/Classes/SharedModel/Exception/TetheredNodesCannotBePartiallyCopied.php b/Neos.ContentRepository.Core/Classes/SharedModel/Exception/TetheredNodesCannotBePartiallyCopied.php new file mode 100644 index 00000000000..95976805acb --- /dev/null +++ b/Neos.ContentRepository.Core/Classes/SharedModel/Exception/TetheredNodesCannotBePartiallyCopied.php @@ -0,0 +1,26 @@ +currentContentRepository->handle($command); } + + /** + * @Given /^the command CopyNodesRecursively is executed with payload and exceptions are caught:$/ + * @param TableNode $payloadTable + * @throws \Exception + */ + public function theCommandCopyNodesRecursivelyIsExecutedWithPayloadAndExceptionsAreCaught(TableNode $payloadTable) + { + try { + $this->theCommandCopyNodesRecursivelyIsExecutedWithPayload($payloadTable); + } catch (\Exception $exception) { + $this->lastCommandException = $exception; + } + } } diff --git a/Neos.ContentRepositoryRegistry/Classes/Command/MigrateEventsCommandController.php b/Neos.ContentRepositoryRegistry/Classes/Command/MigrateEventsCommandController.php index 4825a4420b9..29d39dd159a 100644 --- a/Neos.ContentRepositoryRegistry/Classes/Command/MigrateEventsCommandController.php +++ b/Neos.ContentRepositoryRegistry/Classes/Command/MigrateEventsCommandController.php @@ -116,4 +116,20 @@ public function migrateSetReferencesToMultiNameFormatCommand(string $contentRepo $eventMigrationService = $this->contentRepositoryRegistry->buildService($contentRepositoryId, $this->eventMigrationServiceFactory); $eventMigrationService->migrateReferencesToMultiFormat($this->outputLine(...)); } + + /** + * Migrates "nodeAggregateClassification":"tethered" to "regular", in case for copied tethered nodes. + * + * Needed for #5350: https://github.com/neos/neos-development-collection/issues/5350 + * + * Included in November 2024 - before final Neos 9.0 release + * + * @param string $contentRepository Identifier of the Content Repository to migrate + */ + public function migrateCopyTetheredNodeCommand(string $contentRepository = 'default'): void + { + $contentRepositoryId = ContentRepositoryId::fromString($contentRepository); + $eventMigrationService = $this->contentRepositoryRegistry->buildService($contentRepositoryId, $this->eventMigrationServiceFactory); + $eventMigrationService->migrateCopyTetheredNode($this->outputLine(...)); + } } diff --git a/Neos.ContentRepositoryRegistry/Classes/Service/EventMigrationService.php b/Neos.ContentRepositoryRegistry/Classes/Service/EventMigrationService.php index 2a71cbd4333..a313849415e 100644 --- a/Neos.ContentRepositoryRegistry/Classes/Service/EventMigrationService.php +++ b/Neos.ContentRepositoryRegistry/Classes/Service/EventMigrationService.php @@ -15,7 +15,6 @@ use Neos\ContentRepository\Core\Feature\NodeModification\Command\SetSerializedNodeProperties; use Neos\ContentRepository\Core\Feature\NodeMove\Command\MoveNodeAggregate; use Neos\ContentRepository\Core\Feature\NodeReferencing\Command\SetSerializedNodeReferences; -use Neos\ContentRepository\Core\Feature\NodeReferencing\Event\NodeReferencesWereSet; use Neos\ContentRepository\Core\Feature\NodeRemoval\Command\RemoveNodeAggregate; use Neos\ContentRepository\Core\Feature\NodeRenaming\Command\ChangeNodeAggregateName; use Neos\ContentRepository\Core\Feature\NodeTypeChange\Command\ChangeNodeAggregateType; @@ -23,6 +22,7 @@ use Neos\ContentRepository\Core\Feature\RootNodeCreation\Command\CreateRootNodeAggregateWithNode; use Neos\ContentRepository\Core\Feature\RootNodeCreation\Command\UpdateRootNodeAggregateDimensions; use Neos\ContentRepository\Core\SharedModel\ContentRepository\ContentRepositoryId; +use Neos\ContentRepository\Core\SharedModel\Node\NodeAggregateClassification; use Neos\ContentRepository\Core\SharedModel\Workspace\WorkspaceName; use Neos\ContentRepositoryRegistry\Command\MigrateEventsCommandController; use Neos\ContentRepositoryRegistry\Factory\EventStore\DoctrineEventStoreFactory; @@ -65,6 +65,58 @@ public function backup(\Closure $outputFn): void $outputFn(sprintf('Backup. Copied events table to %s', $backupEventTableName)); } + public function migrateCopyTetheredNode(\Closure $outputFn): void + { + $this->eventsModified = []; + $warnings = 0; + + $backupEventTableName = DoctrineEventStoreFactory::databaseTableName($this->contentRepositoryId) + . '_bkp_' . date('Y_m_d_H_i_s'); + $outputFn(sprintf('Backup: copying events table to %s', $backupEventTableName)); + + $this->copyEventTable($backupEventTableName); + + $streamName = VirtualStreamName::all(); + $eventStream = $this->eventStore->load($streamName, EventStreamFilter::create(EventTypes::create(EventType::fromString('NodeAggregateWithNodeWasCreated')))); + foreach ($eventStream as $eventEnvelope) { + $outputRewriteNotice = fn(string $message) => $outputFn(sprintf('%s@%s %s', $eventEnvelope->sequenceNumber->value, $eventEnvelope->event->type->value, $message)); + if ($eventEnvelope->event->type->value !== 'NodeAggregateWithNodeWasCreated') { + throw new \RuntimeException(sprintf('Unhandled event: %s', $eventEnvelope->event->type->value)); + } + + $eventMetaData = $eventEnvelope->event->metadata?->value; + // a copy is basically a NodeAggregateWithNodeWasCreated with CopyNodesRecursively command, so we skip others: + if (!$eventMetaData || ($eventMetaData['commandClass'] ?? null) !== CopyNodesRecursively::class) { + continue; + } + + $eventData = self::decodeEventPayload($eventEnvelope); + if ($eventData['nodeAggregateClassification'] !== NodeAggregateClassification::CLASSIFICATION_TETHERED->value) { + // this copy is okay + continue; + } + + $eventData['nodeAggregateClassification'] = NodeAggregateClassification::CLASSIFICATION_REGULAR->value; + $this->updateEventPayload($eventEnvelope->sequenceNumber, $eventData); + + $eventMetaData['commandPayload']['nodeTreeToInsert']['nodeAggregateClassification'] = NodeAggregateClassification::CLASSIFICATION_REGULAR->value; + + $this->updateEventMetaData($eventEnvelope->sequenceNumber, $eventMetaData); + $outputRewriteNotice(sprintf('Copied tethered node "%s" of type "%s" (name: %s) was migrated', $eventData['nodeAggregateId'], $eventData['nodeTypeName'], json_encode($eventData['nodeName']))); + } + + if (!count($this->eventsModified)) { + $outputFn('Migration was not necessary.'); + return; + } + + $outputFn(); + $outputFn(sprintf('Migration applied to %s events. Please replay the projections `./flow cr:projectionReplayAll`', count($this->eventsModified))); + if ($warnings) { + $outputFn(sprintf('WARNING: Finished but %d warnings emitted.', $warnings)); + } + } + /** * The following things have to be migrated: *