Skip to content

Commit 058f573

Browse files
meistermeiermeistermeier
committed
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
1 parent cb61f79 commit 058f573

File tree

5 files changed

+128
-2
lines changed

5 files changed

+128
-2
lines changed

‎src/main/java/org/springframework/data/neo4j/core/Neo4jTemplate.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -676,7 +676,8 @@ private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Pers
676676
Long relatedInternalId;
677677
// No need to save values if processed
678678
if (stateMachine.hasProcessedValue(relatedValueToStore)) {
679-
relatedInternalId = queryRelatedNode(newRelatedObject, targetEntity);
679+
Object newRelatedObjectForQuery = stateMachine.getProcessedAs(newRelatedObject);
680+
relatedInternalId = queryRelatedNode(newRelatedObjectForQuery, targetEntity);
680681
} else {
681682
relatedInternalId = saveRelatedNode(newRelatedObject, targetEntity);
682683
}
@@ -712,6 +713,7 @@ private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Pers
712713
// if an internal id is used this must be set to link this entity in the next iteration
713714
if (targetEntity.isUsingInternalIds()) {
714715
targetPropertyAccessor.setProperty(targetEntity.getRequiredIdProperty(), relatedInternalId);
716+
stateMachine.markValueAsProcessedAs(newRelatedObject, targetPropertyAccessor.getBean());
715717
}
716718

