Skip to content

Commit 27015a6

Browse files
meistermeiermeistermeier
committed
GH-2240 - Stabilize immutable mapping.
Use cache to retrieve previously persisted objects and their object's ids. Closes #2240
1 parent a04706a commit 27015a6

12 files changed

+510
-93
lines changed

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

+32-16
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
import org.neo4j.cypherdsl.core.Node;
4242
import org.neo4j.cypherdsl.core.Statement;
4343
import org.neo4j.cypherdsl.core.renderer.Renderer;
44+
import org.neo4j.driver.Value;
4445
import org.neo4j.driver.exceptions.NoSuchRecordException;
4546
import org.neo4j.driver.summary.ResultSummary;
4647
import org.neo4j.driver.summary.SummaryCounters;
@@ -244,15 +245,21 @@ private <T> T saveImpl(T instance, @Nullable String inDatabase) {
244245
.with(neo4jMappingContext.getRequiredBinderFunctionFor((Class<T>) entityToBeSaved.getClass()))
245246
.fetchAs(Long.class).one();
246247

247-
if (entityMetaData.hasVersionProperty() && !optionalInternalId.isPresent()) {
248-
throw new OptimisticLockingFailureException(OPTIMISTIC_LOCKING_ERROR_MESSAGE);
248+
249+
if (!optionalInternalId.isPresent()) {
250+
if (entityMetaData.hasVersionProperty()) {
251+
throw new OptimisticLockingFailureException(OPTIMISTIC_LOCKING_ERROR_MESSAGE);
252+
}
253+
// defensive exception throwing
254+
throw new IllegalStateException("Could not retrieve an internal id while saving.");
249255
}
250256

257+
Long internalId = optionalInternalId.get();
251258
PersistentPropertyAccessor<T> propertyAccessor = entityMetaData.getPropertyAccessor(entityToBeSaved);
252259
if (entityMetaData.isUsingInternalIds()) {
253-
propertyAccessor.setProperty(entityMetaData.getRequiredIdProperty(), optionalInternalId.get());
260+
propertyAccessor.setProperty(entityMetaData.getRequiredIdProperty(), internalId);
254261
}
255-
processRelations(entityMetaData, instance, propertyAccessor, inDatabase, isEntityNew);
262+
processRelations(entityMetaData, instance, internalId, propertyAccessor, inDatabase, isEntityNew);
256263

257264
return propertyAccessor.getBean();
258265
}
@@ -452,6 +459,14 @@ private <T> ExecutableQuery<T> createExecutableQuery(Class<T> domainType, String
452459
* @param parentPropertyAccessor The property accessor of the parent, to modify the relationships
453460
* @param isParentObjectNew A flag if the parent was new
454461
*/
462+
private <T> T processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, T originalInstance, Long internalId,
463+
PersistentPropertyAccessor<?> parentPropertyAccessor,
464+
@Nullable String inDatabase, boolean isParentObjectNew) {
465+
466+
return processNestedRelations(neo4jPersistentEntity, parentPropertyAccessor, isParentObjectNew, inDatabase,
467+
new NestedRelationshipProcessingStateMachine(originalInstance, internalId));
468+
}
469+
455470
private <T> T processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, T originalInstance,
456471
PersistentPropertyAccessor<?> parentPropertyAccessor,
457472
@Nullable String inDatabase, boolean isParentObjectNew) {
@@ -534,22 +549,22 @@ private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Pers
534549
for (Object relatedValueToStore : relatedValuesToStore) {
535550

536551
// here a map entry is not always anymore a dynamic association
537-
Object relatedObjectBeforeCallbacks = relationshipContext.identifyAndExtractRelationshipTargetNode(relatedValueToStore);
538-
Neo4jPersistentEntity<?> targetEntity = neo4jMappingContext.getPersistentEntity(relatedObjectBeforeCallbacks.getClass());
539-
540-
boolean isEntityNew = targetEntity.isNew(relatedObjectBeforeCallbacks);
552+
Object relatedObjectBeforeCallbacksApplied = relationshipContext.identifyAndExtractRelationshipTargetNode(relatedValueToStore);
553+
Neo4jPersistentEntity<?> targetEntity = neo4jMappingContext.getPersistentEntity(relatedObjectBeforeCallbacksApplied.getClass());
541554

542-
Object newRelatedObject = eventSupport.maybeCallBeforeBind(relatedObjectBeforeCallbacks);
555+
boolean isEntityNew = targetEntity.isNew(relatedObjectBeforeCallbacksApplied);
556+
Object newRelatedObject = stateMachine.hasProcessedValue(relatedObjectBeforeCallbacksApplied)
557+
? stateMachine.getProcessedAs(relatedObjectBeforeCallbacksApplied)
558+
: eventSupport.maybeCallBeforeBind(relatedObjectBeforeCallbacksApplied);
543559

544560
Long relatedInternalId;
545561
// No need to save values if processed
546562
if (stateMachine.hasProcessedValue(relatedValueToStore)) {
547-
Object newRelatedObjectForQuery = stateMachine.getProcessedAs(newRelatedObject);
548-
relatedInternalId = queryRelatedNode(newRelatedObjectForQuery, targetEntity, inDatabase);
563+
relatedInternalId = stateMachine.getInternalId(relatedObjectBeforeCallbacksApplied);
549564
} else {
550565
relatedInternalId = saveRelatedNode(newRelatedObject, targetEntity, inDatabase);
551566
}
552-
stateMachine.markValueAsProcessed(relatedValueToStore);
567+
stateMachine.markValueAsProcessed(relatedValueToStore, relatedInternalId);
553568

554569
Object idValue = idProperty != null
555570
? relationshipContext
@@ -581,8 +596,8 @@ private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Pers
581596
// if an internal id is used this must be set to link this entity in the next iteration
582597
if (targetEntity.isUsingInternalIds()) {
583598
targetPropertyAccessor.setProperty(targetEntity.getRequiredIdProperty(), relatedInternalId);
584-
stateMachine.markValueAsProcessedAs(newRelatedObject, targetPropertyAccessor.getBean());
585599
}
600+
stateMachine.markValueAsProcessedAs(relatedObjectBeforeCallbacksApplied, targetPropertyAccessor.getBean());
586601

587602
if (processState != ProcessState.PROCESSED_ALL_VALUES) {
588603
processNestedRelations(targetEntity, targetPropertyAccessor, isEntityNew, inDatabase, stateMachine);
@@ -594,7 +609,7 @@ private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Pers
594609
relatedValueToStore,
595610
targetPropertyAccessor);
596611

597-
relationshipHandler.handle(relatedValueToStore, relatedObjectBeforeCallbacks, potentiallyRecreatedNewRelatedObject);
612+
relationshipHandler.handle(relatedValueToStore, relatedObjectBeforeCallbacksApplied, potentiallyRecreatedNewRelatedObject);
598613
}
599614

600615
relationshipHandler.applyFinalResultToOwner(propertyAccessor);
@@ -615,8 +630,9 @@ private <Y> Long queryRelatedNode(Object entity, Neo4jPersistentEntity<?> target
615630
targetNodeDescription.getIdExpression().isEqualTo(parameter(Constants.NAME_OF_ID)))
616631
.returning(Constants.NAME_OF_INTERNAL_ID)
617632
.build())
618-
)
619-
.in(inDatabase).bindAll(Collections.singletonMap(Constants.NAME_OF_ID, idValue))
633+
)
634+
.in(inDatabase).bindAll(Collections.singletonMap(Constants.NAME_OF_ID,
635+
neo4jMappingContext.getConversionService().convert(idValue, Value.class)))
620636
.fetchAs(Long.class).one().get();
621637
}
622638

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

