GH-2281 - Support derived deleteBy methods.

michael-simons · michael-simons · commit 7bd18330add6 · 2021-06-08T19:38:18.000+02:00
This changes adds support for the derived delete by supports. To make this possible, both clients (imperative and reactive) needs to be a bit more lenient about what happens when they receive a `null` on `one`: Technically, when there has been a result but the mapping function was asked to return a `void`, this is correct. Apart from that, the same `detach delete` logic like in `deleteById` is applied. Valid return types for `deleteBy` are `void` or `long` or wrappers. We don’t match and return. This closes #2281.
diff --git a/src/main/java/org/springframework/data/neo4j/core/DefaultNeo4jClient.java b/src/main/java/org/springframework/data/neo4j/core/DefaultNeo4jClient.java
@@ -281,7 +281,7 @@ public Optional<T> one() {
 			try (AutoCloseableQueryRunner statementRunner = getQueryRunner(this.targetDatabase)) {
 				Result result = runnableStatement.runWith(statementRunner);
 				Optional<T> optionalValue = result.hasNext() ?
-						Optional.of(mappingFunction.apply(typeSystem, result.single())) :
+						Optional.ofNullable(mappingFunction.apply(typeSystem, result.single())) :
 						Optional.empty();
 				ResultSummaries.process(result.consume());
 				return optionalValue;
diff --git a/src/main/java/org/springframework/data/neo4j/core/DefaultReactiveNeo4jClient.java b/src/main/java/org/springframework/data/neo4j/core/DefaultReactiveNeo4jClient.java
@@ -222,7 +222,7 @@ Mono<Tuple2<String, Map<String, Object>>> prepareStatement() {
 		Flux<T> executeWith(Tuple2<String, Map<String, Object>> t, RxQueryRunner runner) {
 
 			return Flux.usingWhen(Flux.just(runner.run(t.getT1(), t.getT2())),
-					result -> Flux.from(result.records()).map(r -> mappingFunction.apply(typeSystem, r)),
+					result -> Flux.from(result.records()).mapNotNull(r -> mappingFunction.apply(typeSystem, r)),
 					result -> Flux.from(result.consume()).doOnNext(ResultSummaries::process));
 		}
 
diff --git a/src/main/java/org/springframework/data/neo4j/core/SingleValueMappingFunction.java b/src/main/java/org/springframework/data/neo4j/core/SingleValueMappingFunction.java
@@ -53,6 +53,9 @@ public T apply(TypeSystem typeSystem, Record record) {
 		}
 
 		Value source = record.get(0);
+		if (targetClass == Void.class || targetClass == void.class) {
+			return null;
+		}
 		return source == null || source == Values.NULL ? null : conversionService.convert(source, targetClass);
 	}
 }
diff --git a/src/main/java/org/springframework/data/neo4j/core/mapping/CypherGenerator.java b/src/main/java/org/springframework/data/neo4j/core/mapping/CypherGenerator.java
@@ -49,6 +49,7 @@
 import org.neo4j.cypherdsl.core.Statement;
 import org.neo4j.cypherdsl.core.StatementBuilder;
 import org.neo4j.cypherdsl.core.StatementBuilder.OngoingMatchAndUpdate;
+import org.neo4j.cypherdsl.core.StatementBuilder.OngoingUpdate;
 import org.neo4j.cypherdsl.core.SymbolicName;
 import org.neo4j.cypherdsl.core.renderer.Renderer;
 import org.springframework.data.domain.Sort;
@@ -246,9 +247,18 @@ public Statement prepareDeleteOf(NodeDescription<?> nodeDescription) {
 
 	public Statement prepareDeleteOf(NodeDescription<?> nodeDescription, @Nullable Condition condition) {
 
+		return prepareDeleteOf(nodeDescription, condition, false);
+	}
+
+	public Statement prepareDeleteOf(NodeDescription<?> nodeDescription, @Nullable Condition condition, boolean count) {
+
 		Node rootNode = node(nodeDescription.getPrimaryLabel(), nodeDescription.getAdditionalLabels())
 				.named(Constants.NAME_OF_ROOT_NODE);
-		return match(rootNode).where(conditionOrNoCondition(condition)).detachDelete(rootNode).build();
+		OngoingUpdate ongoingUpdate = match(rootNode).where(conditionOrNoCondition(condition)).detachDelete(rootNode);
+		if (count) {
+			return ongoingUpdate.returning(Functions.count(rootNode)).build();
+		}
+		return ongoingUpdate.build();
 	}
 
 	public Statement prepareSaveOf(NodeDescription<?> nodeDescription,
diff --git a/src/main/java/org/springframework/data/neo4j/repository/query/CypherQueryCreator.java b/src/main/java/org/springframework/data/neo4j/repository/query/CypherQueryCreator.java
@@ -44,6 +44,7 @@
 import org.neo4j.cypherdsl.core.Property;
 import org.neo4j.cypherdsl.core.RelationshipPattern;
 import org.neo4j.cypherdsl.core.SortItem;
+import org.neo4j.cypherdsl.core.Statement;
 import org.neo4j.driver.Value;
 import org.neo4j.driver.types.Point;
 import org.springframework.data.domain.Pageable;
@@ -56,6 +57,7 @@
 import org.springframework.data.mapping.PersistentProperty;
 import org.springframework.data.mapping.PersistentPropertyPath;
 import org.springframework.data.neo4j.core.mapping.Constants;
+import org.springframework.data.neo4j.core.mapping.CypherGenerator;
 import org.springframework.data.neo4j.core.mapping.Neo4jMappingContext;
 import org.springframework.data.neo4j.core.mapping.Neo4jPersistentEntity;
 import org.springframework.data.neo4j.core.mapping.Neo4jPersistentProperty;
@@ -256,12 +258,17 @@ protected Condition or(Condition base, Condition condition) {
 	@Override
 	protected QueryFragmentsAndParameters complete(@Nullable Condition condition, Sort sort) {
 
-		QueryFragmentsAndParameters.QueryFragments queryFragments = createQueryFragments(condition, sort);
-
 		Map<String, Object> convertedParameters = this.boundedParameters.stream()
 				.peek(p -> Neo4jQuerySupport.logParameterIfNull(p.nameOrIndex, p.value))
 				.collect(Collectors.toMap(p -> p.nameOrIndex, p -> parameterConversion.apply(p.value, p.conversionOverride)));
-		return new QueryFragmentsAndParameters(nodeDescription, queryFragments, convertedParameters);
+
+		if (queryType == Neo4jQueryType.DELETE) {
+			Statement statement = CypherGenerator.INSTANCE.prepareDeleteOf(nodeDescription, condition, true);
+			return new QueryFragmentsAndParameters(statement.getCypher(), convertedParameters);
+		} else {
+			QueryFragmentsAndParameters.QueryFragments queryFragments = createQueryFragments(condition, sort);
+			return new QueryFragmentsAndParameters(nodeDescription, queryFragments, convertedParameters);
+		}
 	}
 
 	@NonNull
diff --git a/src/main/java/org/springframework/data/neo4j/repository/query/Neo4jQuerySupport.java b/src/main/java/org/springframework/data/neo4j/repository/query/Neo4jQuerySupport.java
@@ -17,11 +17,14 @@
 
 import java.time.Instant;
 import java.time.ZoneOffset;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.function.BiFunction;
 import java.util.function.Function;
 import java.util.function.Supplier;
@@ -63,6 +66,8 @@ abstract class Neo4jQuerySupport {
 	 * The query type.
 	 */
 	protected final Neo4jQueryType queryType;
+	private static final Set<Class<?>> VALID_RETURN_TYPES_FOR_DELETE = Collections.unmodifiableSet(new HashSet<>(
+			Arrays.asList(Long.class, long.class, Void.class, void.class)));
 
 	static final LogAccessor REPOSITORY_QUERY_LOG = new LogAccessor(LogFactory.getLog(Neo4jQuerySupport.class));
 
@@ -83,6 +88,9 @@ static Class<?> getDomainType(QueryMethod queryMethod) {
 		Assert.notNull(mappingContext, "The mapping context is required.");
 		Assert.notNull(queryMethod, "Query method must not be null!");
 		Assert.notNull(queryType, "Query type must not be null!");
+		Assert.isTrue(queryType != Neo4jQueryType.DELETE || hasValidReturnTypeForDelete(queryMethod),
+				"A derived delete query can only return the number of deleted nodes as a long or void."
+		);
 
 		this.mappingContext = mappingContext;
 		this.queryMethod = queryMethod;
@@ -116,6 +124,10 @@ protected final List<String> getInputProperties(final ResultProcessor resultProc
 		return returnedType.isProjecting() ? returnedType.getInputProperties() : Collections.emptyList();
 	}
 
+	private static boolean hasValidReturnTypeForDelete(Neo4jQueryMethod queryMethod) {
+		return VALID_RETURN_TYPES_FOR_DELETE.contains(queryMethod.getResultProcessor().getReturnedType().getReturnedType());
+	}
+
 	static void logParameterIfNull(String name, Object value) {
 
 		if (value != null || !REPOSITORY_QUERY_LOG.isDebugEnabled()) {
diff --git a/src/main/java/org/springframework/data/neo4j/repository/query/PartTreeNeo4jQuery.java b/src/main/java/org/springframework/data/neo4j/repository/query/PartTreeNeo4jQuery.java
@@ -67,7 +67,6 @@ protected <T extends Object> PreparedQuery<T> prepareQuery(Class<T> returnedType
 				includedProperties, this::convertParameter, limitModifier);
 
 		QueryFragmentsAndParameters queryAndParameters = queryCreator.createQuery();
-
 		return PreparedQuery.queryFor(returnedType).withQueryFragmentsAndParameters(queryAndParameters)
 				.usingMappingFunction(mappingFunction).build();
 	}
diff --git a/src/main/java/org/springframework/data/neo4j/repository/query/QueryFragmentsAndParameters.java b/src/main/java/org/springframework/data/neo4j/repository/query/QueryFragmentsAndParameters.java
@@ -72,9 +72,13 @@ public QueryFragmentsAndParameters(NodeDescription<?> nodeDescription, QueryFrag
 	}
 
 	public QueryFragmentsAndParameters(String cypherQuery) {
+		this(cypherQuery, null);
+	}
+
+	public QueryFragmentsAndParameters(String cypherQuery, Map<String, Object> parameters) {
 		this.cypherQuery = cypherQuery;
 		this.queryFragments = new QueryFragments();
-		this.parameters = null;
+		this.parameters = parameters;
 	}
 
 	public Map<String, Object> getParameters() {
diff --git a/src/test/java/org/springframework/data/neo4j/integration/imperative/RepositoryIT.java b/src/test/java/org/springframework/data/neo4j/integration/imperative/RepositoryIT.java
@@ -2495,11 +2495,11 @@ class Delete extends IntegrationTestBase {
 
 		@Override
 		void setupData(Transaction transaction) {
-			id1 = transaction.run("CREATE (n:PersonWithAllConstructor) RETURN id(n)").next().get(0).asLong();
-			id2 = transaction.run("CREATE (n:PersonWithAllConstructor) RETURN id(n)").next().get(0).asLong();
+			id1 = transaction.run("CREATE (n:PersonWithAllConstructor {name: $name}) RETURN id(n)", Collections.singletonMap("name", TEST_PERSON1_NAME)).next().get(0).asLong();
+			id2 = transaction.run("CREATE (n:PersonWithAllConstructor {name: $name}) RETURN id(n)", Collections.singletonMap("name", TEST_PERSON2_NAME)).next().get(0).asLong();
 
-			person1 = new PersonWithAllConstructor(id1, null, null, null, null, null, null, null, null, null, null);
-			person2 = new PersonWithAllConstructor(id2, null, null, null, null, null, null, null, null, null, null);
+			person1 = new PersonWithAllConstructor(id1, TEST_PERSON1_NAME, null, null, null, null, null, null, null, null, null);
+			person2 = new PersonWithAllConstructor(id2, TEST_PERSON2_NAME, null, null, null, null, null, null, null, null, null);
 		}
 
 		@Test
@@ -2520,6 +2520,25 @@ void deleteById(@Autowired PersonRepository repository) {
 			assertThat(repository.existsById(id2)).isTrue();
 		}
 
+		@Test // GH-2281
+		void deleteByDerivedQuery1(@Autowired PersonRepository repository) {
+
+			repository.deleteAllByName(TEST_PERSON1_NAME);
+
+			assertThat(repository.existsById(id1)).isFalse();
+			assertThat(repository.existsById(id2)).isTrue();
+		}
+
+		@Test // GH-2281
+		void deleteByDerivedQuery2(@Autowired PersonRepository repository) {
+
+			long deleted = repository.deleteAllByNameOrName(TEST_PERSON1_NAME, TEST_PERSON2_NAME);
+
+			assertThat(deleted).isEqualTo(2L);
+			assertThat(repository.existsById(id1)).isFalse();
+			assertThat(repository.existsById(id2)).isFalse();
+		}
+
 		@Test
 		void deleteAllEntities(@Autowired PersonRepository repository) {
 
diff --git a/src/test/java/org/springframework/data/neo4j/integration/imperative/repositories/PersonRepository.java b/src/test/java/org/springframework/data/neo4j/integration/imperative/repositories/PersonRepository.java
@@ -305,4 +305,8 @@ public DtoPersonProjectionContainingAdditionalFields getBySomeLongValue(long val
 
 	@Query(value = "MATCH (n:PersonWithAllConstructor) RETURN n :#{ orderBy (#pageable.sort)} SKIP $skip LIMIT $limit")
 	List<PersonWithAllConstructor> orderBySpel(Pageable page);
+
+	void deleteAllByName(String name);
+
+	long deleteAllByNameOrName(String name, String otherName);
 }
diff --git a/src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveRepositoryIT.java b/src/test/java/org/springframework/data/neo4j/integration/reactive/ReactiveRepositoryIT.java
@@ -2274,6 +2274,29 @@ void deleteCollectionRelationship(@Autowired ReactiveRelationshipRepository repo
 				assertThat(person.getPets()).hasSize(1);
 			}).verifyComplete();
 		}
+
+		@Test // GH-2281
+		void deleteByDerivedQuery1(@Autowired ReactivePersonRepository repository) {
+
+			repository.deleteAllByName(TEST_PERSON1_NAME)
+					.as(StepVerifier::create)
+					.verifyComplete();
+
+			repository.existsById(id1).as(StepVerifier::create).expectNext(false).verifyComplete();
+			repository.existsById(id2).as(StepVerifier::create).expectNext(true).verifyComplete();
+		}
+
+		@Test // GH-2281
+		void deleteByDerivedQuery2(@Autowired ReactivePersonRepository repository) {
+
+			repository.deleteAllByNameOrName(TEST_PERSON1_NAME, TEST_PERSON2_NAME)
+					.as(StepVerifier::create)
+					.expectNext(2L)
+					.verifyComplete();
+
+			repository.existsById(id1).as(StepVerifier::create).expectNext(false).verifyComplete();
+			repository.existsById(id2).as(StepVerifier::create).expectNext(false).verifyComplete();
+		}
 	}
 
 	@Nested
diff --git a/src/test/java/org/springframework/data/neo4j/integration/reactive/repositories/ReactivePersonRepository.java b/src/test/java/org/springframework/data/neo4j/integration/reactive/repositories/ReactivePersonRepository.java
@@ -95,4 +95,8 @@ Flux<PersonWithAllConstructor> getOptionalPersonViaQueryWithSort(@Param("part1")
 
 	Flux<PersonWithAllConstructor> getOptionalPersonViaNamedQuery(@Param("part1") String part1,
 			@Param("part2") String part2);
+
+	Mono<Void> deleteAllByName(String name);
+
+	Mono<Long> deleteAllByNameOrName(String name, String otherName);
 }
diff --git a/src/test/java/org/springframework/data/neo4j/repository/support/Neo4jRepositoryFactoryTest.java b/src/test/java/org/springframework/data/neo4j/repository/support/Neo4jRepositoryFactoryTest.java
@@ -113,7 +113,6 @@ void prepareContext() {
 		@Test
 		void validateIgnoreCaseShouldWork() {
 
-
 			assertThatIllegalArgumentException().isThrownBy(() -> repositoryFactory.getRepository(InvalidIgnoreCase.class))
 					.withMessageMatching("Can not derive query for '.*': Only the case of String based properties can be ignored within the following keywords: \\[IsNotLike, NotLike, IsLike, Like, IsStartingWith, StartingWith, StartsWith, IsEndingWith, EndingWith, EndsWith, IsNotContaining, NotContaining, NotContains, IsContaining, Containing, Contains, IsNot, Not, Is, Equals\\].");
 		}
@@ -145,6 +144,13 @@ void validateNotACompositePropertyShouldWork() {
 			assertThatIllegalArgumentException().isThrownBy(() -> repositoryFactory.getRepository(DerivedWithComposite.class))
 					.withMessageMatching("Can not derive query for '.*': Derived queries are not supported for composite properties.");
 		}
+
+		@Test // GH-2281
+		void validateDeleteReturnType() {
+
+			assertThatIllegalArgumentException().isThrownBy(() -> repositoryFactory.getRepository(InvalidDeleteBy.class))
+					.withMessageMatching("A derived delete query can only return the number of deleted nodes as a long or void.");
+		}
 	}
 
 	interface InvalidIgnoreCase extends Neo4jRepository<ThingWithAllAdditionalTypes, Long> {
@@ -167,6 +173,11 @@ interface InvalidSpatial extends Neo4jRepository<ThingWithAllCypherTypes, Long>
 		Optional<ThingWithAllCypherTypes> findOneByALongIsNear(Point point);
 	}
 
+	interface InvalidDeleteBy extends Neo4jRepository<ThingWithAllCypherTypes, Long> {
+
+		Optional<ThingWithAllCypherTypes> deleteAllBy(Point point);
+	}
+
 	interface DerivedWithComposite extends Neo4jRepository<ThingWithCompositeProperties, Long> {
 
 		Optional<ThingWithCompositeProperties> findOneByCustomTypeMapTrue();

Original file line number	Diff line number	Diff line change
`@@ -222,7 +222,7 @@ Mono<Tuple2<String, Map<String, Object>>> prepareStatement() {`
`222`	`222`	`Flux<T> executeWith(Tuple2<String, Map<String, Object>> t, RxQueryRunner runner) {`
`223`	`223`
`224`	`224`	`return Flux.usingWhen(Flux.just(runner.run(t.getT1(), t.getT2())),`
`225`		`- result -> Flux.from(result.records()).map(r -> mappingFunction.apply(typeSystem, r)),`
	`225`	`+ result -> Flux.from(result.records()).mapNotNull(r -> mappingFunction.apply(typeSystem, r)),`
`226`	`226`	`result -> Flux.from(result.consume()).doOnNext(ResultSummaries::process));`
`227`	`227`	`}`
`228`	`228`
Original file line number	Diff line number	Diff line change
`@@ -53,6 +53,9 @@ public T apply(TypeSystem typeSystem, Record record) {`
`53`	`53`	`}`
`54`	`54`
`55`	`55`	`Value source = record.get(0);`
	`56`	`+ if (targetClass == Void.class \|\| targetClass == void.class) {`
	`57`	`+ return null;`
	`58`	`+ }`
`56`	`59`	`return source == null \|\| source == Values.NULL ? null : conversionService.convert(source, targetClass);`
`57`	`60`	`}`
`58`	`61`	`}`
Original file line number	Diff line number	Diff line change
`@@ -67,7 +67,6 @@ protected <T extends Object> PreparedQuery<T> prepareQuery(Class<T> returnedType`
`67`	`67`	`includedProperties, this::convertParameter, limitModifier);`
`68`	`68`
`69`	`69`	`QueryFragmentsAndParameters queryAndParameters = queryCreator.createQuery();`
`70`		`-`
`71`	`70`	`return PreparedQuery.queryFor(returnedType).withQueryFragmentsAndParameters(queryAndParameters)`
`72`	`71`	`.usingMappingFunction(mappingFunction).build();`
`73`	`72`	`}`
Original file line number	Diff line number	Diff line change
`@@ -72,9 +72,13 @@ public QueryFragmentsAndParameters(NodeDescription<?> nodeDescription, QueryFrag`
`72`	`72`	`}`
`73`	`73`
`74`	`74`	`public QueryFragmentsAndParameters(String cypherQuery) {`
	`75`	`+ this(cypherQuery, null);`
	`76`	`+ }`
	`77`	`+`
	`78`	`+ public QueryFragmentsAndParameters(String cypherQuery, Map<String, Object> parameters) {`
`75`	`79`	`this.cypherQuery = cypherQuery;`
`76`	`80`	`this.queryFragments = new QueryFragments();`
`77`		`- this.parameters = null;`
	`81`	`+ this.parameters = parameters;`
`78`	`82`	`}`
`79`	`83`
`80`	`84`	`public Map<String, Object> getParameters() {`
Original file line number	Diff line number	Diff line change
`@@ -305,4 +305,8 @@ public DtoPersonProjectionContainingAdditionalFields getBySomeLongValue(long val`
`305`	`305`
`306`	`306`	`@Query(value = "MATCH (n:PersonWithAllConstructor) RETURN n :#{ orderBy (#pageable.sort)} SKIP $skip LIMIT $limit")`
`307`	`307`	`List<PersonWithAllConstructor> orderBySpel(Pageable page);`
	`308`	`+`
	`309`	`+ void deleteAllByName(String name);`
	`310`	`+`
	`311`	`+ long deleteAllByNameOrName(String name, String otherName);`
`308`	`312`	`}`