Skip to content

Commit 319a84d

Browse files
committed
GH-2191 - Fix state machine for more complex scenarios.
1 parent a211951 commit 319a84d

File tree

5 files changed

+160
-29
lines changed

5 files changed

+160
-29
lines changed

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

+8-7
Original file line numberDiff line numberDiff line change
@@ -482,14 +482,15 @@ private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Obje
482482
}
483483

484484
// break recursive procession and deletion of previously created relationships
485-
ProcessState processState = stateMachine.getStateOf(relationshipDescriptionObverse, relatedValuesToStore);
485+
ProcessState processState = stateMachine.getStateOf(fromId, relationshipDescriptionObverse, relatedValuesToStore);
486486
if (processState == ProcessState.PROCESSED_ALL_RELATIONSHIPS || processState == ProcessState.PROCESSED_BOTH) {
487487
return;
488488
}
489489

490-
// remove all relationships before creating all new if the entity is not new
491-
// this avoids the usage of cache but might have significant impact on overall performance
492-
if (!isParentObjectNew) {
490+
// Remove all relationships before creating all new if the entity is not new and the relationship
491+
// has not been processed before.
492+
// This avoids the usage of cache but might have significant impact on overall performance
493+
if (!isParentObjectNew && !stateMachine.hasProcessedRelationship(fromId, relationshipDescription)) {
493494

494495
List<Long> knownRelationshipsIds = new ArrayList<>();
495496
if (idProperty != null) {
@@ -520,7 +521,7 @@ private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Obje
520521
return;
521522
}
522523

523-
stateMachine.markAsProcessed(relationshipDescription, relatedValuesToStore);
524+
stateMachine.markRelationshipAsProcessed(fromId, relationshipDescription);
524525

525526
for (Object relatedValueToStore : relatedValuesToStore) {
526527

@@ -534,12 +535,13 @@ private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Obje
534535

535536
Long relatedInternalId;
536537
// No need to save values if processed
537-
if (processState == ProcessState.PROCESSED_ALL_VALUES) {
538+
if (stateMachine.hasProcessedValue(relatedValueToStore)) {
538539
relatedInternalId = queryRelatedNode(relatedNode, targetEntity);
539540
} else {
540541
relatedInternalId = saveRelatedNode(relatedNode, relationshipContext.getAssociationTargetType(),
541542
targetEntity);
542543
}
544+
stateMachine.markValueAsProcessed(relatedValueToStore);
543545

544546
CreateRelationshipStatementHolder statementHolder = neo4jMappingContext.createStatement(
545547
sourceEntity, relationshipContext, relatedValueToStore);
@@ -568,7 +570,6 @@ private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Obje
568570
}
569571
}
570572

571-
572573
});
573574

574575
return (T) propertyAccessor.getBean();

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

+8-6
Original file line numberDiff line numberDiff line change
@@ -602,14 +602,15 @@ private Mono<Void> processNestedRelations(Neo4jPersistentEntity<?> sourceEntity,
602602
}
603603

604604
// break recursive procession and deletion of previously created relationships
605-
ProcessState processState = stateMachine.getStateOf(relationshipDescriptionObverse, relatedValuesToStore);
605+
ProcessState processState = stateMachine.getStateOf(fromId, relationshipDescriptionObverse, relatedValuesToStore);
606606
if (processState == ProcessState.PROCESSED_ALL_RELATIONSHIPS || processState == ProcessState.PROCESSED_BOTH) {
607607
return;
608608
}
609609

