Skip to content

Commit 0461235

Browse files
michael-simonsmichael-simons
committed
GH-2355 - Improve handling of non-distinct collections and already visited objects.
This commit started as a bunch of tests that should show which self-referential relationships with optimistic locking are supported. The bug in #2355 could be reproduced with the `saveAll` call: The underlying issue bieng that one or more items had been visited at least twice: The first time as related object, than as root. The next root of course still be on the old version. That has been fixed by keeping the state machine tracking visited items for the duration of the `saveAll` calls around. Thus an related object will not be updated twice. For all of this to work in the most efficient way possible, a `java.util.Set` should be used for related objects, along with a valid `equals/hashCode` pair. This is however neglected. By adding an additional check in the state machine falling back to ID extraction, we can improve the user experience a lot and remove that need in many cases. This commit fixes #2355.
1 parent c4a7551 commit 0461235

15 files changed

+1543
-90
lines changed

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

Lines changed: 41 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -230,10 +230,14 @@ private Object convertIdValues(@Nullable Neo4jPersistentProperty idProperty, Obj
230230
@Override
231231
public <T> T save(T instance) {
232232

233-
return saveImpl(instance, getDatabaseName());
233+
return saveImpl(instance, getDatabaseName(), null);
234234
}
235235

236-
private <T> T saveImpl(T instance, @Nullable String inDatabase) {
236+
private <T> T saveImpl(T instance, @Nullable String inDatabase, @Nullable NestedRelationshipProcessingStateMachine stateMachine) {
237+
238+
if (stateMachine != null && stateMachine.hasProcessedValue(instance)) {
239+
return instance;
240+
}
237241

238242
Neo4jPersistentEntity<?> entityMetaData = neo4jMappingContext.getPersistentEntity(instance.getClass());
239243
boolean isEntityNew = entityMetaData.isNew(instance);
@@ -264,7 +268,12 @@ private <T> T saveImpl(T instance, @Nullable String inDatabase) {
264268
propertyAccessor.setProperty(entityMetaData.getRequiredIdProperty(), internalId);
265269
}
266270
TemplateSupport.updateVersionPropertyIfPossible(entityMetaData, propertyAccessor, newOrUpdatedNode.get());
267-
processRelations(entityMetaData, instance, internalId, propertyAccessor, inDatabase, isEntityNew);
271+
if (stateMachine == null) {
272+
stateMachine = new NestedRelationshipProcessingStateMachine(neo4jMappingContext, instance, internalId);
273+
}
274+
275+
stateMachine.markValueAsProcessed(instance, internalId);
276+
processRelations(entityMetaData, propertyAccessor, isEntityNew, stateMachine, inDatabase);
268277

269278
return propertyAccessor.getBean();
270279
}
@@ -316,7 +325,8 @@ public <T> List<T> saveAll(Iterable<T> instances) {
316325
|| entityMetaData.getDynamicLabelsProperty().isPresent()) {
317326
log.debug("Saving entities using single statements.");
318327

319-
return entities.stream().map(e -> saveImpl(e, databaseName)).collect(Collectors.toList());
328+
NestedRelationshipProcessingStateMachine stateMachine = new NestedRelationshipProcessingStateMachine(neo4jMappingContext);
329+
return entities.stream().map(e -> saveImpl(e, databaseName, stateMachine)).collect(Collectors.toList());
320330
}
321331

322332
class Tuple3<T> {
@@ -354,7 +364,7 @@ class Tuple3<T> {
354364
Neo4jPersistentProperty idProperty = entityMetaData.getRequiredIdProperty();
355365
Object id = convertIdValues(idProperty, propertyAccessor.getProperty(idProperty));
356366
Long internalId = idToInternalIdMapping.get(id);
357-
return processRelations(entityMetaData, t.t1, internalId, propertyAccessor, databaseName, t.t2);
367+
return this.<T>processRelations(entityMetaData, propertyAccessor, t.t2, new NestedRelationshipProcessingStateMachine(neo4jMappingContext, t.t1, internalId), databaseName);
358368
}).collect(Collectors.toList());
359369
}
360370

@@ -460,22 +470,21 @@ private <T> ExecutableQuery<T> createExecutableQuery(Class<T> domainType, String
460470
* Starts of processing of the relationships.
461471
*
462472
* @param neo4jPersistentEntity The description of the instance to save
463-
* @param originalInstance The original parent instance. It is paramount to pass in the original instance (prior
464-
* to generating the id and prior to eventually create new instances via the property accessor,
465-
* so that we can reliable stop traversing relationships.
466473
* @param parentPropertyAccessor The property accessor of the parent, to modify the relationships
467474
* @param isParentObjectNew A flag if the parent was new
475+
* @param stateMachine Statemachine containing process data
476+
* @param inDatabase Optional target database
477+
* @param <T> The type of the entity to save
468478
*/
469-
private <T> T processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, T originalInstance, Long internalId,
479+
private <T> T processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity,
470480
PersistentPropertyAccessor<?> parentPropertyAccessor,
471-
@Nullable String inDatabase, boolean isParentObjectNew) {
481+
boolean isParentObjectNew, NestedRelationshipProcessingStateMachine stateMachine, @Nullable String inDatabase) {
472482

473-
return processNestedRelations(neo4jPersistentEntity, parentPropertyAccessor, isParentObjectNew, inDatabase,
474-
new NestedRelationshipProcessingStateMachine(originalInstance, internalId));
483+
return processNestedRelations(neo4jPersistentEntity, parentPropertyAccessor, isParentObjectNew, stateMachine, inDatabase);
475484
}
476485

477486
private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, PersistentPropertyAccessor<?> propertyAccessor,
478-
boolean isParentObjectNew, @Nullable String inDatabase, NestedRelationshipProcessingStateMachine stateMachine) {
487+
boolean isParentObjectNew, NestedRelationshipProcessingStateMachine stateMachine, @Nullable String inDatabase) {
479488

480489
Object fromId = propertyAccessor.getProperty(sourceEntity.getRequiredIdProperty());
481490

@@ -559,12 +568,28 @@ private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Pers
559568
Entity savedEntity = null;
560569
// No need to save values if processed
561570
if (stateMachine.hasProcessedValue(relatedValueToStore)) {
562-
relatedInternalId = stateMachine.getInternalId(relatedObjectBeforeCallbacksApplied);
571+
relatedInternalId = stateMachine.getInternalId(relatedValueToStore);
563572
} else {
564573
savedEntity = saveRelatedNode(newRelatedObject, targetEntity, inDatabase);
565574
relatedInternalId = savedEntity.id();
575+
stateMachine.markValueAsProcessed(relatedValueToStore, relatedInternalId);
566576
}
567-
stateMachine.markValueAsProcessed(relatedValueToStore, relatedInternalId);
577+
578+
PersistentPropertyAccessor<?> targetPropertyAccessor = targetEntity.getPropertyAccessor(newRelatedObject);
579+
// if an internal id is used this must be set to link this entity in the next iteration
580+
if (targetEntity.isUsingInternalIds()) {
581+
Neo4jPersistentProperty requiredIdProperty = targetEntity.getRequiredIdProperty();
582+
if (relatedInternalId == null && targetPropertyAccessor.getProperty(requiredIdProperty) != null) {
583+
relatedInternalId = (Long) targetPropertyAccessor.getProperty(requiredIdProperty);
584+
} else if (targetPropertyAccessor.getProperty(requiredIdProperty) == null) {
585+
targetPropertyAccessor.setProperty(requiredIdProperty, relatedInternalId);
586+
}
587+
}
588+
if (savedEntity != null) {
589+
TemplateSupport.updateVersionPropertyIfPossible(targetEntity, targetPropertyAccessor, savedEntity);
590+
}
591+
stateMachine.markValueAsProcessedAs(relatedObjectBeforeCallbacksApplied, targetPropertyAccessor.getBean());
592+
stateMachine.markRelationshipAsProcessed(relatedInternalId, relationshipDescription.getRelationshipObverse());
568593

569594
Object idValue = idProperty != null
570595
? relationshipContext
@@ -592,19 +617,8 @@ private <T> T processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, Pers
592617
.setProperty(idProperty, relationshipInternalId.get());
593618
}
594619

595-
PersistentPropertyAccessor<?> targetPropertyAccessor = targetEntity.getPropertyAccessor(newRelatedObject);
596-
// if an internal id is used this must be set to link this entity in the next iteration
597-
if (targetEntity.isUsingInternalIds()) {
598-
targetPropertyAccessor.setProperty(targetEntity.getRequiredIdProperty(), relatedInternalId);
599-
}
600-
if (savedEntity != null) {
601-
TemplateSupport.updateVersionPropertyIfPossible(targetEntity, targetPropertyAccessor, savedEntity);
602-
}
603-
stateMachine.markValueAsProcessedAs(relatedObjectBeforeCallbacksApplied, targetPropertyAccessor.getBean());
604-
stateMachine.markRelationshipAsProcessed(relatedInternalId, relationshipDescription.getRelationshipObverse());
605-
606620
if (processState != ProcessState.PROCESSED_ALL_VALUES) {
607-
processNestedRelations(targetEntity, targetPropertyAccessor, isEntityNew, inDatabase, stateMachine);
621+
processNestedRelations(targetEntity, targetPropertyAccessor, isEntityNew, stateMachine, inDatabase);
608622
}
609623

610624
Object potentiallyRecreatedNewRelatedObject = MappingSupport.getRelationshipOrRelationshipPropertiesObject(neo4jMappingContext,

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

Lines changed: 67 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -232,13 +232,25 @@ private Object convertIdValues(@Nullable Neo4jPersistentProperty idProperty, Obj
232232
@Override
233233
public <T> Mono<T> save(T instance) {
234234

235-
return getDatabaseName().flatMap(databaseName -> saveImpl(instance, databaseName.getValue()));
235+
return getDatabaseName().flatMap(databaseName -> saveImpl(instance, databaseName.getValue(), null));
236236
}
237237

238-
private <T> Mono<T> saveImpl(T instance, @Nullable String inDatabase) {
238+
private <T> Mono<T> saveImpl(T instance, @Nullable String inDatabase, @Nullable NestedRelationshipProcessingStateMachine stateMachine) {
239+
240+
if (stateMachine != null && stateMachine.hasProcessedValue(instance)) {
241+
return Mono.just(instance);
242+
}
239243

240244
Neo4jPersistentEntity entityMetaData = neo4jMappingContext.getPersistentEntity(instance.getClass());
241245
boolean isNewEntity = entityMetaData.isNew(instance);
246+
247+
NestedRelationshipProcessingStateMachine finalStateMachine;
248+
if (stateMachine == null) {
249+
finalStateMachine = new NestedRelationshipProcessingStateMachine(neo4jMappingContext);
250+
} else {
251+
finalStateMachine = stateMachine;
252+
}
253+
242254
return Mono.just(instance).flatMap(eventSupport::maybeCallBeforeBind)
243255
.flatMap(entityToBeSaved -> determineDynamicLabels(entityToBeSaved, entityMetaData, inDatabase)).flatMap(t -> {
244256
T entityToBeSaved = t.getT1();
@@ -258,12 +270,13 @@ private <T> Mono<T> saveImpl(T instance, @Nullable String inDatabase) {
258270

259271
PersistentPropertyAccessor<T> propertyAccessor = entityMetaData.getPropertyAccessor(entityToBeSaved);
260272
return idMono.doOnNext(newOrUpdatedNode -> {
261-
if (entityMetaData.isUsingInternalIds()) {
262-
propertyAccessor.setProperty(entityMetaData.getRequiredIdProperty(), newOrUpdatedNode.id());
263-
}
264-
TemplateSupport.updateVersionPropertyIfPossible(entityMetaData, propertyAccessor, newOrUpdatedNode);
265-
}).map(Entity::id)
266-
.flatMap(internalId -> processRelations(entityMetaData, instance, internalId, propertyAccessor, inDatabase, isNewEntity));
273+
if (entityMetaData.isUsingInternalIds()) {
274+
propertyAccessor.setProperty(entityMetaData.getRequiredIdProperty(), newOrUpdatedNode.id());
275+
}
276+
TemplateSupport.updateVersionPropertyIfPossible(entityMetaData, propertyAccessor, newOrUpdatedNode);
277+
finalStateMachine.markValueAsProcessed(instance, newOrUpdatedNode.id());
278+
}).map(Entity::id)
279+
.flatMap(internalId -> processRelations(entityMetaData, propertyAccessor, isNewEntity, finalStateMachine, inDatabase));
267280
});
268281
}
269282

@@ -312,8 +325,9 @@ public <T> Flux<T> saveAll(Iterable<T> instances) {
312325
|| entityMetaData.getDynamicLabelsProperty().isPresent()) {
313326
log.debug("Saving entities using single statements.");
314327

328+
NestedRelationshipProcessingStateMachine stateMachine = new NestedRelationshipProcessingStateMachine(neo4jMappingContext);
315329
return getDatabaseName().flatMapMany(
316-
databaseName -> Flux.fromIterable(entities).flatMap(e -> this.saveImpl(e, databaseName.getValue())));
330+
databaseName -> Flux.fromIterable(entities).concatMap(e -> this.saveImpl(e, databaseName.getValue(), stateMachine)));
317331
}
318332

319333
Function<T, Map<String, Object>> binderFunction = neo4jMappingContext.getRequiredBinderFunctionFor(domainClass);
@@ -343,8 +357,7 @@ public <T> Flux<T> saveAll(Iterable<T> instances) {
343357
Neo4jPersistentProperty idProperty = entityMetaData.getRequiredIdProperty();
344358
Object id = convertIdValues(idProperty, propertyAccessor.getProperty(idProperty));
345359
Long internalId = idToInternalIdMapping.get(id);
346-
return processRelations(entityMetaData, t.getT1(), internalId,
347-
propertyAccessor, databaseName.getValue(), t.getT2());
360+
return processRelations(entityMetaData, propertyAccessor, t.getT2(), new NestedRelationshipProcessingStateMachine(neo4jMappingContext, t.getT1(), internalId), databaseName.getValue());
348361
}))
349362
));
350363
}
@@ -445,8 +458,8 @@ private <T> Mono<ExecutableQuery<T>> createExecutableQuery(Class<T> domainType,
445458
private <T> Mono<ExecutableQuery<T>> createExecutableQuery(Class<T> domainType,
446459
QueryFragmentsAndParameters queryFragmentsAndParameters) {
447460

448-
Neo4jPersistentEntity<?> entityMetaData = neo4jMappingContext.getPersistentEntity(domainType);
449461
QueryFragmentsAndParameters.QueryFragments queryFragments = queryFragmentsAndParameters.getQueryFragments();
462+
Neo4jPersistentEntity<?> entityMetaData = (Neo4jPersistentEntity<?>) queryFragmentsAndParameters.getNodeDescription();
450463

451464
boolean containsPossibleCircles = entityMetaData != null && entityMetaData.containsPossibleCircles(queryFragments::includeField);
452465
if (containsPossibleCircles && !queryFragments.isScalarValueReturn()) {
@@ -574,24 +587,24 @@ Publisher<Tuple2<Collection<Long>, Collection<Long>>>> iterateAndMapNextLevel(
574587
* Starts of processing of the relationships.
575588
*
576589
* @param neo4jPersistentEntity The description of the instance to save
577-
* @param originalInstance The original parent instance. It is paramount to pass in the original instance (prior
578-
* to generating the id and prior to eventually create new instances via the property accessor,
579-
* so that we can reliable stop traversing relationships.
580590
* @param parentPropertyAccessor The property accessor of the parent, to modify the relationships
581591
* @param isParentObjectNew A flag if the parent was new
592+
* @param stateMachine Statemachine containing process data
593+
* @param inDatabase Optional target database
582594
* @param <T> The type of the entity to save
583595
* @return A mono representing the whole stream of save operations.
584596
*/
585-
private <T> Mono<T> processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, T originalInstance,
586-
Long internalId, PersistentPropertyAccessor<?> parentPropertyAccessor,
587-
@Nullable String inDatabase, boolean isParentObjectNew) {
597+
private <T> Mono<T> processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity,
598+
PersistentPropertyAccessor<?> parentPropertyAccessor,
599+
boolean isParentObjectNew, NestedRelationshipProcessingStateMachine stateMachine,
600+
@Nullable String inDatabase) {
588601

589-
return processNestedRelations(neo4jPersistentEntity, parentPropertyAccessor, isParentObjectNew, inDatabase,
590-
new NestedRelationshipProcessingStateMachine(originalInstance, internalId));
602+
return processNestedRelations(neo4jPersistentEntity, parentPropertyAccessor, isParentObjectNew, stateMachine,
603+
inDatabase);
591604
}
592605

593606
private <T> Mono<T> processNestedRelations(Neo4jPersistentEntity<?> sourceEntity, PersistentPropertyAccessor<?> parentPropertyAccessor,
594-
boolean isParentObjectNew, @Nullable String inDatabase, NestedRelationshipProcessingStateMachine stateMachine) {
607+
boolean isParentObjectNew, NestedRelationshipProcessingStateMachine stateMachine, @Nullable String inDatabase) {
595608

596609
Object fromId = parentPropertyAccessor.getProperty(sourceEntity.getRequiredIdProperty());
597610
List<Mono<Void>> relationshipDeleteMonos = new ArrayList<>();
@@ -665,36 +678,53 @@ private <T> Mono<T> processNestedRelations(Neo4jPersistentEntity<?> sourceEntity
665678
Flux<RelationshipHandler> relationshipCreation = Flux.fromIterable(relatedValuesToStore).concatMap(relatedValueToStore -> {
666679

667680
Object relatedObjectBeforeCallbacksApplied = relationshipContext.identifyAndExtractRelationshipTargetNode(relatedValueToStore);
668-
return Mono.deferContextual(ctx -> eventSupport
669-
.maybeCallBeforeBind(relatedObjectBeforeCallbacksApplied)
681+
return Mono.deferContextual(ctx ->
682+
683+
(stateMachine.hasProcessedValue(relatedObjectBeforeCallbacksApplied)
684+
? Mono.just(stateMachine.getProcessedAs(relatedObjectBeforeCallbacksApplied))
685+
: eventSupport.maybeCallBeforeBind(relatedObjectBeforeCallbacksApplied))
686+
670687
.flatMap(newRelatedObject -> {
671688
Neo4jPersistentEntity<?> targetEntity = neo4jMappingContext.getPersistentEntity(relatedObjectBeforeCallbacksApplied.getClass());
672689

673-
Mono<Tuple2<Long, Long>> queryOrSave;
690+
Mono<Tuple2<Long[], Long[]>> queryOrSave;
674691
long noVersion = Long.MIN_VALUE;
692+
long noId = Long.MIN_VALUE;
675693
if (stateMachine.hasProcessedValue(relatedValueToStore)) {
676-
queryOrSave = Mono.just(stateMachine.getInternalId(relatedObjectBeforeCallbacksApplied))
677-
.map(id -> Tuples.of(id, noVersion));
694+
queryOrSave = Mono.just(new Long[] { stateMachine.getInternalId(relatedObjectBeforeCallbacksApplied) })
695+
.map(id -> Tuples.of(id, new Long[1]));
678696
} else {
679697
queryOrSave = saveRelatedNode(newRelatedObject, targetEntity, inDatabase)
680-
.map(entity -> Tuples.of(entity.id(), targetEntity.hasVersionProperty() ?
681-
entity.get(targetEntity.getVersionProperty().getPropertyName())
682-
.asLong() :
683-
noVersion));
698+
.doOnNext(entity -> {
699+
stateMachine.markValueAsProcessed(relatedValueToStore, entity.id());
700+
})
701+
.map(entity -> {
702+
Long version = targetEntity.hasVersionProperty() ?
703+
entity.get(targetEntity.getVersionProperty().getPropertyName()).asLong() :
704+
null;
705+
return Tuples.of(
706+
new Long[] { entity.id() },
707+
new Long[] { version });
708+
});
684709
}
685710

686711
return queryOrSave.flatMap(idAndVersion -> {
687-
long relatedInternalId = idAndVersion.getT1();
688-
stateMachine.markValueAsProcessed(relatedValueToStore, relatedInternalId);
712+
Long relatedInternalId = idAndVersion.getT1()[0];
689713
// if an internal id is used this must be set to link this entity in the next iteration
690714
PersistentPropertyAccessor<?> targetPropertyAccessor = targetEntity.getPropertyAccessor(newRelatedObject);
691715
if (targetEntity.isUsingInternalIds()) {
692-
targetPropertyAccessor.setProperty(targetEntity.getRequiredIdProperty(), relatedInternalId);
693-
stateMachine.markValueAsProcessedAs(newRelatedObject, targetPropertyAccessor.getBean());
716+
Neo4jPersistentProperty requiredIdProperty = targetEntity.getRequiredIdProperty();
717+
if (relatedInternalId == null
718+
&& targetPropertyAccessor.getProperty(requiredIdProperty) != null) {
719+
relatedInternalId = (Long) targetPropertyAccessor.getProperty(requiredIdProperty);
720+
} else if (targetPropertyAccessor.getProperty(requiredIdProperty) == null) {
721+
targetPropertyAccessor.setProperty(requiredIdProperty, relatedInternalId);
722+
}
694723
}
695-
if (targetEntity.hasVersionProperty() && idAndVersion.getT2() != noVersion) {
696-
targetPropertyAccessor.setProperty(targetEntity.getVersionProperty(), idAndVersion.getT2());
724+
if (targetEntity.hasVersionProperty() && idAndVersion.getT2()[0] != null) {
725+
targetPropertyAccessor.setProperty(targetEntity.getVersionProperty(), idAndVersion.getT2()[0]);
697726
}
727+
stateMachine.markValueAsProcessedAs(relatedObjectBeforeCallbacksApplied, targetPropertyAccessor.getBean());
698728
stateMachine.markRelationshipAsProcessed(relatedInternalId, relationshipDescription.getRelationshipObverse());
699729

700730
Object idValue = idProperty != null
@@ -726,7 +756,7 @@ private <T> Mono<T> processNestedRelations(Neo4jPersistentEntity<?> sourceEntity
726756

727757
Mono<Object> nestedRelationshipsSignal = null;
728758
if (processState != ProcessState.PROCESSED_ALL_VALUES) {
729-
nestedRelationshipsSignal = processNestedRelations(targetEntity, targetPropertyAccessor, targetEntity.isNew(newRelatedObject), inDatabase, stateMachine);
759+
nestedRelationshipsSignal = processNestedRelations(targetEntity, targetPropertyAccessor, targetEntity.isNew(newRelatedObject), stateMachine, inDatabase);
730760
}
731761

732762
Mono<Object> getRelationshipOrRelationshipPropertiesObject = Mono.fromSupplier(() -> MappingSupport.getRelationshipOrRelationshipPropertiesObject(

0 commit comments

Comments
 (0)