Open grizooo opened 1 year ago
Target Environment:
neos/neos-development-collection: 9.0.x-dev
Apparently there is a problem with nodes with the same identifier having different parent nodes.
The problem point is here: https://github.com/neos/neos-development-collection/blob/9.0/Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Projection/Feature/NodeVariation.php#L320
In detail we can have a look at one of the problem nodes, with the id '96a48c08-0286-47de-bfe2-7607d5cc59a0'.
Here are all the DB entries with the corresponding node identifier:
The german entry has a different parentpath which corresponds to the following identifier. Only one node with this identifier exists:
All of the other nodes have the "same" parent node with the same identifier:
For now, I wrote a migration that changes the identifier of all nodes that have sibilings with a different path, but as the migration is still not running through (for different reasons) I cannot check if this breaks anything.
I am unsure if this is an issue unique to our neos installation or if this is a usual.
My really quick fix quick and dirty migration solution (just for reference, it is not tested and should not be used):
public function isTransformable(NodeData $node)
{
$sibilings = $this->nodeDataRepository->findByNodeIdentifier($node->getIdentifier())->toArray();
if(empty($sibilings)){
return false;
}
$pathlist = [];
foreach ($sibilings as $sib){
if(isset($pathlist[$sib->getPath()])){
$pathlist[$sib->getPath()]++;
} else {
$pathlist[$sib->getPath()] = 1;
}
}
if(count($pathlist) == 1){
return false;
}
if($pathlist[$node->getPath()] == 1){
var_dump('Siblings: '. count($sibilings) . ' Pathlist: '.json_encode($pathlist));
return true;
}
return false;
}
public function execute(NodeData $node)
{
$identifier = Algorithms::generateUUID();
$node->setIdentifier($identifier);
}
I could reproduce the bug on Neos Demo with Neos 8.3 and an upgrade to Neos 9.
Steps to reproduce:
Expected result:
Migration runs through
Actual result:
Migration throws error
What I found out so far:
The migration creates two events that are interesting in this case, first the NodePeerVariantWasCreated event for german, and then if the parent node is different, it creates a NodeAggregateWasMoved event to move the node variant to a new location:
In this case, the following steps happen
As we can see, when the Event 263 is running, the parent node only exists in one dimension (DE) and only has to exist in one dimension since we move it later on. When replaying the projections, in the 'whenNodePeerVariantWasCreated' method we expect the parent to exists all dimensions the child exists in(which is quite reasonable). The method tries to load the parent for the peerNode to create the hierarchy with the parent.
Solutions that I can think of right now:
In the migration, if the parent is different, check in the visited nodes if the parent exists in the right dimension. If the parent does not exist in the right dimension, create a new event 'NodeAggregateWithNodeWasCreated' instead of 'NodePeerVariantWasCreated' with new unique identifier. Unsure about this since we create a new node element from 'nothing' here.
Rethink the whole logic of creating a peerVariant and nodeAggregateWasMoved logic for elements with different parents and instead immediately create a new NodeAggregateWithNodeWasCreated. Disadvantage is that we would create lots of new nodes, but perhaps that is something we want since we cannot be sure that any reference between the nodes needs to exist?
In the migration, if the parent is different and does not exist in the right dimension, create a 'helper' peerNode in the dimension. Then create a new event for deleting it after the nodeAggregateWasMoved. This kind of feels like a bad workaround but is a thing we could do.
In the whenNodePeerVariantWasCreated projection, accept that the parent in the dimension can be nonexistent. I cannot think of a use case, but perhaps there is one other than the migration?
Since I am quite new to the whole events thing I would be happy for any input here 😄
@pKallert Thanks so much for the excellent bug report including a PR to reproduce this and sorry for the delay on this one.
First little finding: This is the error the projection runs into (note to self: we urgently need to improve error handling, this was hidden underneath some DBAL transaction exceptions).
I'm still trying to wrap my head around the desired behavior currently
I looked into this issue again together with @kitsunet and unfortunately have to surrender for now because I just can't make the required time and brain power available at the moment..
At least I finally understood the issue (which is not that complicated by itself), to put it into my own words:
At first I tried to detect that case and to make the variant into a separate node aggregate underneath the new parent (see 3a01416019379cb31c368b274fd49e30ea87fdf5) but that solution seems somehow wrong because we have to invent new node ids and because the relation to the original aggregate is lost.
Christian and me came to the conclusion that we would probably need to extend the *VariantWasCreated
events by some optional newParentNodeAggregateId
property, such that the can be created and moved at the same time. But I did not manage to adjust the projection logic accordingly.. Maybe @skurfuerst or @nezaniel can help with that?
This should be the content graph before the Cut & Paste operation:
So if I understand the UI's cut&paste behavior correctly, what happens next is that "Text" is moved in from Testing1...col1 to Testing2...col1, but only in German(yellow), which is a valid operation in both Neos 8 and 9.
In the migration, we should do the exact same thing: 1) PeerNodeVariantWasCreated(node: Text, source: en_US, target: de) 2) NodeAggregateWasMoved(node: Text, newParent: col1(Testing2), affected DSPs: [de])
Or in general: If a node aggregate is scattered (meaning it has different parents in different DSPs), we must first completely constitute it with variation events before we can move it partially. It is possible that the source parent does not exist in the variant DSP, meaning it has been deleted there in the meantime. In this case have to do the same: Create the parent via variation, apply the move and other operations, and in the end remove it again. This also resembles closest what happened in the Neos8 CR before
Is there an existing issue for this?
Current Behavior
Expected Behavior
Migration done whithout errors.
Steps To Reproduce
Environment
Anything else?
We tried to remove nodes without parents first -> no success. We started node migration to set node without language to default language (suggested from bernhard) -> no success