BUGFIX: make tethered node ids deterministic #4313

mhsdesign · 2023-06-06T19:43:23Z

This was @bwaidelich idea as he saw me fooling around with pre-filing them randomly - you dont need to prefil random ids if you can determine them.

The logic was copied from Neos 8.3's Neos\ContentRepository\Utility::buildAutoCreatedChildNodeIdentifier

neos-development-collection/Neos.ContentRepository/Classes/Utility.php

Lines 76 to 91 in f90d29f

    
               /** 
        
                * Generate a stable identifier for auto-created child nodes 
        
                * 
        
                * This is needed if multiple node variants are created through "createNode" with different dimension values. If 
        
                * child nodes with the same path and different identifiers exist, bad things can happen. 
        
                * 
        
                * @param string $childNodeName 
        
                * @param string $identifier Identifier of the node where the child node should be created 
        
                * @return string The generated UUID like identifier 
        
                */ 
        
               public static function buildAutoCreatedChildNodeIdentifier($childNodeName, $identifier) 
        
               { 
        
                   $hex = md5($identifier . '-' . $childNodeName); 
        
                   return substr($hex, 0, 8) . '-' . substr($hex, 8, 4) . '-' . substr($hex, 12, 4) . '-' . substr($hex, 16, 4) . '-' . substr($hex, 20, 12); 
        
               }

Related: #4343
Related: #4375

mhsdesign · 2023-06-06T19:43:49Z

Neos.ContentRepository.Core/Classes/Feature/NodeCreation/NodeCreation.php

        // 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);


i think we can remove this step now?

good point.. yes, I think so

…d child nodes

mhsdesign · 2023-06-06T20:39:44Z

Neos.ContentRepository.Core/Classes/NodeType/NodeType.php

+            );
+
+            // handle multiple levels of autocreated child nodes
+            foreach ($childNodeType->tetheredNodeAggregateIds($childNodeAggregateId)->getNodeAggregateIds() as $nestedChildNodeName => $nodeAggregateId) {


howdy there, my name is re cursion.

howdy there, I think there should be some kind of endless-recursion-check :)
I mean.. If you have something like

'Some:NodeType': children: 'foo': type: 'Some:NodeType'

it probably won't work anyways. But maybe we should detect those cases anyways to provide a better error message than "memory limit exceeded..."

bwaidelich

Looks great, thanks.
Just a couple of mini nitpicks and some comment re endless recursions

bwaidelich · 2023-06-16T14:23:21Z

....ContentRepository.Core/Classes/Feature/NodeCreation/Command/CreateNodeAggregateWithNode.php

@@ -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.


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.

bwaidelich · 2023-06-16T14:24:04Z

Neos.ContentRepository.Core/Classes/Feature/NodeCreation/NodeCreation.php

        // 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);


good point.. yes, I think so

bwaidelich · 2023-06-16T14:24:33Z

Neos.ContentRepository.Core/Classes/NodeType/NodeType.php

@@ -490,11 +493,34 @@ public function hasAutoCreatedChildNode(NodeName $nodeName): bool
     */
    public function getTypeOfAutoCreatedChildNode(NodeName $nodeName): ?NodeType
    {
+        $this->initialize();


good catch!

haha actually not the only missing 😂
#4333

will revert this and must be fixed correctly in another pr

bwaidelich · 2023-06-16T14:26:11Z

Neos.ContentRepository.Core/Classes/NodeType/NodeType.php

+            );
+
+            // handle multiple levels of autocreated child nodes
+            foreach ($childNodeType->tetheredNodeAggregateIds($childNodeAggregateId)->getNodeAggregateIds() as $nestedChildNodeName => $nodeAggregateId) {


howdy there, I think there should be some kind of endless-recursion-check :)
I mean.. If you have something like

'Some:NodeType': children: 'foo': type: 'Some:NodeType'

it probably won't work anyways. But maybe we should detect those cases anyways to provide a better error message than "memory limit exceeded..."

mhsdesign · 2023-06-27T09:32:20Z