610-
// remove all relationships before creating all new if the entity is not new
611-
// this avoids the usage of cache but might have significant impact on overall performance
612-
if (!isParentObjectNew) {
610+
// Remove all relationships before creating all new if the entity is not new and the relationship
611+
// has not been processed before.
612+
// This avoids the usage of cache but might have significant impact on overall performance
613+
if (!isParentObjectNew && !stateMachine.hasProcessedRelationship(fromId, relationshipDescription)) {
613614

614615
List<Long> knownRelationshipsIds = new ArrayList<>();
615616
if (idProperty != null) {
@@ -643,7 +644,7 @@ private Mono<Void> processNestedRelations(Neo4jPersistentEntity<?> sourceEntity,
643644
return;
644645
}
645646

646-
stateMachine.markAsProcessed(relationshipDescription, relatedValuesToStore);
647+
stateMachine.markRelationshipAsProcessed(fromId, relationshipDescription);
647648

648649
for (Object relatedValueToStore : relatedValuesToStore) {
649650

@@ -656,12 +657,13 @@ private Mono<Void> processNestedRelations(Neo4jPersistentEntity<?> sourceEntity,
656657
return Mono.just(targetEntity.isNew(relatedNode)).flatMap(isNew -> {
657658
Mono<Long> relatedIdMono;
658659

659-
if (processState == ProcessState.PROCESSED_ALL_VALUES) {
660+
if (stateMachine.hasProcessedValue(relatedValueToStore)) {
660661
relatedIdMono = queryRelatedNode(relatedNode, targetEntity);
661662
} else {
662663
relatedIdMono = saveRelatedNode(relatedNode, relationshipContext.getAssociationTargetType(),
663664
targetEntity);
664665
}
666+
stateMachine.markValueAsProcessed(relatedValueToStore);
665667
return relatedIdMono.flatMap(relatedInternalId -> {
666668

667669
// if an internal id is used this must get set to link this entity in the next iteration

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

+76-16
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
import java.util.Collection;
1919
import java.util.HashSet;
20+
import java.util.Objects;
2021
import java.util.Set;
2122
import java.util.concurrent.locks.Lock;
2223
import java.util.concurrent.locks.ReentrantReadWriteLock;
@@ -48,7 +49,7 @@ public enum ProcessState {
4849
/**
4950
* The set of already processed relationships.
5051
*/
51-
private final Set<RelationshipDescription> processedRelationshipDescriptions = new HashSet<>();
52+
private final Set<RelationshipDescriptionWithSourceId> processedRelationshipDescriptions = new HashSet<>();
5253

5354
/**
5455
* The set of already processed related objects.
@@ -64,11 +65,11 @@ public NestedRelationshipProcessingStateMachine(Object initialObject) {
6465
* @param valuesToStore Check whether all the values in the collection have been processed
6566
* @return The state of things processed
6667
*/
67-
public ProcessState getStateOf(RelationshipDescription relationshipDescription, @Nullable Collection<?> valuesToStore) {
68+
public ProcessState getStateOf(Object fromId, RelationshipDescription relationshipDescription, @Nullable Collection<?> valuesToStore) {
6869

6970
try {
7071
read.lock();
71-
boolean hasProcessedRelationship = hasProcessed(relationshipDescription);
72+
boolean hasProcessedRelationship = hasProcessedRelationship(fromId, relationshipDescription);
7273
boolean hasProcessedAllValues = hasProcessedAllOf(valuesToStore);
7374
if (hasProcessedRelationship && hasProcessedAllValues) {
7475
return ProcessState.PROCESSED_BOTH;
@@ -85,38 +86,97 @@ public ProcessState getStateOf(RelationshipDescription relationshipDescription,
8586
}
8687
}
8788

89+
/**
90+
* Combination of relationship description and fromId to differentiate between `equals`-wise equal relationship
91+
* descriptions by their source identifier. This is needed because sometimes the very same relationship definition
92+
* can get processed for different objects of the same entity.
93+
* One could say that this is a Tuple but it has a nicer name.
94+
*/
95+
private static class RelationshipDescriptionWithSourceId {
96+
private final Object id;
97+
private final RelationshipDescription relationshipDescription;
98+
99+
RelationshipDescriptionWithSourceId(Object id, RelationshipDescription relationshipDescription) {
100+
this.id = id;
101+
this.relationshipDescription = relationshipDescription;
102+
}
103+
104+
@Override
105+
public boolean equals(Object o) {
106+
if (this == o) {
107+
return true;
108+
}
109+
if (o == null || getClass() != o.getClass()) {
110+
return false;
111+
}
112+
RelationshipDescriptionWithSourceId that = (RelationshipDescriptionWithSourceId) o;
113+
return id.equals(that.id) && relationshipDescription.equals(that.relationshipDescription);
114+
}
115+
116+
@Override
117+
public int hashCode() {
118+
return Objects.hash(id, relationshipDescription);
119+
}
120+
}
121+
88122
/**
89123
* Marks the passed objects as processed
90124
*
91125
* @param relationshipDescription To be marked as processed
92-
* @param valuesToStore If not {@literal null}, all non-null values will be marked as processed
93126
*/
94-
public void markAsProcessed(RelationshipDescription relationshipDescription, @Nullable Collection<?> valuesToStore) {
127+
public void markRelationshipAsProcessed(Object fromId, RelationshipDescription relationshipDescription) {
95128

96129
try {
97130
write.lock();
98-
this.processedRelationshipDescriptions.add(relationshipDescription);
99-
if (valuesToStore != null) {
100-
valuesToStore.stream().filter(v -> v != null).forEach(processedObjects::add);
101-
}
131+
this.processedRelationshipDescriptions.add(new RelationshipDescriptionWithSourceId(fromId, relationshipDescription));
102132
} finally {
103133
write.unlock();
104134
}
105135
}
136+
/**
137+
* Marks the passed objects as processed
138+
*
139+
* @param valueToStore If not {@literal null}, all non-null values will be marked as processed
140+
*/
141+
public void markValueAsProcessed(Object valueToStore) {
106142

107-
private boolean hasProcessedAllOf(@Nullable Collection<?> valuesToStore) {
108-
// there can be null elements in the unified collection of values to store.
109-
if (valuesToStore == null) {
110-
return false;
143+
try {
144+
write.lock();
145+
this.processedObjects.add(valueToStore);
146+
} finally {
147+
write.unlock();
111148
}
112-
return processedObjects.containsAll(valuesToStore);
113149
}
114150

115-
private boolean hasProcessed(RelationshipDescription relationshipDescription) {
151+
/**
152+
* Checks if the value has already been processed.
153+
*
154+
* @param value the object that should be looked for in the registry.
155+
* @return processed yes (true) / no (false)
156+
*/
157+
public boolean hasProcessedValue(Object value) {
158+
return processedObjects.contains(value);
159+
}
116160

161+
/**
162+
* Checks if the relationship has already been processed.
163+
*
164+
* @param relationshipDescription the relationship that should be looked for in the registry.
165+
* @return processed yes (true) / no (false)
166+
*/
167+
public boolean hasProcessedRelationship(Object fromId, @Nullable RelationshipDescription relationshipDescription) {
117168
if (relationshipDescription != null) {
118-
return processedRelationshipDescriptions.contains(relationshipDescription);
169+
return processedRelationshipDescriptions.contains(new RelationshipDescriptionWithSourceId(fromId, relationshipDescription));
119170
}
120171
return false;
121172
}
173+
174+
private boolean hasProcessedAllOf(@Nullable Collection<?> valuesToStore) {
175+
// there can be null elements in the unified collection of values to store.
176+
if (valuesToStore == null) {
177+
return false;
178+
}
179+
return processedObjects.containsAll(valuesToStore);
180+
}
181+
122182
}

src/test/java/org/springframework/data/neo4j/integration/imperative/OptimisticLockingIT.java

+34
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import static org.assertj.core.api.Assertions.assertThat;
1919
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
2020

21+
import java.util.ArrayList;
2122
import java.util.Arrays;
2223
import java.util.Collections;
2324
import java.util.List;
@@ -307,6 +308,39 @@ void shouldDoThings(@Autowired VersionedThingRepository repository) {
307308
}
308309
}
309310

311+
@Test // GH-2191
312+
void shouldNotTraverseToBidiRelatedThingsWithOldVersion(@Autowired VersionedThingRepository repository) {
313+
VersionedThing thing1 = new VersionedThing("Thing1");
314+
VersionedThing thing2 = new VersionedThing("Thing2");
315+
VersionedThing thing3 = new VersionedThing("Thing3");
316+
VersionedThing thing4 = new VersionedThing("Thing4");
317+
318+
List<VersionedThing> thing1Relationships = new ArrayList<>();
319+
thing1Relationships.add(thing2);
320+
thing1Relationships.add(thing3);
321+
thing1Relationships.add(thing4);
322+
thing1.setOtherVersionedThings(thing1Relationships);
323+
repository.save(thing1);
324+
// Initially creates:
325+
// Thing1-[:HAS]->Thing2
326+
// Thing1-[:HAS]->Thing3
327+
// Thing1-[:HAS]->Thing4
328+
329+
thing1 = repository.findById(thing1.getId()).get();
330+
thing3 = repository.findById(thing3.getId()).get();
331+
thing3.setOtherVersionedThings(Collections.singletonList(thing1));
332+
repository.save(thing3);
333+
// adds
334+
// Thing3-[:HAS]->Thing1
335+
336+
try (Session session = driver.session()) {
337+
Long relationshipCount = session
338+
.run("MATCH (:VersionedThing)-[r:HAS]->(:VersionedThing) return count(r) as relationshipCount")
339+
.single().get("relationshipCount").asLong();
340+
assertThat(relationshipCount).isEqualTo(4);
341+
}
342+
}
343+
310344
interface VersionedThingRepository extends Neo4jRepository<VersionedThing, Long> {}
311345

312346
interface VersionedThingWithAssignedIdRepository extends Neo4jRepository<VersionedThingWithAssignedId, Long> {}

src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveOptimisticLockingIT.java

+34
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

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

20+
import reactor.core.publisher.Flux;
2021
import reactor.test.StepVerifier;
2122

2223
import java.util.ArrayList;
@@ -267,6 +268,39 @@ void shouldFailOnDeleteByEntityWithWrongVersion(@Autowired VersionedThingWithAss
267268

268269
}
269270

271+
@Test // GH-2191
272+
void shouldNotTraverseToBidiRelatedThingsWithOldVersion(@Autowired VersionedThingRepository repository) {
273+
VersionedThing thing1 = new VersionedThing("Thing1");
274+
VersionedThing thing2 = new VersionedThing("Thing2");
275+
VersionedThing thing3 = new VersionedThing("Thing3");
276+
VersionedThing thing4 = new VersionedThing("Thing4");
277+
278+
List<VersionedThing> thing1Relationships = new ArrayList<>();
279+
thing1Relationships.add(thing2);
280+
thing1Relationships.add(thing3);
281+
thing1Relationships.add(thing4);
282+
thing1.setOtherVersionedThings(thing1Relationships);
283+
StepVerifier.create(repository.save(thing1))
284+
.expectNextCount(1)
285+
.verifyComplete();
286+
287+
Flux.zip(repository.findById(thing1.getId()), repository.findById(thing3.getId()))
288+
.flatMap(tuple -> {
289+
tuple.getT2().setOtherVersionedThings(Collections.singletonList(tuple.getT1()));
290+
return repository.save(tuple.getT2());
291+
})
292+
.as(StepVerifier::create)
293+
.expectNextCount(1)
294+
.verifyComplete();
295+
296+
try (Session session = driver.session()) {
297+
Long relationshipCount = session
298+
.run("MATCH (:VersionedThing)-[r:HAS]->(:VersionedThing) return count(r) as relationshipCount")
299+
.single().get("relationshipCount").asLong();
300+
assertThat(relationshipCount).isEqualTo(4);
301+
}
302+
}
303+
270304
interface VersionedThingRepository extends ReactiveNeo4jRepository<VersionedThing, Long> {}
271305

272306
interface VersionedThingWithAssignedIdRepository

0 commit comments

Comments
 (0)