From 76a0f5e409f39e34942009886abc97230e1850fa Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 29 Jan 2025 15:07:47 +0100 Subject: [PATCH] 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 --- .../query/QueryParameterSetterFactory.java | 7 ++- .../jpa/repository/query/StringQuery.java | 42 ++++++++++++---- .../jpa/repository/UserRepositoryTests.java | 36 +++++++++---- .../ExpressionBasedStringQueryUnitTests.java | 6 +-- .../query/StringQueryUnitTests.java | 50 +++++++++++++++++++ .../jpa/repository/sample/UserRepository.java | 45 ++++++++--------- 6 files changed, 135 insertions(+), 51 deletions(-) diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryParameterSetterFactory.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryParameterSetterFactory.java index cd6ff2d62..c45c3d8aa 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryParameterSetterFactory.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/QueryParameterSetterFactory.java @@ -240,10 +240,13 @@ abstract class QueryParameterSetterFactory { 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 // diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java index 0424d5ff6..8bc8dff0a 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java @@ -73,6 +73,15 @@ class StringQuery implements DeclaredQuery { * @param query must not be {@literal null} or empty. */ public StringQuery(String query, boolean isNative) { + this(query, isNative, it -> {}); + } + + /** + * Creates a new {@link StringQuery} from the given JPQL query. + * + * @param query must not be {@literal null} or empty. + */ + private StringQuery(String query, boolean isNative, Consumer> parameterPostProcessor) { Assert.hasText(query, "Query must not be null or empty"); @@ -87,6 +96,8 @@ class StringQuery implements DeclaredQuery { this.usesJdbcStyleParameters = queryMeta.usesJdbcStyleParameters; this.queryEnhancer = QueryEnhancerFactory.forQuery(this); + parameterPostProcessor.accept(this.bindings); + boolean hasNamedParameters = false; for (ParameterBinding parameterBinding : getParameterBindings()) { if (parameterBinding.getIdentifier().hasName() && parameterBinding.getOrigin().isMethodArgument()) { @@ -117,15 +128,26 @@ class StringQuery implements DeclaredQuery { @Override 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); - } + return new StringQuery(this.queryEnhancer.createCountQueryFor(countQueryProjection), // + this.isNative, derivedBindings -> { - return stringQuery; + // 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(derivedBindings)) { + + for (ParameterBinding binding : bindings) { + + if (binding.getOrigin().isExpression() && derivedBindings.removeIf( + it -> !it.getOrigin().isExpression() && it.getIdentifier().equals(binding.getIdentifier()))) { + derivedBindings.add(binding); + } + } + } + }); } @Override @@ -243,8 +265,7 @@ class StringQuery implements DeclaredQuery { } ValueExpressionQueryRewriter.ParsedQuery parsedQuery = createSpelExtractor(query, - parametersShouldBeAccessedByIndex, - greatestParameterIndex); + parametersShouldBeAccessedByIndex, greatestParameterIndex); String resultingQuery = parsedQuery.getQueryString(); Matcher matcher = PARAMETER_BINDING_PATTERN.matcher(resultingQuery); @@ -350,8 +371,7 @@ class StringQuery implements DeclaredQuery { } private static ValueExpressionQueryRewriter.ParsedQuery createSpelExtractor(String queryWithSpel, - boolean parametersShouldBeAccessedByIndex, - int greatestParameterIndex) { + 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 diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java index b4df229da..c433415b9 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java @@ -3069,22 +3069,36 @@ class UserRepositoryTests { 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 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 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 diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/ExpressionBasedStringQueryUnitTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/ExpressionBasedStringQueryUnitTests.java index 1df1abde1..2b8187182 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/ExpressionBasedStringQueryUnitTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/ExpressionBasedStringQueryUnitTests.java @@ -177,7 +177,7 @@ class ExpressionBasedStringQueryUnitTests { } @Test - public void doesTemplatingWhenEntityNameSpelIsPresent() { + void doesTemplatingWhenEntityNameSpelIsPresent() { StringQuery query = new ExpressionBasedStringQuery("select #{#entityName + 'Hallo'} from #{#entityName} u", metadata, PARSER, false); @@ -186,7 +186,7 @@ class ExpressionBasedStringQueryUnitTests { } @Test - public void doesNoTemplatingWhenEntityNameSpelIsNotPresent() { + void doesNoTemplatingWhenEntityNameSpelIsNotPresent() { StringQuery query = new ExpressionBasedStringQuery("select #{#entityName + 'Hallo'} from User u", metadata, PARSER, false); @@ -195,7 +195,7 @@ class ExpressionBasedStringQueryUnitTests { } @Test - public void doesTemplatingWhenEntityNameSpelIsPresentForBindParameter() { + void doesTemplatingWhenEntityNameSpelIsPresentForBindParameter() { StringQuery query = new ExpressionBasedStringQuery("select u from #{#entityName} u where name = :#{#something}", metadata, PARSER, false); diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/StringQueryUnitTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/StringQueryUnitTests.java index 4db398991..04bf714ae 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/StringQueryUnitTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/StringQueryUnitTests.java @@ -310,6 +310,56 @@ class StringQueryUnitTests { 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() { diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java index a382fa70a..94b1873d6 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java @@ -83,9 +83,8 @@ public interface UserRepository extends JpaRepository, JpaSpecifi java.util.Optional 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 @@ -419,7 +418,8 @@ public interface UserRepository extends JpaRepository, JpaSpecifi @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 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 findByWithSpelParameterOnlyUsedForCountQuery(String lastname, Pageable page); // DATAJPA-564 @@ -573,26 +573,23 @@ public interface UserRepository extends JpaRepository, JpaSpecifi @Query("SELECT u FROM User u where u.firstname >= ?1 and u.lastname = '000:1'") List 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 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 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 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 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 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 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 findAllOrderedBySpecialNameMultipleParamsIndexed(String other, String name, Pageable page); // DATAJPA-928 Page findByNativeNamedQueryWithPageable(Pageable pageable);