From 21f9d3338ee35d49d0fd180bebb39c9a29943521 Mon Sep 17 00:00:00 2001 From: Gerrit Meier Date: Tue, 20 Apr 2021 13:01:38 +0200 Subject: [PATCH] GH-2223 - Consider created objects as processed. onSave immutable entities will get re-created if they have generated ids. In those cases if a node gets referenced more than once (e.g. A->B->C and A->C) the resulting entities will not be considered as the same because one has already an identifier set, the other not yet. In thoses cases the logic either persisted a duplicate or it could not find the already processed entity because the identifier of the entity in question is still `null`. Closes #2223 --- .../data/neo4j/core/Neo4jTemplate.java | 4 +- .../neo4j/core/ReactiveNeo4jTemplate.java | 4 +- ...tedRelationshipProcessingStateMachine.java | 25 ++++++++++ .../imperative/ImmutableGeneratedIdsIT.java | 47 +++++++++++++++++ .../ReactiveImmutableGeneratedIdsIT.java | 50 +++++++++++++++++++ 5 files changed, 128 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/springframework/data/neo4j/core/Neo4jTemplate.java b/src/main/java/org/springframework/data/neo4j/core/Neo4jTemplate.java index 60ac8d2a8..b0d426b9a 100644 --- a/src/main/java/org/springframework/data/neo4j/core/Neo4jTemplate.java +++ b/src/main/java/org/springframework/data/neo4j/core/Neo4jTemplate.java @@ -676,7 +676,8 @@ public final class Neo4jTemplate implements Neo4jOperations, FluentNeo4jOperatio Long relatedInternalId; // No need to save values if processed if (stateMachine.hasProcessedValue(relatedValueToStore)) { - relatedInternalId = queryRelatedNode(newRelatedObject, targetEntity); + Object newRelatedObjectForQuery = stateMachine.getProcessedAs(newRelatedObject); + relatedInternalId = queryRelatedNode(newRelatedObjectForQuery, targetEntity); } else { relatedInternalId = saveRelatedNode(newRelatedObject, targetEntity); } @@ -712,6 +713,7 @@ public final class Neo4jTemplate implements Neo4jOperations, FluentNeo4jOperatio // if an internal id is used this must be set to link this entity in the next iteration if (targetEntity.isUsingInternalIds()) { targetPropertyAccessor.setProperty(targetEntity.getRequiredIdProperty(), relatedInternalId); + stateMachine.markValueAsProcessedAs(newRelatedObject, targetPropertyAccessor.getBean()); } if (processState != ProcessState.PROCESSED_ALL_VALUES) { diff --git a/src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jTemplate.java b/src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jTemplate.java index 7ea8a0868..d3b8b592f 100644 --- a/src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jTemplate.java +++ b/src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jTemplate.java @@ -772,7 +772,8 @@ public final class ReactiveNeo4jTemplate implements ReactiveNeo4jOperations, Rea Mono queryOrSave; if (stateMachine.hasProcessedValue(relatedValueToStore)) { - queryOrSave = queryRelatedNode(newRelatedObject, targetEntity); + Object newRelatedObjectForQuery = stateMachine.getProcessedAs(newRelatedObject); + queryOrSave = queryRelatedNode(newRelatedObjectForQuery, targetEntity); } else { queryOrSave = saveRelatedNode(newRelatedObject, targetEntity); } @@ -782,6 +783,7 @@ public final class ReactiveNeo4jTemplate implements ReactiveNeo4jOperations, Rea PersistentPropertyAccessor targetPropertyAccessor = targetEntity.getPropertyAccessor(newRelatedObject); if (targetEntity.isUsingInternalIds()) { targetPropertyAccessor.setProperty(targetEntity.getRequiredIdProperty(), relatedInternalId); + stateMachine.markValueAsProcessedAs(newRelatedObject, targetPropertyAccessor.getBean()); } Object idValue = idProperty != null diff --git a/src/main/java/org/springframework/data/neo4j/core/mapping/NestedRelationshipProcessingStateMachine.java b/src/main/java/org/springframework/data/neo4j/core/mapping/NestedRelationshipProcessingStateMachine.java index 8f7ad25b8..028ae1727 100644 --- a/src/main/java/org/springframework/data/neo4j/core/mapping/NestedRelationshipProcessingStateMachine.java +++ b/src/main/java/org/springframework/data/neo4j/core/mapping/NestedRelationshipProcessingStateMachine.java @@ -16,7 +16,9 @@ package org.springframework.data.neo4j.core.mapping; import java.util.Collection; +import java.util.HashMap; import java.util.HashSet; +import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.concurrent.locks.Lock; @@ -56,6 +58,11 @@ public final class NestedRelationshipProcessingStateMachine { */ private final Set processedObjects = new HashSet<>(); + /** + * Set of aliases. e.g. One object without id set represents the same as the one with the id set. + */ + private final Map processedObjectsAlias = new HashMap<>(); + public NestedRelationshipProcessingStateMachine(Object initialObject) { processedObjects.add(initialObject); } @@ -179,4 +186,22 @@ public final class NestedRelationshipProcessingStateMachine { return processedObjects.containsAll(valuesToStore); } + public void markValueAsProcessedAs(Object relatedValueToStore, Object bean) { + try { + write.lock(); + processedObjectsAlias.put(relatedValueToStore, bean); + } finally { + write.unlock(); + } + } + + public Object getProcessedAs(Object entity) { + try { + read.lock(); + return processedObjectsAlias.getOrDefault(entity, entity); + } finally { + read.unlock(); + } + } + } diff --git a/src/test/java/org/springframework/data/neo4j/integration/imperative/ImmutableGeneratedIdsIT.java b/src/test/java/org/springframework/data/neo4j/integration/imperative/ImmutableGeneratedIdsIT.java index 79ecfead5..25d8db69e 100644 --- a/src/test/java/org/springframework/data/neo4j/integration/imperative/ImmutableGeneratedIdsIT.java +++ b/src/test/java/org/springframework/data/neo4j/integration/imperative/ImmutableGeneratedIdsIT.java @@ -15,8 +15,11 @@ */ package org.springframework.data.neo4j.integration.imperative; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.neo4j.driver.Driver; +import org.neo4j.driver.Record; +import org.neo4j.driver.Session; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @@ -36,6 +39,7 @@ import org.springframework.data.neo4j.test.Neo4jExtension; import org.springframework.data.neo4j.test.Neo4jIntegrationTest; import org.springframework.transaction.annotation.EnableTransactionManagement; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -54,6 +58,19 @@ public class ImmutableGeneratedIdsIT { protected static Neo4jExtension.Neo4jConnectionSupport neo4jConnectionSupport; + private final Driver driver; + + public ImmutableGeneratedIdsIT(@Autowired Driver driver) { + this.driver = driver; + } + + @BeforeEach + void cleanUp() { + try (Session session = driver.session()) { + session.run("MATCH (n) DETACH DELETE n").consume(); + } + } + @Test // GH-2141 void saveWithGeneratedIdsReturnsObjectWithIdSet( @Autowired ImmutablePersonWithGeneratedIdRepository repository) { @@ -295,6 +312,36 @@ public class ImmutableGeneratedIdsIT { assertThat(saved.getChildren()).allMatch(c -> c.getId() != null && children.contains(c)); } + @Test // GH-2223 + void saveWithGeneratedIdsWithMultipleRelationshipsToOneNode( + @Autowired ImmutablePersonWithGeneratedIdRepository repository) { + + ImmutablePersonWithGeneratedId person1 = new ImmutablePersonWithGeneratedId(); + ImmutablePersonWithGeneratedId person2 = ImmutablePersonWithGeneratedId.fallback(person1); + List onboardedBy = new ArrayList<>(); + onboardedBy.add(person1); + onboardedBy.add(person2); + ImmutablePersonWithGeneratedId person3 = ImmutablePersonWithGeneratedId.wasOnboardedBy(onboardedBy); + + ImmutablePersonWithGeneratedId savedPerson = repository.save(person3); + assertThat(savedPerson.id).isNotNull(); + assertThat(savedPerson.wasOnboardedBy).allMatch(ob -> ob.id != null); + + ImmutablePersonWithGeneratedId savedPerson2 = savedPerson.wasOnboardedBy.stream().filter(p -> p.fallback != null).findFirst().get(); + assertThat(savedPerson2.fallback.id).isNotNull(); + + try (Session session = driver.session()) { + List result = session.run( + "MATCH (person3:ImmutablePersonWithGeneratedId) " + + "-[:ONBOARDED_BY]->(person2:ImmutablePersonWithGeneratedId) " + + "-[:FALLBACK]->(person1:ImmutablePersonWithGeneratedId), " + + "(person3)-[:ONBOARDED_BY]->(person1) " + + "return person3") + .list(); + assertThat(result).hasSize(1); + } + } + @Configuration @EnableNeo4jRepositories(considerNestedRepositories = true) @EnableTransactionManagement diff --git a/src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveImmutableGeneratedIdsIT.java b/src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveImmutableGeneratedIdsIT.java index 37265141c..0d7e079f4 100644 --- a/src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveImmutableGeneratedIdsIT.java +++ b/src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveImmutableGeneratedIdsIT.java @@ -17,8 +17,12 @@ package org.springframework.data.neo4j.integration.reactive; import static org.assertj.core.api.Assertions.assertThat; +import org.junit.jupiter.api.BeforeEach; +import org.neo4j.driver.Record; +import org.neo4j.driver.Session; import reactor.test.StepVerifier; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -54,6 +58,19 @@ public class ReactiveImmutableGeneratedIdsIT { protected static Neo4jExtension.Neo4jConnectionSupport neo4jConnectionSupport; + private final Driver driver; + + public ReactiveImmutableGeneratedIdsIT(@Autowired Driver driver) { + this.driver = driver; + } + + @BeforeEach + void cleanUp() { + try (Session session = driver.session()) { + session.run("MATCH (n) DETACH DELETE n").consume(); + } + } + @Test // GH-2141 void saveWithGeneratedIdsReturnsObjectWithIdSet( @Autowired ReactiveImmutablePersonWithGeneratedIdRepository repository) { @@ -302,6 +319,39 @@ public class ReactiveImmutableGeneratedIdsIT { .verifyComplete(); } + @Test // GH-2223 + void saveWithGeneratedIdsWithMultipleRelationshipsToOneNode( + @Autowired ReactiveImmutablePersonWithGeneratedIdRepository repository) { + + ImmutablePersonWithGeneratedId person1 = new ImmutablePersonWithGeneratedId(); + ImmutablePersonWithGeneratedId person2 = ImmutablePersonWithGeneratedId.fallback(person1); + List onboardedBy = new ArrayList<>(); + onboardedBy.add(person1); + onboardedBy.add(person2); + ImmutablePersonWithGeneratedId person3 = ImmutablePersonWithGeneratedId.wasOnboardedBy(onboardedBy); + + StepVerifier.create(repository.save(person3)) + .assertNext(savedPerson -> { + assertThat(savedPerson.id).isNotNull(); + assertThat(savedPerson.wasOnboardedBy).allMatch(ob -> ob.id != null); + + ImmutablePersonWithGeneratedId savedPerson2 = savedPerson.wasOnboardedBy.stream().filter(p -> p.fallback != null).findFirst().get(); + assertThat(savedPerson2.fallback.id).isNotNull(); + }) + .verifyComplete(); + + try (Session session = driver.session()) { + List result = session.run( + "MATCH (person3:ImmutablePersonWithGeneratedId) " + + "-[:ONBOARDED_BY]->(person2:ImmutablePersonWithGeneratedId) " + + "-[:FALLBACK]->(person1:ImmutablePersonWithGeneratedId), " + + "(person3)-[:ONBOARDED_BY]->(person1) " + + "return person3") + .list(); + assertThat(result).hasSize(1); + } + } + @Configuration @EnableReactiveNeo4jRepositories(considerNestedRepositories = true) @EnableTransactionManagement