Skip to content

Commit 88b6ea2

Browse files
mp911demp911de
committed
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
1 parent 9f03598 commit 88b6ea2

File tree

6 files changed

+140
-53
lines changed

6 files changed

+140
-53
lines changed

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -240,10 +240,13 @@ public QueryParameterSetter create(ParameterBinding binding, DeclaredQuery decla
240240

241241
BindingIdentifier identifier = mia.identifier();
242242

243-
if (declaredQuery.hasNamedParameter()) {
243+
if (declaredQuery.hasNamedParameter() && identifier.hasName()) {
244244
parameter = findParameterForBinding(parameters, identifier.getName());
245-
} else {
245+
} else if (identifier.hasPosition()) {
246246
parameter = findParameterForBinding(parameters, identifier.getPosition() - 1);
247+
} else {
248+
// this can happen when a query uses parameters in ORDER BY and the COUNT query just needs to drop a binding.
249+
parameter = null;
247250
}
248251

249252
return parameter == null //

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

Lines changed: 31 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,15 @@ class StringQuery implements DeclaredQuery {
7373
* @param query must not be {@literal null} or empty.
7474
*/
7575
public StringQuery(String query, boolean isNative) {
76+
this(query, isNative, it -> {});
77+
}
78+
79+
/**
80+
* Creates a new {@link StringQuery} from the given JPQL query.
81+
*
82+
* @param query must not be {@literal null} or empty.
83+
*/
84+
private StringQuery(String query, boolean isNative, Consumer<List<ParameterBinding>> parameterPostProcessor) {
7685

7786
Assert.hasText(query, "Query must not be null or empty");
7887

@@ -87,6 +96,8 @@ public StringQuery(String query, boolean isNative) {
8796
this.usesJdbcStyleParameters = queryMeta.usesJdbcStyleParameters;
8897
this.queryEnhancer = QueryEnhancerFactory.forQuery(this);
8998

99+
parameterPostProcessor.accept(this.bindings);
100+
90101
boolean hasNamedParameters = false;
91102
for (ParameterBinding parameterBinding : getParameterBindings()) {
92103
if (parameterBinding.getIdentifier().hasName() && parameterBinding.getOrigin().isMethodArgument()) {
@@ -117,15 +128,26 @@ public List<ParameterBinding> getParameterBindings() {
117128
@Override
118129
public DeclaredQuery deriveCountQuery(@Nullable String countQueryProjection) {
119130

120-
StringQuery stringQuery = new StringQuery(this.queryEnhancer.createCountQueryFor(countQueryProjection), //
121-
this.isNative);
131+
// need to copy expression bindings from the declared to the derived query as JPQL query derivation only sees
132+
// JPA parameter markers and not the original expressions anymore.
122133

123-
if (this.hasParameterBindings() && !this.getParameterBindings().equals(stringQuery.getParameterBindings())) {
124-
stringQuery.getParameterBindings().clear();
125-
stringQuery.getParameterBindings().addAll(this.bindings);
126-
}
134+
return new StringQuery(this.queryEnhancer.createCountQueryFor(countQueryProjection), //
135+
this.isNative, derivedBindings -> {
127136

128-
return stringQuery;
137+
// need to copy expression bindings from the declared to the derived query as JPQL query derivation only sees
138+
// JPA
139+
// parameter markers and not the original expressions anymore.
140+
if (this.hasParameterBindings() && !this.getParameterBindings().equals(derivedBindings)) {
141+
142+
for (ParameterBinding binding : bindings) {
143+
144+
if (binding.getOrigin().isExpression() && derivedBindings.removeIf(
145+
it -> !it.getOrigin().isExpression() && it.getIdentifier().equals(binding.getIdentifier()))) {
146+
derivedBindings.add(binding);
147+
}
148+
}
149+
}
150+
});
129151
}
130152

131153
@Override
@@ -243,8 +265,7 @@ String parseParameterBindingsOfQueryIntoBindingsAndReturnCleanedQuery(String que
243265
}
244266

245267
ValueExpressionQueryRewriter.ParsedQuery parsedQuery = createSpelExtractor(query,
246-
parametersShouldBeAccessedByIndex,
247-
greatestParameterIndex);
268+
parametersShouldBeAccessedByIndex, greatestParameterIndex);
248269

249270
String resultingQuery = parsedQuery.getQueryString();
250271
Matcher matcher = PARAMETER_BINDING_PATTERN.matcher(resultingQuery);
@@ -350,8 +371,7 @@ String parseParameterBindingsOfQueryIntoBindingsAndReturnCleanedQuery(String que
350371
}
351372

352373
private static ValueExpressionQueryRewriter.ParsedQuery createSpelExtractor(String queryWithSpel,
353-
boolean parametersShouldBeAccessedByIndex,
354-
int greatestParameterIndex) {
374+
boolean parametersShouldBeAccessedByIndex, int greatestParameterIndex) {
355375

356376
/*
357377
* 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

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3133,22 +3133,36 @@ void handlesColonsFollowedByIntegerInStringLiteral() {
31333133
assertThat(users).extracting(User::getId).containsExactly(expected.getId());
31343134
}
31353135

3136-
@Disabled("ORDER BY CASE appears to be a Hibernate-only feature")
3137-
@Test // DATAJPA-1233
3136+
@Test // DATAJPA-1233, GH-3756
31383137
void handlesCountQueriesWithLessParametersSingleParam() {
3139-
// repository.findAllOrderedBySpecialNameSingleParam("Oliver", PageRequest.of(2, 3));
3138+
3139+
flushTestUsers();
3140+
3141+
Page<User> result = repository.findAllOrderedByNamedParam("Oliver", PageRequest.of(0, 3));
3142+
3143+
assertThat(result.getContent()).containsExactly(firstUser, fourthUser, thirdUser);
3144+
assertThat(result.getTotalElements()).isEqualTo(4);
3145+
3146+
result = repository.findAllOrderedByIndexedParam("Oliver", PageRequest.of(0, 3));
3147+
3148+
assertThat(result.getContent()).containsExactly(firstUser, fourthUser, thirdUser);
3149+
assertThat(result.getTotalElements()).isEqualTo(4);
31403150
}
31413151

3142-
@Disabled("ORDER BY CASE appears to be a Hibernate-only feature")
3143-
@Test // DATAJPA-1233
3152+
@Test // DATAJPA-1233, GH-3756
31443153
void handlesCountQueriesWithLessParametersMoreThanOne() {
3145-
// repository.findAllOrderedBySpecialNameMultipleParams("Oliver", "x", PageRequest.of(2, 3));
3146-
}
31473154

3148-
@Disabled("ORDER BY CASE appears to be a Hibernate-only feature")
3149-
@Test // DATAJPA-1233
3150-
void handlesCountQueriesWithLessParametersMoreThanOneIndexed() {
3151-
// repository.findAllOrderedBySpecialNameMultipleParamsIndexed("x", "Oliver", PageRequest.of(2, 3));
3155+
flushTestUsers();
3156+
3157+
Page<User> result = repository.findAllOrderedBySpecialNameMultipleParams("Oliver", "x", PageRequest.of(0, 3));
3158+
3159+
assertThat(result.getContent()).containsExactly(firstUser, fourthUser, thirdUser);
3160+
assertThat(result.getTotalElements()).isEqualTo(4);
3161+
3162+
result = repository.findAllOrderedBySpecialNameMultipleParamsIndexed("x", "Oliver", PageRequest.of(0, 3));
3163+
3164+
assertThat(result.getContent()).containsExactly(firstUser, fourthUser, thirdUser);
3165+
assertThat(result.getTotalElements()).isEqualTo(4);
31523166
}
31533167

31543168
// DATAJPA-928

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@ void indexedExpressionsShouldCreateLikeBindings() {
177177
}
178178

179179
@Test
180-
public void doesTemplatingWhenEntityNameSpelIsPresent() {
180+
void doesTemplatingWhenEntityNameSpelIsPresent() {
181181

182182
StringQuery query = new ExpressionBasedStringQuery("select #{#entityName + 'Hallo'} from #{#entityName} u",
183183
metadata, PARSER, false);
@@ -186,7 +186,7 @@ public void doesTemplatingWhenEntityNameSpelIsPresent() {
186186
}
187187

188188
@Test
189-
public void doesNoTemplatingWhenEntityNameSpelIsNotPresent() {
189+
void doesNoTemplatingWhenEntityNameSpelIsNotPresent() {
190190

191191
StringQuery query = new ExpressionBasedStringQuery("select #{#entityName + 'Hallo'} from User u", metadata,
192192
PARSER, false);
@@ -195,7 +195,7 @@ public void doesNoTemplatingWhenEntityNameSpelIsNotPresent() {
195195
}
196196

197197
@Test
198-
public void doesTemplatingWhenEntityNameSpelIsPresentForBindParameter() {
198+
void doesTemplatingWhenEntityNameSpelIsPresentForBindParameter() {
199199

200200
StringQuery query = new ExpressionBasedStringQuery("select u from #{#entityName} u where name = :#{#something}",
201201
metadata, PARSER, false);

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

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -310,6 +310,56 @@ void allowsReuseOfParameterWithInAndRegularBinding() {
310310
assertNamedBinding(InParameterBinding.class, "foo_1", bindings.get(1));
311311
}
312312

313+
@Test // GH-3126
314+
void countQueryDerivationRetainsNamedExpressionParameters() {
315+
316+
StringQuery query = new StringQuery(
317+
"select u from User u where foo = :#{bar} ORDER BY CASE WHEN (u.firstname >= :#{name}) THEN 0 ELSE 1 END",
318+
false);
319+
320+
DeclaredQuery countQuery = query.deriveCountQuery(null);
321+
322+
assertThat(countQuery.getParameterBindings()).hasSize(1);
323+
assertThat(countQuery.getParameterBindings()).extracting(ParameterBinding::getOrigin)
324+
.extracting(ParameterOrigin::isExpression).isEqualTo(List.of(true));
325+
326+
query = new StringQuery(
327+
"select u from User u where foo = :#{bar} and bar = :bar ORDER BY CASE WHEN (u.firstname >= :bar) THEN 0 ELSE 1 END",
328+
false);
329+
330+
countQuery = query.deriveCountQuery(null);
331+
332+
assertThat(countQuery.getParameterBindings()).hasSize(2);
333+
assertThat(countQuery.getParameterBindings()) //
334+
.extracting(ParameterBinding::getOrigin) //
335+
.extracting(ParameterOrigin::isExpression).contains(true, false);
336+
}
337+
338+
@Test // GH-3126
339+
void countQueryDerivationRetainsIndexedExpressionParameters() {
340+
341+
StringQuery query = new StringQuery(
342+
"select u from User u where foo = ?#{bar} ORDER BY CASE WHEN (u.firstname >= ?#{name}) THEN 0 ELSE 1 END",
343+
false);
344+
345+
DeclaredQuery countQuery = query.deriveCountQuery(null);
346+
347+
assertThat(countQuery.getParameterBindings()).hasSize(1);
348+
assertThat(countQuery.getParameterBindings()).extracting(ParameterBinding::getOrigin)
349+
.extracting(ParameterOrigin::isExpression).isEqualTo(List.of(true));
350+
351+
query = new StringQuery(
352+
"select u from User u where foo = ?#{bar} and bar = ?1 ORDER BY CASE WHEN (u.firstname >= ?1) THEN 0 ELSE 1 END",
353+
false);
354+
355+
countQuery = query.deriveCountQuery(null);
356+
357+
assertThat(countQuery.getParameterBindings()).hasSize(2);
358+
assertThat(countQuery.getParameterBindings()) //
359+
.extracting(ParameterBinding::getOrigin) //
360+
.extracting(ParameterOrigin::isExpression).contains(true, false);
361+
}
362+
313363
@Test // DATAJPA-461
314364
void detectsMultiplePositionalInParameterBindings() {
315365

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

Lines changed: 26 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -88,9 +88,8 @@ public interface UserRepository extends JpaRepository<User, Integer>, JpaSpecifi
8888
java.util.Optional<User> findById(Integer primaryKey);
8989

9090
/**
91-
* Redeclaration of {@link CrudRepository#deleteById(java.lang.Object)}. to make sure the transaction
92-
* configuration of the original method is considered if the redeclaration does not carry a {@link Transactional}
93-
* annotation.
91+
* Redeclaration of {@link CrudRepository#deleteById(java.lang.Object)}. to make sure the transaction configuration of
92+
* the original method is considered if the redeclaration does not carry a {@link Transactional} annotation.
9493
*/
9594
@Override
9695
void deleteById(Integer id); // DATACMNS-649
@@ -424,7 +423,8 @@ Window<User> findTop3ByFirstnameStartingWithOrderByFirstnameAscEmailAddressAsc(S
424423
@Query("select u from User u where u.firstname = ?#{[0]} and u.firstname = ?1 and u.lastname like %?#{[1]}% and u.lastname like %?2%")
425424
List<User> findByFirstnameAndLastnameWithSpelExpression(String firstname, String lastname);
426425

427-
@Query(value = "select * from SD_User", countQuery = "select count(1) from SD_User u where u.lastname = :#{#lastname}", nativeQuery = true)
426+
@Query(value = "select * from SD_User",
427+
countQuery = "select count(1) from SD_User u where u.lastname = :#{#lastname}", nativeQuery = true)
428428
Page<User> findByWithSpelParameterOnlyUsedForCountQuery(String lastname, Pageable page);
429429

430430
// DATAJPA-564
@@ -578,26 +578,23 @@ List<User> findUsersByFirstnameForSpELExpressionWithParameterIndexOnlyWithEntity
578578
@Query("SELECT u FROM User u where u.firstname >= ?1 and u.lastname = '000:1'")
579579
List<User> queryWithIndexedParameterAndColonFollowedByIntegerInString(String firstname);
580580

581-
/**
582-
* TODO: ORDER BY CASE appears to only with Hibernate. The examples attempting to do this through pure JPQL don't
583-
* appear to work with Hibernate, so we must set them aside until we can implement HQL.
584-
*/
585-
// // DATAJPA-1233
586-
// @Query(value = "SELECT u FROM User u ORDER BY CASE WHEN (u.firstname >= :name) THEN 0 ELSE 1 END, u.firstname")
587-
// Page<User> findAllOrderedBySpecialNameSingleParam(@Param("name") String name, Pageable page);
588-
//
589-
// // DATAJPA-1233
590-
// @Query(
591-
// value = "SELECT u FROM User u WHERE :other = 'x' ORDER BY CASE WHEN (u.firstname >= :name) THEN 0 ELSE 1 END,
592-
// u.firstname")
593-
// Page<User> findAllOrderedBySpecialNameMultipleParams(@Param("name") String name, @Param("other") String other,
594-
// Pageable page);
595-
//
596-
// // DATAJPA-1233
597-
// @Query(
598-
// value = "SELECT u FROM User u WHERE ?2 = 'x' ORDER BY CASE WHEN (u.firstname >= ?1) THEN 0 ELSE 1 END,
599-
// u.firstname")
600-
// Page<User> findAllOrderedBySpecialNameMultipleParamsIndexed(String other, String name, Pageable page);
581+
@Query(value = "SELECT u FROM User u ORDER BY CASE WHEN (u.firstname >= :name) THEN 0 ELSE 1 END, u.firstname")
582+
Page<User> findAllOrderedByNamedParam(@Param("name") String name, Pageable page);
583+
584+
@Query(value = "SELECT u FROM User u ORDER BY CASE WHEN (u.firstname >= ?1) THEN 0 ELSE 1 END, u.firstname")
585+
Page<User> findAllOrderedByIndexedParam(String name, Pageable page);
586+
587+
@Query(
588+
value = "SELECT u FROM User u WHERE :other = 'x' ORDER BY CASE WHEN (u.firstname >= :name) THEN 0 ELSE 1 END, u.firstname")
589+
Page<User> findAllOrderedBySpecialNameMultipleParams(@Param("name") String name, @Param("other") String other,
590+
Pageable page);
591+
592+
// Note that parameters used in the order-by statement are just cut off, so we must declare a query that parameter
593+
// label order remains valid even after truncating the order by part. (i.e. WHERE ?2 = 'x' ORDER BY CASE WHEN
594+
// (u.firstname >= ?1) isn't going to work).
595+
@Query(
596+
value = "SELECT u FROM User u WHERE ?1 = 'x' ORDER BY CASE WHEN (u.firstname >= ?2) THEN 0 ELSE 1 END, u.firstname")
597+
Page<User> findAllOrderedBySpecialNameMultipleParamsIndexed(String other, String name, Pageable page);
601598

602599
// DATAJPA-928
603600
Page<User> findByNativeNamedQueryWithPageable(Pageable pageable);
@@ -751,7 +748,8 @@ List<String> findAllAndSortByFunctionResultNamedParameter(@Param("namedParameter
751748

752749
@Retention(RetentionPolicy.RUNTIME)
753750
@Query("select u, count(r) from User u left outer join u.roles r group by u")
754-
@interface UserRoleCountProjectingQuery {}
751+
@interface UserRoleCountProjectingQuery {
752+
}
755753

756754
interface RolesAndFirstname {
757755

@@ -781,10 +779,12 @@ record UserExcerpt(String firstname, String lastname) {
781779

782780
}
783781

784-
record UserRoleCountDtoProjection(User user, Long roleCount) {}
782+
record UserRoleCountDtoProjection(User user, Long roleCount) {
783+
}
785784

786785
interface UserRoleCountInterfaceProjection {
787786
User getUser();
787+
788788
Long getRoleCount();
789789
}
790790

0 commit comments

Comments
 (0)