Inconsistent behavior of Neo4jMappingContext.getChildNodeDescriptionsInHierarchy() #2574
Description
Hello,
I found some strange/inconsistent behavior of Neo4jMappingContext.getChildNodeDescriptionsInHierarchy().
Demo showcasing the behavior can be found here: https://github.com/nk-coding/sdn-nodedescription-bug
Consider the following class hierarchy: A4 -> A3 -> A2 -> A1, where all are annotate with @Node, and A3..A1 are abstract.
When getting all NodeDescriptions from the Neo4jMappingContext, and calling getChildNodeDescriptionsInHierarchy(), I get the following results:
A1
[A1]
- A2
- A3
A2
[A2, A1]
- A3
- A4
A3
[A3, A2, A1]
- A4
A4
[A4, A3, A2, A1]
(format: primary label, list of all labels, results of getChildNodeDescriptionsInHierarchy())
Most important, A2 and A3 "know" about the child NodeDescription A4, however A1 does not (it only knows about A2 and A3)
This behavior gets even more strange with the following example:
- B3a -> B3 -> B2 -> B1
- B2a -> B2 -> B1
where all classes are annotated with @Node, but only B2a and B3a are not abstract
When running this multiple times, I get the following results:
|
|
As far as I know, this behavior is caused by how the initialization of the childNodes work on DefaultNeo4jPersistentEntity:
@Override
public void addChildNodeDescription(NodeDescription<?> child) {
this.childNodeDescriptions.add(child);
updateChildNodeDescriptionCache();
if (this.parentNodeDescription != null) {
((DefaultNeo4jPersistentEntity<?>) this.parentNodeDescription).updateChildNodeDescriptionCache();
}
}
private void updateChildNodeDescriptionCache() {
this.childNodeDescriptionsInHierarchy = computeChildNodeDescriptionInHierarchy();
}For the first example, when the NodeDescription for A4 is created, addChildNodeDescription is called on A3. This updates the childNodeDescriptionsInHierarchy for A3 and its parent A2, but not for A1.
The probabilistic behavior of the second example can be explained by the order of NodeDescription creation, if A2a is created after A3a, A2a gets added to A2, causing A2 and A1 to be updated, therefore A1 "knowing" about A3a, if A3a is created after A2a, only A3 and A2 are updated, but not A1.
A simple fix would be to make updateChildNodeDescriptionCache recursive:
@Override
public void addChildNodeDescription(NodeDescription<?> child) {
this.childNodeDescriptions.add(child);
updateChildNodeDescriptionCache();
}
private void updateChildNodeDescriptionCache() {
this.childNodeDescriptionsInHierarchy = computeChildNodeDescriptionInHierarchy();
if (this.parentNodeDescription != null) {
((DefaultNeo4jPersistentEntity<?>) this.parentNodeDescription).updateChildNodeDescriptionCache();
}
}This fixes the inconsistent behavior, however, for deeper hierarchies, performance may be impacted due to more unnecessary calls to updateChildNodeDescriptionCache. A more intelligent algorithm would probably only make the recursive call if a non-abstract NodeDefinition is added, however this assumes that for each abstract NodeDefinition, a non-abstract one exists (and I'm not sure if this is always given) (also, this optimization is most likely not necessary, however I wanted to mention it in case you are worried about the performance impact of the recursive call).
Last, I want to mention, that this might not even be a bug. The documentation of getChildNodeDescriptionsInHierarchy says Retrieve all direct child node descriptions which extend this entity., however, "InHierarchy" (and the actual behavior) imply to me, that in most cases, it also returns indirect children (or my understanding of direct children is incorrect).
However, NodeDescriptionStore.computeConcreteNodeDescription seems to depend on it also returning the indirect children. In fact, not returning indirect children (which is sometimes the case due to this bug) can cause MappingExceptions, as then a NodeDescription for an abstract superclass fits the returned labels best (as the even better fitting non-abstract class is unknown to the NodeDescriptionStore)