Skip to content

Support multiple relationships properties to same node #2228

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

Closed
VladoKuruc opened this issue Apr 14, 2021 · 7 comments
Closed

Support multiple relationships properties to same node #2228

VladoKuruc opened this issue Apr 14, 2021 · 7 comments
Assignees
Labels
type: bug A general bug

Comments

@VladoKuruc
Copy link

In my case, I need a fully hydrated Node for modifying and save() without data loss.
My datas has more @RelationshipProperties between same nodes.

To simulate this I modified existing unit test:
org.springframework.data.neo4j.integration.imperative.RepositoryIT.RelationshipProperties

		@Test
		void loadSameNodeWithDoubleRelationship(@Autowired HobbyWithRelationshipWithPropertiesRepository repository) {
			long personId;

			try (Session session = createSession()) {
				Record record = session.run("CREATE (n:AltPerson{name:'Freddie'})," +
						" (n)-[l1:LIKES {rating: 5}]->(h1:AltHobby{name:'Music'})," +
						" (n)-[l2:LIKES {rating: 1}]->(h1)" +
						" RETURN n, h1").single();
				personId = record.get("n").asNode().id();
			}

			AltHobby hobby = repository.loadFromCustomQuery(personId);
			assertThat(hobby.getName()).isEqualTo("Music");
			List<AltLikedByPersonRelationship> likedBy = hobby.getLikedBy();
			assertThat(likedBy).hasSize(2);

			AltPerson altPerson = new AltPerson("Freddie");
			altPerson.setId(personId);
			AltLikedByPersonRelationship rel1 = new AltLikedByPersonRelationship();
			rel1.setRating(5);
			rel1.setAltPerson(altPerson);

			AltLikedByPersonRelationship rel2 = new AltLikedByPersonRelationship();
			rel2.setRating(1);
			rel2.setAltPerson(altPerson);

			assertThat(likedBy).containsExactlyInAnyOrder(rel1, rel2);

			Optional<AltHobby> optHobby = repository.findById(hobby.getId());
			assertThat(optHobby.isPresent()).isTrue();
			hobby = optHobby.get();			
			assertThat(hobby.getName()).isEqualTo("Music");
			likedBy = hobby.getLikedBy();
			assertThat(likedBy).hasSize(2);
			assertThat(likedBy).containsExactlyInAnyOrder(rel1, rel2);
		}

Everything is OK until I add to AltHobby node new property with getter()

	@Relationship(type = "CHILD", direction = Relationship.Direction.INCOMING)
	private List<AltHobby> memberOf = new ArrayList<>();

	public List<AltHobby> getMemberOf() {
		return memberOf;
	}

Then I have error on assertThat(likedBy).hasSize(2)

java.lang.AssertionError: Expected size:<2> but was:<1> in: <[org.springframework.data.neo4j.integration.shared.common.AltLikedByPersonRelationship@3f3af232]> at org.springframework.data.neo4j.integration.imperative.RepositoryIT$RelationshipProperties.loadSameNodeWithDoubleRelationship(RepositoryIT.java:1570)

My model has more complex mappings but this is the simple case where this error is visible. Maybe is there another way how to change and save node without data lose and loading whole tree of relationships.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 14, 2021
@meistermeier meistermeier added the status: needs-investigation An issue that has been triaged but needs further investigation label Apr 14, 2021
@meistermeier meistermeier self-assigned this Apr 14, 2021
@meistermeier meistermeier removed the status: waiting-for-triage An issue we've not yet triaged label Apr 14, 2021
@meistermeier
Copy link
Collaborator

Thanks for reporting this. I can reproduce it with the expanded test case, you provided.

@meistermeier
Copy link
Collaborator

We target this issue with another bugfix. If you want, you could give 6.0.9-SNAPSHOT a try and tell us if it is working for you.
(Your test ^^ passes now)

@meistermeier meistermeier added blocked: awaiting feedback and removed status: needs-investigation An issue that has been triaged but needs further investigation labels May 3, 2021
@VladoKuruc
Copy link
Author

Sorry, but that test doesn't pass even with the latest changes. I've tried both the 6.0.x branch and main. Maybe you missed here the changes in the AltHobby class

@meistermeier
Copy link
Collaborator

Maybe you missed here the changes in the AltHobby class

I did in the last run...back to the drawing board.

@meistermeier meistermeier added status: needs-investigation An issue that has been triaged but needs further investigation and removed blocked: awaiting feedback labels May 4, 2021
meistermeier added a commit that referenced this issue May 5, 2021
meistermeier added a commit that referenced this issue May 5, 2021
meistermeier added a commit that referenced this issue May 5, 2021
When using relationship properties, it is possible to map towards the same node multiple times.
While loading this was not supported because the logic assumed that after one relationship was found, the mapping to this node is done.
@meistermeier
Copy link
Collaborator

Ok, finally. I somehow forgot about the change on my machine.
Just for you, fresh from the build pipeline: 6.0.9-GH-2228-SNAPSHOT.
If this is working for you, I am happy to merge this in for the next patch release.

@meistermeier meistermeier added blocked: awaiting feedback and removed status: needs-investigation An issue that has been triaged but needs further investigation labels May 5, 2021
@VladoKuruc
Copy link
Author

I can confirm that this error no longer occurs even in my more complex model

@meistermeier
Copy link
Collaborator

Great to hear and thanks for your feedback.

@meistermeier meistermeier changed the title findById () does not return Node with all RelationProperties Support multiple relationships properties to same node May 5, 2021
@meistermeier meistermeier added this to the 6.0.9 (2020.0.9) milestone May 5, 2021
meistermeier added a commit that referenced this issue May 6, 2021
When using relationship properties, it is possible to map towards the same node multiple times.
While loading this was not supported because the logic assumed that after one relationship was found, the mapping to this node is done.
meistermeier added a commit that referenced this issue May 6, 2021
When using relationship properties, it is possible to map towards the same node multiple times.
While loading this was not supported because the logic assumed that after one relationship was found, the mapping to this node is done.

Closes #2228
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants