Skip to content

Commit 9287f89

Browse files
meistermeiermeistermeier
committed
DATAGRAPH-1430 - Avoid unnessaccary delete statements for new entities.
1 parent b77404d commit 9287f89

File tree

7 files changed

+345
-26
lines changed

7 files changed

+345
-26
lines changed

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

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,9 @@ public <T> T save(T instance) {
227227

228228
private <T> T saveImpl(T instance, @Nullable String inDatabase) {
229229

230-
Neo4jPersistentEntity entityMetaData = neo4jMappingContext.getPersistentEntity(instance.getClass());
230+
Neo4jPersistentEntity<?> entityMetaData = neo4jMappingContext.getPersistentEntity(instance.getClass());
231+
boolean isEntityNew = entityMetaData.isNew(instance);
232+
231233
T entityToBeSaved = eventSupport.maybeCallBeforeBind(instance);
232234

233235
DynamicLabels dynamicLabels = determineDynamicLabels(entityToBeSaved, entityMetaData, inDatabase);
@@ -245,11 +247,11 @@ private <T> T saveImpl(T instance, @Nullable String inDatabase) {
245247

246248
PersistentPropertyAccessor<T> propertyAccessor = entityMetaData.getPropertyAccessor(entityToBeSaved);
247249
if (!entityMetaData.isUsingInternalIds()) {
248-
processRelations(entityMetaData, entityToBeSaved, inDatabase);
250+
processRelations(entityMetaData, entityToBeSaved, isEntityNew, inDatabase);
249251
return entityToBeSaved;
250252
} else {
251253
propertyAccessor.setProperty(entityMetaData.getRequiredIdProperty(), optionalInternalId.get());
252-
processRelations(entityMetaData, entityToBeSaved, inDatabase);
254+
processRelations(entityMetaData, entityToBeSaved, isEntityNew, inDatabase);
253255

254256
return propertyAccessor.getBean();
255257
}
@@ -303,6 +305,11 @@ public <T> List<T> saveAll(Iterable<T> instances) {
303305
return entities.stream().map(e -> saveImpl(e, databaseName)).collect(Collectors.toList());
304306
}
305307

308+
// we need to determine the `isNew` state of the entities before calling the id generator
309+
List<Boolean> isNewIndicator = entities.stream().map(entity ->
310+
neo4jMappingContext.getPersistentEntity(entity.getClass()).isNew(entity)
311+
).collect(Collectors.toList());
312+
306313
List<T> entitiesToBeSaved = entities.stream().map(eventSupport::maybeCallBeforeBind)
307314
.collect(Collectors.toList());
308315

@@ -316,7 +323,8 @@ public <T> List<T> saveAll(Iterable<T> instances) {
316323
.bind(entityList).to(Constants.NAME_OF_ENTITY_LIST_PARAM).run();
317324

318325
// Save related
319-
entitiesToBeSaved.forEach(entityToBeSaved -> processRelations(entityMetaData, entityToBeSaved, databaseName));
326+
entitiesToBeSaved.forEach(entityToBeSaved -> processRelations(entityMetaData, entityToBeSaved,
327+
isNewIndicator.get(entitiesToBeSaved.indexOf(entityToBeSaved)), databaseName));
320328

321329
SummaryCounters counters = resultSummary.counters();
322330
log.debug(() -> String.format(
@@ -428,14 +436,14 @@ private <T> ExecutableQuery<T> createExecutableQuery(Class<T> domainType, String
428436
}
429437

430438
private void processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, Object parentObject,
431-
@Nullable String inDatabase) {
439+
boolean isParentObjectNew, @Nullable String inDatabase) {
432440

433-
processNestedRelations(neo4jPersistentEntity, parentObject, inDatabase,
441+
processNestedRelations(neo4jPersistentEntity, parentObject, isParentObjectNew, inDatabase,
434442
new NestedRelationshipProcessingStateMachine());
435443
}
436444

437445
private void processNestedRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, Object parentObject,
438-
@Nullable String inDatabase, NestedRelationshipProcessingStateMachine stateMachine) {
446+
boolean isParentObjectNew, @Nullable String inDatabase, NestedRelationshipProcessingStateMachine stateMachine) {
439447

440448
PersistentPropertyAccessor<?> propertyAccessor = neo4jPersistentEntity.getPropertyAccessor(parentObject);
441449
Object fromId = propertyAccessor.getProperty(neo4jPersistentEntity.getRequiredIdProperty());
@@ -460,7 +468,7 @@ private void processNestedRelations(Neo4jPersistentEntity<?> neo4jPersistentEnti
460468

461469
// remove all relationships before creating all new if the entity is not new
462470
// this avoids the usage of cache but might have significant impact on overall performance
463-
if (!neo4jPersistentEntity.isNew(parentObject)) {
471+
if (!isParentObjectNew) {
464472
Neo4jPersistentEntity<?> previouslyRelatedPersistentEntity = neo4jMappingContext
465473
.getPersistentEntity(relationshipContext.getAssociationTargetType());
466474

@@ -483,11 +491,13 @@ private void processNestedRelations(Neo4jPersistentEntity<?> neo4jPersistentEnti
483491

484492
// here map entry is not always anymore a dynamic association
485493
Object relatedNode = relationshipContext.identifyAndExtractRelationshipTargetNode(relatedValueToStore);
486-
relatedNode = eventSupport.maybeCallBeforeBind(relatedNode);
487-
488494
Neo4jPersistentEntity<?> targetNodeDescription = neo4jMappingContext
489495
.getPersistentEntity(relatedNode.getClass());
490496

497+
boolean isEntityNew = targetNodeDescription.isNew(relatedNode);
498+
499+
relatedNode = eventSupport.maybeCallBeforeBind(relatedNode);
500+
491501
Long relatedInternalId = saveRelatedNode(relatedNode, relationshipContext.getAssociationTargetType(),
492502
targetNodeDescription, inDatabase);
493503

@@ -507,7 +517,7 @@ private void processNestedRelations(Neo4jPersistentEntity<?> neo4jPersistentEnti
507517
.setProperty(targetNodeDescription.getRequiredIdProperty(), relatedInternalId);
508518
}
509519
if (processState != ProcessState.PROCESSED_ALL_VALUES) {
510-
processNestedRelations(targetNodeDescription, relatedNode, inDatabase, stateMachine);
520+
processNestedRelations(targetNodeDescription, relatedNode, isEntityNew, inDatabase, stateMachine);
511521
}
512522
}
513523
});

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

Lines changed: 35 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
import static org.neo4j.cypherdsl.core.Cypher.asterisk;
1919
import static org.neo4j.cypherdsl.core.Cypher.parameter;
2020

21-
import org.neo4j.cypherdsl.core.Cypher;
2221
import reactor.core.publisher.Flux;
2322
import reactor.core.publisher.Mono;
2423
import reactor.util.function.Tuple2;
@@ -36,6 +35,7 @@
3635
import org.apache.commons.logging.LogFactory;
3736
import org.apiguardian.api.API;
3837
import org.neo4j.cypherdsl.core.Condition;
38+
import org.neo4j.cypherdsl.core.Cypher;
3939
import org.neo4j.cypherdsl.core.Functions;
4040
import org.neo4j.cypherdsl.core.Statement;
4141
import org.neo4j.cypherdsl.core.renderer.Renderer;
@@ -223,7 +223,8 @@ public <T> Mono<T> save(T instance) {
223223
private <T> Mono<T> saveImpl(T instance, @Nullable String inDatabase) {
224224

225225
Neo4jPersistentEntity entityMetaData = neo4jMappingContext.getPersistentEntity(instance.getClass());
226-
return Mono.just(instance).flatMap(eventSupport::maybeCallBeforeBind)
226+
return Mono.just(entityMetaData.isNew(instance))
227+
.flatMap(isNewEntity -> Mono.just(instance).flatMap(eventSupport::maybeCallBeforeBind)
227228
.flatMap(entity -> determineDynamicLabels(entity, entityMetaData, inDatabase)).flatMap(t -> {
228229
T entity = t.getT1();
229230
DynamicLabels dynamicLabels = t.getT2();
@@ -240,17 +241,19 @@ private <T> Mono<T> saveImpl(T instance, @Nullable String inDatabase) {
240241
}));
241242

242243
if (!entityMetaData.isUsingInternalIds()) {
243-
return idMono.then(processRelations(entityMetaData, entity, inDatabase)).thenReturn(entity);
244+
return idMono.then(processRelations(entityMetaData, entity, isNewEntity, inDatabase))
245+
.thenReturn(entity);
244246
} else {
245247
return idMono.map(internalId -> {
246248
PersistentPropertyAccessor<T> propertyAccessor = entityMetaData.getPropertyAccessor(entity);
247249
propertyAccessor.setProperty(entityMetaData.getRequiredIdProperty(), internalId);
248250

249251
return propertyAccessor.getBean();
250252
}).flatMap(
251-
savedEntity -> processRelations(entityMetaData, savedEntity, inDatabase).thenReturn(savedEntity));
253+
savedEntity -> processRelations(entityMetaData, savedEntity, isNewEntity, inDatabase)
254+
.thenReturn(savedEntity));
252255
}
253-
});
256+
}));
254257
}
255258

256259
private <T> Mono<Tuple2<T, DynamicLabels>> determineDynamicLabels(T entityToBeSaved,
@@ -302,12 +305,12 @@ public <T> Flux<T> saveAll(Iterable<T> instances) {
302305
}
303306

304307
Function<T, Map<String, Object>> binderFunction = neo4jMappingContext.getRequiredBinderFunctionFor(domainClass);
308+
String isNewIndicatorKey = "isNewIndicator";
305309
return getDatabaseName().flatMapMany(databaseName -> Flux.fromIterable(entities)
306310
.flatMap(eventSupport::maybeCallBeforeBind).collectList().flatMapMany(entitiesToBeSaved -> Mono.defer(() -> {
307311
// Defer the actual save statement until the previous flux completes
308312
List<Map<String, Object>> boundedEntityList = entitiesToBeSaved.stream().map(binderFunction)
309313
.collect(Collectors.toList());
310-
311314
return neo4jClient
312315
.query(() -> renderer.render(cypherGenerator.prepareSaveOfMultipleInstancesOf(entityMetaData)))
313316
.in(databaseName.getValue()).bind(boundedEntityList).to(Constants.NAME_OF_ENTITY_LIST_PARAM).run();
@@ -317,7 +320,23 @@ public <T> Flux<T> saveAll(Iterable<T> instances) {
317320
"Created %d and deleted %d nodes, created %d and deleted %d relationships and set %d properties.",
318321
counters.nodesCreated(), counters.nodesDeleted(), counters.relationshipsCreated(),
319322
counters.relationshipsDeleted(), counters.propertiesSet()));
320-
}).thenMany(Flux.fromIterable(entitiesToBeSaved))));
323+
}).thenMany(
324+
Flux.deferContextual(ctx -> {
325+
List<Boolean> isNewIndicator = ctx.get(isNewIndicatorKey);
326+
return Flux.fromIterable(entitiesToBeSaved)
327+
.index()
328+
.flatMap(t -> {
329+
T entityToBeSaved = t.getT2();
330+
boolean isNew = isNewIndicator.get(Math.toIntExact(t.getT1()));
331+
return processRelations(entityMetaData, entityToBeSaved, isNew,
332+
databaseName.getValue())
333+
.then(Mono.just(entityToBeSaved));
334+
}
335+
);
336+
})
337+
)))
338+
.contextWrite(ctx -> ctx.put(isNewIndicatorKey, entities.stream()
339+
.map(entity -> entityMetaData.isNew(entity)).collect(Collectors.toList())));
321340
}
322341

