From 93385c793cf2dfabd9853b93d2690aee7f33376e Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Mon, 22 Jan 2024 15:15:09 +0100 Subject: [PATCH] Create `ParameterBindings` for count query instead of reusing query bindings. This commit makes sure we create individual parameter bindings for the count query instead of reusing the bindings from the actual query. This ensures we do not miss bindings that are present only in the count query. Closes: #3293 Original pull request: #3339 --- .../query/AbstractStringBasedJpaQuery.java | 22 +++++++++-- .../jpa/repository/query/StringQuery.java | 14 ++++++- .../jpa/repository/UserRepositoryTests.java | 20 ++++++++++ .../query/SimpleJpaQueryUnitTests.java | 38 ++++++++++++++++++- .../jpa/repository/sample/UserRepository.java | 3 ++ 5 files changed, 89 insertions(+), 8 deletions(-) diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/AbstractStringBasedJpaQuery.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/AbstractStringBasedJpaQuery.java index 4502ac526..135faddd8 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/AbstractStringBasedJpaQuery.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/AbstractStringBasedJpaQuery.java @@ -17,7 +17,6 @@ package org.springframework.data.jpa.repository.query; import jakarta.persistence.EntityManager; import jakarta.persistence.Query; - import org.springframework.data.domain.Pageable; import org.springframework.data.domain.Sort; import org.springframework.data.jpa.repository.QueryRewriter; @@ -28,6 +27,7 @@ import org.springframework.data.util.Lazy; import org.springframework.expression.spel.standard.SpelExpressionParser; import org.springframework.lang.Nullable; import org.springframework.util.Assert; +import org.springframework.util.StringUtils; /** * Base class for {@link String} based JPA queries. @@ -49,6 +49,7 @@ abstract class AbstractStringBasedJpaQuery extends AbstractJpaQuery { private final SpelExpressionParser parser; private final QueryParameterSetter.QueryMetadataCache metadataCache = new QueryParameterSetter.QueryMetadataCache(); private final QueryRewriter queryRewriter; + private final Lazy countParameterBinder; /** * Creates a new {@link AbstractStringBasedJpaQuery} from the given {@link JpaQueryMethod}, {@link EntityManager} and @@ -78,8 +79,17 @@ abstract class AbstractStringBasedJpaQuery extends AbstractJpaQuery { method.isNativeQuery()); this.countQuery = Lazy.of(() -> { - DeclaredQuery countQuery = query.deriveCountQuery(countQueryString, method.getCountQueryProjection()); - return ExpressionBasedStringQuery.from(countQuery, method.getEntityInformation(), parser, method.isNativeQuery()); + + if(StringUtils.hasText(countQueryString)) { + + return new ExpressionBasedStringQuery(countQueryString, method.getEntityInformation(), parser, + method.isNativeQuery()); + } + return query.deriveCountQuery(null, method.getCountQueryProjection()); + }); + + this.countParameterBinder = Lazy.of(() -> { + return this.createCountBinder(this.countQuery.get()); }); this.parser = parser; @@ -113,6 +123,10 @@ abstract class AbstractStringBasedJpaQuery extends AbstractJpaQuery { evaluationContextProvider); } + protected ParameterBinder createCountBinder(DeclaredQuery countQuery) { + return ParameterBinderFactory.createQueryAwareBinder(getQueryMethod().getParameters(), countQuery, parser, evaluationContextProvider); + } + @Override protected Query doCreateCountQuery(JpaParametersParameterAccessor accessor) { @@ -125,7 +139,7 @@ abstract class AbstractStringBasedJpaQuery extends AbstractJpaQuery { QueryParameterSetter.QueryMetadata metadata = metadataCache.getMetadata(queryString, query); - parameterBinder.get().bind(metadata.withQuery(query), accessor, QueryParameterSetter.ErrorHandling.LENIENT); + countParameterBinder.get().bind(metadata.withQuery(query), accessor, QueryParameterSetter.ErrorHandling.LENIENT); return query; } 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 d1a3696c7..bb7fb3c2a 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 @@ -111,9 +111,19 @@ class StringQuery implements DeclaredQuery { @Override public DeclaredQuery deriveCountQuery(@Nullable String countQuery, @Nullable String countQueryProjection) { - return DeclaredQuery.of( // - countQuery != null ? countQuery : this.queryEnhancer.createCountQueryFor(countQueryProjection), // + if(StringUtils.hasText(countQuery)) { + return new StringQuery(countQuery, this.isNative); + } + + StringQuery stringQuery = new StringQuery(this.queryEnhancer.createCountQueryFor(countQueryProjection), // this.isNative); + + if(this.hasParameterBindings() && !this.getParameterBindings().equals(stringQuery.getParameterBindings())) { + stringQuery.getParameterBindings().clear(); + stringQuery.getParameterBindings().addAll(this.bindings); + } + + return stringQuery; } @Override 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 bf5c8e1d4..81e153cfb 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 @@ -42,6 +42,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.stream.IntStream; import java.util.stream.Stream; import org.assertj.core.api.SoftAssertions; @@ -1747,6 +1748,25 @@ class UserRepositoryTests { assertThat(users).containsOnly(firstUser); } + @Test // GH-2393 + void bindsSpELParameterOnlyUsedInCountQuery() { + + flushTestUsers(); // add some noise + + IntStream.range(0, 10).mapToObj(counter -> { + User source = new User(); + source.setFirstname("%d-Spring".formatted(counter)); + source.setLastname("Data"); + source.setEmailAddress("spring-%s@data.org".formatted(counter)); + return source; + }).forEach(repository::save); + em.flush(); + + Page users = repository.findByWithSpelParameterOnlyUsedForCountQuery("Data", PageRequest.of(0, 2)); + assertThat(users.getSize()).isEqualTo(2); + assertThat(users.getTotalElements()).isEqualTo(10); + } + @Test // DATAJPA-564 void shouldFindUserByLastnameWithSpelExpressionInStringBasedQuery() { diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/SimpleJpaQueryUnitTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/SimpleJpaQueryUnitTests.java index ad4f164fa..b35afe8e6 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/SimpleJpaQueryUnitTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/SimpleJpaQueryUnitTests.java @@ -28,10 +28,12 @@ import jakarta.persistence.metamodel.Metamodel; import java.lang.reflect.Method; import java.util.Collection; import java.util.List; +import java.util.Optional; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; @@ -53,6 +55,7 @@ import org.springframework.data.repository.query.QueryMethodEvaluationContextPro import org.springframework.data.repository.query.RepositoryQuery; import org.springframework.data.util.TypeInformation; import org.springframework.expression.spel.standard.SpelExpressionParser; +import org.springframework.lang.Nullable; /** * Unit test for {@link SimpleJpaQuery}. @@ -65,6 +68,7 @@ import org.springframework.expression.spel.standard.SpelExpressionParser; * @author Greg Turnquist * @author Krzysztof Krason * @author Erik Pellizzon + * @author Christoph Strobl */ @ExtendWith(MockitoExtension.class) @MockitoSettings(strictness = Strictness.LENIENT) @@ -215,6 +219,23 @@ class SimpleJpaQueryUnitTests { verify(em).createNativeQuery(anyString()); } + @Test // GH-3293 + void allowsCountQueryUsingParametersNotInOriginalQuery() throws Exception { + + when(em.createNativeQuery(anyString())).thenReturn(query); + + AbstractJpaQuery jpaQuery = createJpaQuery( + SampleRepository.class.getMethod("findAllWithBindingsOnlyInCountQuery", String.class, Pageable.class), Optional.empty()); + + jpaQuery.doCreateCountQuery(new JpaParametersParameterAccessor(jpaQuery.getQueryMethod().getParameters(), + new Object[]{"data", PageRequest.of(0, 10)})); + + ArgumentCaptor queryStringCaptor = ArgumentCaptor.forClass(String.class); + verify(em).createQuery(queryStringCaptor.capture(), eq(Long.class)); + + assertThat(queryStringCaptor.getValue()).startsWith("select count(u.id) from User u where u.name ="); + } + @Test // DATAJPA-885 void projectsWithManuallyDeclaredQuery() throws Exception { @@ -258,10 +279,19 @@ class SimpleJpaQueryUnitTests { } private AbstractJpaQuery createJpaQuery(Method method) { + return createJpaQuery(method, null); + } + + private AbstractJpaQuery createJpaQuery(JpaQueryMethod queryMethod, @Nullable String queryString, @Nullable String countQueryString) { + + return JpaQueryFactory.INSTANCE.fromMethodWithQueryString(queryMethod, em, queryString, countQueryString, + QueryRewriter.IdentityQueryRewriter.INSTANCE, EVALUATION_CONTEXT_PROVIDER); + } + + private AbstractJpaQuery createJpaQuery(Method method, @Nullable Optional countQueryString) { JpaQueryMethod queryMethod = new JpaQueryMethod(method, metadata, factory, extractor); - return JpaQueryFactory.INSTANCE.fromMethodWithQueryString(queryMethod, em, queryMethod.getAnnotatedQuery(), null, - QueryRewriter.IdentityQueryRewriter.INSTANCE, EVALUATION_CONTEXT_PROVIDER); + return createJpaQuery(queryMethod, queryMethod.getAnnotatedQuery(), countQueryString == null ? null : countQueryString.orElse(queryMethod.getCountQuery())); } interface SampleRepository { @@ -293,6 +323,10 @@ class SimpleJpaQueryUnitTests { @Query(value = "select u from #{#entityName} u", countQuery = "select count(u.id) from #{#entityName} u") List findAllWithExpressionInCountQuery(Pageable pageable); + + @Query(value = "select u from User u", countQuery = "select count(u.id) from #{#entityName} u where u.name = :#{#arg0}") + List findAllWithBindingsOnlyInCountQuery(String arg0, Pageable pageable); + } interface UserProjection {} 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 e50779601..44044aff9 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 @@ -446,6 +446,9 @@ 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) + Page findByWithSpelParameterOnlyUsedForCountQuery(String lastname, Pageable page); + // DATAJPA-564 @Query("select u from User u where u.lastname like %:#{[0]}% and u.lastname like %:lastname%") List findByLastnameWithSpelExpression(@Param("lastname") String lastname);