717719
if (processState != ProcessState.PROCESSED_ALL_VALUES) {

‎src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jTemplate.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -772,7 +772,8 @@ private <T> Mono<T> processNestedRelations(Neo4jPersistentEntity<?> sourceEntity
772772

773773
Mono<Long> queryOrSave;
774774
if (stateMachine.hasProcessedValue(relatedValueToStore)) {
775-
queryOrSave = queryRelatedNode(newRelatedObject, targetEntity);
775+
Object newRelatedObjectForQuery = stateMachine.getProcessedAs(newRelatedObject);
776+
queryOrSave = queryRelatedNode(newRelatedObjectForQuery, targetEntity);
776777
} else {
777778
queryOrSave = saveRelatedNode(newRelatedObject, targetEntity);
778779
}
@@ -782,6 +783,7 @@ private <T> Mono<T> processNestedRelations(Neo4jPersistentEntity<?> sourceEntity
782783
PersistentPropertyAccessor<?> targetPropertyAccessor = targetEntity.getPropertyAccessor(newRelatedObject);
783784
if (targetEntity.isUsingInternalIds()) {
784785
targetPropertyAccessor.setProperty(targetEntity.getRequiredIdProperty(), relatedInternalId);
786+
stateMachine.markValueAsProcessedAs(newRelatedObject, targetPropertyAccessor.getBean());
785787
}
786788

787789
Object idValue = idProperty != null

‎src/main/java/org/springframework/data/neo4j/core/mapping/NestedRelationshipProcessingStateMachine.java

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@
1616
package org.springframework.data.neo4j.core.mapping;
1717

1818
import java.util.Collection;
19+
import java.util.HashMap;
1920
import java.util.HashSet;
21+
import java.util.Map;
2022
import java.util.Objects;
2123
import java.util.Set;
2224
import java.util.concurrent.locks.Lock;
@@ -56,6 +58,11 @@ public enum ProcessState {
5658
*/
5759
private final Set<Object> processedObjects = new HashSet<>();
5860

61+
/**
62+
* Set of aliases. e.g. One object without id set represents the same as the one with the id set.
63+
*/
64+
private final Map<Object, Object> processedObjectsAlias = new HashMap<>();
65+
5966
public NestedRelationshipProcessingStateMachine(Object initialObject) {
6067
processedObjects.add(initialObject);
6168
}
@@ -179,4 +186,22 @@ private boolean hasProcessedAllOf(@Nullable Collection<?> valuesToStore) {
179186
return processedObjects.containsAll(valuesToStore);
180187
}
181188

189+
public void markValueAsProcessedAs(Object relatedValueToStore, Object bean) {
190+
try {
191+
write.lock();
192+
processedObjectsAlias.put(relatedValueToStore, bean);
193+
} finally {
194+
write.unlock();
195+
}
196+
}
197+
198+
public Object getProcessedAs(Object entity) {
199+
try {
200+
read.lock();
201+
return processedObjectsAlias.getOrDefault(entity, entity);
202+
} finally {
203+
read.unlock();
204+
}
205+
}
206+
182207
}

‎src/test/java/org/springframework/data/neo4j/integration/imperative/ImmutableGeneratedIdsIT.java

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,11 @@
1515
*/
1616
package org.springframework.data.neo4j.integration.imperative;
1717

18+
import org.junit.jupiter.api.BeforeEach;
1819
import org.junit.jupiter.api.Test;
1920
import org.neo4j.driver.Driver;
21+
import org.neo4j.driver.Record;
22+
import org.neo4j.driver.Session;
2023
import org.springframework.beans.factory.annotation.Autowired;
2124
import org.springframework.context.annotation.Bean;
2225
import org.springframework.context.annotation.Configuration;
@@ -36,6 +39,7 @@
3639
import org.springframework.data.neo4j.test.Neo4jIntegrationTest;
3740
import org.springframework.transaction.annotation.EnableTransactionManagement;
3841

42+
import java.util.ArrayList;
3943
import java.util.Arrays;
4044
import java.util.Collection;
4145
import java.util.Collections;
@@ -54,6 +58,19 @@ public class ImmutableGeneratedIdsIT {
5458

5559
protected static Neo4jExtension.Neo4jConnectionSupport neo4jConnectionSupport;
5660

61+
private final Driver driver;
62+
63+
public ImmutableGeneratedIdsIT(@Autowired Driver driver) {
64+
this.driver = driver;
65+
}
66+
67+
@BeforeEach
68+
void cleanUp() {
69+
try (Session session = driver.session()) {
70+
session.run("MATCH (n) DETACH DELETE n").consume();
71+
}
72+
}
73+
5774
@Test // GH-2141
5875
void saveWithGeneratedIdsReturnsObjectWithIdSet(
5976
@Autowired ImmutablePersonWithGeneratedIdRepository repository) {
@@ -295,6 +312,36 @@ void childrenShouldNotBeRecreatedForNoReasons(@Autowired Neo4jTemplate template)
295312
assertThat(saved.getChildren()).allMatch(c -> c.getId() != null && children.contains(c));
296313
}
297314

315+
@Test // GH-2223
316+
void saveWithGeneratedIdsWithMultipleRelationshipsToOneNode(
317+
@Autowired ImmutablePersonWithGeneratedIdRepository repository) {
318+
319+
ImmutablePersonWithGeneratedId person1 = new ImmutablePersonWithGeneratedId();
320+
ImmutablePersonWithGeneratedId person2 = ImmutablePersonWithGeneratedId.fallback(person1);
321+
List<ImmutablePersonWithGeneratedId> onboardedBy = new ArrayList<>();
322+
onboardedBy.add(person1);
323+
onboardedBy.add(person2);
324+
ImmutablePersonWithGeneratedId person3 = ImmutablePersonWithGeneratedId.wasOnboardedBy(onboardedBy);
325+
326+
ImmutablePersonWithGeneratedId savedPerson = repository.save(person3);
327+
assertThat(savedPerson.id).isNotNull();
328+
assertThat(savedPerson.wasOnboardedBy).allMatch(ob -> ob.id != null);
329+
330+
ImmutablePersonWithGeneratedId savedPerson2 = savedPerson.wasOnboardedBy.stream().filter(p -> p.fallback != null).findFirst().get();
331+
assertThat(savedPerson2.fallback.id).isNotNull();
332+
333+
try (Session session = driver.session()) {
334+
List<Record> result = session.run(
335+
"MATCH (person3:ImmutablePersonWithGeneratedId) " +
336+
"-[:ONBOARDED_BY]->(person2:ImmutablePersonWithGeneratedId) " +
337+
"-[:FALLBACK]->(person1:ImmutablePersonWithGeneratedId), " +
338+
"(person3)-[:ONBOARDED_BY]->(person1) " +
339+
"return person3")
340+
.list();
341+
assertThat(result).hasSize(1);
342+
}
343+
}
344+
298345
@Configuration
299346
@EnableNeo4jRepositories(considerNestedRepositories = true)
300347
@EnableTransactionManagement

‎src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveImmutableGeneratedIdsIT.java

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,12 @@
1717

1818
import static org.assertj.core.api.Assertions.assertThat;
1919

20+
import org.junit.jupiter.api.BeforeEach;
21+
import org.neo4j.driver.Record;
22+
import org.neo4j.driver.Session;
2023
import reactor.test.StepVerifier;
2124

25+
import java.util.ArrayList;
2226
import java.util.Arrays;
2327
import java.util.Collections;
2428
import java.util.List;
@@ -54,6 +58,19 @@ public class ReactiveImmutableGeneratedIdsIT {
5458

5559
protected static Neo4jExtension.Neo4jConnectionSupport neo4jConnectionSupport;
5660

61+
private final Driver driver;
62+
63+
public ReactiveImmutableGeneratedIdsIT(@Autowired Driver driver) {
64+
this.driver = driver;
65+
}
66+
67+
@BeforeEach
68+
void cleanUp() {
69+
try (Session session = driver.session()) {
70+
session.run("MATCH (n) DETACH DELETE n").consume();
71+
}
72+
}
73+
5774
@Test // GH-2141
5875
void saveWithGeneratedIdsReturnsObjectWithIdSet(
5976
@Autowired ReactiveImmutablePersonWithGeneratedIdRepository repository) {
@@ -302,6 +319,39 @@ void childrenShouldNotBeRecreatedForNoReasons(@Autowired ReactiveNeo4jTemplate t
302319
.verifyComplete();
303320
}
304321

322+
@Test // GH-2223
323+
void saveWithGeneratedIdsWithMultipleRelationshipsToOneNode(
324+
@Autowired ReactiveImmutablePersonWithGeneratedIdRepository repository) {
325+
326+
ImmutablePersonWithGeneratedId person1 = new ImmutablePersonWithGeneratedId();
327+
ImmutablePersonWithGeneratedId person2 = ImmutablePersonWithGeneratedId.fallback(person1);
328+
List<ImmutablePersonWithGeneratedId> onboardedBy = new ArrayList<>();
329+
onboardedBy.add(person1);
330+
onboardedBy.add(person2);
331+
ImmutablePersonWithGeneratedId person3 = ImmutablePersonWithGeneratedId.wasOnboardedBy(onboardedBy);
332+
333+
StepVerifier.create(repository.save(person3))
334+
.assertNext(savedPerson -> {
335+
assertThat(savedPerson.id).isNotNull();
336+
assertThat(savedPerson.wasOnboardedBy).allMatch(ob -> ob.id != null);
337+
338+
ImmutablePersonWithGeneratedId savedPerson2 = savedPerson.wasOnboardedBy.stream().filter(p -> p.fallback != null).findFirst().get();
339+
assertThat(savedPerson2.fallback.id).isNotNull();
340+
})
341+
.verifyComplete();
342+
343+
try (Session session = driver.session()) {
344+
List<Record> result = session.run(
345+
"MATCH (person3:ImmutablePersonWithGeneratedId) " +
346+
"-[:ONBOARDED_BY]->(person2:ImmutablePersonWithGeneratedId) " +
347+
"-[:FALLBACK]->(person1:ImmutablePersonWithGeneratedId), " +
348+
"(person3)-[:ONBOARDED_BY]->(person1) " +
349+
"return person3")
350+
.list();
351+
assertThat(result).hasSize(1);
352+
}
353+
}
354+
305355
@Configuration
306356
@EnableReactiveNeo4jRepositories(considerNestedRepositories = true)
307357
@EnableTransactionManagement

0 commit comments

Comments
 (0)