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

BUGFIX: Copy node respect tethered nodes correctly #5350 #5358

Closed
wants to merge 3 commits into from
Closed

BUGFIX: Copy node respect tethered nodes correctly #5350 #5358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c84e341
TASK: Improve testing api for `CopyNodesRecursively` and improve asse…
mhsdesign Nov 10, 2024
bde54bd
TASK: Add failing test for #5350
mhsdesign Nov 10, 2024
ed9efe3
BUGFIX: Copy node respect tethered nodes correctly #5350
mhsdesign Nov 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions ...Tests/Tests/Behavior/Features/NodeCopying/CopyNode_ConstraintChecks_TetheredNodes.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
33 changes: 21 additions & 12 deletions ...ository.BehavioralTests/Tests/Behavior/Features/NodeCopying/CopyNode_NoDimensions.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ Feature: Copy nodes (without dimensions)
And using identifier "default", I define a content repository
And I am in content repository "default"
And the command CreateRootWorkspace is executed with payload:
| Key | Value |
| workspaceName | "live" |
| newContentStreamId | "cs-identifier" |
| Key | Value |
| workspaceName | "live" |
| newContentStreamId | "cs-identifier" |
And I am in workspace "live"
And the command CreateRootNodeAggregateWithNode is executed with payload:
| Key | Value |
Expand Down Expand Up @@ -56,28 +56,37 @@ Feature: Copy nodes (without dimensions)

Scenario: Copy
When I am in workspace "live" and dimension space point {}
# node to copy (currentNode): "sir-nodeward-nodington-iii"
Then I expect node aggregate identifier "sir-nodeward-nodington-iii" to lead to node cs-identifier;sir-nodeward-nodington-iii;{}
When the command CopyNodesRecursively is executed, copying the current node aggregate with payload:
When the command CopyNodesRecursively is executed with payload:
| Key | Value |
| sourceDimensionSpacePoint | {} |
| sourceNodeAggregateId | "sir-nodeward-nodington-iii" |
| targetDimensionSpacePoint | {} |
| targetParentNodeAggregateId | "nody-mc-nodeface" |
| targetNodeName | "target-nn" |
| targetSucceedingSiblingnodeAggregateId | null |
| nodeAggregateIdMapping | {"sir-nodeward-nodington-iii": "sir-nodeward-nodington-iii-copy"} |

Then I expect node aggregate identifier "sir-nodeward-nodington-iii-copy" to lead to node cs-identifier;sir-nodeward-nodington-iii-copy;{}
And I expect the node aggregate "sir-nodeward-nodington-iii-copy" to exist
And I expect this node aggregate to be classified as "regular"
And I expect this node aggregate to be named "target-nn"
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 ["nody-mc-nodeface"]

Scenario: Copy References
When I am in workspace "live" and dimension space point {}
And the command SetNodeReferences is executed with payload:
| Key | Value |
| sourceNodeAggregateId | "sir-nodeward-nodington-iii" |
| Key | Value |
| sourceNodeAggregateId | "sir-nodeward-nodington-iii" |
| references | [{"referenceName": "ref", "references": [{"target": "sir-david-nodenborough"}]}] |

Then I expect node aggregate identifier "sir-nodeward-nodington-iii" to lead to node cs-identifier;sir-nodeward-nodington-iii;{}
And the command CopyNodesRecursively is executed, copying the current node aggregate with payload:
When the command CopyNodesRecursively is executed with payload:
| Key | Value |
| sourceDimensionSpacePoint | {} |
| sourceNodeAggregateId | "sir-nodeward-nodington-iii" |
| targetDimensionSpacePoint | {} |
| targetParentNodeAggregateId | "nody-mc-nodeface" |
| targetNodeName | "target-nn" |
Expand All @@ -86,5 +95,5 @@ Feature: Copy nodes (without dimensions)

And I expect node aggregate identifier "sir-nodeward-nodington-iii-copy" to lead to node cs-identifier;sir-nodeward-nodington-iii-copy;{}
And I expect this node to have the following references:
| Name | Node | Properties |
| ref | cs-identifier;sir-david-nodenborough;{} | null |
| Name | Node | Properties |
| ref | cs-identifier;sir-david-nodenborough;{} | null |
82 changes: 82 additions & 0 deletions ...sitory.BehavioralTests/Tests/Behavior/Features/NodeCopying/CopyNode_TetheredNodes.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
Feature: Copy nodes with tethered nodes

Background:
Given using no content dimensions
And using the following node types:
"""yaml
'Neos.ContentRepository.Testing:Document': []
'Neos.ContentRepository.Testing:DocumentWithTethered':
childNodes:
tethered:
type: 'Neos.ContentRepository.Testing:Document'
"""
And using identifier "default", I define a content repository
And I am in content repository "default"
And the command CreateRootWorkspace is executed with payload:
| Key | Value |
| workspaceName | "live" |
| newContentStreamId | "cs-identifier" |
When I am in workspace "live" and dimension space point {}
And I am user identified by "initiating-user-identifier"
And the command CreateRootNodeAggregateWithNode is executed with payload:
| Key | Value |
| nodeAggregateId | "lady-eleonode-rootford" |
| 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-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
And I expect this node aggregate to be classified as "tethered"

When the command CopyNodesRecursively is executed with payload:
| Key | Value |
| sourceDimensionSpacePoint | {} |
| sourceNodeAggregateId | "nodewyn-tetherton" |
| targetDimensionSpacePoint | {} |
| targetParentNodeAggregateId | "sir-david-nodenborough" |
| nodeAggregateIdMapping | {"nodewyn-tetherton": "nodewyn-tetherton-copy"} |

And I expect the node aggregate "nodewyn-tetherton-copy" to exist
# must not be tethered!
And I expect this node aggregate to be classified as "regular"
And I expect this node aggregate to be of type "Neos.ContentRepository.Testing:Document"
And I expect this node aggregate to be unnamed
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-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"]
23 changes: 17 additions & 6 deletions Neos.ContentRepository.Core/Classes/Feature/NodeDuplication/Dto/NodeSubtreeSnapshot.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Comment on lines 51 to +56
Copy link
Member Author

Choose a reason for hiding this comment

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

jup this looks ugly but it doesnt matter, this is just meant as a hotfix. In order to evaluate constraints correctly (64) and also copying of dimensions we have to move this logic to a command handler imo and create a serialised version of this stuff.

But first things first, hey at least copying is now a little les broken :D

Copy link
Member

Choose a reason for hiding this comment

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

FYI: I created an issue for that: #5359

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

Expand Down
26 changes: 26 additions & 0 deletions ...entRepository.Core/Classes/SharedModel/Exception/TetheredNodesCannotBePartiallyCopied.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php

/*
* This file is part of the Neos.ContentRepository package.
*
* (c) Contributors of the Neos Project - www.neos.io
*
* This package is Open Source Software. For the full copyright and license
* information, please view the LICENSE file which was distributed with this
* source code.
*/

declare(strict_types=1);

namespace Neos\ContentRepository\Core\SharedModel\Exception;

/**
* The exception to be thrown if a tethered node aggregate with further whether nodes, determined by their ancestor is attempted to be copied.
*
* Only leaf tethered nodes can be copied.
*
* @api because exception is thrown during invariant checks on command execution
*/
final class TetheredNodesCannotBePartiallyCopied extends \DomainException
{
}
Loading
Loading