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.

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: make tethered node ids deterministic #4313

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ final class CreateNodeAggregateWithNode implements CommandInterface
* using this assignment registry.
* Since tethered child nodes may have tethered child nodes themselves,
* this registry is indexed using relative node paths to the node to create in the first place.
*
* Specifying them is optional, and otherwise they are calculated deterministic based on the $nodeAggregateId and the tethered nodeName.
Copy link
Member

Choose a reason for hiding this comment

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

Nice addition! Just a mini nitpick

Suggested change
* Specifying them is optional, and otherwise they are calculated deterministic based on the $nodeAggregateId and the tethered nodeName.
* Specifying them is optional, and otherwise they are calculated deterministically based on the $nodeAggregateId and the tethered nodeName.

* So you can determine the child node identifier via {@see NodeAggregateId::fromParentNodeAggregateIdAndNodeName()}
*/
public readonly NodeAggregateIdsByNodePaths $tetheredDescendantNodeAggregateIds;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,10 +199,9 @@ private function handleCreateNodeAggregateWithNodeAndSerializedProperties(
$contentRepository
);
}
$descendantNodeAggregateIds = self::populateNodeAggregateIds(
$nodeType,
$command->tetheredDescendantNodeAggregateIds
);

$descendantNodeAggregateIds = $nodeType->tetheredNodeAggregateIds($command->nodeAggregateId)
->merge($command->tetheredDescendantNodeAggregateIds);
// Write the auto-created descendant node aggregate ids back to the command;
// so that when rebasing the command, it stays fully deterministic.
$command = $command->withTetheredDescendantNodeAggregateIds($descendantNodeAggregateIds);
Comment on lines 205 to 207
Copy link
Member Author

Choose a reason for hiding this comment

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

i think we can remove this step now?

Copy link
Member

Choose a reason for hiding this comment

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

good point.. yes, I think so

Expand Down Expand Up @@ -338,29 +337,4 @@ private function createTetheredWithNode(
$precedingNodeAggregateId
);
}

