Skip to content

Commit fd36b91

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 8dd09c2 commit fd36b91

12 files changed

+478
-113
lines changed

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

+28-29
Original file line numberDiff line numberDiff line change
@@ -341,15 +341,21 @@ private <T> T saveImpl(T instance, @Nullable List<PropertyDescriptor> includedPr
341341
.with(binderFunction)
342342
.fetchAs(Long.class).one();
343343

344-
if (entityMetaData.hasVersionProperty() && !optionalInternalId.isPresent()) {
345-
throw new OptimisticLockingFailureException(OPTIMISTIC_LOCKING_ERROR_MESSAGE);
344+
if (!optionalInternalId.isPresent()) {
345+
if (entityMetaData.hasVersionProperty()) {
346+
throw new OptimisticLockingFailureException(OPTIMISTIC_LOCKING_ERROR_MESSAGE);
347+
}
348+
// defensive exception throwing
349+
throw new IllegalStateException("Could not retrieve an internal id while saving.");
346350
}
347351

352+
Long internalId = optionalInternalId.get();
353+
348354
PersistentPropertyAccessor<T> propertyAccessor = entityMetaData.getPropertyAccessor(entityToBeSaved);
349355
if (entityMetaData.isUsingInternalIds()) {
350-
propertyAccessor.setProperty(entityMetaData.getRequiredIdProperty(), optionalInternalId.get());
356+
propertyAccessor.setProperty(entityMetaData.getRequiredIdProperty(), internalId);
351357
}
352-
processRelations(entityMetaData, instance, propertyAccessor, isEntityNew, includeProperty);
358+
processRelations(entityMetaData, instance, internalId, propertyAccessor, isEntityNew, includeProperty);
353359

354360
return propertyAccessor.getBean();
355361
}
@@ -581,6 +587,14 @@ private <T> ExecutableQuery<T> createExecutableQuery(Class<T> domainType, String
581587
* @param isParentObjectNew A flag if the parent was new
582588
* @param includeProperty A predicate telling to include a relationship property or not
583589
*/
590+
private <T> T processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, T originalInstance, Long internalId,
591+
PersistentPropertyAccessor<?> parentPropertyAccessor,
592+
boolean isParentObjectNew, Predicate<String> includeProperty) {
593+
594+
return processNestedRelations(neo4jPersistentEntity, parentPropertyAccessor, isParentObjectNew,
595+
new NestedRelationshipProcessingStateMachine(originalInstance, internalId), includeProperty);
596+
}
597+
584598
private <T> T processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, T originalInstance,
585599
PersistentPropertyAccessor<?> parentPropertyAccessor,
586600
boolean isParentObjectNew, Predicate<String> includeProperty) {
@@ -666,22 +680,23 @@ private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Pers
666680
for (Object relatedValueToStore : relatedValuesToStore) {
667681

668682
// here a map entry is not always anymore a dynamic association
669-
Object relatedObjectBeforeCallbacks = relationshipContext.identifyAndExtractRelationshipTargetNode(relatedValueToStore);
670-
Neo4jPersistentEntity<?> targetEntity = neo4jMappingContext.getPersistentEntity(relatedObjectBeforeCallbacks.getClass());
683+
Object relatedObjectBeforeCallbacksApplied = relationshipContext.identifyAndExtractRelationshipTargetNode(relatedValueToStore);
684+
Neo4jPersistentEntity<?> targetEntity = neo4jMappingContext.getPersistentEntity(relatedObjectBeforeCallbacksApplied.getClass());
671685

672-
boolean isEntityNew = targetEntity.isNew(relatedObjectBeforeCallbacks);
686+
boolean isEntityNew = targetEntity.isNew(relatedObjectBeforeCallbacksApplied);
673687

674-
Object newRelatedObject = eventSupport.maybeCallBeforeBind(relatedObjectBeforeCallbacks);
688+
Object newRelatedObject = stateMachine.hasProcessedValue(relatedObjectBeforeCallbacksApplied)
689+
? stateMachine.getProcessedAs(relatedObjectBeforeCallbacksApplied)
690+
: eventSupport.maybeCallBeforeBind(relatedObjectBeforeCallbacksApplied);
675691

676692
Long relatedInternalId;
677693
// No need to save values if processed
678694
if (stateMachine.hasProcessedValue(relatedValueToStore)) {
679-
Object newRelatedObjectForQuery = stateMachine.getProcessedAs(newRelatedObject);
680-
relatedInternalId = queryRelatedNode(newRelatedObjectForQuery, targetEntity);
695+
relatedInternalId = stateMachine.getInternalId(relatedObjectBeforeCallbacksApplied);
681696
} else {
682697
relatedInternalId = saveRelatedNode(newRelatedObject, targetEntity);
683698
}
684-
stateMachine.markValueAsProcessed(relatedValueToStore);
699+
stateMachine.markValueAsProcessed(relatedValueToStore, relatedInternalId);
685700

686701
Object idValue = idProperty != null
687702
? relationshipContext
@@ -713,8 +728,8 @@ private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Pers
713728
// if an internal id is used this must be set to link this entity in the next iteration
714729
if (targetEntity.isUsingInternalIds()) {
715730
targetPropertyAccessor.setProperty(targetEntity.getRequiredIdProperty(), relatedInternalId);
716-
stateMachine.markValueAsProcessedAs(newRelatedObject, targetPropertyAccessor.getBean());
717731
}
732+
stateMachine.markValueAsProcessedAs(relatedObjectBeforeCallbacksApplied, targetPropertyAccessor.getBean());
718733

719734
if (processState != ProcessState.PROCESSED_ALL_VALUES) {
720735
processNestedRelations(targetEntity, targetPropertyAccessor, isEntityNew, stateMachine, s -> true);
@@ -726,7 +741,7 @@ private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Pers
726741
relatedValueToStore,
727742
targetPropertyAccessor);
728743

729-
relationshipHandler.handle(relatedValueToStore, relatedObjectBeforeCallbacks, potentiallyRecreatedNewRelatedObject);
744+
relationshipHandler.handle(relatedValueToStore, relatedObjectBeforeCallbacksApplied, potentiallyRecreatedNewRelatedObject);
730745
}
731746

732747
relationshipHandler.applyFinalResultToOwner(propertyAccessor);
@@ -735,22 +750,6 @@ private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Pers
735750
return (T) propertyAccessor.getBean();
736751
}
737752

