From 16495066f8eba96f386f981455f106b6cea06a97 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 5 Feb 2025 09:56:24 +0100 Subject: [PATCH] Consider Sort override from Pageable using Fluent Query API. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We now consider properly a sort value from a given Pageable. Previously, the sort Parameter was not applied from Pageable but from a previous sort(…) call. Also, we apply the given Sort to the resulting Pageable to ensure sort continuity. Closes #3762 --- .../FetchableFluentQueryByPredicate.java | 20 ++++++--- .../FetchableFluentQueryBySpecification.java | 44 +++++++++++++------ .../support/FluentQuerySupport.java | 13 +++++- .../jpa/repository/UserRepositoryTests.java | 42 ++++++++++++++++-- ...QuerydslJpaPredicateExecutorUnitTests.java | 13 ++++++ 5 files changed, 107 insertions(+), 25 deletions(-) diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/FetchableFluentQueryByPredicate.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/FetchableFluentQueryByPredicate.java index 75207eb8a..8bd1aa575 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/FetchableFluentQueryByPredicate.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/FetchableFluentQueryByPredicate.java @@ -147,7 +147,7 @@ class FetchableFluentQueryByPredicate extends FluentQuerySupport imp @Override public R oneValue() { - List results = createSortedAndProjectedQuery() // + List results = createSortedAndProjectedQuery(this.sort) // .limit(2) // Never need more than 2 values .fetch(); @@ -161,7 +161,7 @@ class FetchableFluentQueryByPredicate extends FluentQuerySupport imp @Override public R firstValue() { - List results = createSortedAndProjectedQuery() // + List results = createSortedAndProjectedQuery(this.sort) // .limit(1) // Never need more than 1 value .fetch(); @@ -170,7 +170,11 @@ class FetchableFluentQueryByPredicate extends FluentQuerySupport imp @Override public List all() { - return convert(createSortedAndProjectedQuery().fetch()); + return all(this.sort); + } + + private List all(Sort sort) { + return convert(createSortedAndProjectedQuery(sort).fetch()); } @Override @@ -184,13 +188,13 @@ class FetchableFluentQueryByPredicate extends FluentQuerySupport imp @Override public Page page(Pageable pageable) { - return pageable.isUnpaged() ? new PageImpl<>(all()) : readPage(pageable); + return pageable.isUnpaged() ? new PageImpl<>(all(pageable.getSortOr(this.sort))) : readPage(pageable); } @Override public Stream stream() { - return createSortedAndProjectedQuery() // + return createSortedAndProjectedQuery(this.sort) // .stream() // .map(getConversionFunction()); } @@ -205,7 +209,7 @@ class FetchableFluentQueryByPredicate extends FluentQuerySupport imp return existsOperation.apply(predicate); } - private AbstractJPAQuery createSortedAndProjectedQuery() { + private AbstractJPAQuery createSortedAndProjectedQuery(Sort sort) { AbstractJPAQuery query = finder.apply(sort); applyQuerySettings(this.returnedType, this.limit, query, null); @@ -247,6 +251,7 @@ class FetchableFluentQueryByPredicate extends FluentQuerySupport imp private Page readPage(Pageable pageable) { + Sort sort = pageable.getSortOr(this.sort); AbstractJPAQuery query = pagedFinder.apply(sort, pageable); if (!properties.isEmpty()) { @@ -255,7 +260,8 @@ class FetchableFluentQueryByPredicate extends FluentQuerySupport imp List paginatedResults = convert(query.fetch()); - return PageableExecutionUtils.getPage(paginatedResults, pageable, () -> countOperation.apply(predicate)); + return PageableExecutionUtils.getPage(paginatedResults, withSort(pageable, sort), + () -> countOperation.apply(predicate)); } private List convert(List resultList) { diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/FetchableFluentQueryBySpecification.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/FetchableFluentQueryBySpecification.java index 12ed69ee4..5d87904ec 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/FetchableFluentQueryBySpecification.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/FetchableFluentQueryBySpecification.java @@ -93,8 +93,17 @@ class FetchableFluentQueryBySpecification extends FluentQuerySupport Assert.notNull(sort, "Sort must not be null"); - return new FetchableFluentQueryBySpecification<>(spec, entityType, resultType, this.sort.and(sort), limit, - properties, finder, scroll, countOperation, existsOperation, entityManager, projectionFactory); + return getSorted(this.sort.and(sort)); + } + + private FetchableFluentQueryBySpecification getSorted(Sort sort) { + + if (this.sort == sort) { + return this; + } + + return new FetchableFluentQueryBySpecification<>(spec, entityType, resultType, sort, limit, properties, finder, + scroll, countOperation, existsOperation, entityManager, projectionFactory); } @Override @@ -125,7 +134,7 @@ class FetchableFluentQueryBySpecification extends FluentQuerySupport @Override public R oneValue() { - List results = createSortedAndProjectedQuery() // + List results = createSortedAndProjectedQuery(this.sort) // .setMaxResults(2) // Never need more than 2 values .getResultList(); @@ -139,7 +148,7 @@ class FetchableFluentQueryBySpecification extends FluentQuerySupport @Override public R firstValue() { - List results = createSortedAndProjectedQuery() // + List results = createSortedAndProjectedQuery(this.sort) // .setMaxResults(1) // Never need more than 1 value .getResultList(); @@ -148,7 +157,11 @@ class FetchableFluentQueryBySpecification extends FluentQuerySupport @Override public List all() { - return convert(createSortedAndProjectedQuery().getResultList()); + return all(this.sort); + } + + private List all(Sort sort) { + return convert(createSortedAndProjectedQuery(sort).getResultList()); } @Override @@ -161,24 +174,25 @@ class FetchableFluentQueryBySpecification extends FluentQuerySupport @Override public Slice slice(Pageable pageable) { - return pageable.isUnpaged() ? new PageImpl<>(all()) : readSlice(pageable); + return pageable.isUnpaged() ? new PageImpl<>(all(pageable.getSortOr(this.sort))) : readSlice(pageable); } @Override public Page page(Pageable pageable) { - return pageable.isUnpaged() ? new PageImpl<>(all()) : readPage(pageable, spec); + return pageable.isUnpaged() ? new PageImpl<>(all(pageable.getSortOr(this.sort))) : readPage(pageable, spec); } @Override @SuppressWarnings({ "rawtypes", "unchecked" }) public Page page(Pageable pageable, Specification countSpec) { - return pageable.isUnpaged() ? new PageImpl<>(all()) : readPage(pageable, (Specification) countSpec); + return pageable.isUnpaged() ? new PageImpl<>(all(pageable.getSortOr(this.sort))) + : readPage(pageable, (Specification) countSpec); } @Override public Stream stream() { - return createSortedAndProjectedQuery() // + return createSortedAndProjectedQuery(this.sort) // .getResultStream() // .map(getConversionFunction()); } @@ -193,9 +207,9 @@ class FetchableFluentQueryBySpecification extends FluentQuerySupport return existsOperation.apply(spec); } - private TypedQuery createSortedAndProjectedQuery() { + private TypedQuery createSortedAndProjectedQuery(Sort sort) { - TypedQuery query = finder.apply(this); + TypedQuery query = finder.apply(getSorted(sort)); if (!properties.isEmpty()) { query.setHint(EntityGraphFactory.HINT, EntityGraphFactory.create(entityManager, entityType, properties)); @@ -210,7 +224,7 @@ class FetchableFluentQueryBySpecification extends FluentQuerySupport private Slice readSlice(Pageable pageable) { - TypedQuery pagedQuery = createSortedAndProjectedQuery(); + TypedQuery pagedQuery = createSortedAndProjectedQuery(pageable.getSort()); if (pageable.isPaged()) { pagedQuery.setFirstResult(PageableUtils.getOffsetAsInteger(pageable)); @@ -230,7 +244,8 @@ class FetchableFluentQueryBySpecification extends FluentQuerySupport private Page readPage(Pageable pageable, @Nullable Specification countSpec) { - TypedQuery pagedQuery = createSortedAndProjectedQuery(); + Sort sort = pageable.getSortOr(this.sort); + TypedQuery pagedQuery = createSortedAndProjectedQuery(sort); if (pageable.isPaged()) { pagedQuery.setFirstResult(PageableUtils.getOffsetAsInteger(pageable)); @@ -239,7 +254,8 @@ class FetchableFluentQueryBySpecification extends FluentQuerySupport List paginatedResults = convert(pagedQuery.getResultList()); - return PageableExecutionUtils.getPage(paginatedResults, pageable, () -> countOperation.apply(countSpec)); + return PageableExecutionUtils.getPage(paginatedResults, withSort(pageable, sort), + () -> countOperation.apply(countSpec)); } private List convert(List resultList) { diff --git a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/FluentQuerySupport.java b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/FluentQuerySupport.java index f97da41ee..10b484d98 100644 --- a/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/FluentQuerySupport.java +++ b/spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/support/FluentQuerySupport.java @@ -22,6 +22,8 @@ import java.util.Set; import java.util.function.Function; import org.springframework.core.convert.support.DefaultConversionService; +import org.springframework.data.domain.PageRequest; +import org.springframework.data.domain.Pageable; import org.springframework.data.domain.ScrollPosition; import org.springframework.data.domain.Sort; import org.springframework.data.jpa.repository.query.AbstractJpaQuery; @@ -50,7 +52,7 @@ abstract class FluentQuerySupport { protected final ProjectionFactory projectionFactory; FluentQuerySupport(Class resultType, Sort sort, int limit, @Nullable Collection properties, - Class entityType, ProjectionFactory projectionFactory) { + Class entityType, ProjectionFactory projectionFactory) { this.returnedType = ReturnedType.of(resultType, entityType, projectionFactory); this.resultType = resultType; @@ -94,6 +96,15 @@ abstract class FluentQuerySupport { return o -> DefaultConversionService.getSharedInstance().convert(o, targetType); } + Pageable withSort(Pageable pageable, Sort sort) { + + if (pageable instanceof PageRequest pr && pageable.getSort() != sort) { + return pr.withSort(sort); + } + + return pageable; + } + interface ScrollQueryFactory { Q createQuery(FluentQuerySupport query, ScrollPosition scrollPosition); } 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 78dc30791..49ebc0af9 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 @@ -2485,6 +2485,24 @@ class UserRepositoryTests { assertThat(page1.getContent()).containsExactly(fourthUser); } + @Test // GH-3762 + void findByFluentExamplePageSortOverride() { + + flushTestUsers(); + + User prototype = new User(); + prototype.setFirstname("v"); + + Example userProbe = of(prototype, matching().withIgnorePaths("age", "createdAt", "active") + .withMatcher("firstname", GenericPropertyMatcher::contains)); + + Page page = repository.findBy(userProbe, // + q -> q.sortBy(Sort.by("firstname")).page(PageRequest.of(0, 2, Sort.by(DESC, "firstname")))); + + assertThat(page.getContent()).containsExactly(fourthUser, firstUser); + assertThat(repository.findAll(page.nextPageable())).containsExactly(secondUser, thirdUser); + } + @Test // GH-2294 void findByFluentExampleWithInterfaceBasedProjection() { @@ -2706,20 +2724,38 @@ class UserRepositoryTests { assertThat(page1.getContent()).containsExactly(fourthUser); } - @Test // GH-2274 + @Test // GH-3762 + void findByFluentSpecificationSortOverridePage() { + + flushTestUsers(); + + Page page = repository.findBy(userHasFirstnameLike("v"), + q -> q.sortBy(Sort.by("firstname")).page(PageRequest.of(0, 2, Sort.by(DESC, "firstname")))); + + assertThat(page.getContent()).containsExactly(fourthUser, firstUser); + assertThat(repository.findAll(page.nextPageable())).containsExactly(secondUser, thirdUser); + + Slice slice = repository.findBy(userHasFirstnameLike("v"), + q -> q.sortBy(Sort.by("firstname")).slice(PageRequest.of(0, 2, Sort.by(DESC, "firstname")))); + + assertThat(slice.getContent()).containsExactly(fourthUser, firstUser); + assertThat(repository.findAll(slice.nextPageable())).containsExactly(secondUser, thirdUser); + } + + @Test // GH-2274, 3762 void findByFluentSpecificationSlice() { flushTestUsers(); Slice slice = repository.findBy(userHasFirstnameLike("v"), - q -> q.sortBy(Sort.by("firstname")).slice(PageRequest.of(0, 2))); + q -> q.sortBy(Sort.by(DESC, "firstname")).slice(PageRequest.of(0, 2, Sort.by("firstname")))); assertThat(slice).isNotInstanceOf(Page.class); assertThat(slice.getContent()).containsExactly(thirdUser, firstUser); assertThat(slice.hasNext()).isTrue(); slice = repository.findBy(userHasFirstnameLike("v"), - q -> q.sortBy(Sort.by("firstname")).slice(PageRequest.of(0, 3))); + q -> q.sortBy(Sort.by("firstname")).slice(PageRequest.of(0, 3, Sort.by("firstname")))); assertThat(slice).isNotInstanceOf(Page.class); assertThat(slice).hasSize(3); diff --git a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/support/QuerydslJpaPredicateExecutorUnitTests.java b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/support/QuerydslJpaPredicateExecutorUnitTests.java index 141ecb1dd..0956617aa 100644 --- a/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/support/QuerydslJpaPredicateExecutorUnitTests.java +++ b/spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/support/QuerydslJpaPredicateExecutorUnitTests.java @@ -399,6 +399,19 @@ class QuerydslJpaPredicateExecutorUnitTests { assertThat(page1.getContent()).containsExactly(oliver); } + @Test // GH-3762 + void findByFluentPredicateSortOverridePage() { + + Predicate predicate = user.firstname.contains("v"); + + Page page = predicateExecutor.findBy(predicate, + q -> q.sortBy(Sort.by("firstname")).page(PageRequest.of(0, 1, Sort.by(Direction.DESC, "firstname")))); + + assertThat(page.getContent()).containsOnly(oliver); + assertThat(predicateExecutor.findAll(predicate, page.nextPageable())).containsOnly(dave); + + } + @Test // GH-2294 void findByFluentPredicateWithInterfaceBasedProjection() {