[BUG] Multi Relationships cannot get right conversion

[yangyaofei]

If I have a BaseNode with Map type relationships defined with a base class

@Node
public class BaseNode {
    @Id
    @GeneratedValue
    private UUID id;

    @Relationship(direction = Relationship.Direction.OUTGOING)
    private Map<String, List<BaseRelationship>> relationships = new HashMap<>();
}

@RelationshipProperties
public abstract class BaseRelationship {
    @RelationshipId
    @GeneratedValue
    private Long id;
    @TargetNode
    private BaseNode targetNode;
}

And I have two real class for relationship:

@RelationshipProperties
public class Relationship extend BaseRelationship{

}

@RelationshipProperties
public class AnotherRelationship extend BaseRelationship{

}

If there is a BaseNode with both two relationships :

var node = new BaseNode();
node.setRelationships(Map.of(
    "relation", List.of(
        new Relationship(), new AnotherRelationship()
    )
))
baseNodeRepository.save(node)
// the node and relations store in neo4j is good with clear type

Since SDN in node can get right instance, I think the relationship will get right,
but the code below, the relationships will get same type of instance, both the
Relationship or AnotherRelationship, cannot Get Proper type.

var node = baseNodeRepository.findById(id);
List<BaseRelationship> relationships = node.getRelationships().get("relation");

[yangyaofei]

I dig into the code, I found in DefaultNeo4jEntityConverter, In getLabels and map function, I think SDN use labels to get the type of node and relationship, but relationship don't have a label, it has type.

So, the getLabels function cannot get type information with relationship (always get BaseRelationship), and this cause this bug I think.

spring-data-neo4j/src/main/java/org/springframework/data/neo4j/core/mapping/DefaultNeo4jEntityConverter.java

Line 435 in f10c95f

