From dbe983c3cb88c6538e1db305364a9dbdfc5e5e4c Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Fri, 19 Sep 2014 09:32:49 +0200 Subject: [PATCH] DATAMONGO-1057 - Fix SliceExecution skipping elements. We now directly set the offset to use instead of reading it from the used pageable. This asserts that every single element is read from the store. Prior to this change the altered pageSize lead to an unintended increase of the number of elements to skip. Original pull request: #226. --- .../repository/query/AbstractMongoQuery.java | 6 +- ...tractPersonRepositoryIntegrationTests.java | 23 ++++++ .../query/AbstractMongoQueryUnitTests.java | 74 +++++++++++++++++++ 3 files changed, 100 insertions(+), 3 deletions(-) diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/AbstractMongoQuery.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/AbstractMongoQuery.java index 75c86b025..b65104660 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/AbstractMongoQuery.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/AbstractMongoQuery.java @@ -21,7 +21,6 @@ import java.util.List; import org.springframework.core.convert.ConversionService; import org.springframework.core.convert.support.DefaultConversionService; import org.springframework.data.domain.PageImpl; -import org.springframework.data.domain.PageRequest; import org.springframework.data.domain.Pageable; import org.springframework.data.domain.Slice; import org.springframework.data.domain.SliceImpl; @@ -211,6 +210,7 @@ public abstract class AbstractMongoQuery implements RepositoryQuery { * {@link Execution} for {@link Slice} query methods. * * @author Oliver Gierke + * @author Christoph Strobl * @since 1.5 */ @@ -232,9 +232,9 @@ public abstract class AbstractMongoQuery implements RepositoryQuery { MongoEntityMetadata metadata = method.getEntityInformation(); int pageSize = pageable.getPageSize(); - Pageable slicePageable = new PageRequest(pageable.getPageNumber(), pageSize + 1, pageable.getSort()); - List result = operations.find(query.with(slicePageable), metadata.getJavaType(), metadata.getCollectionName()); + List result = operations.find(query.skip(pageable.getOffset()).limit(pageSize + 1).with(pageable.getSort()), + metadata.getJavaType(), metadata.getCollectionName()); boolean hasNext = result.size() > pageSize; diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/AbstractPersonRepositoryIntegrationTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/AbstractPersonRepositoryIntegrationTests.java index 3ca40debc..910f59ccd 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/AbstractPersonRepositoryIntegrationTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/AbstractPersonRepositoryIntegrationTests.java @@ -19,6 +19,7 @@ import static java.util.Arrays.*; import static org.hamcrest.Matchers.*; import static org.junit.Assert.*; +import java.util.ArrayList; import java.util.Arrays; import java.util.HashSet; import java.util.List; @@ -1023,6 +1024,28 @@ public abstract class AbstractPersonRepositoryIntegrationTests { assertThat(result, is(notNullValue())); assertThat(result.firstname, is("Carter")); assertThat(result.lastname, is("Beauford")); + } + /** + * @see DATAMONGO-1057 + */ + @Test + public void sliceShouldTraverseElementsWithoutSkippingOnes() { + + repository.deleteAll(); + + List persons = new ArrayList(100); + for (int i = 0; i < 100; i++) { + // format firstname to assert sorting retains proper order + persons.add(new Person(String.format("%03d", i), "ln" + 1, 100)); + } + + repository.save(persons); + + Slice slice = repository.findByAgeGreaterThan(50, new PageRequest(0, 20, Direction.ASC, "firstname")); + assertThat(slice, contains(persons.subList(0, 20).toArray())); + + slice = repository.findByAgeGreaterThan(50, slice.nextPageable()); + assertThat(slice, contains(persons.subList(20, 40).toArray())); } } diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/query/AbstractMongoQueryUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/query/AbstractMongoQueryUnitTests.java index 8735367ca..9ffd7efb9 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/query/AbstractMongoQueryUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/query/AbstractMongoQueryUnitTests.java @@ -36,6 +36,8 @@ import org.mockito.runners.MockitoJUnitRunner; import org.springframework.data.domain.Page; import org.springframework.data.domain.PageRequest; import org.springframework.data.domain.Pageable; +import org.springframework.data.domain.Slice; +import org.springframework.data.domain.Sort; import org.springframework.data.mongodb.MongoDbFactory; import org.springframework.data.mongodb.core.MongoOperations; import org.springframework.data.mongodb.core.Person; @@ -50,6 +52,8 @@ import org.springframework.data.mongodb.repository.Meta; import org.springframework.data.mongodb.repository.MongoRepository; import org.springframework.data.repository.core.RepositoryMetadata; +import com.mongodb.BasicDBObjectBuilder; +import com.mongodb.DBObject; import com.mongodb.WriteResult; /** @@ -211,6 +215,73 @@ public class AbstractMongoQueryUnitTests { assertThat(captor.getValue().getMeta().getComment(), is("comment")); } + /** + * @see DATAMONGO-1057 + */ + @Test + public void slicedExecutionShouldRetainNrOfElementsToSkip() { + + MongoQueryFake query = createQueryForMethod("findByLastname", String.class, Pageable.class); + Pageable page1 = new PageRequest(0, 10); + Pageable page2 = page1.next(); + + query.execute(new Object[] { "fake", page1 }); + query.execute(new Object[] { "fake", page2 }); + + ArgumentCaptor captor = ArgumentCaptor.forClass(Query.class); + + verify(this.mongoOperationsMock, times(2)) + .find(captor.capture(), Matchers.eq(Person.class), Matchers.eq("persons")); + + assertThat(captor.getAllValues().get(0).getSkip(), is(0)); + assertThat(captor.getAllValues().get(1).getSkip(), is(10)); + } + + /** + * @see DATAMONGO-1057 + */ + @Test + public void slicedExecutionShouldIncrementLimitByOne() { + + MongoQueryFake query = createQueryForMethod("findByLastname", String.class, Pageable.class); + Pageable page1 = new PageRequest(0, 10); + Pageable page2 = page1.next(); + + query.execute(new Object[] { "fake", page1 }); + query.execute(new Object[] { "fake", page2 }); + + ArgumentCaptor captor = ArgumentCaptor.forClass(Query.class); + + verify(this.mongoOperationsMock, times(2)) + .find(captor.capture(), Matchers.eq(Person.class), Matchers.eq("persons")); + + assertThat(captor.getAllValues().get(0).getLimit(), is(11)); + assertThat(captor.getAllValues().get(1).getLimit(), is(11)); + } + + /** + * @see DATAMONGO-1057 + */ + @Test + public void slicedExecutionShouldRetainSort() { + + MongoQueryFake query = createQueryForMethod("findByLastname", String.class, Pageable.class); + Pageable page1 = new PageRequest(0, 10, Sort.Direction.DESC, "bar"); + Pageable page2 = page1.next(); + + query.execute(new Object[] { "fake", page1 }); + query.execute(new Object[] { "fake", page2 }); + + ArgumentCaptor captor = ArgumentCaptor.forClass(Query.class); + + verify(this.mongoOperationsMock, times(2)) + .find(captor.capture(), Matchers.eq(Person.class), Matchers.eq("persons")); + + DBObject expectedSortObject = new BasicDBObjectBuilder().add("bar", -1).get(); + assertThat(captor.getAllValues().get(0).getSortObject(), is(expectedSortObject)); + assertThat(captor.getAllValues().get(1).getSortObject(), is(expectedSortObject)); + } + private MongoQueryFake createQueryForMethod(String methodName, Class... paramTypes) { try { @@ -272,5 +343,8 @@ public class AbstractMongoQueryUnitTests { @org.springframework.data.mongodb.repository.Query("{}") Page findByAnnotatedQuery(String firstnanme, Pageable pageable); + /** @see DATAMONGO-1057 */ + Slice findByLastname(String lastname, Pageable page); + } }