Inconsistent behavior of Neo4jMappingContext.getChildNodeDescriptionsInHierarchy()

[nk-coding]

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:

B1 [B1] - B2 - B3 - B2a - B3a B2 [B2, B1] - B3 - B2a - B3a B2a [B2a, B2, B1] B3 [B3, B2, B1] - B3a B3a [B3a, B3, B2, B1]

B1 [B1] - B2 - B3 - B2a B2 [B2, B1] - B3 - B2a - B3a B2a [B2a, B2, B1] B3 [B3, B2, B1] - B3a B3a [B3a, B3, B2, B1]

Please note the difference for B1: in some runs, it knows about B3a, while in others, it does not.

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)

[michael-simons]

Thanks for the detailed analysis. I don't have much to add to it, actually apart from labelling this as a bug:

• the result must be consistent with a given set of entities (in all the cases the mapping context is set to strict and has a predefined set of entities)
• if this is the case, the hierarchy must contain all entries

I will ponder a moment whether to make the scanning smarter, but I am reluctant to premature optimise it. We are hopefully not speaking about models with an inheritance tree in the 3 digits or so.

[nk-coding]

Thanks for the quick response and fix
Looking forward to the 6.3.3 release

[michael-simons]

Our pleasure, Niklas!