738-
private <Y> Long queryRelatedNode(Object entity, Neo4jPersistentEntity<?> targetNodeDescription) {
739-
740-
Neo4jPersistentProperty requiredIdProperty = targetNodeDescription.getRequiredIdProperty();
741-
PersistentPropertyAccessor<Object> targetPropertyAccessor = targetNodeDescription.getPropertyAccessor(entity);
742-
Object idValue = targetPropertyAccessor.getProperty(requiredIdProperty);
743-
744-
return neo4jClient.query(() ->
745-
renderer.render(cypherGenerator.prepareMatchOf(targetNodeDescription,
746-
targetNodeDescription.getIdExpression().isEqualTo(parameter(Constants.NAME_OF_ID)))
747-
.returning(Constants.NAME_OF_INTERNAL_ID)
748-
.build())
749-
)
750-
.bindAll(Collections.singletonMap(Constants.NAME_OF_ID, idValue))
751-
.fetchAs(Long.class).one().get();
752-
}
753-
754753
private <Y> Long saveRelatedNode(Object entity, NodeDescription targetNodeDescription) {
755754

756755
DynamicLabels dynamicLabels = determineDynamicLabels(entity, (Neo4jPersistentEntity) targetNodeDescription);

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

+15-24
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,7 @@ private <T> Mono<T> saveImpl(T instance, @Nullable List<PropertyDescriptor> incl
345345
if (entityMetaData.isUsingInternalIds()) {
346346
propertyAccessor.setProperty(entityMetaData.getRequiredIdProperty(), internalId);
347347
}
348-
}).then(Mono.defer(() -> processRelations(entityMetaData, instance, propertyAccessor, isNewEntity, includeProperty)));
348+
}).flatMap(internalId -> processRelations(entityMetaData, instance, internalId, propertyAccessor, isNewEntity, includeProperty));
349349
});
350350
}
351351