protected static function populateNodeAggregateIds(
NodeType $nodeType,
?NodeAggregateIdsByNodePaths $nodeAggregateIds,
NodePath $childPath = null
): NodeAggregateIdsByNodePaths {
if ($nodeAggregateIds === null) {
$nodeAggregateIds = NodeAggregateIdsByNodePaths::createEmpty();
}
// TODO: handle Multiple levels of autocreated child nodes
foreach ($nodeType->getAutoCreatedChildNodes() as $rawChildName => $childNodeType) {
$childName = NodeName::fromString($rawChildName);
$childPath = $childPath
? $childPath->appendPathSegment($childName)
: NodePath::fromString($childName->value);
if (!$nodeAggregateIds->getNodeAggregateId($childPath)) {
$nodeAggregateIds = $nodeAggregateIds->add(
$childPath,
NodeAggregateId::create()
);
}
}

return $nodeAggregateIds;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,6 @@ abstract protected function areNodeTypeConstraintsImposedByGrandparentValid(
NodeType $nodeType
): bool;

abstract protected static function populateNodeAggregateIds(
NodeType $nodeType,
NodeAggregateIdsByNodePaths $nodeAggregateIds,
NodePath $childPath = null
): NodeAggregateIdsByNodePaths;

abstract protected function createEventsForMissingTetheredNode(
NodeAggregate $parentNodeAggregate,
Node $parentNode,
Expand Down Expand Up @@ -156,10 +150,9 @@ private function handleChangeNodeAggregateType(
/**************
* Preparation - make the command fully deterministic in case of rebase
**************/
$descendantNodeAggregateIds = static::populateNodeAggregateIds(
$newNodeType,
$command->tetheredDescendantNodeAggregateIds
);
$descendantNodeAggregateIds = $newNodeType->tetheredNodeAggregateIds($command->nodeAggregateId)
->merge($command->tetheredDescendantNodeAggregateIds ?? NodeAggregateIdsByNodePaths::createEmpty());

// Write the auto-created descendant node aggregate ids back to the command;
// so that when rebasing the command, it stays fully deterministic.
$command = $command->withTetheredDescendantNodeAggregateIds($descendantNodeAggregateIds);
Expand Down
23 changes: 23 additions & 0 deletions Neos.ContentRepository.Core/Classes/NodeType/NodeType.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@
*/


use Neos\ContentRepository\Core\Feature\NodeCreation\Dto\NodeAggregateIdsByNodePaths;
use Neos\ContentRepository\Core\Projection\ContentGraph\NodePath;
use Neos\ContentRepository\Core\SharedModel\Exception\NodeTypeNotFoundException;
use Neos\ContentRepository\Core\SharedModel\Node\NodeAggregateId;
use Neos\ContentRepository\Core\SharedModel\Node\NodeName;
use Neos\Utility\ObjectAccess;
use Neos\Utility\Arrays;
Expand Down Expand Up @@ -490,11 +493,31 @@ public function hasAutoCreatedChildNode(NodeName $nodeName): bool
*/
public function getTypeOfAutoCreatedChildNode(NodeName $nodeName): ?NodeType
{
$this->initialize();
Copy link
Member

Choose a reason for hiding this comment

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

good catch!

Copy link
Member Author

Choose a reason for hiding this comment

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

haha actually not the only missing 😂
#4333

will revert this and must be fixed correctly in another pr

return isset($this->fullConfiguration['childNodes'][$nodeName->value]['type'])
? $this->nodeTypeManager->getNodeType($this->fullConfiguration['childNodes'][$nodeName->value]['type'])
: null;
}

/**
* Calculates the deterministic nodeAggregateIds for the auto-created childNodes.
*/
public function tetheredNodeAggregateIds(NodeAggregateId $parentNodeAggregateId): NodeAggregateIdsByNodePaths
{
$this->initialize();
$nodeAggregateIdsByNodePaths = NodeAggregateIdsByNodePaths::createEmpty();
if (!isset($this->fullConfiguration['childNodes'])) {
return $nodeAggregateIdsByNodePaths;
}
// TODO: handle Multiple levels of autocreated child nodes
foreach (array_keys($this->fullConfiguration['childNodes']) as $tetheredNodeName) {
$nodeAggregateIdsByNodePaths = $nodeAggregateIdsByNodePaths->add(
NodePath::fromString($tetheredNodeName),
NodeAggregateId::fromParentNodeAggregateIdAndNodeName($parentNodeAggregateId, NodeName::fromString($tetheredNodeName))
);
}
return $nodeAggregateIdsByNodePaths;
}

/**
* Checks if the given NodeType is acceptable as sub-node with the configured constraints,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,23 @@ public static function fromString(string $value): self
return new self($value);
}

/**
* Calculates a deterministic nodeAggregateId.
*
Copy link
Member Author

Choose a reason for hiding this comment

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

used for new nodes, but previously created tethered node ids were random

comment

* Used for determining the nodeAggregateIds of tethered nodes.
*
* e.g. in case you want to get the main content collection node aggregate id:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* e.g. in case you want to get the main content collection node aggregate id:
* e.g. in case you want to get the main content collection node aggregate id:
*
* NOTE: Previously ids of tethered nodes were _not_ deterministic, so this method should not be used to look up tethered nodes
* Instead {@see \Neos\ContentRepository\Core\Projection\ContentGraph\ContentSubgraphInterface::findChildNodeConnectedThroughEdgeName()} can be used

* NodeAggregateId::fromParentNodeAggregateIdAndNodeName($parentNodeAggregateId, NodeName::fromString('main'))
*
*/
public static function fromParentNodeAggregateIdAndNodeName(NodeAggregateId $parentNodeAggregateId, NodeName $tetheredNodeName): self
Copy link
Member Author

Choose a reason for hiding this comment

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

move to NodeName

Copy link
Member

Choose a reason for hiding this comment

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

I agree that this should be moved because it contains a concept that does not belong to the Node Aggregate ID itself.. It's not too bad though IMO and I'm not sure if NodeName is a better place.. @skurfuerst do you have an idea where to put this (and please don't say "utility class" ;))

{
$hex = md5($parentNodeAggregateId->value . '-' . $tetheredNodeName->value);
return new self(
substr($hex, 0, 8) . '-' . substr($hex, 8, 4) . '-' . substr($hex, 12, 4) . '-' . substr($hex, 16, 4) . '-' . substr($hex, 20, 12)
);
Copy link
Member

Choose a reason for hiding this comment

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

to discuss: does this have to be a UUID pattern?

Copy link
Member

Choose a reason for hiding this comment

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

to follow up on this:
node ids used to be UUIDs and that is the reason for this algorithm.
But now they can be anything basically (as long as they match the regex /^([a-z0-9\-]{1,64})$/).

I would suggest to just do $parentNodeAggregateId->value . '-' . $tetheredNodeName->value really (with some deterministic changes to satisfy the regex)

}

public function equals(self $other): bool
{
return $this->value === $other->value;
Expand Down