DATAMONGO-1120 - Fix execution of query methods using pagination and field mapping customizations.

odrotbohm · odrotbohm · commit f3d2ae366ea4 · 2014-12-18T15:47:54.000+01:00
Repository queries that used pagination and referred to a field that was customized were failing as the count query executed was not mapped correctly in MongoOperations.

This result from the fix for DATAMONGO-1080 which removed the premature field name translation from AbstractMongoQuery and thus lead to unmapped field names being used for the count query.

We now expose the previously existing, but not public count(…) method on MongoOperations that takes both an entity type as well as an explicit collection name to be able to count-query a dedicated collection but still get the query mapping applied for a certain type.

Related ticket: DATAMONGO-1080.
diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoOperations.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoOperations.java
@@ -652,14 +652,28 @@ <T> T findAndModify(Query query, Update update, FindAndModifyOptions options, Cl
 	long count(Query query, Class<?> entityClass);
 
 	/**
-	 * Returns the number of documents for the given {@link Query} querying the given collection.
+	 * Returns the number of documents for the given {@link Query} querying the given collection. The given {@link Query}
+	 * must solely consist of document field references as we lack type information to map potential property references
+	 * onto document fields. TO make sure the query gets mapped, use {@link #count(Query, Class, String)}.
 	 * 
 	 * @param query
 	 * @param collectionName must not be {@literal null} or empty.
 	 * @return
+	 * @see #count(Query, Class, String)
 	 */
 	long count(Query query, String collectionName);
 
+	/**
+	 * Returns the number of documents for the given {@link Query} by querying the given collection using the given entity
+	 * class to map the given {@link Query}.
+	 * 
+	 * @param query
+	 * @param entityClass must not be {@literal null}.
+	 * @param collectionName must not be {@literal null} or empty.
+	 * @return
+	 */
+	long count(Query query, Class<?> entityClass, String collectionName);
+
 	/**
 	 * Insert the object into the collection for the entity type of the object to save.
 	 * <p/>
diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java
@@ -642,7 +642,11 @@ public long count(final Query query, String collectionName) {
 		return count(query, null, collectionName);
 	}
 
-	private long count(Query query, Class<?> entityClass, String collectionName) {
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.mongodb.core.MongoOperations#count(org.springframework.data.mongodb.core.query.Query, java.lang.Class, java.lang.String)
+	 */
+	public long count(Query query, Class<?> entityClass, String collectionName) {
 
 		Assert.hasText(collectionName);
 		final DBObject dbObject = query == null ? null : queryMapper.getMappedObject(query.getQueryObject(),
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
@@ -259,9 +259,11 @@ public PagedExecution(Pageable pageable) {
 		Object execute(Query query) {
 
 			MongoEntityMetadata<?> metadata = method.getEntityInformation();
+			String collectionName = metadata.getCollectionName();
+			Class<?> type = metadata.getJavaType();
 
 			int overallLimit = query.getLimit();
-			long count = operations.count(query, metadata.getCollectionName());
+			long count = operations.count(query, type, collectionName);
 			count = overallLimit != 0 ? Math.min(count, query.getLimit()) : count;
 
 			boolean pageableOutOfScope = pageable.getOffset() > count;
@@ -278,7 +280,7 @@ Object execute(Query query) {
 				query.limit(overallLimit - pageable.getOffset());
 			}
 
-			List<?> result = operations.find(query, metadata.getJavaType(), metadata.getCollectionName());
+			List<?> result = operations.find(query, type, collectionName);
 			return new PageImpl(result, pageable, count);
 		}
 	}
diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/Person.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/Person.java
@@ -25,6 +25,7 @@
 import org.springframework.data.mongodb.core.index.Indexed;
 import org.springframework.data.mongodb.core.mapping.DBRef;
 import org.springframework.data.mongodb.core.mapping.Document;
+import org.springframework.data.mongodb.core.mapping.Field;
 
 /**
  * Sample domain class.
@@ -50,7 +51,7 @@ public enum Sex {
 
 	@GeoSpatialIndexed private Point location;
 
-	private Address address;
+	private @Field("add") Address address;
 	private Set<Address> shippingAddresses;
 
 	@DBRef User creator;
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
@@ -15,7 +15,7 @@
  */
 package org.springframework.data.mongodb.repository.query;
 
-import static org.hamcrest.CoreMatchers.*;
+import static org.hamcrest.Matchers.*;
 import static org.junit.Assert.*;
 import static org.mockito.Matchers.*;
 import static org.mockito.Mockito.*;
@@ -100,8 +100,7 @@ public void testDeleteExecutionCallsRemoveCorreclty() {
 
 		createQueryForMethod("deletePersonByLastname", String.class).setDeleteQuery(true).execute(new Object[] { "booh" });
 
-		verify(this.mongoOperationsMock, times(1)).remove(Matchers.any(Query.class), Matchers.eq(Person.class),
-				Matchers.eq("persons"));
+		verify(this.mongoOperationsMock, times(1)).remove(Matchers.any(Query.class), eq(Person.class), eq("persons"));
 		verify(this.mongoOperationsMock, times(0)).find(Matchers.any(Query.class), Matchers.any(Class.class),
 				Matchers.anyString());
 	}
@@ -119,8 +118,8 @@ public void testDeleteExecutionLoadsListOfRemovedDocumentsWhenReturnTypeIsCollec
 
 		createQueryForMethod("deleteByLastname", String.class).setDeleteQuery(true).execute(new Object[] { "booh" });
 
-		verify(this.mongoOperationsMock, times(1)).findAllAndRemove(Matchers.any(Query.class), Matchers.eq(Person.class),
-				Matchers.eq("persons"));
+		verify(this.mongoOperationsMock, times(1)).findAllAndRemove(Matchers.any(Query.class), eq(Person.class),
+				eq("persons"));
 	}
 
 	/**
@@ -143,15 +142,14 @@ public void testDeleteExecutionReturnsZeroWhenWriteResultIsNull() {
 	public void testDeleteExecutionReturnsNrDocumentsDeletedFromWriteResult() {
 
 		when(writeResultMock.getN()).thenReturn(100);
-		when(this.mongoOperationsMock.remove(Matchers.any(Query.class), Matchers.eq(Person.class), Matchers.eq("persons")))
-				.thenReturn(writeResultMock);
+		when(this.mongoOperationsMock.remove(Matchers.any(Query.class), eq(Person.class), eq("persons"))).thenReturn(
+				writeResultMock);
 
 		MongoQueryFake query = createQueryForMethod("deletePersonByLastname", String.class);
 		query.setDeleteQuery(true);
 
 		assertThat(query.execute(new Object[] { "fake" }), is((Object) 100L));
-		verify(this.mongoOperationsMock, times(1)).remove(Matchers.any(Query.class), Matchers.eq(Person.class),
-				Matchers.eq("persons"));
+		verify(this.mongoOperationsMock, times(1)).remove(Matchers.any(Query.class), eq(Person.class), eq("persons"));
 	}
 
 	/**
@@ -165,8 +163,7 @@ public void metadataShouldNotBeAddedToQueryWhenNotPresent() {
 
 		ArgumentCaptor<Query> captor = ArgumentCaptor.forClass(Query.class);
 
-		verify(this.mongoOperationsMock, times(1))
-				.find(captor.capture(), Matchers.eq(Person.class), Matchers.eq("persons"));
+		verify(this.mongoOperationsMock, times(1)).find(captor.capture(), eq(Person.class), eq("persons"));
 
 		assertThat(captor.getValue().getMeta().getComment(), nullValue());
 	}
@@ -182,8 +179,7 @@ public void metadataShouldBeAddedToQueryCorrectly() {
 
 		ArgumentCaptor<Query> captor = ArgumentCaptor.forClass(Query.class);
 
-		verify(this.mongoOperationsMock, times(1))
-				.find(captor.capture(), Matchers.eq(Person.class), Matchers.eq("persons"));
+		verify(this.mongoOperationsMock, times(1)).find(captor.capture(), eq(Person.class), eq("persons"));
 		assertThat(captor.getValue().getMeta().getComment(), is("comment"));
 	}
 
@@ -198,7 +194,7 @@ public void metadataShouldBeAddedToCountQueryCorrectly() {
 
 		ArgumentCaptor<Query> captor = ArgumentCaptor.forClass(Query.class);
 
-		verify(this.mongoOperationsMock, times(1)).count(captor.capture(), Matchers.eq("persons"));
+		verify(this.mongoOperationsMock, times(1)).count(captor.capture(), eq(Person.class), eq("persons"));
 		assertThat(captor.getValue().getMeta().getComment(), is("comment"));
 	}
 
@@ -213,8 +209,7 @@ public void metadataShouldBeAddedToStringBasedQueryCorrectly() {
 
 		ArgumentCaptor<Query> captor = ArgumentCaptor.forClass(Query.class);
 
-		verify(this.mongoOperationsMock, times(1))
-				.find(captor.capture(), Matchers.eq(Person.class), Matchers.eq("persons"));
+		verify(this.mongoOperationsMock, times(1)).find(captor.capture(), eq(Person.class), eq("persons"));
 		assertThat(captor.getValue().getMeta().getComment(), is("comment"));
 	}
 
@@ -233,8 +228,7 @@ public void slicedExecutionShouldRetainNrOfElementsToSkip() {
 
 		ArgumentCaptor<Query> captor = ArgumentCaptor.forClass(Query.class);
 
-		verify(this.mongoOperationsMock, times(2))
-				.find(captor.capture(), Matchers.eq(Person.class), Matchers.eq("persons"));
+		verify(this.mongoOperationsMock, times(2)).find(captor.capture(), eq(Person.class), eq("persons"));
 
 		assertThat(captor.getAllValues().get(0).getSkip(), is(0));
 		assertThat(captor.getAllValues().get(1).getSkip(), is(10));
@@ -255,8 +249,7 @@ public void slicedExecutionShouldIncrementLimitByOne() {
 
 		ArgumentCaptor<Query> captor = ArgumentCaptor.forClass(Query.class);
 
-		verify(this.mongoOperationsMock, times(2))
-				.find(captor.capture(), Matchers.eq(Person.class), Matchers.eq("persons"));
+		verify(this.mongoOperationsMock, times(2)).find(captor.capture(), eq(Person.class), eq("persons"));
 
 		assertThat(captor.getAllValues().get(0).getLimit(), is(11));
 		assertThat(captor.getAllValues().get(1).getLimit(), is(11));
@@ -277,8 +270,7 @@ public void slicedExecutionShouldRetainSort() {
 
 		ArgumentCaptor<Query> captor = ArgumentCaptor.forClass(Query.class);
 
-		verify(this.mongoOperationsMock, times(2))
-				.find(captor.capture(), Matchers.eq(Person.class), Matchers.eq("persons"));
+		verify(this.mongoOperationsMock, times(2)).find(captor.capture(), eq(Person.class), eq("persons"));
 
 		DBObject expectedSortObject = new BasicDBObjectBuilder().add("bar", -1).get();
 		assertThat(captor.getAllValues().get(0).getSortObject(), is(expectedSortObject));

Original file line number	Diff line number	Diff line change
`@@ -259,9 +259,11 @@ public PagedExecution(Pageable pageable) {`
`259`	`259`	`Object execute(Query query) {`
`260`	`260`
`261`	`261`	`MongoEntityMetadata<?> metadata = method.getEntityInformation();`
	`262`	`+ String collectionName = metadata.getCollectionName();`
	`263`	`+ Class<?> type = metadata.getJavaType();`
`262`	`264`
`263`	`265`	`int overallLimit = query.getLimit();`
`264`		`- long count = operations.count(query, metadata.getCollectionName());`
	`266`	`+ long count = operations.count(query, type, collectionName);`
`265`	`267`	`count = overallLimit != 0 ? Math.min(count, query.getLimit()) : count;`
`266`	`268`
`267`	`269`	`boolean pageableOutOfScope = pageable.getOffset() > count;`
`@@ -278,7 +280,7 @@ Object execute(Query query) {`
`278`	`280`	`query.limit(overallLimit - pageable.getOffset());`
`279`	`281`	`}`
`280`	`282`
`281`		`- List<?> result = operations.find(query, metadata.getJavaType(), metadata.getCollectionName());`
	`283`	`+ List<?> result = operations.find(query, type, collectionName);`
`282`	`284`	`return new PageImpl(result, pageable, count);`
`283`	`285`	`}`
`284`	`286`	`}`