From fd8ea4f85281e29bb9beb2254c7c21145539ad97 Mon Sep 17 00:00:00 2001 From: Jens Schauder Date: Tue, 30 Jan 2018 10:13:03 +0100 Subject: [PATCH] DATAJPA-1255 - Fixed application of JPA pagination to only queries that don't use #pageable explicitly. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Query.setMaximumResults(…) and Query.setFirstResult(…) now only get applied when the underlying query does not mention #pageable. Before we called it unconditionally which caused the JPA implementation to apply it’s transformation to SQL statements that already were correctly doing pagination. Interestingly the existing test cases didn’t fail although the executed SQL was pretty bogus. Therefore the tests got changed a little to make them more strict. Original pull request: #247. --- .../jpa/repository/query/DeclaredQuery.java | 7 +++++++ .../jpa/repository/query/ParameterBinder.java | 21 +++++++++++++++++-- .../query/ParameterBinderFactory.java | 6 +++--- .../jpa/repository/query/StringQuery.java | 7 +++++++ .../jpa/repository/UserRepositoryTests.java | 12 +++++++---- .../jpa/repository/sample/UserRepository.java | 9 ++++++-- 6 files changed, 51 insertions(+), 11 deletions(-) diff --git a/src/main/java/org/springframework/data/jpa/repository/query/DeclaredQuery.java b/src/main/java/org/springframework/data/jpa/repository/query/DeclaredQuery.java index 9aadc2e7f..bd98a6619 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/DeclaredQuery.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/DeclaredQuery.java @@ -83,4 +83,11 @@ interface DeclaredQuery { * @return a new {@literal DeclaredQuery} instance. */ DeclaredQuery deriveCountQuery(@Nullable String countQuery, @Nullable String countQueryProjection); + + /** + * @return whether paging is implemented in the query itself, e.g. using SpEL expressions. + */ + default boolean implementsPaging() { + return false; + } } diff --git a/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinder.java b/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinder.java index 30db019fd..b5f901950 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinder.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinder.java @@ -37,20 +37,37 @@ public class ParameterBinder { private final JpaParameters parameters; private final Iterable parameterSetters; + private final boolean useJpaForPaging; /** * Creates a new {@link ParameterBinder} for the given {@link JpaParameters} and {@link QueryParameterSetter}s. * * @param parameters must not be {@literal null}. * @param parameterSetters must not be {@literal null}. + * @param useJpaForPaging determines whether {@link Query#setFirstResult(int)} and {@link Query#setMaxResults(int)} + * shall be used for paging. */ - public ParameterBinder(JpaParameters parameters, Iterable parameterSetters) { + public ParameterBinder(JpaParameters parameters, Iterable parameterSetters, + boolean useJpaForPaging) { Assert.notNull(parameters, "JpaParameters must not be null!"); Assert.notNull(parameterSetters, "Parameter setters must not be null!"); this.parameters = parameters; this.parameterSetters = parameterSetters; + this.useJpaForPaging = useJpaForPaging; + } + + /** + * Only for backward compatibility. + * + * @param parameters must not be {@literal null}. + * @param parameterSetters must not be {@literal null}. + * @deprecated use three argument constructor instead} + */ + @Deprecated + public ParameterBinder(JpaParameters parameters, Iterable parameterSetters) { + this(parameters, parameterSetters, true); } public T bind(T jpaQuery, Object[] values) { @@ -78,7 +95,7 @@ public class ParameterBinder { Query result = bind(query, values); - if (!parameters.hasPageableParameter() || accessor.getPageable().isUnpaged()) { + if (!useJpaForPaging || !parameters.hasPageableParameter() || accessor.getPageable().isUnpaged()) { return result; } diff --git a/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinderFactory.java b/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinderFactory.java index bc6ac6d04..b8dca10a1 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinderFactory.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinderFactory.java @@ -52,7 +52,7 @@ class ParameterBinderFactory { QueryParameterSetterFactory setterFactory = QueryParameterSetterFactory.basic(parameters); List bindings = getBindings(parameters); - return new ParameterBinder(parameters, createSetters(bindings, setterFactory)); + return new ParameterBinder(parameters, createSetters(bindings, setterFactory), true); } /** @@ -72,7 +72,7 @@ class ParameterBinderFactory { QueryParameterSetterFactory setterFactory = QueryParameterSetterFactory.forCriteriaQuery(parameters, metadata); List bindings = getBindings(parameters); - return new ParameterBinder(parameters, createSetters(bindings, setterFactory)); + return new ParameterBinder(parameters, createSetters(bindings, setterFactory), true); } /** @@ -100,7 +100,7 @@ class ParameterBinderFactory { evaluationContextProvider, parameters); QueryParameterSetterFactory basicSetterFactory = QueryParameterSetterFactory.basic(parameters); - return new ParameterBinder(parameters, createSetters(bindings, query, expressionSetterFactory, basicSetterFactory)); + return new ParameterBinder(parameters, createSetters(bindings, query, expressionSetterFactory, basicSetterFactory), !query.implementsPaging()); } private static List getBindings(JpaParameters parameters) { diff --git a/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java b/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java index 5e58b01ce..6ff965016 100644 --- a/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java +++ b/src/main/java/org/springframework/data/jpa/repository/query/StringQuery.java @@ -53,6 +53,7 @@ class StringQuery implements DeclaredQuery { private final List bindings; private final @Nullable String alias; private final boolean hasConstructorExpression; + private final boolean containsPageableInSpel; /** * Creates a new {@link StringQuery} from the given JPQL query. @@ -64,6 +65,7 @@ class StringQuery implements DeclaredQuery { Assert.hasText(query, "Query must not be null or empty!"); this.bindings = new ArrayList<>(); + this.containsPageableInSpel = query.contains("#pageable"); this.query = ParameterBindingParser.INSTANCE.parseParameterBindingsOfQueryIntoBindingsAndReturnCleanedQuery(query, this.bindings); @@ -102,6 +104,11 @@ class StringQuery implements DeclaredQuery { .of(countQuery != null ? countQuery : QueryUtils.createCountQueryFor(query, countQueryProjection)); } + @Override + public boolean implementsPaging() { + return containsPageableInSpel; + } + /* * (non-Javadoc) * @see org.springframework.data.jpa.repository.query.DeclaredQuery#getQueryString() diff --git a/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java b/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java index e503f52e8..e94d4d463 100644 --- a/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java +++ b/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java @@ -1524,13 +1524,17 @@ public class UserRepositoryTests { flushTestUsers(); - Page users = repository.findUsersInNativeQueryWithPagination(PageRequest.of(0, 2)); + Page users = repository.findUsersInNativeQueryWithPagination(PageRequest.of(0, 3)); - assertThat(users.getContent()).hasSize(2).containsExactly(firstUser, secondUser); + SoftAssertions softly = new SoftAssertions(); - users = repository.findUsersInNativeQueryWithPagination(PageRequest.of(1, 2)); + softly.assertThat(users.getContent()).extracting(User::getFirstname).containsExactly("Dave", "Joachim", "kevin"); - assertThat(users.getContent()).hasSize(2).containsExactly(thirdUser, fourthUser); + users = repository.findUsersInNativeQueryWithPagination(PageRequest.of(1, 3)); + + softly.assertThat(users.getContent()).extracting(User::getFirstname).containsExactly("Oliver"); + + softly.assertAll(); } @Test // DATAJPA-1140 diff --git a/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java b/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java index ab97d9677..18008d26a 100644 --- a/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java +++ b/src/test/java/org/springframework/data/jpa/repository/sample/UserRepository.java @@ -425,7 +425,11 @@ public interface UserRepository // DATAJPA-564 @Query( - value = "select * from (select rownum() as RN, u.* from SD_User u) where RN between ?#{ #pageable.offset -1} and ?#{#pageable.offset + #pageable.pageSize}", + value = "select * from (" + + "select u.*, rownum() as RN from (" + + "select * from SD_User ORDER BY ucase(firstname)" + + ") u" + + ") where RN between ?#{ #pageable.offset +1 } and ?#{#pageable.offset + #pageable.pageSize}", countQuery = "select count(u.id) from SD_User u", nativeQuery = true) Page findUsersInNativeQueryWithPagination(Pageable pageable); @@ -534,7 +538,8 @@ public interface UserRepository Page findByNativeNamedQueryWithPageable(Pageable pageable); // DATAJPA-928 - @Query(value = "SELECT firstname FROM SD_User ORDER BY UCASE(firstname)", countQuery = "SELECT count(*) FROM SD_User", nativeQuery = true) + @Query(value = "SELECT firstname FROM SD_User ORDER BY UCASE(firstname)", countQuery = "SELECT count(*) FROM SD_User", + nativeQuery = true) Page findByNativeQueryWithPageable(@Param("pageable") Pageable pageable); // DATAJPA-1273