From e7989df04f92fe14feaa8d71fb88f3558717e602 Mon Sep 17 00:00:00 2001 From: Gerrit Meier Date: Thu, 12 Nov 2020 15:47:44 +0100 Subject: [PATCH] DATAGRAPH-1427 - Optimistic locking check on delete. --- etc/adr/adr-007.adoc | 30 +++++++++ .../data/neo4j/core/Neo4jOperations.java | 3 + .../data/neo4j/core/Neo4jTemplate.java | 28 ++++++++ .../neo4j/core/ReactiveNeo4jOperations.java | 4 ++ .../neo4j/core/ReactiveNeo4jTemplate.java | 32 +++++++++ .../support/SimpleNeo4jRepository.java | 11 +++- .../SimpleReactiveNeo4jRepository.java | 13 +++- .../imperative/OptimisticLockingIT.java | 65 +++++++++++++++++++ .../reactive/ReactiveOptimisticLockingIT.java | 63 ++++++++++++++++++ 9 files changed, 245 insertions(+), 4 deletions(-) create mode 100644 etc/adr/adr-007.adoc diff --git a/etc/adr/adr-007.adoc b/etc/adr/adr-007.adoc new file mode 100644 index 000000000..bdb885901 --- /dev/null +++ b/etc/adr/adr-007.adoc @@ -0,0 +1,30 @@ +== ADR 7: Optimistic locking on delete + +=== Status + +accepted + +=== Context + +At the moment there is no optimistic locking check at all when it comes to entity deletion via `delete...` API calls. + +=== Decision + +For `deleteById`: +It won't make any sense with just an identifier provided, to load the current data from the database and compare it on save with the retrieved version. +In 99.999(...)% of the use-cases it will be the same version. +For the rest it would mean that the data has just been modified between loading the version and the delete call. +If the latter would have been the only call against the database, it would have removed the entity correctly. + +For `delete(entity)`: +There should be a check against the version of the previously loaded entity. +As a consequent throw a `OptimisticLockingFailureException` if the versions have a mismatch on delete. + +Non-existing version parameters during `delete(entity)`: +Fall back to `OR IS NULL` to provide a relaxed mode. +Otherwise, we would initialize the version actively to just delete the node. + +=== Consequences + +A clean line between what is supposed to work with optimistic locking and what does not. +Additionally, this aligns the Neo4j module with the other Spring Data modules. diff --git a/src/main/java/org/springframework/data/neo4j/core/Neo4jOperations.java b/src/main/java/org/springframework/data/neo4j/core/Neo4jOperations.java index a7fdce8b2..2843c9847 100644 --- a/src/main/java/org/springframework/data/neo4j/core/Neo4jOperations.java +++ b/src/main/java/org/springframework/data/neo4j/core/Neo4jOperations.java @@ -22,6 +22,7 @@ import java.util.Optional; import org.apiguardian.api.API; import org.neo4j.cypherdsl.core.Statement; import org.springframework.dao.IncorrectResultSizeDataAccessException; +import org.springframework.data.neo4j.core.mapping.Neo4jPersistentProperty; import org.springframework.data.neo4j.repository.NoResultException; /** @@ -196,6 +197,8 @@ public interface Neo4jOperations { */ void deleteById(Object id, Class domainType); + void deleteByIdWithVersion(Object id, Class domainType, Neo4jPersistentProperty versionProperty, Object versionValue); + /** * Deletes all entities with one of the given ids, including all entities related to that entity. * diff --git a/src/main/java/org/springframework/data/neo4j/core/Neo4jTemplate.java b/src/main/java/org/springframework/data/neo4j/core/Neo4jTemplate.java index 42ae4cdc4..b45a9352c 100644 --- a/src/main/java/org/springframework/data/neo4j/core/Neo4jTemplate.java +++ b/src/main/java/org/springframework/data/neo4j/core/Neo4jTemplate.java @@ -21,6 +21,7 @@ import static org.neo4j.cypherdsl.core.Cypher.parameter; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Optional; @@ -30,6 +31,7 @@ import java.util.stream.Collectors; import org.apache.commons.logging.LogFactory; import org.apiguardian.api.API; import org.neo4j.cypherdsl.core.Condition; +import org.neo4j.cypherdsl.core.Cypher; import org.neo4j.cypherdsl.core.Functions; import org.neo4j.cypherdsl.core.Statement; import org.neo4j.cypherdsl.core.renderer.Renderer; @@ -343,6 +345,32 @@ public final class Neo4jTemplate implements Neo4jOperations, BeanFactoryAware { summary.counters().relationshipsDeleted())); } + @Override + public void deleteByIdWithVersion(Object id, Class domainType, Neo4jPersistentProperty versionProperty, + Object versionValue) { + + Neo4jPersistentEntity entityMetaData = neo4jMappingContext.getPersistentEntity(domainType); + + String nameOfParameter = "id"; + Condition condition = entityMetaData.getIdExpression().isEqualTo(parameter(nameOfParameter)) + .and(Cypher.property(Constants.NAME_OF_ROOT_NODE, versionProperty.getPropertyName()) + .isEqualTo(parameter(Constants.NAME_OF_VERSION_PARAM)) + .or(Cypher.property(Constants.NAME_OF_ROOT_NODE, versionProperty.getPropertyName()).isNull())); + + Statement statement = cypherGenerator.prepareMatchOf(entityMetaData, condition) + .returning(Constants.NAME_OF_ROOT_NODE).build(); + + Map parameters = new HashMap<>(); + parameters.put(nameOfParameter, convertIdValues(entityMetaData.getRequiredIdProperty(), id)); + parameters.put(Constants.NAME_OF_VERSION_PARAM, versionValue); + + createExecutableQuery(domainType, statement, parameters).getSingleResult().orElseThrow( + () -> new OptimisticLockingFailureException(OPTIMISTIC_LOCKING_ERROR_MESSAGE) + ); + + deleteById(id, domainType); + } + @Override public void deleteAllById(Iterable ids, Class domainType) { diff --git a/src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jOperations.java b/src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jOperations.java index 0651715da..42304fefb 100644 --- a/src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jOperations.java +++ b/src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jOperations.java @@ -15,6 +15,7 @@ */ package org.springframework.data.neo4j.core; +import org.springframework.data.neo4j.core.mapping.Neo4jPersistentProperty; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; @@ -195,6 +196,9 @@ public interface ReactiveNeo4jOperations { */ Mono deleteById(Object id, Class domainType); + Mono deleteByIdWithVersion(Object id, Class domainType, Neo4jPersistentProperty versionProperty, + Object versionValue); + /** * Deletes all entities with one of the given ids, including all entities related to that entity. * diff --git a/src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jTemplate.java b/src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jTemplate.java index f227a80a5..84c78794c 100644 --- a/src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jTemplate.java +++ b/src/main/java/org/springframework/data/neo4j/core/ReactiveNeo4jTemplate.java @@ -18,6 +18,7 @@ package org.springframework.data.neo4j.core; import static org.neo4j.cypherdsl.core.Cypher.asterisk; import static org.neo4j.cypherdsl.core.Cypher.parameter; +import org.neo4j.cypherdsl.core.Cypher; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import reactor.util.function.Tuple2; @@ -26,6 +27,7 @@ import reactor.util.function.Tuples; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.function.Function; @@ -348,6 +350,36 @@ public final class ReactiveNeo4jTemplate implements ReactiveNeo4jOperations, Bea .to(nameOfParameter).run().then()); } + @Override + public Mono deleteByIdWithVersion(Object id, Class domainType, Neo4jPersistentProperty versionProperty, + Object versionValue) { + + String nameOfParameter = "id"; + Neo4jPersistentEntity entityMetaData = neo4jMappingContext.getPersistentEntity(domainType); + Condition condition = entityMetaData.getIdExpression().isEqualTo(parameter(nameOfParameter)) + .and(Cypher.property(Constants.NAME_OF_ROOT_NODE, versionProperty.getPropertyName()) + .isEqualTo(parameter(Constants.NAME_OF_VERSION_PARAM)) + .or(Cypher.property(Constants.NAME_OF_ROOT_NODE, versionProperty.getPropertyName()).isNull())); + + Statement statement = cypherGenerator.prepareMatchOf(entityMetaData, condition) + .returning(Constants.NAME_OF_ROOT_NODE).build(); + + Map parameters = new HashMap<>(); + parameters.put(nameOfParameter, convertIdValues(entityMetaData.getRequiredIdProperty(), id)); + parameters.put(Constants.NAME_OF_VERSION_PARAM, versionValue); + + return getDatabaseName().flatMap(databaseName -> this.neo4jClient.query(() -> renderer.render(statement)) + .in(databaseName.getValue()) + .bindAll(parameters) + .fetch().one().switchIfEmpty(Mono.defer(() -> { + if (entityMetaData.hasVersionProperty()) { + return Mono.error(() -> new OptimisticLockingFailureException(OPTIMISTIC_LOCKING_ERROR_MESSAGE)); + } + return Mono.empty(); + }))) + .then(deleteById(id, domainType)); + } + @Override public Mono deleteAll(Class domainType) { diff --git a/src/main/java/org/springframework/data/neo4j/repository/support/SimpleNeo4jRepository.java b/src/main/java/org/springframework/data/neo4j/repository/support/SimpleNeo4jRepository.java index da64763d2..692d4e935 100644 --- a/src/main/java/org/springframework/data/neo4j/repository/support/SimpleNeo4jRepository.java +++ b/src/main/java/org/springframework/data/neo4j/repository/support/SimpleNeo4jRepository.java @@ -29,8 +29,9 @@ import org.springframework.data.domain.Page; import org.springframework.data.domain.Pageable; import org.springframework.data.domain.Sort; import org.springframework.data.neo4j.core.Neo4jOperations; -import org.springframework.data.neo4j.core.mapping.Neo4jPersistentEntity; import org.springframework.data.neo4j.core.mapping.CypherGenerator; +import org.springframework.data.neo4j.core.mapping.Neo4jPersistentEntity; +import org.springframework.data.neo4j.core.mapping.Neo4jPersistentProperty; import org.springframework.data.neo4j.repository.query.CypherAdapterUtils; import org.springframework.data.repository.PagingAndSortingRepository; import org.springframework.data.repository.support.PageableExecutionUtils; @@ -149,7 +150,13 @@ public class SimpleNeo4jRepository implements PagingAndSortingRepository< public void delete(T entity) { ID id = this.entityInformation.getId(entity); - this.deleteById(id); + if (entityMetaData.hasVersionProperty()) { + Neo4jPersistentProperty versionProperty = entityMetaData.getRequiredVersionProperty(); + Object versionValue = entityMetaData.getPropertyAccessor(entity).getProperty(versionProperty); + this.neo4jOperations.deleteByIdWithVersion(id, this.entityInformation.getJavaType(), versionProperty, versionValue); + } else { + this.deleteById(id); + } } @Override diff --git a/src/main/java/org/springframework/data/neo4j/repository/support/SimpleReactiveNeo4jRepository.java b/src/main/java/org/springframework/data/neo4j/repository/support/SimpleReactiveNeo4jRepository.java index e316f5840..65d3795ac 100644 --- a/src/main/java/org/springframework/data/neo4j/repository/support/SimpleReactiveNeo4jRepository.java +++ b/src/main/java/org/springframework/data/neo4j/repository/support/SimpleReactiveNeo4jRepository.java @@ -15,6 +15,7 @@ */ package org.springframework.data.neo4j.repository.support; +import org.springframework.data.neo4j.core.mapping.Neo4jPersistentProperty; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; @@ -170,9 +171,17 @@ public class SimpleReactiveNeo4jRepository implements ReactiveSortingRepo @Override @Transactional public Mono delete(T entity) { - Assert.notNull(entity, "The given entity must not be null!"); - return deleteById(this.entityInformation.getId(entity)); + + ID id = this.entityInformation.getId(entity); + if (entityMetaData.hasVersionProperty()) { + Neo4jPersistentProperty versionProperty = entityMetaData.getRequiredVersionProperty(); + Object versionValue = entityMetaData.getPropertyAccessor(entity).getProperty(versionProperty); + return this.neo4jOperations.deleteByIdWithVersion(id, this.entityInformation.getJavaType(), versionProperty, versionValue); + } else { + return this.deleteById(id); + } + } /* diff --git a/src/test/java/org/springframework/data/neo4j/integration/imperative/OptimisticLockingIT.java b/src/test/java/org/springframework/data/neo4j/integration/imperative/OptimisticLockingIT.java index 4598cd80a..8bfe56a0c 100644 --- a/src/test/java/org/springframework/data/neo4j/integration/imperative/OptimisticLockingIT.java +++ b/src/test/java/org/springframework/data/neo4j/integration/imperative/OptimisticLockingIT.java @@ -205,6 +205,71 @@ class OptimisticLockingIT { } + @Test + void shouldNotFailOnDeleteByIdWithNullVersion(@Autowired VersionedThingWithAssignedIdRepository repository) { + try (Session session = driver.session()) { + session.writeTransaction(tx -> tx.run("CREATE (v:VersionedThingWithAssignedId {id:1})").consume()); + } + + repository.deleteById(1L); + + try (Session session = driver.session()) { + long count = session.readTransaction(tx -> + tx.run("MATCH (v:VersionedThingWithAssignedId) return count(v) as vCount").next() + .get("vCount").asLong()); + + assertThat(count).isEqualTo(0); + } + } + + @Test + void shouldNotFailOnDeleteByEntityWithNullVersion(@Autowired VersionedThingWithAssignedIdRepository repository) { + try (Session session = driver.session()) { + session.writeTransaction(tx -> tx.run("CREATE (v:VersionedThingWithAssignedId {id:1})").consume()); + } + + VersionedThingWithAssignedId thing = repository.findById(1L).get(); + repository.delete(thing); + + try (Session session = driver.session()) { + long count = session.readTransaction(tx -> + tx.run("MATCH (v:VersionedThingWithAssignedId) return count(v) as vCount").next() + .get("vCount").asLong()); + + assertThat(count).isEqualTo(0); + } + } + + @Test + void shouldNotFailOnDeleteByIdWithAnyVersion(@Autowired VersionedThingWithAssignedIdRepository repository) { + try (Session session = driver.session()) { + session.writeTransaction(tx -> + tx.run("CREATE (v:VersionedThingWithAssignedId {id:1, myVersion:3})").consume()); + } + + repository.deleteById(1L); + + try (Session session = driver.session()) { + long count = session.readTransaction(tx -> + tx.run("MATCH (v:VersionedThingWithAssignedId) return count(v) as vCount").next() + .get("vCount").asLong()); + + assertThat(count).isEqualTo(0); + } + } + + @Test + void shouldFailOnDeleteByEntityWithWrongVersion(@Autowired VersionedThingWithAssignedIdRepository repository) { + try (Session session = driver.session()) { + session.writeTransaction(tx -> + tx.run("CREATE (v:VersionedThingWithAssignedId {id:1, myVersion:2})").consume()); + } + + VersionedThingWithAssignedId thing = repository.findById(1L).get(); + thing.setMyVersion(3L); + assertThatExceptionOfType(OptimisticLockingFailureException.class).isThrownBy(() -> repository.delete(thing)); + } + interface VersionedThingRepository extends Neo4jRepository {} interface VersionedThingWithAssignedIdRepository extends Neo4jRepository {} diff --git a/src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveOptimisticLockingIT.java b/src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveOptimisticLockingIT.java index 6eff4e26b..20e5fe5b3 100644 --- a/src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveOptimisticLockingIT.java +++ b/src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveOptimisticLockingIT.java @@ -204,6 +204,69 @@ class ReactiveOptimisticLockingIT { } + @Test + void shouldNotFailOnDeleteByIdWithNullVersion(@Autowired VersionedThingWithAssignedIdRepository repository) { + try (Session session = driver.session()) { + session.writeTransaction(tx -> tx.run("CREATE (v:VersionedThingWithAssignedId {id:1})").consume()); + } + + StepVerifier.create(repository.deleteById(1L)) + .verifyComplete(); + + try (Session session = driver.session()) { + long count = session.readTransaction(tx -> + tx.run("MATCH (v:VersionedThingWithAssignedId) return count(v) as vCount").next() + .get("vCount").asLong()); + + assertThat(count).isEqualTo(0); + } + } + + @Test + void shouldNotFailOnDeleteByEntityWithNullVersion(@Autowired VersionedThingWithAssignedIdRepository repository) { + try (Session session = driver.session()) { + session.writeTransaction(tx -> tx.run("CREATE (v:VersionedThingWithAssignedId {id:1})").consume()); + } + + StepVerifier.create(repository.findById(1L).map(thing -> repository.deleteById(1L))) + .expectNextCount(1L) + .verifyComplete(); + + } + + @Test + void shouldNotFailOnDeleteByIdWithAnyVersion(@Autowired VersionedThingWithAssignedIdRepository repository) { + try (Session session = driver.session()) { + session.writeTransaction(tx -> + tx.run("CREATE (v:VersionedThingWithAssignedId {id:1, myVersion:3})").consume()); + } + + StepVerifier.create(repository.deleteById(1L)) + .verifyComplete(); + + try (Session session = driver.session()) { + long count = session.readTransaction(tx -> + tx.run("MATCH (v:VersionedThingWithAssignedId) return count(v) as vCount").next() + .get("vCount").asLong()); + + assertThat(count).isEqualTo(0); + } + } + + @Test + void shouldFailOnDeleteByEntityWithWrongVersion(@Autowired VersionedThingWithAssignedIdRepository repository) { + try (Session session = driver.session()) { + session.writeTransaction(tx -> tx.run("CREATE (v:VersionedThingWithAssignedId {id:1, myVersion:2})").consume()); + } + + StepVerifier.create(repository.findById(1L) + .flatMap(thing -> { + thing.setMyVersion(3L); + return repository.delete(thing); + })).verifyError(OptimisticLockingFailureException.class); + + } + interface VersionedThingRepository extends ReactiveNeo4jRepository {} interface VersionedThingWithAssignedIdRepository