+16-8
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,7 @@ private <T> Mono<T> saveImpl(T instance, @Nullable String inDatabase) {
256256
if (entityMetaData.isUsingInternalIds()) {
257257
propertyAccessor.setProperty(entityMetaData.getRequiredIdProperty(), internalId);
258258
}
259-
}).then(Mono.defer(() -> processRelations(entityMetaData, instance, propertyAccessor, inDatabase, isNewEntity)));
259+
}).flatMap(internalId -> processRelations(entityMetaData, instance, internalId, propertyAccessor, inDatabase, isNewEntity));
260260
});
261261
}
262262

@@ -570,6 +570,14 @@ Publisher<Tuple2<Collection<Long>, Collection<Long>>>> iterateAndMapNextLevel(
570570
* @param <T> The type of the entity to save
571571
* @return A mono representing the whole stream of save operations.
572572
*/
573+
private <T> Mono<T> processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, T originalInstance,
574+
Long internalId, PersistentPropertyAccessor<?> parentPropertyAccessor,
575+
@Nullable String inDatabase, boolean isParentObjectNew) {
576+
577+
return processNestedRelations(neo4jPersistentEntity, parentPropertyAccessor, isParentObjectNew, inDatabase,
578+
new NestedRelationshipProcessingStateMachine(originalInstance, internalId));
579+
}
580+
573581
private <T> Mono<T> processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, T originalInstance,
574582
PersistentPropertyAccessor<?> parentPropertyAccessor,
575583
@Nullable String inDatabase, boolean isParentObjectNew) {
@@ -653,21 +661,21 @@ private <T> Mono<T> processNestedRelations(Neo4jPersistentEntity<?> sourceEntity
653661
stateMachine.markRelationshipAsProcessed(fromId, relationshipDescription);
654662
Flux<RelationshipHandler> relationshipCreation = Flux.fromIterable(relatedValuesToStore).concatMap(relatedValueToStore -> {
655663

656-
Object relatedNodePreEvt = relationshipContext.identifyAndExtractRelationshipTargetNode(relatedValueToStore);
664+
Object relatedObjectBeforeCallbacksApplied = relationshipContext.identifyAndExtractRelationshipTargetNode(relatedValueToStore);
657665
return Mono.deferContextual(ctx -> eventSupport
658-
.maybeCallBeforeBind(relatedNodePreEvt)
666+
.maybeCallBeforeBind(relatedObjectBeforeCallbacksApplied)
659667
.flatMap(newRelatedObject -> {
660-
Neo4jPersistentEntity<?> targetEntity = neo4jMappingContext.getPersistentEntity(relatedNodePreEvt.getClass());
668+
Neo4jPersistentEntity<?> targetEntity = neo4jMappingContext.getPersistentEntity(relatedObjectBeforeCallbacksApplied.getClass());
661669

662670
Mono<Long> queryOrSave;
663671
if (stateMachine.hasProcessedValue(relatedValueToStore)) {
664-
Object newRelatedObjectForQuery = stateMachine.getProcessedAs(newRelatedObject);
665-
queryOrSave = queryRelatedNode(newRelatedObjectForQuery, targetEntity, inDatabase);
672+
queryOrSave = Mono.just(stateMachine.getInternalId(relatedObjectBeforeCallbacksApplied));
666673
} else {
667674
queryOrSave = saveRelatedNode(newRelatedObject, targetEntity, inDatabase);
668675
}
669-
stateMachine.markValueAsProcessed(relatedValueToStore);
676+
670677
return queryOrSave.flatMap(relatedInternalId -> {
678+
stateMachine.markValueAsProcessed(relatedValueToStore, relatedInternalId);
671679
// if an internal id is used this must be set to link this entity in the next iteration
672680
PersistentPropertyAccessor<?> targetPropertyAccessor = targetEntity.getPropertyAccessor(newRelatedObject);
673681
if (targetEntity.isUsingInternalIds()) {
@@ -719,7 +727,7 @@ private <T> Mono<T> processNestedRelations(Neo4jPersistentEntity<?> sourceEntity
719727
})
720728
.doOnNext(potentiallyRecreatedRelatedObject -> {
721729
RelationshipHandler handler = ctx.get(CONTEXT_RELATIONSHIP_HANDLER);
722-
handler.handle(relatedValueToStore, relatedNodePreEvt, potentiallyRecreatedRelatedObject);
730+
handler.handle(relatedValueToStore, relatedObjectBeforeCallbacksApplied, potentiallyRecreatedRelatedObject);
723731
});
724732
})
725733
.then(Mono.fromSupplier(() -> ctx.<RelationshipHandler>get(CONTEXT_RELATIONSHIP_HANDLER))));

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

+47-19
Original file line numberDiff line numberDiff line change
@@ -37,24 +37,6 @@
3737
@API(status = API.Status.INTERNAL, since = "6.0")
3838
public final class NestedRelationshipProcessingStateMachine {
3939

40-
public void markValueAsProcessedAs(Object relatedValueToStore, Object bean) {
41-
try {
42-
write.lock();
43-
processedObjectsAlias.put(relatedValueToStore, bean);
44-
} finally {
45-
write.unlock();
46-
}
47-
}
48-
49-
public Object getProcessedAs(Object entity) {
50-
try {
51-
read.lock();
52-
return processedObjectsAlias.getOrDefault(entity, entity);
53-
} finally {
54-
read.unlock();
55-
}
56-
}
57-
5840
/**
5941
* Valid processing states.
6042
*/
@@ -75,8 +57,24 @@ public enum ProcessState {
7557
* The set of already processed related objects.
7658
*/
7759
private final Set<Object> processedObjects = new HashSet<>();
60+
61+
/**
62+
* A map of processed objects pointing towards a possible new instance of themself.
63+
* This will happen for immutable entities.
64+
*/
7865
private final Map<Object, Object> processedObjectsAlias = new HashMap<>();
7966

67+
/**
68+
* A map pointing from a processed object to the internal id.
69+
* This will be useful during the persistence to avoid another DB network round-trip.
70+
*/
71+
private final Map<Object, Long> processedObjectsIds = new HashMap<>();
72+
73+
public NestedRelationshipProcessingStateMachine(Object initialObject, Long internalId) {
74+
this(initialObject);
75+
processedObjectsIds.put(initialObject, internalId);
76+
}
77+
8078
public NestedRelationshipProcessingStateMachine(Object initialObject) {
8179
processedObjects.add(initialObject);
8280
}
@@ -159,11 +157,12 @@ public void markRelationshipAsProcessed(Object fromId, RelationshipDescription r
159157
*
160158
* @param valueToStore If not {@literal null}, all non-null values will be marked as processed
161159
*/
162-
public void markValueAsProcessed(Object valueToStore) {
160+
public void markValueAsProcessed(Object valueToStore, Long internalId) {
163161

164162
try {
165163
write.lock();
166164
this.processedObjects.add(valueToStore);
165+
this.processedObjectsIds.put(valueToStore, internalId);
167166
} finally {
168167
write.unlock();
169168
}
@@ -192,6 +191,35 @@ public boolean hasProcessedRelationship(Object fromId, @Nullable RelationshipDes
192191
return false;
193192
}
194193

194+
public void markValueAsProcessedAs(Object relatedValueToStore, Object bean) {
195+
try {
196+
write.lock();
197+
processedObjectsAlias.put(relatedValueToStore, bean);
198+
} finally {
199+
write.unlock();
200+
}
201+
}
202+
203+
public Long getInternalId(Object object) {
204+
try {
205+
read.lock();
206+
Long possibleId = processedObjectsIds.get(object);
207+
return possibleId != null ? possibleId : processedObjectsIds.get(processedObjectsAlias.get(object));
208+
} finally {
209+
read.unlock();
210+
}
211+
212+
}
213+
214+
public Object getProcessedAs(Object entity) {
215+
try {
216+
read.lock();
217+
return processedObjectsAlias.getOrDefault(entity, entity);
218+
} finally {
219+
read.unlock();
220+
}
221+
}
222+
195223
private boolean hasProcessedAllOf(@Nullable Collection<?> valuesToStore) {
196224
// there can be null elements in the unified collection of values to store.
197225
if (valuesToStore == null) {

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

+47
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;
@@ -34,6 +37,7 @@
3437
import org.springframework.data.neo4j.test.Neo4jIntegrationTest;
3538
import org.springframework.transaction.annotation.EnableTransactionManagement;
3639

40+
import java.util.ArrayList;
3741
import java.util.Arrays;
3842
import java.util.Collection;
3943
import java.util.Collections;
@@ -53,6 +57,19 @@ public class ImmutableAssignedIdsIT {
5357
public static final String SOME_VALUE_VALUE = "testValue";
5458
protected static Neo4jExtension.Neo4jConnectionSupport neo4jConnectionSupport;
5559

60+
private final Driver driver;
61+
62+
public ImmutableAssignedIdsIT(@Autowired Driver driver) {
63+
this.driver = driver;
64+
}
65+
66+
@BeforeEach
67+
void cleanUp() {
68+
try (Session session = driver.session()) {
69+
session.run("MATCH (n) DETACH DELETE n").consume();
70+
}
71+
}
72+
5673
@Test // GH-2141
5774
void saveWithAssignedIdsReturnsObjectWithIdSet(
5875
@Autowired ImmutablePersonWithAssignedIdRepository repository) {
@@ -300,6 +317,36 @@ void saveRelationshipWithAssignedIdsContainsAllRelationshipTypes(
300317
assertThat(savedPerson.relationshipPropertiesDynamicCollection.values().iterator().next().get(0).target.id).isNotNull();
301318
}
302319

320+
@Test // GH-2235
321+
void saveWithGeneratedIdsWithMultipleRelationshipsToOneNode(@Autowired ImmutablePersonWithAssignedIdRepository repository) {
322+
ImmutablePersonWithAssignedId person1 = new ImmutablePersonWithAssignedId();
323+
ImmutablePersonWithAssignedId person2 = ImmutablePersonWithAssignedId.fallback(person1);
324+
List<ImmutablePersonWithAssignedId> onboardedBy = new ArrayList<>();
325+
onboardedBy.add(person1);
326+
onboardedBy.add(person2);
327+
ImmutablePersonWithAssignedId person3 = ImmutablePersonWithAssignedId.wasOnboardedBy(onboardedBy);
328+
329+
ImmutablePersonWithAssignedId savedPerson = repository.save(person3);
330+
assertThat(savedPerson.id).isNotNull();
331+
assertThat(savedPerson.wasOnboardedBy).allMatch(ob -> ob.id != null);
332+
333+
ImmutablePersonWithAssignedId savedPerson2 = savedPerson.wasOnboardedBy.stream().filter(p -> p.fallback != null)
334+
.findFirst().get();
335+
336+
assertThat(savedPerson2.fallback.id).isNotNull();
337+
338+
try (Session session = driver.session()) {
339+
List<Record> result = session.run(
340+
"MATCH (person3:ImmutablePersonWithAssignedId) " +
341+
"-[:ONBOARDED_BY]->(person2:ImmutablePersonWithAssignedId) " +
342+
"-[:FALLBACK]->(person1:ImmutablePersonWithAssignedId), " +
343+
"(person3)-[:ONBOARDED_BY]->(person1) " +
344+
"return person3")
345+
.list();
346+
assertThat(result).hasSize(1);
347+
}
348+
}
349+
303350
interface ImmutablePersonWithAssignedIdRepository extends Neo4jRepository<ImmutablePersonWithAssignedId, Long> {}
304351

305352
@Configuration

0 commit comments

Comments
 (0)