Skip to content

Commit 1823b9d

Browse files
committed
GH-2191 - Fix state machine for more complex scenarios.
1 parent 5b85656 commit 1823b9d

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
@@ -470,14 +470,15 @@ private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Obje
470470
}
471471

472472
// break recursive procession and deletion of previously created relationships
473-
ProcessState processState = stateMachine.getStateOf(relationshipDescriptionObverse, relatedValuesToStore);
473+
ProcessState processState = stateMachine.getStateOf(fromId, relationshipDescriptionObverse, relatedValuesToStore);
474474
if (processState == ProcessState.PROCESSED_ALL_RELATIONSHIPS || processState == ProcessState.PROCESSED_BOTH) {
475475
return;
476476
}
477477

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

482483
List<Long> knownRelationshipsIds = new ArrayList<>();
483484
if (idProperty != null) {
@@ -508,7 +509,7 @@ private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Obje
508509
return;
509510
}
510511

511-
stateMachine.markAsProcessed(relationshipDescription, relatedValuesToStore);
512+
stateMachine.markRelationshipAsProcessed(fromId, relationshipDescription);
512513

513514
for (Object relatedValueToStore : relatedValuesToStore) {
514515

@@ -522,12 +523,13 @@ private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Obje
522523

523524
Long relatedInternalId;
524525
// No need to save values if processed
525-
if (processState == ProcessState.PROCESSED_ALL_VALUES) {
526+
if (stateMachine.hasProcessedValue(relatedValueToStore)) {
526527
relatedInternalId = queryRelatedNode(relatedNode, targetEntity, inDatabase);
527528
} else {
528529
relatedInternalId = saveRelatedNode(relatedNode, relationshipContext.getAssociationTargetType(),
529530
targetEntity, inDatabase);
530531
}
532+
stateMachine.markValueAsProcessed(relatedValueToStore);
531533

532534
CreateRelationshipStatementHolder statementHolder = neo4jMappingContext.createStatement(
533535
sourceEntity, relationshipContext, relatedValueToStore);
@@ -556,7 +558,6 @@ private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Obje
556558
}
557559
}
558560

559-
560561
});
561562

562563
return (T) propertyAccessor.getBean();

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

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

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

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

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

645-
stateMachine.markAsProcessed(relationshipDescription, relatedValuesToStore);
646+
stateMachine.markRelationshipAsProcessed(fromId, relationshipDescription);
646647

647648
for (Object relatedValueToStore : relatedValuesToStore) {
648649

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

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

666668
// 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)