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

BUG: CopyNodesRecursively vs Tethered nodes #5350

Closed
mhsdesign opened this issue Nov 8, 2024 · 0 comments · Fixed by #5371
Closed

BUG: CopyNodesRecursively vs Tethered nodes #5350

mhsdesign opened this issue Nov 8, 2024 · 0 comments · Fixed by #5371

Comments

@mhsdesign
Copy link
Member

mhsdesign commented Nov 8, 2024

There is a bug in CopyNodesRecursively because it takes the intention to literally and copies also the classification of the start node to copy.
Initially this must have created tethered nodes just inside the tree even though they are not tethered to anything.

By removing the constraints from the main content collection, we can reproduce this using the Neos Ui: Copy the content collection and insert it as its child:

image

(Its not forbidden via the Neos ui to do that, in fact there are no special constraints in javascript for tethered nodes being copied: https://github.com/neos/neos-ui/blob/8271a89585c83ec0f3cf141c43e59c7f5ce1c194/packages/neos-ui-redux-store/src/CR/Nodes/selectors.ts#L420)


It only gets interesting with these two "never" changes:

They made us aware of this problem because 2. was a cleanup to prevent copying the obsolete initial name (in this case main) and would insert null, which is works but not for tethered nodes. And 1. which added a sanity check to the Node's constructor that all tethered nodes better be named!

So we end up with a wrong command, which is not further validated (because we TRUST user input (what is trust?)) and emits a NodeAggregateWithNodeWasCreated event which says the nodeName is null but the state tethered

...
  "nodeName": null,
  "initialPropertyValues": [],
  "nodeAggregateClassification": "tethered",
...

That will just be projected in the database:

nodetypename classification name
Neos.Demo:Collection.Content.Main tethered

Only fetching the entry is NOT possible because of the sanity check (1):

That will be thrown either when querying the node or even attempting a replay, as many catchup hooks will fetch the node from the database:

The NodeName must be set if the Node is tethered.

Currently the Neos Ui will just crash at this point when copying this and will not be reloadable:

image

So we are in the state of having wrong events here which need to be corrected.

As in my opinion the starting node aggregate classification should have NEVER been considered i think we need a migration that untethers these nodes:

Go through all NodeAggregateWithNodeWasCreated events, see if the class is a CopyNodesRecursively, if the classification is tethered just set it to regular as tethered is NOT allowed.

see also our slack discussions

mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this issue Nov 10, 2024
>  Node aggregate classifications do not match. Expected "regular", got "tethered".

Or a catchup will fail with

> The NodeName must be set if the Node is tethered
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this issue Nov 10, 2024
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.
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this issue Nov 18, 2024
In case a leaf node that is tethered is copied, we un-tether it.
A migration `flow migrateevents:migratecopytetherednode` fixes this for previous cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant