Skip to content

Commit 76a0f5e

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 9bc7dac commit 76a0f5e

File tree

6 files changed

+135
-51
lines changed

6 files changed

+135
-51
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
@@ -3069,22 +3069,36 @@ void handlesColonsFollowedByIntegerInStringLiteral() {
30693069
assertThat(users).extracting(User::getId).containsExactly(expected.getId());
30703070
}
30713071

3072-
@Disabled("ORDER BY CASE appears to be a Hibernate-only feature")
3073-
@Test // DATAJPA-1233
3072+
@Test // DATAJPA-1233, GH-3756
30743073
void handlesCountQueriesWithLessParametersSingleParam() {
3075-
// repository.findAllOrderedBySpecialNameSingleParam("Oliver", PageRequest.of(2, 3));
3074+
3075+
flushTestUsers();
3076+
3077+
Page<User> result = repository.findAllOrderedByNamedParam("Oliver", PageRequest.of(0, 3));
3078+
3079+
assertThat(result.getContent()).containsExactly(firstUser, fourthUser, thirdUser);
3080+
assertThat(result.getTotalElements()).isEqualTo(4);
3081+
3082+
result = repository.findAllOrderedByIndexedParam("Oliver", PageRequest.of(0, 3));
3083+
3084+
assertThat(result.getContent()).containsExactly(firstUser, fourthUser, thirdUser);
3085+
assertThat(result.getTotalElements()).isEqualTo(4);
30763086
}
30773087

3078-
@Disabled("ORDER BY CASE appears to be a Hibernate-only feature")
3079-
@Test // DATAJPA-1233
3088+
@Test // DATAJPA-1233, GH-3756
30803089
void handlesCountQueriesWithLessParametersMoreThanOne() {
3081-
// repository.findAllOrderedBySpecialNameMultipleParams("Oliver", "x", PageRequest.of(2, 3));
3082-
}
30833090

3084-
@Disabled("ORDER BY CASE appears to be a Hibernate-only feature")
3085-
@Test // DATAJPA-1233
3086-
void handlesCountQueriesWithLessParametersMoreThanOneIndexed() {
3087-
// repository.findAllOrderedBySpecialNameMultipleParamsIndexed("x", "Oliver", PageRequest.of(2, 3));
3091+
flushTestUsers();
3092+
3093+
Page<User> result = repository.findAllOrderedBySpecialNameMultipleParams("Oliver", "x", PageRequest.of(0, 3));
3094+
3095+
assertThat(result.getContent()).containsExactly(firstUser, fourthUser, thirdUser);
3096+
assertThat(result.getTotalElements()).isEqualTo(4);
3097+
3098+
result = repository.findAllOrderedBySpecialNameMultipleParamsIndexed("x", "Oliver", PageRequest.of(0, 3));
3099+
3100+
assertThat(result.getContent()).containsExactly(firstUser, fourthUser, thirdUser);
3101+
assertThat(result.getTotalElements()).isEqualTo(4);
30883102
}
30893103

30903104
// 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: 21 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -83,9 +83,8 @@ public interface UserRepository extends JpaRepository<User, Integer>, JpaSpecifi
8383
java.util.Optional<User> findById(Integer primaryKey);
8484

8585
/**
86-
* Redeclaration of {@link CrudRepository#deleteById(java.lang.Object)}. to make sure the transaction
87-
* configuration of the original method is considered if the redeclaration does not carry a {@link Transactional}
88-
* annotation.
86+
* Redeclaration of {@link CrudRepository#deleteById(java.lang.Object)}. to make sure the transaction configuration of
87+
* the original method is considered if the redeclaration does not carry a {@link Transactional} annotation.
8988
*/
9089
@Override
9190
void deleteById(Integer id); // DATACMNS-649
@@ -419,7 +418,8 @@ Window<User> findTop3ByFirstnameStartingWithOrderByFirstnameAscEmailAddressAsc(S
419418
@Query("select u from User u where u.firstname = ?#{[0]} and u.firstname = ?1 and u.lastname like %?#{[1]}% and u.lastname like %?2%")
420419
List<User> findByFirstnameAndLastnameWithSpelExpression(String firstname, String lastname);
421420

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

425425
// DATAJPA-564
@@ -573,26 +573,23 @@ List<User> findUsersByFirstnameForSpELExpressionWithParameterIndexOnlyWithEntity
573573
@Query("SELECT u FROM User u where u.firstname >= ?1 and u.lastname = '000:1'")
574574
List<User> queryWithIndexedParameterAndColonFollowedByIntegerInString(String firstname);
575575

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

597594
// DATAJPA-928
598595
Page<User> findByNativeNamedQueryWithPageable(Pageable pageable);

0 commit comments

Comments
 (0)