323342
@Override
@@ -414,14 +433,14 @@ private <T> Mono<ExecutableQuery<T>> createExecutableQuery(Class<T> domainType,
414433
}
415434

416435
private Mono<Void> processRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, Object parentObject,
417-
@Nullable String inDatabase) {
436+
boolean isParentObjectNew, @Nullable String inDatabase) {
418437

419-
return processNestedRelations(neo4jPersistentEntity, parentObject, inDatabase,
438+
return processNestedRelations(neo4jPersistentEntity, parentObject, isParentObjectNew, inDatabase,
420439
new NestedRelationshipProcessingStateMachine());
421440
}
422441

423442
private Mono<Void> processNestedRelations(Neo4jPersistentEntity<?> neo4jPersistentEntity, Object parentObject,
424-
@Nullable String inDatabase, NestedRelationshipProcessingStateMachine stateMachine) {
443+
boolean isParentObjectNew, @Nullable String inDatabase, NestedRelationshipProcessingStateMachine stateMachine) {
425444

426445
return Mono.defer(() -> {
427446
PersistentPropertyAccessor<?> propertyAccessor = neo4jPersistentEntity.getPropertyAccessor(parentObject);
@@ -448,7 +467,7 @@ private Mono<Void> processNestedRelations(Neo4jPersistentEntity<?> neo4jPersiste
448467

449468
// remove all relationships before creating all new if the entity is not new
450469
// this avoids the usage of cache but might have significant impact on overall performance
451-
if (!neo4jPersistentEntity.isNew(parentObject)) {
470+
if (!isParentObjectNew) {
452471
Neo4jPersistentEntity<?> previouslyRelatedPersistentEntity = neo4jMappingContext
453472
.getPersistentEntity(relationshipContext.getAssociationTargetType());
454473

@@ -475,7 +494,8 @@ private Mono<Void> processNestedRelations(Neo4jPersistentEntity<?> neo4jPersiste
475494
.flatMap(valueToBeSaved -> {
476495
Neo4jPersistentEntity<?> targetNodeDescription = neo4jMappingContext
477496
.getPersistentEntity(valueToBeSavedPreEvt.getClass());
478-
return saveRelatedNode(valueToBeSaved, relationshipContext.getAssociationTargetType(),
497+
return Mono.just(targetNodeDescription.isNew(valueToBeSaved)).flatMap(isNew ->
498+
saveRelatedNode(valueToBeSaved, relationshipContext.getAssociationTargetType(),
479499
targetNodeDescription, inDatabase).flatMap(relatedInternalId -> {
480500

481501
// if an internal id is used this must get set to link this entity in the next iteration
@@ -498,11 +518,12 @@ private Mono<Void> processNestedRelations(Neo4jPersistentEntity<?> neo4jPersiste
498518

499519
if (processState != ProcessState.PROCESSED_ALL_VALUES) {
500520
return relationshipCreationMonoNested.checkpoint().then(
501-
processNestedRelations(targetNodeDescription, valueToBeSaved, inDatabase, stateMachine));
521+
processNestedRelations(targetNodeDescription, valueToBeSaved,
522+
isNew, inDatabase, stateMachine));
502523
} else {
503524
return relationshipCreationMonoNested.checkpoint().then();
504525
}
505-
}).checkpoint();
526+
}).checkpoint());
506527
});
507528
relationshipCreationMonos.add(createRelationship);
508529
}

‎src/main/java/org/springframework/data/neo4j/repository/event/IdPopulator.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ Object populateIfNecessary(Object entity) {
6363
return entity;
6464
}
6565

66-
PersistentPropertyAccessor propertyAccessor = nodeDescription.getPropertyAccessor(entity);
66+
PersistentPropertyAccessor<?> propertyAccessor = nodeDescription.getPropertyAccessor(entity);
6767
Neo4jPersistentProperty idProperty = nodeDescription.getRequiredIdProperty();
6868

6969
// Check existing ID

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

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,9 @@
111111
import org.springframework.data.neo4j.integration.shared.common.Pet;
112112
import org.springframework.data.neo4j.integration.shared.common.SimilarThing;
113113
import org.springframework.data.neo4j.integration.shared.common.SimpleEntityWithRelationshipA;
114+
import org.springframework.data.neo4j.integration.shared.common.SimplePerson;
114115
import org.springframework.data.neo4j.integration.shared.common.ThingWithAssignedId;
116+
import org.springframework.data.neo4j.integration.shared.common.ThingWithFixedGeneratedId;
115117
import org.springframework.data.neo4j.integration.shared.common.ThingWithGeneratedId;
116118
import org.springframework.data.neo4j.integration.shared.common.WorksInClubRelationship;
117119
import org.springframework.data.neo4j.repository.Neo4jRepository;
@@ -1407,6 +1409,108 @@ void saveSingleEntity(@Autowired PersonRepository repository) {
14071409
}
14081410
}
14091411

1412+
@Test // DATAGRAPH-1430
1413+
void saveNewEntityWithGeneratedIdShouldNotIssueRelationshipDeleteStatement(
1414+
@Autowired ThingWithFixedGeneratedIdRepository repository) {
1415+
1416+
try (Session session = createSession()) {
1417+
session.writeTransaction(tx ->
1418+
tx.run("CREATE (:ThingWithFixedGeneratedId{theId:'ThingWithFixedGeneratedId'})" +
1419+
"-[r:KNOWS]->(:SimplePerson) return id(r) as rId").consume());
1420+
}
1421+
1422+
ThingWithFixedGeneratedId thing = new ThingWithFixedGeneratedId("name");
1423+
// this will create a duplicated relationship because we use the same ids
1424+
thing.setPerson(new SimplePerson("someone"));
1425+
repository.save(thing);
1426+
1427+
// ensure that no relationship got deleted upfront
1428+
try (Session session = createSession()) {
1429+
Long relCount = session.readTransaction(tx ->
1430+
tx.run("MATCH (:ThingWithFixedGeneratedId{theId:'ThingWithFixedGeneratedId'})" +
1431+
"-[r:KNOWS]-(:SimplePerson) return count(r) as rCount")
1432+
.next().get("rCount").asLong());
1433+
1434+
assertThat(relCount).isEqualTo(2);
1435+
}
1436+
}
1437+
1438+
@Test // DATAGRAPH-1430
1439+
void updateEntityWithGeneratedIdShouldIssueRelationshipDeleteStatement(
1440+
@Autowired ThingWithFixedGeneratedIdRepository repository) {
1441+
1442+
Long rId;
1443+
try (Session session = createSession()) {
1444+
rId = session.writeTransaction(tx ->
1445+
tx.run("CREATE (:ThingWithFixedGeneratedId{theId:'ThingWithFixedGeneratedId'})" +
1446+
"-[r:KNOWS]->(:SimplePerson) return id(r) as rId")
1447+
.next().get("rId").asLong());
1448+
}
1449+
1450+
ThingWithFixedGeneratedId loadedThing = repository.findById("ThingWithFixedGeneratedId").get();
1451+
repository.save(loadedThing);
1452+
1453+
try (Session session = createSession()) {
1454+
Long newRid = session.readTransaction(tx ->
1455+
tx.run("MATCH (:ThingWithFixedGeneratedId{theId:'ThingWithFixedGeneratedId'})" +
1456+
"-[r:KNOWS]-(:SimplePerson) return id(r) as rId")
1457+
.next().get("rId").asLong());
1458+
1459+
assertThat(rId).isNotEqualTo(newRid);
1460+
}
1461+
}
1462+
1463+
@Test // DATAGRAPH-1430
1464+
void saveAllNewEntityWithGeneratedIdShouldNotIssueRelationshipDeleteStatement(
1465+
@Autowired ThingWithFixedGeneratedIdRepository repository) {
1466+
1467+
try (Session session = createSession()) {
1468+
session.writeTransaction(tx ->
1469+
tx.run("CREATE (:ThingWithFixedGeneratedId{theId:'ThingWithFixedGeneratedId'})" +
1470+
"-[r:KNOWS]->(:SimplePerson) return id(r) as rId").consume());
1471+
}
1472+
1473+
ThingWithFixedGeneratedId thing = new ThingWithFixedGeneratedId("name");
1474+
// this will create a duplicated relationship because we use the same ids
1475+
thing.setPerson(new SimplePerson("someone"));
1476+
repository.saveAll(Collections.singletonList(thing));
1477+
1478+
// ensure that no relationship got deleted upfront
1479+
try (Session session = createSession()) {
1480+
Long relCount = session.readTransaction(tx ->
1481+
tx.run("MATCH (:ThingWithFixedGeneratedId{theId:'ThingWithFixedGeneratedId'})" +
1482+
"-[r:KNOWS]-(:SimplePerson) return count(r) as rCount")
1483+
.next().get("rCount").asLong());
1484+
1485+
assertThat(relCount).isEqualTo(2);
1486+
}
1487+
}
1488+
1489+
@Test // DATAGRAPH-1430
1490+
void updateAllEntityWithGeneratedIdShouldIssueRelationshipDeleteStatement(
1491+
@Autowired ThingWithFixedGeneratedIdRepository repository) {
1492+
1493+
Long rId;
1494+
try (Session session = createSession()) {
1495+
rId = session.writeTransaction(tx ->
1496+
tx.run("CREATE (:ThingWithFixedGeneratedId{theId:'ThingWithFixedGeneratedId'})" +
1497+
"-[r:KNOWS]->(:SimplePerson) return id(r) as rId")
1498+
.next().get("rId").asLong());
1499+
}
1500+
1501+
ThingWithFixedGeneratedId loadedThing = repository.findById("ThingWithFixedGeneratedId").get();
1502+
repository.saveAll(Collections.singletonList(loadedThing));
1503+
1504+
try (Session session = createSession()) {
1505+
Long newRid = session.readTransaction(tx ->
1506+
tx.run("MATCH (:ThingWithFixedGeneratedId{theId:'ThingWithFixedGeneratedId'})" +
1507+
"-[r:KNOWS]-(:SimplePerson) return id(r) as rId")
1508+
.next().get("rId").asLong());
1509+
1510+
assertThat(rId).isNotEqualTo(newRid);
1511+
}
1512+
}
1513+
14101514
@Test
14111515
void saveAll(@Autowired PersonRepository repository) {
14121516

@@ -3341,6 +3445,8 @@ interface ParentRepository extends Neo4jRepository<ParentNode, Long> {
33413445

33423446
interface SimpleEntityWithRelationshipARepository extends Neo4jRepository<SimpleEntityWithRelationshipA, Long> {}
33433447

3448+
interface ThingWithFixedGeneratedIdRepository extends Neo4jRepository<ThingWithFixedGeneratedId, String> {}
3449+
33443450
@SpringJUnitConfig(Config.class)
33453451
static abstract class IntegrationTestBase {
33463452

0 commit comments

Comments
 (0)