@@ -678,6 +678,14 @@ Publisher<Tuple2<Collection<Long>, Collection<Long>>>> iterateAndMapNextLevel(
678678
* @param <T> The type of the entity to save
679679
* @return A mono representing the whole stream of save operations.
680680
*/
681+
private <T> Mono<T> processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, T originalInstance,
682+
Long internalId, PersistentPropertyAccessor<?> parentPropertyAccessor,
683+
boolean isParentObjectNew, Predicate<String> includeProperty) {
684+
685+
return processNestedRelations(neo4jPersistentEntity, parentPropertyAccessor, isParentObjectNew,
686+
new NestedRelationshipProcessingStateMachine(originalInstance, internalId), includeProperty);
687+
}
688+
681689
private <T> Mono<T> processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, T originalInstance,
682690
PersistentPropertyAccessor<?> parentPropertyAccessor,
683691
boolean isParentObjectNew, Predicate<String> includeProperty) {
@@ -764,21 +772,20 @@ private <T> Mono<T> processNestedRelations(Neo4jPersistentEntity<?> sourceEntity
764772
stateMachine.markRelationshipAsProcessed(fromId, relationshipDescription);
765773
Flux<RelationshipHandler> relationshipCreation = Flux.fromIterable(relatedValuesToStore).concatMap(relatedValueToStore -> {
766774

767-
Object relatedNodePreEvt = relationshipContext.identifyAndExtractRelationshipTargetNode(relatedValueToStore);
775+
Object relatedObjectBeforeCallbacksApplied = relationshipContext.identifyAndExtractRelationshipTargetNode(relatedValueToStore);
768776
return Mono.deferContextual(ctx -> eventSupport
769-
.maybeCallBeforeBind(relatedNodePreEvt)
777+
.maybeCallBeforeBind(relatedObjectBeforeCallbacksApplied)
770778
.flatMap(newRelatedObject -> {
771-
Neo4jPersistentEntity<?> targetEntity = neo4jMappingContext.getPersistentEntity(relatedNodePreEvt.getClass());
779+
Neo4jPersistentEntity<?> targetEntity = neo4jMappingContext.getPersistentEntity(relatedObjectBeforeCallbacksApplied.getClass());
772780

773781
Mono<Long> queryOrSave;
774782
if (stateMachine.hasProcessedValue(relatedValueToStore)) {
775-
Object newRelatedObjectForQuery = stateMachine.getProcessedAs(newRelatedObject);
776-
queryOrSave = queryRelatedNode(newRelatedObjectForQuery, targetEntity);
783+
queryOrSave = Mono.just(stateMachine.getInternalId(relatedObjectBeforeCallbacksApplied));
777784
} else {
778785
queryOrSave = saveRelatedNode(newRelatedObject, targetEntity);
779786
}
780-
stateMachine.markValueAsProcessed(relatedValueToStore);
781787
return queryOrSave.flatMap(relatedInternalId -> {
788+
stateMachine.markValueAsProcessed(relatedValueToStore, relatedInternalId);
782789
// if an internal id is used this must be set to link this entity in the next iteration
783790
PersistentPropertyAccessor<?> targetPropertyAccessor = targetEntity.getPropertyAccessor(newRelatedObject);
784791
if (targetEntity.isUsingInternalIds()) {
@@ -830,7 +837,7 @@ private <T> Mono<T> processNestedRelations(Neo4jPersistentEntity<?> sourceEntity
830837
})
831838
.doOnNext(potentiallyRecreatedRelatedObject -> {
832839
RelationshipHandler handler = ctx.get(CONTEXT_RELATIONSHIP_HANDLER);
833-
handler.handle(relatedValueToStore, relatedNodePreEvt, potentiallyRecreatedRelatedObject);
840+
handler.handle(relatedValueToStore, relatedObjectBeforeCallbacksApplied, potentiallyRecreatedRelatedObject);
834841
});
835842
})
836843
.then(Mono.fromSupplier(() -> ctx.<RelationshipHandler>get(CONTEXT_RELATIONSHIP_HANDLER))));
@@ -851,22 +858,6 @@ private <T> Mono<T> processNestedRelations(Neo4jPersistentEntity<?> sourceEntity
851858

852859
}
853860

854-
private <Y> Mono<Long> queryRelatedNode(Object entity, Neo4jPersistentEntity<?> targetNodeDescription) {
855-
856-
Neo4jPersistentProperty requiredIdProperty = targetNodeDescription.getRequiredIdProperty();
857-
PersistentPropertyAccessor<Object> targetPropertyAccessor = targetNodeDescription.getPropertyAccessor(entity);
858-
Object idValue = targetPropertyAccessor.getProperty(requiredIdProperty);
859-
860-
return neo4jClient.query(() ->
861-
renderer.render(cypherGenerator.prepareMatchOf(targetNodeDescription,
862-
targetNodeDescription.getIdExpression().isEqualTo(parameter(Constants.NAME_OF_ID)))
863-
.returning(Constants.NAME_OF_INTERNAL_ID)
864-
.build())
865-
)
866-
.bindAll(Collections.singletonMap(Constants.NAME_OF_ID, idValue))
867-
.fetchAs(Long.class).one();
868-
}
869-
870861
private <Y> Mono<Long> saveRelatedNode(Object relatedNode, Neo4jPersistentEntity<?> targetNodeDescription) {
871862

872863
return determineDynamicLabels((Y) relatedNode, targetNodeDescription)

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

+34-10
Original file line numberDiff line numberDiff line change
@@ -59,10 +59,22 @@ public enum ProcessState {
5959
private final Set<Object> processedObjects = new HashSet<>();
6060

6161
/**
62-
* Set of aliases. e.g. One object without id set represents the same as the one with the id set.
62+
* A map of processed objects pointing towards a possible new instance of themself.
63+
* This will happen for immutable entities.
6364
*/
6465
private final Map<Object, Object> processedObjectsAlias = new HashMap<>();
6566

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+
6678
public NestedRelationshipProcessingStateMachine(Object initialObject) {
6779
processedObjects.add(initialObject);
6880
}
@@ -145,11 +157,12 @@ public void markRelationshipAsProcessed(Object fromId, RelationshipDescription r
145157
*
146158
* @param valueToStore If not {@literal null}, all non-null values will be marked as processed
147159
*/
148-
public void markValueAsProcessed(Object valueToStore) {
160+
public void markValueAsProcessed(Object valueToStore, Long internalId) {
149161

150162
try {
151163
write.lock();
152164
this.processedObjects.add(valueToStore);
165+
this.processedObjectsIds.put(valueToStore, internalId);
153166
} finally {
154167
write.unlock();
155168
}
@@ -178,14 +191,6 @@ public boolean hasProcessedRelationship(Object fromId, @Nullable RelationshipDes
178191
return false;
179192
}
180193

181-
private boolean hasProcessedAllOf(@Nullable Collection<?> valuesToStore) {
182-
// there can be null elements in the unified collection of values to store.
183-
if (valuesToStore == null) {
184-
return false;
185-
}
186-
return processedObjects.containsAll(valuesToStore);
187-
}
188-
189194
public void markValueAsProcessedAs(Object relatedValueToStore, Object bean) {
190195
try {
191196
write.lock();
@@ -195,6 +200,17 @@ public void markValueAsProcessedAs(Object relatedValueToStore, Object bean) {
195200
}
196201
}
197202

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+
198214
public Object getProcessedAs(Object entity) {
199215
try {
200216
read.lock();
@@ -204,4 +220,12 @@ public Object getProcessedAs(Object entity) {
204220
}
205221
}
206222

223+
private boolean hasProcessedAllOf(@Nullable Collection<?> valuesToStore) {
224+
// there can be null elements in the unified collection of values to store.
225+
if (valuesToStore == null) {
226+
return false;
227+
}
228+
return processedObjects.containsAll(valuesToStore);
229+
}
230+
207231
}

‎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)