Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Storing a relation with @RelationshipProperties corrupts existing relations if the relation is modeled bidirectional #2905

Open
ma-ku opened this issue May 22, 2024 · 4 comments
Assignees
Labels
status: works-as-expected An issue that is not valid as the error or outcome is expected.

Comments

@ma-ku
Copy link

ma-ku commented May 22, 2024

In our project we have encountered a strange behavior with relations with additional properties when either
(a) the target class referenced by the @TargetNode is a base class with inheriting classes,
(b) or the target class has a back reference to the source node.

Both conditions lead to corrupted data when the second relation is stored to the same target node. The diagram below shows the case how data model below should be stored in the database:
image

This is a simplified set of classes that represents the problem while the actual code is a bit more complex. Also did we remove additional annotations for Lombok away to avoid cluttering the code:

@Node
public class BugFrom {
    @Id @GeneratedValue(UUIDStringGenerator.class)
    protected String uuid;

    private String name;

    @Relationship(type = "RELI", direction = Relationship.Direction.INCOMING)
    private BugRelationship reli;
}

@RelationshipProperties
public class BugRelationship  {
    @RelationshipId
    protected Long id;

    protected String comment;
    
    @TargetNode
    private BugTargetBase target;
}

@Node
public abstract class BugTargetBase {
    @Id @GeneratedValue(UUIDStringGenerator.class)
    @EqualsAndHashCode.Include
    protected String uuid;

    private String name;

    @Relationship(type = "RELI", direction = Relationship.Direction.OUTGOING)
    Set<BugFrom> relatedBugs;
}

@Node
public class BugTarget extends BugTargetBase { 
    private String type;
}

@Node
public class BugContainer extends BugTargetBase {
    private String externalSystemId;
    
    @Relationship(type = "INCLUDES", direction = Relationship.Direction.OUTGOING)
    private Set<BugTargetBase> components;
}

The following simple testcode help reproducing the problem:

var to1 = new BugTarget().builder().name("T1").type("BUG").build();		
to1 = toRepository.save(to1);

var from1 = new BugFrom().builder()
		.name("F1")
		.reli(BugRelationship.builder().target(to1).comment("F1<-T1").build())
		.build();
from1 = fromRepository.save(from1);

List<BugFrom> list = new ArrayList<>();		
list.add(from1);

var from2 = new BugFrom().builder()
		.name("F2")
		.reli(BugRelationship.builder().target(to1).comment("F2<-T1").build())
		.build();
list.add(from2);

var from3 = new BugFrom().builder()
		.name("F3")
		.reli(BugRelationship.builder().target(to1).comment("F3<-T1").build())
		.build();
list.add(from3);		
list = fromRepository.saveAll(list);

The above code only created the correct graph if either the property BugTargetBase in the relationship class is pointing to one of the deriving concrete classes, or if the reverse property relatedBugs in class BugTargetBase is deleted. Otherwise the second save operation creates the following graph where only the last saved relation exists and not all three relations as it would be expected.

image

We are using Spring Boot 3.2.1 with the corresponding starter packages but we could also reproduce the behavior with spring-data-neo4j version 7.3.0.

Question is if that is a bug or how this could be modeled so that we have relationships with properties pointing to nodes that are also pointing back to the other side of the relation.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 22, 2024
@michael-simons michael-simons self-assigned this May 23, 2024
@michael-simons
Copy link
Collaborator

Hi @ma-ku thanks for your report.

I have to say, "sorry, you're holding it wrong".

A couple of things:

  • On the entity storing the relationship properties (in your sample, BugRelationship) you forgot to annotated it with @RelationshipProperties. Without that, it won't be recognised as a "entity" (in tick marks, because SDN does not consider those as root entities) holding relationship properties, but only as a normal entity being stored, graph would than look as shown in the picture below
  • If you add the annotation and actually maintain the bidirectional relationships you defined, things do work as expected.

I will push a commit shortly demonstrating that and closing the issue.

The commit will contain two solutions: First, you're fixed solution, storing the related entities separately. I would however recommend you to follow a solution in which you just persist your root aggregate.

image

@michael-simons michael-simons added status: works-as-expected An issue that is not valid as the error or outcome is expected. and removed status: waiting-for-triage An issue we've not yet triaged labels May 23, 2024
@ma-ku
Copy link
Author

ma-ku commented May 23, 2024

OK, you are right if the annotation is missing but I unfortunately stripped this when I removed all the additional annotations such as Lombok and such. The classes are annotated with @Node and @RelationshipProperties as expected. So at least on our side it still is not working. Not sure what you are going to commit but I would like to see such an example somewhere working. Actually I believe that this is either a nasty edge case or that we have forgotten something in the config of the classes that makes this fail.

This error occurred after we had introduced the abstract base class for the target nodes as we now wanted to model them as nested containers of items. Before that, we only had one concrete target entity and everything was fine. So I would ask you to double check this particular scenario.

@michael-simons
Copy link
Collaborator

See 7d38286

I will be traveling the next 2 weeks, I would defer this than to @meistermeier next week.

@michael-simons
Copy link
Collaborator

Your test cases did not cover one important aspect; the relationships are still there in the sense that the total number is correct, but the properties stored with the relation get dropped. So we experience a loss of information on entities we do not touch and that is what this ticket was about. So maybe holding it wrong is not entirely correct here but I liked the approach how that was handled as a test case and provided more insight with this ticket. I would have appreciated a bit more communication so I could have clarified this instead of just rejecting it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: works-as-expected An issue that is not valid as the error or outcome is expected.
Projects
None yet
Development

No branches or pull requests

3 participants