should we then remove (as a follow up) tetheredDescendantNodeAggregateIds?

bwaidelich · 2023-06-28T10:41:24Z

Neos.ContentRepository.Core/Classes/SharedModel/Node/NodeAggregateId.php

+        $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)
+        );


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

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)

mhsdesign · 2023-06-30T09:49:36Z

Neos.ContentRepository.Core/Classes/SharedModel/Node/NodeAggregateId.php

@@ -50,6 +50,23 @@ public static function fromString(string $value): self
        return new self($value);
    }

+    /**
+     * Calculates a deterministic nodeAggregateId.
+     *


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

comment

mhsdesign · 2023-06-30T09:50:39Z

Neos.ContentRepository.Core/Classes/SharedModel/Node/NodeAggregateId.php

+     *  NodeAggregateId::fromParentNodeAggregateIdAndNodeName($parentNodeAggregateId, NodeName::fromString('main'))
+     *
+     */
+    public static function fromParentNodeAggregateIdAndNodeName(NodeAggregateId $parentNodeAggregateId, NodeName $tetheredNodeName): self


move to NodeName

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" ;))

bwaidelich

A few more comments after discussing this in our weekly.

And some more thoughts:

tetheredDescendantNodeAggregateIds should be removed from the CreateNodeAggregateWithNode command, but this has some implications and can be done in a separate changeset (see #4375)

In the long(!) run we would like to get rid of Node::nodeName and all the path related complexity. As a result, a tethered node would not know its own name. To retrieve a tethered node we could use ContentSubgraphInterface::findChildNodeConnectedThroughEdgeName() like today.

bwaidelich · 2023-06-30T10:38:59Z

Neos.ContentRepository.Core/Classes/SharedModel/Node/NodeAggregateId.php

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


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

bwaidelich · 2023-06-30T10:40:28Z

Neos.ContentRepository.Core/Classes/SharedModel/Node/NodeAggregateId.php

+     *  NodeAggregateId::fromParentNodeAggregateIdAndNodeName($parentNodeAggregateId, NodeName::fromString('main'))
+     *
+     */
+    public static function fromParentNodeAggregateIdAndNodeName(NodeAggregateId $parentNodeAggregateId, NodeName $tetheredNodeName): self


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" ;))

bwaidelich · 2023-06-30T10:43:18Z

Neos.ContentRepository.Core/Classes/SharedModel/Node/NodeAggregateId.php

+        $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)
+        );


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)

bwaidelich · 2023-07-14T08:26:43Z

@mhsdesign I'm going through open 9.0 PRs right now and was wondering: Are you still working on this one?

mhsdesign · 2023-07-14T12:12:56Z

Not actively since June ^^ as you had some legit points, they need to be implemented and #4313 (comment) also needs to be discussed (its currently a bit over my head ^^)

So feel free to continue my work - otherwise ill pick it up in half a month or something ;)

bwaidelich · 2023-08-01T18:53:10Z

@mhsdesign half a month is over now :)
just kidding, but we want to discuss this topic on Friday – would be cool if you could join us <3

