BUGFIX: After moving newly created nodes, findByIdentifier() should return the correct node

[skurfuerst]

This fixes a non-deterministic React UI Error…

Problem Description

In the new React UI, do the following:

• create node A
• create node B
• move node B underneath node A
• chances are that in at least 20-30% of the cases, you cannot navigate to node B because it has an empty URL, but instead, you are redirected to the root page.
• this error is persistent, i.e. you are never able to navigate to node B, unless you clear the routing cache.

Problem Drilldown

I initially suspected some race condition of different PHP requests going on concurrently, but it turned out not to be the case. Instead, I found out the following:

• in the 20-30% of failed requests, the URI Builder returned strange/wrong values for the just-moved node: The method FrontendNodeRoutePartHandler::getRequestPathByNode returned an empty string instead of proper URIs.
• This is because the following code snippet returned NULL (from FrontendNodeRoutePartHandler):

  $contextProperties = $node->getContext()->getProperties();
  $contextAllowingHiddenNodes = $this->contextFactory->create(array_merge($contextProperties, ['invisibleContentShown' => true]));
  $currentNode = $contextAllowingHiddenNodes->getNodeByIdentifier($node->getIdentifier());
  

After the node has been moved, we have two entries in the NodeData table, looking roughly as follows:

- entry1:
    - persistence_object_identifier uuid_A
    - workspace: <myWorkspace>
    - identifier: <myUUID>
    - removed: true
    - movedto: uuid_B
- entry2
    - persistence_object_identifier uuid_B
    - workspace: <myWorkspace>
    - identifier: <myUUID>
    - removed: true
    - movedto: uuid_B

After some digging in NodeDataRepository::findOneByIdentifier, I figured out the following happens in there:

• we build a query where we fetch the node in all base workspaces, regardless of whether they are removed or not - so basically a WHERE workspace IN (myWorkspace, live) AND identifier=<myUUID>.
• after that, we run reduceNodeVariantsByWorkspacesAndDimensions(), which implements the workspace fallbacks.
  • this method groups nodes by their identifier, i.e. if the identifier is already used, it will return only one of the two entries.
  • IN OUR CASE, reduceNodeVariantsByWorkspacesAndDimensions() should not do anything as we do not have workspace fallbacks or dimension fallbacks used for the node. However, depending on the ordering of the result of MySQL, we get back entry1 or entry2.
• After that, we filter for removed nodes. (using withoutRemovedNodes)
  • in case entry2 is returned from reduceNodeVariantsByWorkspacesAndDimensions(), we filter it to an empty set, and thus return NULL.

Thus, we build up a wrong URI for the node, which is then cached in the Resolve Cache (permanently).

In the old UI, I believe we did not hit the bug because there are quite some cases which mask the bug:

• if you first use findByParentAndNodeType(), e.g. by traversing the tree, then the correct NodeData object is cached in the FirstLevelNodeCache.
• if the routing cache is flushed after the move, there is a higher likelyhood the error will be masked; as in the new request, we only again have a 30% chance of hitting the error (depending on the code path used).

Bugfix

We can not first filter for deleted nodes, and then handle the workspace fallback, as this would lead to un-intended side effects (i.e. if a node in a workspace has been deleted, it would still be findable using "findOneByIdentifier"). (This case is tested by the second behat test).

However, we can ensure that when being in findOneByIdentifier, we ensure that non- shadow nodes get preferred over shadow nodes if they have the same dimension points.

Note: The first added Behat test fails in 50% of the times (depending on result ordering) without the fix.

[kitsunet]

Short note, it may have unintended effects with "shadow nodes" (which are also flagged as removed). Might want to check that first.

[skurfuerst]

@kitsunet well that exactly was the problem that the shadow node was returned instead of the real one :)

[skurfuerst]

FYI: no clue why the build fails, seems unrelated

[kitsunet]

OK, yeah, I thought about it, and the "shadow nodes" were always just there as "path" hiding placeholders, so in findByIdentifier they should indeed be ignored.

[kitsunet]

Also shouldn't we fix this also in 2.3 ? Despite being just visible in the React UI I am sure it also impacts the Ember UI in some ways and at least any application that uses the API potentially.

[skurfuerst]

sure, we can also fix it for 2.3 if you like :) Bad thing is, I feel that there might be scenarios where my change actually makes things worse, i.e. if you delete a node in the "sub-workspace" but it is not deleted in the parent yet; and then you do a "getNodeByIdentifier()" for the UUID - then I am quite sure the node will be returned with the change (wrong!) but not without (correct!).

Best would be if the reduceNodeVariantsByWorkspacesAndDimensions method would respect the shadowNode feature; but this method is too magic for me to understand it fully ;) Maybe @hlubek has an idea about this :)

All the best, Sebastian

[skurfuerst]

Just trying to implement a new solution without the drawbacks

[skurfuerst]

@kitsunet I think the updated bugfix should not have the un-intended side effects from the last change

[skurfuerst]

https://github.com/neos/neos-ui/issues/966

[kitsunet]

Would merge in 24h if no objections.

[bwaidelich]

ping

[skurfuerst]

Hey,

sorry for not answering for so long.

I believe that generally, the problem still exists in the CR - however, it might be masked by a behavior change in Neos UI at https://github.com/neos/neos-ui/pull/1931/files#diff-6243015383111271b184f17625598c11R327

We do not call the UriBuilder at all anymore in the NodeInfoHelper (for performance reasons); but we generate Redirect URLs instead - without the UriBuilder. That's why I believe this issue might be masked.

Unfortunately, due to being in parental leave, I cannot check reproduce/verify the issue currently. I can just post my unverified thoughts ;)

All the best, Sebastian

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions!

[stale[bot]]

This issue has been automatically closed because it has not had activity for some time. But that does not need to be final. If you find the time to work on it, feel free to open it again. Thanks again for your contributions!

[mhsdesign]

is this maybe somehow related? https://github.com/neos/neos-development-collection/issues/1103

[kdambekalns]

Maybe @skurfuerst can dig out old memories and refresh this one?