From dffc334af0dab8423c70e6880dff07ddbbeff112 Mon Sep 17 00:00:00 2001 From: mikereiche Date: Tue, 17 Aug 2021 14:34:51 -0700 Subject: [PATCH] Remove the sort from query before executing count. Also creates the query with skip and limit from any Pageable parameter instead of applying it later. Closes #1191. --- .../data/couchbase/core/query/Query.java | 15 ++++++++--- .../query/CouchbaseQueryExecution.java | 8 +++--- .../repository/query/N1qlQueryCreator.java | 25 +++++++++++++++---- ...chbaseRepositoryQueryIntegrationTests.java | 3 ++- 4 files changed, 37 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/springframework/data/couchbase/core/query/Query.java b/src/main/java/org/springframework/data/couchbase/core/query/Query.java index d6d24c977..4599d25e5 100644 --- a/src/main/java/org/springframework/data/couchbase/core/query/Query.java +++ b/src/main/java/org/springframework/data/couchbase/core/query/Query.java @@ -136,7 +136,9 @@ public Query with(final Pageable pageable) { } this.limit = pageable.getPageSize(); this.skip = pageable.getOffset(); - return with(pageable.getSort()); + if(!this.sort.equals(pageable.getSort())) + this.sort.and(pageable.getSort()); + return this; } /** @@ -174,6 +176,11 @@ public Query with(final Sort sort) { return this; } + public Query withoutSort(){ + this.sort = Sort.unsorted(); + return this; + } + public void appendSkipAndLimit(final StringBuilder sb) { if (limit > 0) { sb.append(" LIMIT ").append(limit); @@ -287,8 +294,10 @@ public String toN1qlSelectString(ReactiveCouchbaseTemplate template, String coll appendString(statement, n1ql.selectEntity); // select ... appendWhereString(statement, n1ql.filter); // typeKey = typeValue appendWhere(statement, new int[] { 0 }, template.getConverter()); // criteria on this Query - appendSort(statement); - appendSkipAndLimit(statement); + if(!isCount){ + appendSort(statement); + appendSkipAndLimit(statement); + } return statement.toString(); } diff --git a/src/main/java/org/springframework/data/couchbase/repository/query/CouchbaseQueryExecution.java b/src/main/java/org/springframework/data/couchbase/repository/query/CouchbaseQueryExecution.java index ba04f2878..599f38ee0 100644 --- a/src/main/java/org/springframework/data/couchbase/repository/query/CouchbaseQueryExecution.java +++ b/src/main/java/org/springframework/data/couchbase/repository/query/CouchbaseQueryExecution.java @@ -19,8 +19,8 @@ import org.springframework.core.convert.converter.Converter; import org.springframework.data.couchbase.core.CouchbaseOperations; -import org.springframework.data.couchbase.core.ExecutableFindByQueryOperation.TerminatingFindByQuery; import org.springframework.data.couchbase.core.ExecutableFindByQueryOperation.ExecutableFindByQuery; +import org.springframework.data.couchbase.core.ExecutableFindByQueryOperation.TerminatingFindByQuery; import org.springframework.data.couchbase.core.query.Query; import org.springframework.data.domain.Pageable; import org.springframework.data.domain.Slice; @@ -112,7 +112,7 @@ public SlicedExecution(ExecutableFindByQuery find, Pageable pageable) { public Object execute(Query query, Class type, String collection) { int pageSize = pageable.getPageSize(); // Apply Pageable but tweak limit to peek into next page - Query modifiedQuery = query.with(pageable).limit(pageSize + 1); + Query modifiedQuery = query.skip(pageable.getOffset()).limit(pageSize + 1); List result = find.matching(modifiedQuery).all(); boolean hasNext = result.size() > pageSize; return new SliceImpl(hasNext ? result.subList(0, pageSize) : result, pageable, hasNext); @@ -142,14 +142,12 @@ public PagedExecution(ExecutableFindByQuery operation, Pageable pageable) { public Object execute(Query query, Class type, String collection) { int overallLimit = 0; // query.getLimit(); TerminatingFindByQuery matching = operation.matching(query); - // Apply raw pagination - query.with(pageable); // Adjust limit if page would exceed the overall limit if (overallLimit != 0 && pageable.getOffset() + pageable.getPageSize() > overallLimit) { query.limit((int) (overallLimit - pageable.getOffset())); } return PageableExecutionUtils.getPage(matching.all(), pageable, () -> { - long count = operation.matching(query.skip(-1).limit(-1)).count(); + long count = operation.matching(query.skip(-1).limit(-1).withoutSort()).count(); return overallLimit != 0 ? Math.min(count, overallLimit) : count; }); } diff --git a/src/main/java/org/springframework/data/couchbase/repository/query/N1qlQueryCreator.java b/src/main/java/org/springframework/data/couchbase/repository/query/N1qlQueryCreator.java index 896a1a066..66f8c37aa 100644 --- a/src/main/java/org/springframework/data/couchbase/repository/query/N1qlQueryCreator.java +++ b/src/main/java/org/springframework/data/couchbase/repository/query/N1qlQueryCreator.java @@ -22,6 +22,7 @@ import static org.springframework.data.couchbase.core.query.QueryCriteria.where; import java.util.Iterator; +import java.util.Optional; import org.springframework.core.convert.converter.Converter; import org.springframework.data.couchbase.core.convert.CouchbaseConverter; @@ -29,6 +30,7 @@ import org.springframework.data.couchbase.core.query.N1QLExpression; import org.springframework.data.couchbase.core.query.Query; import org.springframework.data.couchbase.core.query.QueryCriteria; +import org.springframework.data.domain.Pageable; import org.springframework.data.domain.Sort; import org.springframework.data.mapping.PersistentPropertyPath; import org.springframework.data.mapping.context.MappingContext; @@ -37,6 +39,7 @@ import org.springframework.data.repository.query.parser.AbstractQueryCreator; import org.springframework.data.repository.query.parser.Part; import org.springframework.data.repository.query.parser.PartTree; +import org.springframework.util.Assert; /** * @author Michael Nitschinger @@ -72,12 +75,24 @@ protected QueryCriteria create(final Part part, final Iterator iterator) return from(part, property, where(addMetaIfRequired(bucketName, path, property)), iterator); } + @Override + public Query createQuery() { + Query q = this.createQuery((Optional.of(this.accessor).map(ParameterAccessor::getSort).orElse(Sort.unsorted()))); + Pageable pageable = accessor.getPageable(); + if(pageable.isPaged()) { + q.skip(pageable.getOffset()); + q.limit(pageable.getPageSize()); + } + return q; + } + static Converter cvtr = new MyConverter(); static class MyConverter implements Converter { @Override public String convert(CouchbasePersistentProperty source) { - return new StringBuilder(source.getFieldName().length()+2).append("`").append(source.getFieldName()).append("`").toString(); + return new StringBuilder(source.getFieldName().length() + 2).append("`").append(source.getFieldName()).append("`") + .toString(); } } @@ -90,7 +105,7 @@ protected QueryCriteria and(final Part part, final QueryCriteria base, final Ite PersistentPropertyPath path = context.getPersistentPropertyPath(part.getProperty()); CouchbasePersistentProperty property = path.getLeafProperty(); - return from(part, property, base.and(addMetaIfRequired(bucketName,path, property)), iterator); + return from(part, property, base.and(addMetaIfRequired(bucketName, path, property)), iterator); } @Override @@ -100,7 +115,8 @@ protected QueryCriteria or(QueryCriteria base, QueryCriteria criteria) { @Override protected Query complete(QueryCriteria criteria, Sort sort) { - return (criteria == null ? new Query() : new Query().addCriteria(criteria)).with(sort); + Query q = (criteria == null ? new Query() : new Query().addCriteria(criteria)).with(sort); + return q; } private QueryCriteria from(final Part part, final CouchbasePersistentProperty property, final QueryCriteria criteria, @@ -166,8 +182,7 @@ private QueryCriteria from(final Part part, final CouchbasePersistentProperty pr } } - public static N1QLExpression addMetaIfRequired( - String bucketName, + public static N1QLExpression addMetaIfRequired(String bucketName, final PersistentPropertyPath persistentPropertyPath, final CouchbasePersistentProperty property) { if (property.isIdProperty()) { diff --git a/src/test/java/org/springframework/data/couchbase/repository/CouchbaseRepositoryQueryIntegrationTests.java b/src/test/java/org/springframework/data/couchbase/repository/CouchbaseRepositoryQueryIntegrationTests.java index 49d12f4ba..399a90c1c 100644 --- a/src/test/java/org/springframework/data/couchbase/repository/CouchbaseRepositoryQueryIntegrationTests.java +++ b/src/test/java/org/springframework/data/couchbase/repository/CouchbaseRepositoryQueryIntegrationTests.java @@ -75,6 +75,7 @@ import org.springframework.data.domain.Page; import org.springframework.data.domain.PageRequest; import org.springframework.data.domain.Pageable; +import org.springframework.data.domain.Sort; import org.springframework.data.projection.SpelAwareProxyProjectionFactory; import org.springframework.data.repository.core.support.DefaultRepositoryMetadata; import org.springframework.test.context.junit.jupiter.SpringJUnitConfig; @@ -448,7 +449,7 @@ void count() { Long count = airportRepository.countFancyExpression(asList("JFK"), asList("jfk"), false); assertEquals(1, count); - Pageable pageable = PageRequest.of(0, 2); + Pageable pageable = PageRequest.of(0, 2).withSort(Sort.by("iata")); Page aPage = airportRepository.findAllByIataNot("JFK", pageable); assertEquals(iatas.length - 1, aPage.getTotalElements()); assertEquals(pageable.getPageSize(), aPage.getContent().size());