…eIdByParent($parentId)` instead of `NodeAggregateId::fromParentNodeAggregateIdAndNodeName`

mhsdesign · 2023-09-01T19:44:36Z

Important points of our (extensive ^^) discussions from the last weeklies:

A future without `node.nodeName`? -> no!

... at least not by leveraging this algorithm ... (which would be possible at first)

The deterministic node ids will be mostly a good to have perk but we wont allow it to replace the concept of the nodeNames for tethered nodes, as relying on the algorithm for the id for finding the childNode of a certain name (main) will give us horrible trouble once we change the algorithm.

To better understand what we thought let me demonstrate:

We have the parent node lady-eleonode with the tethered childNode sir-david-nodenborough connected through the edge main.

Fetching the childNode of name main is possible via the subgraph:

$childNode = $subgraph->findChildNodeConnectedThroughEdgeName($parent, NodeName::fromString("main"));

But our - now discarded - idea was that at one point, we could remove the nodeName from the node model and altogether from the database and only leverage the algorithm to find the node of name main.

$childNodeIdGuessedByAlgorithm = NodeName::fromString('main')->tetheredNodeAggregateIdByParent($parentNode->nodeAggregateId);
$childNode = $subgraph->findNodeById($childNodeIdGuessedByAlgorithm);

The code will obviously only work, if the childNode doesnt have the id sir-david-nodenborough but something like a hash of "lady-eleonode" + "main". This gives us the first problem: What about node structures which have been migrated or were created before the deterministic node ids? Also another point is: what if the nodeName main changes (due to some structure adjustment or nodeMigration) - now the id must be recalculated too.

All in all we decided against this approach as this would make the algorithm too important and the cr structure too fragile.

Further points ...

To guarantee a successful replay - even if we change the algorithm - we should still save the ids in the events.
Remove the tetheredDescendantNodeAggregateIds from the constructor of CreateNodeAggregateWithNode but still allow setting it as less important option with a with method. See issue -> Remove tetheredDescendantNodeAggregateIds from CreateNodeAggregateWithNode constructor #4375
Adjust the algorithm to something we prefer more like sha1 or even simply parentId + name?

skurfuerst · 2023-09-02T06:39:28Z

Thanks so much ❤️❤️

mhsdesign · 2023-09-02T07:57:13Z

Hmm upsi - i just thought of a case were these deterministic id's wont be fun:

What should happen when the tethered node "main" is renamed to "foo"? The algorithm will encode that the name is still "main" - but what if we actually now want to later reintroduce a "main" childNode? The "correct" id is already in use now :/

bwaidelich · 2023-09-02T08:41:16Z

Good point. What happens today in that case?

kitsunet · 2023-09-02T09:15:08Z

I guess strictly speaking it's not a use case we ever cared about? But the repair command at least should reassign new ids based on the new name AFAIK? So currently it could be solved that way.

bwaidelich · 2023-09-02T09:18:24Z

the repair command at least should reassign new ids based on the new name AFAIK?

That would defeat the purpose of allowing custom ids to be specified via command.

mhsdesign · 2023-09-02T09:18:33Z

hmm maybe keeping it totally random then is still better than trying to bring to much structure into the ids?

mhsdesign · 2023-09-05T13:40:56Z

because of the fact that a migration to rename auto created nodes will give us trouble with the node id: #4313 (comment)

we will close this pr and keep the node ids random to avoid any conflicts.

but to make things like node templates easier, we want a “helper“ to create all random node ids before the command is issued

… tethered node ids The pr neos/neos-development-collection#4313 was declined and tethered node ids cannot be calculated deterministically but must be generated and specified in the command beforehand.

TASK: Make tethered node ids deterministic

fb89fb6

mhsdesign commented Jun 6, 2023

View reviewed changes

github-actions bot added Task 9.0 labels Jun 6, 2023

TASK: tetheredNodeAggregateIds handle multiple levels of autocreate…

7f1103c

…d child nodes

mhsdesign commented Jun 6, 2023

View reviewed changes

mhsdesign mentioned this pull request Jun 6, 2023

!!! FEATURE: Overhaul NodeCreationHandlerInterface neos/neos-ui#3519

Merged

mhsdesign changed the title ~~Task: make tethered node ids deterministic~~ 9.0 Task: make tethered node ids deterministic Jun 6, 2023

crydotsnake changed the title ~~9.0 Task: make tethered node ids deterministic~~ 9.0 TASK: make tethered node ids deterministic Jun 10, 2023

bwaidelich changed the title ~~9.0 TASK: make tethered node ids deterministic~~ BUGFIX: make tethered node ids deterministic Jun 13, 2023

github-actions bot added the Bug label Jun 13, 2023

bwaidelich reviewed Jun 16, 2023

View reviewed changes

bwaidelich reviewed Jun 28, 2023

View reviewed changes

mhsdesign commented Jun 30, 2023

View reviewed changes

bwaidelich mentioned this pull request Jun 30, 2023

Remove tetheredDescendantNodeAggregateIds from CreateNodeAggregateWithNode constructor #4375

Closed

bwaidelich reviewed Jun 30, 2023

View reviewed changes

bwaidelich mentioned this pull request Jul 7, 2023

9.0 BUG: Changing node names allows to create two nodes with identical names below a single #4343

Closed

1 task

TASK: Refactor to `NodeName::fromString('main')->tetheredNodeAggregat…

