Skip parameter lookup for unused query parameters.

We now skip binding parameter lookup if the query isn't using named parameters and the parameter is not associated with a name. Also, we check for presence of lookup identifiers to avoid parameter binding that cannot be looked up as they are not used anymore.

This can happen when a declared query uses parameters only in the ORDER BY clause that is truncated during count query derivation. Then the query object reports parameters althtough they are not being used.

We also refined parameter carryover during count query derivation. Previously, we copied all parameters without introspecting their origin. now, we copy only expression parameters to the derived query as count query derivation doesn't have access to expressions as our query parsers require valid JPQL.

Closes #3756

Author: mp911de

spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryParameterSetterFactory.java

@@ -245,10 +245,13 @@ public QueryParameterSetter create(ParameterBinding binding, DeclaredQuery decla
 
 			BindingIdentifier identifier = mia.identifier();
 
-			if (declaredQuery.hasNamedParameter()) {
+			if (declaredQuery.hasNamedParameter() && identifier.hasName()) {
 				parameter = findParameterForBinding(parameters, identifier.getName());
-			} else {
+			} else if (identifier.hasPosition()) {
 				parameter = findParameterForBinding(parameters, identifier.getPosition() - 1);
+			} else {
+				// this can happen when a query uses parameters in ORDER BY and the COUNT query just needs to drop a binding.
+				parameter = null;
 			}
 
 			return parameter == null //

spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java

@@ -113,9 +113,19 @@ public DeclaredQuery deriveCountQuery(@Nullable String countQueryProjection) {
 		StringQuery stringQuery = new StringQuery(this.queryEnhancer.createCountQueryFor(countQueryProjection), //
 				this.isNative);
 
+		// need to copy expression bindings from the declared to the derived query as JPQL query derivation only sees JPA
+		// parameter markers and not the original expressions anymore.
 		if (this.hasParameterBindings() && !this.getParameterBindings().equals(stringQuery.getParameterBindings())) {
-			stringQuery.getParameterBindings().clear();
-			stringQuery.getParameterBindings().addAll(this.bindings);
+
+			List<ParameterBinding> derivedBindings = stringQuery.getParameterBindings();
+
+			for (ParameterBinding binding : bindings) {
+
+				if (binding.getOrigin().isExpression() && derivedBindings
+						.removeIf(it -> !it.getOrigin().isExpression() && it.getIdentifier().equals(binding.getIdentifier()))) {
+					derivedBindings.add(binding);
+				}
+			}
 		}
 
 		return stringQuery;
@@ -235,8 +245,7 @@ private String parseParameterBindingsOfQueryIntoBindingsAndReturnCleanedQuery(St
 				greatestParameterIndex = 0;
 			}
 
-			SpelExtractor spelExtractor = createSpelExtractor(query, parametersShouldBeAccessedByIndex,
-					greatestParameterIndex);
+			SpelExtractor spelExtractor = createSpelExtractor(query, parametersShouldBeAccessedByIndex, greatestParameterIndex);
 
 			String resultingQuery = spelExtractor.getQueryString();
 			Matcher matcher = PARAMETER_BINDING_PATTERN.matcher(resultingQuery);
@@ -340,8 +349,7 @@ private String parseParameterBindingsOfQueryIntoBindingsAndReturnCleanedQuery(St
 			return resultingQuery;
 		}
 
-		private static SpelExtractor createSpelExtractor(String queryWithSpel, boolean parametersShouldBeAccessedByIndex,
-				int greatestParameterIndex) {
+		private static SpelExtractor createSpelExtractor(String queryWithSpel, boolean parametersShouldBeAccessedByIndex, int greatestParameterIndex) {
 
 			/*
 			 * If parameters need to be bound by index, we bind the synthetic expression parameters starting from position of the greatest discovered index parameter in order to

spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java

@@ -3016,22 +3016,36 @@ void handlesColonsFollowedByIntegerInStringLiteral() {
 		assertThat(users).extracting(User::getId).containsExactly(expected.getId());
 	}
 
-	@Disabled("ORDER BY CASE appears to be a Hibernate-only feature")
-	@Test // DATAJPA-1233
+	@Test // DATAJPA-1233, GH-3756
 	void handlesCountQueriesWithLessParametersSingleParam() {
-		// repository.findAllOrderedBySpecialNameSingleParam("Oliver", PageRequest.of(2, 3));
+
+		flushTestUsers();
+
+		Page<User> result = repository.findAllOrderedByNamedParam("Oliver", PageRequest.of(0, 3));
+
+		assertThat(result.getContent()).containsExactly(firstUser, fourthUser, thirdUser);
+		assertThat(result.getTotalElements()).isEqualTo(4);
+
+		result = repository.findAllOrderedByIndexedParam("Oliver", PageRequest.of(0, 3));
+
+		assertThat(result.getContent()).containsExactly(firstUser, fourthUser, thirdUser);
+		assertThat(result.getTotalElements()).isEqualTo(4);
 	}
 
-	@Disabled("ORDER BY CASE appears to be a Hibernate-only feature")
-	@Test // DATAJPA-1233
+	@Test // DATAJPA-1233, GH-3756
 	void handlesCountQueriesWithLessParametersMoreThanOne() {
-		// repository.findAllOrderedBySpecialNameMultipleParams("Oliver", "x", PageRequest.of(2, 3));
-	}
 
-	@Disabled("ORDER BY CASE appears to be a Hibernate-only feature")
-	@Test // DATAJPA-1233
-	void handlesCountQueriesWithLessParametersMoreThanOneIndexed() {
-		// repository.findAllOrderedBySpecialNameMultipleParamsIndexed("x", "Oliver", PageRequest.of(2, 3));
+		flushTestUsers();
+
+		Page<User> result = repository.findAllOrderedBySpecialNameMultipleParams("Oliver", "x", PageRequest.of(0, 3));
+
+		assertThat(result.getContent()).containsExactly(firstUser, fourthUser, thirdUser);
+		assertThat(result.getTotalElements()).isEqualTo(4);
+
+		result = repository.findAllOrderedBySpecialNameMultipleParamsIndexed("x", "Oliver", PageRequest.of(0, 3));
+
+		assertThat(result.getContent()).containsExactly(firstUser, fourthUser, thirdUser);
+		assertThat(result.getTotalElements()).isEqualTo(4);
 	}
 
 	// DATAJPA-928

spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/ExpressionBasedStringQueryUnitTests.java

@@ -176,7 +176,7 @@ void indexedExpressionsShouldCreateLikeBindings() {
 	}
 
 	@Test
-	public void doesTemplatingWhenEntityNameSpelIsPresent() {
+	void doesTemplatingWhenEntityNameSpelIsPresent() {
 
 		StringQuery query = new ExpressionBasedStringQuery("select #{#entityName + 'Hallo'} from #{#entityName} u",
 				metadata, SPEL_PARSER, false);
@@ -185,7 +185,7 @@ public void doesTemplatingWhenEntityNameSpelIsPresent() {
 	}
 
 	@Test
-	public void doesNoTemplatingWhenEntityNameSpelIsNotPresent() {
+	void doesNoTemplatingWhenEntityNameSpelIsNotPresent() {
 
 		StringQuery query = new ExpressionBasedStringQuery("select #{#entityName + 'Hallo'} from User u", metadata,
 				SPEL_PARSER, false);
@@ -194,7 +194,7 @@ public void doesNoTemplatingWhenEntityNameSpelIsNotPresent() {
 	}
 
 	@Test
-	public void doesTemplatingWhenEntityNameSpelIsPresentForBindParameter() {
+	void doesTemplatingWhenEntityNameSpelIsPresentForBindParameter() {
 
 		StringQuery query = new ExpressionBasedStringQuery("select u from #{#entityName} u where name = :#{#something}",
 				metadata, SPEL_PARSER, false);

spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/StringQueryUnitTests.java

@@ -309,6 +309,56 @@ void allowsReuseOfParameterWithInAndRegularBinding() {
 		assertNamedBinding(InParameterBinding.class, "foo_1", bindings.get(1));
 	}
 
+	@Test // GH-3126
+	void countQueryDerivationRetainsNamedExpressionParameters() {
+
+		StringQuery query = new StringQuery(
+				"select u from User u where foo = :#{bar} ORDER BY CASE WHEN (u.firstname >= :#{name}) THEN 0 ELSE 1 END",
+				false);
+
+		DeclaredQuery countQuery = query.deriveCountQuery(null);
+
+		assertThat(countQuery.getParameterBindings()).hasSize(1);
+		assertThat(countQuery.getParameterBindings()).extracting(ParameterBinding::getOrigin)
+				.extracting(ParameterOrigin::isExpression).isEqualTo(List.of(true));
+
+		query = new StringQuery(
+				"select u from User u where foo = :#{bar} and bar = :bar ORDER BY CASE WHEN (u.firstname >= :bar) THEN 0 ELSE 1 END",
+				false);
+
+		countQuery = query.deriveCountQuery(null);
+
+		assertThat(countQuery.getParameterBindings()).hasSize(2);
+		assertThat(countQuery.getParameterBindings()) //
+				.extracting(ParameterBinding::getOrigin) //
+				.extracting(ParameterOrigin::isExpression).contains(true, false);
+	}
+
+	@Test // GH-3126
+	void countQueryDerivationRetainsIndexedExpressionParameters() {
+
+		StringQuery query = new StringQuery(
+				"select u from User u where foo = ?#{bar} ORDER BY CASE WHEN (u.firstname >= ?#{name}) THEN 0 ELSE 1 END",
+				false);
+
+		DeclaredQuery countQuery = query.deriveCountQuery(null);
+
+		assertThat(countQuery.getParameterBindings()).hasSize(1);
+		assertThat(countQuery.getParameterBindings()).extracting(ParameterBinding::getOrigin)
+				.extracting(ParameterOrigin::isExpression).isEqualTo(List.of(true));
+
+		query = new StringQuery(
+				"select u from User u where foo = ?#{bar} and bar = ?1 ORDER BY CASE WHEN (u.firstname >= ?1) THEN 0 ELSE 1 END",
+				false);
+
+		countQuery = query.deriveCountQuery(null);
+
+		assertThat(countQuery.getParameterBindings()).hasSize(2);
+		assertThat(countQuery.getParameterBindings()) //
+				.extracting(ParameterBinding::getOrigin) //
+				.extracting(ParameterOrigin::isExpression).contains(true, false);
+	}
+
 	@Test // DATAJPA-461
 	void detectsMultiplePositionalInParameterBindings() {
 

spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java

@@ -82,9 +82,8 @@ public interface UserRepository extends JpaRepository<User, Integer>, JpaSpecifi
 	java.util.Optional<User> findById(Integer primaryKey);
 
 	/**
-	 * Redeclaration of {@link CrudRepository#deleteById(java.lang.Object)}. to make sure the transaction
-	 * configuration of the original method is considered if the redeclaration does not carry a {@link Transactional}
-	 * annotation.
+	 * Redeclaration of {@link CrudRepository#deleteById(java.lang.Object)}. to make sure the transaction configuration of
+	 * the original method is considered if the redeclaration does not carry a {@link Transactional} annotation.
 	 */
 	@Override
 	void deleteById(Integer id); // DATACMNS-649
@@ -416,7 +415,8 @@ Window<User> findTop3ByFirstnameStartingWithOrderByFirstnameAscEmailAddressAsc(S
 	@Query("select u from User u where u.firstname = ?#{[0]} and u.firstname = ?1 and u.lastname like %?#{[1]}% and u.lastname like %?2%")
 	List<User> findByFirstnameAndLastnameWithSpelExpression(String firstname, String lastname);
 
-	@Query(value = "select * from SD_User", countQuery = "select count(1) from SD_User u where u.lastname = :#{#lastname}", nativeQuery = true)
+	@Query(value = "select * from SD_User",
+			countQuery = "select count(1) from SD_User u where u.lastname = :#{#lastname}", nativeQuery = true)
 	Page<User> findByWithSpelParameterOnlyUsedForCountQuery(String lastname, Pageable page);
 
 	// DATAJPA-564
@@ -563,26 +563,23 @@ List<User> findUsersByFirstnameForSpELExpressionWithParameterIndexOnlyWithEntity
 	@Query("SELECT u FROM User u where u.firstname >= ?1 and u.lastname = '000:1'")
 	List<User> queryWithIndexedParameterAndColonFollowedByIntegerInString(String firstname);
 
-	/**
-	 * TODO: ORDER BY CASE appears to only with Hibernate. The examples attempting to do this through pure JPQL don't
-	 * appear to work with Hibernate, so we must set them aside until we can implement HQL.
-	 */
-	// // DATAJPA-1233
-	// @Query(value = "SELECT u FROM User u ORDER BY CASE WHEN (u.firstname >= :name) THEN 0 ELSE 1 END, u.firstname")
-	// Page<User> findAllOrderedBySpecialNameSingleParam(@Param("name") String name, Pageable page);
-	//
-	// // DATAJPA-1233
-	// @Query(
-	// value = "SELECT u FROM User u WHERE :other = 'x' ORDER BY CASE WHEN (u.firstname >= :name) THEN 0 ELSE 1 END,
-	// u.firstname")
-	// Page<User> findAllOrderedBySpecialNameMultipleParams(@Param("name") String name, @Param("other") String other,
-	// Pageable page);
-	//
-	// // DATAJPA-1233
-	// @Query(
-	// value = "SELECT u FROM User u WHERE ?2 = 'x' ORDER BY CASE WHEN (u.firstname >= ?1) THEN 0 ELSE 1 END,
-	// u.firstname")
-	// Page<User> findAllOrderedBySpecialNameMultipleParamsIndexed(String other, String name, Pageable page);
+	@Query(value = "SELECT u FROM User u ORDER BY CASE WHEN (u.firstname >= :name) THEN 0 ELSE 1 END, u.firstname")
+	Page<User> findAllOrderedByNamedParam(@Param("name") String name, Pageable page);
+
+	@Query(value = "SELECT u FROM User u ORDER BY CASE WHEN (u.firstname >= ?1) THEN 0 ELSE 1 END, u.firstname")
+	Page<User> findAllOrderedByIndexedParam(String name, Pageable page);
+
+	@Query(
+			value = "SELECT u FROM User u WHERE :other = 'x' ORDER BY CASE WHEN (u.firstname >= :name) THEN 0 ELSE 1 END, u.firstname")
+	Page<User> findAllOrderedBySpecialNameMultipleParams(@Param("name") String name, @Param("other") String other,
+			Pageable page);
+
+	// Note that parameters used in the order-by statement are just cut off, so we must declare a query that parameter
+	// label order remains valid even after truncating the order by part. (i.e. WHERE ?2 = 'x' ORDER BY CASE WHEN
+	// (u.firstname >= ?1) isn't going to work).
+	@Query(
+			value = "SELECT u FROM User u WHERE ?1 = 'x' ORDER BY CASE WHEN (u.firstname >= ?2) THEN 0 ELSE 1 END, u.firstname")
+	Page<User> findAllOrderedBySpecialNameMultipleParamsIndexed(String other, String name, Pageable page);
 
 	// DATAJPA-928
 	Page<User> findByNativeNamedQueryWithPageable(Pageable pageable);