private List<String> getLabels(MapAccessor queryResult, @Nullable NodeDescription<?> nodeDescription) {

spring-data-neo4j/src/main/java/org/springframework/data/neo4j/core/mapping/DefaultNeo4jEntityConverter.java

Line 317 in f10c95f

List<String> allLabels = getLabels(queryResult, nodeDescription);

I think, it should add few code in getLabels like this line:

spring-data-neo4j/src/main/java/org/springframework/data/neo4j/core/mapping/DefaultNeo4jEntityConverter.java

Line 440 in f10c95f

} else if (queryResult instanceof Node) {

add code below can solve this bug:

 else if (queryResult instanceof Relationship) {
	Relationship relationshipRepresentation = (Relationship) queryResult;
	labels.add(relationshipRepresentation.type());
}

[meistermeier]

I tried to understand the problem and your solution to this with your description and the pull request, but somewhere I lose track.
Rephrasing your situation and problem with my own words, to make sure I understand this correctly:
You are persisting a relationship of type relation with Relationship or AnotherRelationship. This means that the relationship type itself is already set and will be returned from the database accordingly (relation).
Given that on-save SDN exactly knows the instances and their properties to persist, you will get also all properties set.

Now when it comes to loading, SDN cannot determine in any way which was the exact class the relationship got constructed with. How should it, given your example? There is just a (:BaseNode)-[:relation]->(:BaseNode) pattern to inspect. That's also the reason why Spring Data Neo4j randomly picks one of those two types: It guesses from an unordered set which one fits best, and both implementations are equal. I don't know how this should be solved by the pull request. Maybe there is some missing information regarding the details in your initial post.

[yangyaofei]

Hi @meistermeier , let's me to explain it

For node, the SND can get Node with right types like:

@Node
public class BaseNode {
    @Id
    @GeneratedValue
    private UUID id;
}

@Node 
public class NodeA extend BaseNode{
    String a;
}

@Node 
public class NodeB extend BaseNode{
    String b;
}

@Repository
public interface BaseNodeRepository extends Neo4jRepository<BaseNode, UUID>,
        CypherdslConditionExecutor<BaseNode>,
        CypherdslStatementExecutor<BaseNode>,
        QuerydslPredicateExecutor<BaseNode> {
}

List<BaseNode> nodes = baseNodeRepository.findAll()
// the nodes may contains NodeA and NodeB, they can get proper class type

The nodes may contains NodeA and NodeB, they can get proper class type,
and this because they have labels and SDN can get the right type from it.

The code I think is :

spring-data-neo4j/src/main/java/org/springframework/data/neo4j/core/mapping/DefaultNeo4jEntityConverter.java

Lines 314 to 326 in f10c95f

	Supplier<ET> mappedObjectSupplier = () -> {
	knownObjects.setInCreation(internalId);
	List<String> allLabels = getLabels(queryResult, nodeDescription);
	NodeDescriptionAndLabels nodeDescriptionAndLabels = nodeDescriptionStore
	.deriveConcreteNodeDescription(nodeDescription, allLabels);
	@SuppressWarnings("unchecked")
	Neo4jPersistentEntity<ET> concreteNodeDescription = (Neo4jPersistentEntity<ET>) nodeDescriptionAndLabels
	.getNodeDescription();
	ET instance = instantiate(concreteNodeDescription, genericTargetNodeDescription, queryResult,
	nodeDescriptionAndLabels.getDynamicLabels(), lastMappedEntity, relationshipsFromResult, nodesFromResult);

The ET instance this the right instance SDN created for me. And the allLabels is the
type information to created it.

---

I think the relationship has same mechanism, so I write it like below:

@Node
public class BaseNode {
    @Id
    @GeneratedValue
    private UUID id;

    @Relationship(direction = Relationship.Direction.OUTGOING)
    private Map<String, List<BaseRelationship>> relationships = new HashMap<>();
}

@RelationshipProperties
public abstract class BaseRelationship {
    @RelationshipId
    @GeneratedValue
    private Long id;
    @TargetNode
    private BaseNode targetNode;
}
@RelationshipProperties
public class RelationshipA extend BaseRelationship{
    String a;
}
@RelationshipProperties
public class RelationshipAA extend RelationshipA{
    String aa;
}

@RelationshipProperties
public class RelationshipB extend BaseRelationship{
    String b;
}

I want the BaseNode can contains all kind of relationship even I don't know, but from BaseRelationship. And I can use it like:

relationships = {
    "relation_1": [RelationshipA, RelationshipB, RelationshipAA],
    "relation_2": [RelationshipA, RelationshipAA, RelationshipAA]
}

But, In this scenario, If I get the Node with relationships, I will get:

relationships = {
    "relation_1": [RelationshipAA, RelationshipAA, RelationshipAA],
    "relation_2": [RelationshipAA, RelationshipAA, RelationshipAA]
}

or

relationships = {
    "relation_1": [RelationshipB, RelationshipB, RelationshipB],
    "relation_2": [RelationshipB, RelationshipB, RelationshipB]
}

The type recognized by SDN is wrong, the code:

spring-data-neo4j/src/main/java/org/springframework/data/neo4j/core/mapping/DefaultNeo4jEntityConverter.java

Lines 314 to 326 in f10c95f

	Supplier<ET> mappedObjectSupplier = () -> {
	knownObjects.setInCreation(internalId);
	List<String> allLabels = getLabels(queryResult, nodeDescription);
	NodeDescriptionAndLabels nodeDescriptionAndLabels = nodeDescriptionStore
	.deriveConcreteNodeDescription(nodeDescription, allLabels);
	@SuppressWarnings("unchecked")
	Neo4jPersistentEntity<ET> concreteNodeDescription = (Neo4jPersistentEntity<ET>) nodeDescriptionAndLabels
	.getNodeDescription();
	ET instance = instantiate(concreteNodeDescription, genericTargetNodeDescription, queryResult,
	nodeDescriptionAndLabels.getDynamicLabels(), lastMappedEntity, relationshipsFromResult, nodesFromResult);

I dig into the code, found relationship type recognition use same code with node.
But the allLabels will be empty and the type in nodeDescription is BaseRelationship.

Because empty of allLabels, SDN downgrade to use the mechanism in Spring data to determine the type from all Entity (I don't exactly understand it). Here, I think I cannot get the right type from it.

But I found,when constructing relationship,the nodeDescription also has property label and the value is the SimpleClassName.
Because the relationship's declaration in BaseNode is BaseRelationship so it is always BaseRelationship before call nodeDescriptionStore.deriveConcreteNodeDescription.

For Node, it is same, before call nodeDescriptionStore.deriveConcreteNodeDescription the lable is BaseNode, and after deriveConcreteNodeDescription, it has all the labels.

And for relationship the queryResult has property type which value is simpleClassName, so I think If the function getLabels(queryResult, nodeDescription) can return the relationship's type, the code should be get right class. (SDN contains all entities (node and relationship) and convert to descriptions as cache, and use labels to find the proper type in it. )

And I test it, it works. so I create this issue and the pr

Now when it comes to loading, SDN cannot determine in any way which was the exact class the relationship got constructed with. How should it, given your example? There is just a (:BaseNode)-[:relation]->(:BaseNode) pattern to inspect. That's also the reason why Spring Data Neo4j randomly picks one of those two types: It guesses from an unordered set which one fits best, and both implementations are equal. I don't know how this should be solved by the pull request. Maybe there is some missing information regarding the details in your initial post.

I think this can answer your question

[yangyaofei]

You are persisting a relationship of type relation with Relationship or AnotherRelationship. This means that the relationship type itself is already set and will be returned from the database accordingly (relation).

I think the database do save the type of relationship, but different from node. the type of relation saved as __sr__ define in org.springframework.data.neo4j.core.mapping.Constants.NAME_OF_SYNTHESIZED_RELATIONS, and label save as NAME_OF_LABELS.

[meistermeier]

Given your example, I used your changes and created this test:

@Test
void testTypeMapping(@Autowired BaseNodeRepository baseNodeRepository) {
	var node = new BaseNode();
	RelationshipA e1 = new RelationshipA();
	e1.setTargetNode(new BaseNode());
	RelationshipB e2 = new RelationshipB();
	e2.setTargetNode(new BaseNode());
	node.setRelationships(Map.of(
		"relation", List.of(
			e1, e2
		)
	));
	var persistedNode = baseNodeRepository.save(node);

	var loadedNode = baseNodeRepository.findById(persistedNode.getId()).get();
	List<BaseRelationship> relationships = loadedNode.getRelationships().get("relation");
	assertThat(relationships).hasExactlyElementsOfTypes(RelationshipA.class, RelationshipB.class);
}

(Hope this is also your assumption)
It fails because even with the modified code, SDN decides on one type of relationship.

The only chance I see, how SDN could be helped determine which type to instantiate would be check on incoming properties on the relationship. There is only one type on relationships and because you are using the dynamic relationship approach, this will always be the same type for a list. There is just no chance to detect in this case what is the right relationship class to instantiate.
On any of your relationships defining a BaseNode target could be either a NodeA or NodeB. And even the thought about looking at the properties of the relationship properties class will only work in cases where there is incoming data that represents those properties. I am currently not convinced that the simple change in your PR does anything better. But I am happy to be proven wrong.

[yangyaofei]

@meistermeier hi, I check it again, I was wrong for this, I'm sorry for this.

Here is the right test for me, the relationship type from key in map, I misunderstand this
my project is too complex to hide this.

But the bug still here, if I set type, and set list of same type relations, still not get right instance. if the type is the simpleClassName, my PR works (but it's not good to solve this)

void testTypeMapping(@Autowired BaseNodeRepository baseNodeRepository) {
	var node = new BaseNode();
	RelationshipA a1 = new RelationshipA();
	RelationshipB b1 = new RelationshipB();
	a1.setTargetNode(new BaseNode());
	b1.setTargetNode(new BaseNode());
	node.setRelationships(Map.of(
		RelationshipA.class.getSimpleName(), List.of(a1), 
                RelationshipB.class.getSimpleName(), List.of(b1)
	));
	var persistedNode = baseNodeRepository.save(node);

	var loadedNode = baseNodeRepository.findById(persistedNode.getId()).get();
	List<BaseRelationship> relationshipsA = loadedNode.getRelationships().get(RelationshipA.class.getSimpleName());
	List<BaseRelationship> relationshipsB = loadedNode.getRelationships().get(RelationshipB.class.getSimpleName());
	assertThat(relationshipsA).hasExactlyElementsOfTypes(RelationshipA.class);
	assertThat(relationshipsB).hasExactlyElementsOfTypes(RelationshipB.class);
}

I am wander, is there any property for relation like label we can set the class type information in it?

[yangyaofei]

@meistermeier Hi, I update my PR, and that can fix all.

You can test with the code in the PR.

I add __relationshipType__ as Constants.NAME_OF_RELATIONSHIP_TYPE and write with relationship's primaryLabel in nodeDescription (which is className I think).

And When get labels to determine the type, get __relationshipType__ in getLabels.

With this change, we can get proper type in a list of relations.

[meistermeier]

I had a look at your update. Providing a type hint for keeping the information in place, is in general a good idea. Solving this by adding technical parameters as properties in the database on the other hand is nothing I want SDN to do.
The key aspect here is that we don't want people to be surprised by the created data that they did not intentionally provided in the first place or explicit declared within Spring Data Neo4j.
It makes sense for your situation where there is no possibility to give an additional hint to SDN. But the PR would write the __relationshipType__ to every relationship property entity, even to every node right now.
Had some thoughts about this, and tweaked your PR locally, but in the end I think that your generic model just cannot work as you expected with SDN without those additional information stored.
This is right now some early feedback because I don't want to discard the idea completely.
Thinking out loud: Maybe there is some solution with e.g. an annotation parameter on RelationshipProperties that explicit says that the type should be saved and we could jump on this information, as you do in your PR, later but keep it completely optional to opt-in.

[yangyaofei]

@meistermeier haha, I know you will say that. I admit that add a property is not a good way to do, but is the only way as you say.

I agree that adding an annotation parameter is a good idea, but I don't know how to do this.
If you give me some hint, maybe I can do this in my PR.

[meistermeier]

My idea would be that a boolean parameter gets added to the RelationshipProperties annotation that reflects the persistence of the type by its name (and javadoc), like persistTypeInformation, but I am happy if you come up with a better name. It defaults to false and could be used as a condition around your already committed changes. If not set, SDN would behave as it did before, if it is set, SDN will write the property and respect it on load.
Thanks for the effort in advance.

[yangyaofei]

@meistermeier I can imagine your idea, what I am confused is where to proper get the annotation property.

In public void write(Object source, Map<String, Object> parameters) ? get annotation from source?

spring-data-neo4j/src/main/java/org/springframework/data/neo4j/core/mapping/DefaultNeo4jEntityConverter.java

Line 231 in a6d15d2

public void write(Object source, Map<String, Object> parameters) {

Is it a proper way? If it is I can do this.

[meistermeier]

You have access to the Neo4jPersistentEntity. This should reflect, of course after you implemented it, the state of the flag.

spring-data-neo4j/src/main/java/org/springframework/data/neo4j/core/mapping/DefaultNeo4jEntityConverter.java

Line 234 in e5ac953

Neo4jPersistentEntity<?> nodeDescription = (Neo4jPersistentEntity<?>) nodeDescriptionStore

This would end up in something like

if (nodeDescription.hasRelationshipPropertyShouldPersistTypeInformationFlag())...

of course...naming is up to you 😃

[yangyaofei]

@meistermeier okay, I will try this tomorrow.

[meistermeier]

Take your time. And don't hesitate to ask questions or push unpolished code. We will support you.

[yangyaofei]

@meistermeier Hi, I pushed new commit to PR, you can check it