a37961d

…eIdByParent($parentId)` instead of `NodeAggregateId::fromParentNodeAggregateIdAndNodeName`

mhsdesign mentioned this pull request Sep 1, 2023

9.0 Discuss Node.nodeName #4311

Closed

mhsdesign closed this Sep 5, 2023

mhsdesign mentioned this pull request Jun 21, 2024

FEATURE: Neos 9.0 compatibility Flowpack/Flowpack.NodeTemplates#51

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUGFIX: make tethered node ids deterministic #4313

BUGFIX: make tethered node ids deterministic #4313

mhsdesign commented Jun 6, 2023 •

edited by bwaidelich

Loading

mhsdesign Jun 6, 2023

bwaidelich Jun 16, 2023

mhsdesign Jun 6, 2023

bwaidelich Jun 16, 2023

bwaidelich left a comment

bwaidelich Jun 16, 2023

bwaidelich Jun 16, 2023

bwaidelich Jun 16, 2023

mhsdesign Jun 16, 2023

bwaidelich Jun 16, 2023

mhsdesign commented Jun 27, 2023

bwaidelich Jun 28, 2023

bwaidelich Jun 30, 2023

mhsdesign Jun 30, 2023

mhsdesign Jun 30, 2023

bwaidelich Jun 30, 2023

bwaidelich left a comment

bwaidelich Jun 30, 2023

bwaidelich Jun 30, 2023

bwaidelich Jun 30, 2023

bwaidelich commented Jul 14, 2023

mhsdesign commented Jul 14, 2023

bwaidelich commented Aug 1, 2023

mhsdesign commented Sep 1, 2023 •

edited

Loading

skurfuerst commented Sep 2, 2023

mhsdesign commented Sep 2, 2023

bwaidelich commented Sep 2, 2023

kitsunet commented Sep 2, 2023

bwaidelich commented Sep 2, 2023

mhsdesign commented Sep 2, 2023

mhsdesign commented Sep 5, 2023

	/**
	* Generate a stable identifier for auto-created child nodes
	*
	* This is needed if multiple node variants are created through "createNode" with different dimension values. If
	* child nodes with the same path and different identifiers exist, bad things can happen.
	*
	* @param string $childNodeName
	* @param string $identifier Identifier of the node where the child node should be created
	* @return string The generated UUID like identifier
	*/
	public static function buildAutoCreatedChildNodeIdentifier($childNodeName, $identifier)
	{
	$hex = md5($identifier . '-' . $childNodeName);

	return substr($hex, 0, 8) . '-' . substr($hex, 8, 4) . '-' . substr($hex, 12, 4) . '-' . substr($hex, 16, 4) . '-' . substr($hex, 20, 12);
	}

	* 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.

BUGFIX: make tethered node ids deterministic #4313

BUGFIX: make tethered node ids deterministic #4313

Conversation

mhsdesign commented Jun 6, 2023 • edited by bwaidelich Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwaidelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsdesign commented Jun 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwaidelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwaidelich commented Jul 14, 2023

mhsdesign commented Jul 14, 2023

bwaidelich commented Aug 1, 2023

mhsdesign commented Sep 1, 2023 • edited Loading

A future without node.nodeName? -> no!

Further points ...

skurfuerst commented Sep 2, 2023

mhsdesign commented Sep 2, 2023

bwaidelich commented Sep 2, 2023

kitsunet commented Sep 2, 2023

bwaidelich commented Sep 2, 2023

mhsdesign commented Sep 2, 2023

mhsdesign commented Sep 5, 2023

mhsdesign commented Jun 6, 2023 •

edited by bwaidelich

Loading

mhsdesign commented Sep 1, 2023 •

edited

Loading

A future without `node.nodeName`? -> no!