DATAMONGO-566 - Polishing.

Inlined a few methods to reduce the number of indirections. Added a bit of missing JavaDoc here and there. StringBasedMongoQuery now prevents a manually defined query from being marked as both count and delete query.

Polished test cases a little.

Original pull request: #147.

spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/AbstractMongoQuery.java

@@ -372,6 +372,10 @@ public abstract class AbstractMongoQuery implements RepositoryQuery {
 	 */
 	final class DeleteExecution extends Execution {
 
+		/*
+		 * (non-Javadoc)
+		 * @see org.springframework.data.mongodb.repository.query.AbstractMongoQuery.Execution#execute(org.springframework.data.mongodb.core.query.Query)
+		 */
 		@Override
 		Object execute(Query query) {
 
@@ -382,20 +386,11 @@ public abstract class AbstractMongoQuery implements RepositoryQuery {
 		private Object deleteAndConvertResult(Query query, MongoEntityMetadata<?> metadata) {
 
 			if (method.isCollectionQuery()) {
-				return findAndRemove(query, metadata);
+				return operations.findAllAndRemove(query, metadata.getJavaType());
 			}
 
-			WriteResult writeResult = remove(query, metadata);
+			WriteResult writeResult = operations.remove(query, metadata.getCollectionName());
 			return writeResult != null ? writeResult.getN() : 0L;
 		}
-
-		private List<?> findAndRemove(Query query, MongoEntityMetadata<?> metadata) {
-			return operations.findAllAndRemove(query, metadata.getJavaType());
-		}
-
-		private WriteResult remove(Query query, MongoEntityMetadata<?> metadata) {
-			return operations.remove(query, metadata.getCollectionName());
-		}
 	}
-
 }

spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/StringBasedMongoQuery.java

@@ -34,6 +34,7 @@ import com.mongodb.util.JSON;
  */
 public class StringBasedMongoQuery extends AbstractMongoQuery {
 
+	private static final String COUND_AND_DELETE = "Manually defined query for %s cannot be both a count and delete query at the same time!";
 	private static final Pattern PLACEHOLDER = Pattern.compile("\\?(\\d+)");
 	private static final Logger LOG = LoggerFactory.getLogger(StringBasedMongoQuery.class);
 
@@ -43,7 +44,18 @@ public class StringBasedMongoQuery extends AbstractMongoQuery {
 	private final boolean isDeleteQuery;
 
 	/**
-	 * Creates a new {@link StringBasedMongoQuery}.
+	 * Creates a new {@link StringBasedMongoQuery} for the given {@link MongoQueryMethod} and {@link MongoOperations}.
+	 * 
+	 * @param method must not be {@literal null}.
+	 * @param mongoOperations must not be {@literal null}.
+	 */
+	public StringBasedMongoQuery(MongoQueryMethod method, MongoOperations mongoOperations) {
+		this(method.getAnnotatedQuery(), method, mongoOperations);
+	}
+
+	/**
+	 * Creates a new {@link StringBasedMongoQuery} for the given {@link String}, {@link MongoQueryMethod} and
+	 * {@link MongoOperations}.
 	 * 
 	 * @param method must not be {@literal null}.
 	 * @param template must not be {@literal null}.
@@ -56,10 +68,10 @@ public class StringBasedMongoQuery extends AbstractMongoQuery {
 		this.fieldSpec = method.getFieldSpecification();
 		this.isCountQuery = method.hasAnnotatedQuery() ? method.getQueryAnnotation().count() : false;
 		this.isDeleteQuery = method.hasAnnotatedQuery() ? method.getQueryAnnotation().delete() : false;
-	}
 
-	public StringBasedMongoQuery(MongoQueryMethod method, MongoOperations mongoOperations) {
+		if (isCountQuery && isDeleteQuery) {
-		this(method.getAnnotatedQuery(), method, mongoOperations);
+			throw new IllegalArgumentException(String.format(COUND_AND_DELETE, method));
+		}
 	}
 
 	/*
@@ -98,6 +110,10 @@ public class StringBasedMongoQuery extends AbstractMongoQuery {
 		return isCountQuery;
 	}
 
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.mongodb.repository.query.AbstractMongoQuery#isDeleteQuery()
+	 */
 	@Override
 	protected boolean isDeleteQuery() {
 		return this.isDeleteQuery;

spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateUnitTests.java

@@ -319,6 +319,9 @@ public class MongoTemplateUnitTests extends MongoOperationsUnitTests {
 		assertThat((Object[]) idField.get("$in"), is(new Object[] { Integer.valueOf(0), Integer.valueOf(1) }));
 	}
 
+	/**
+	 * @see DATAMONGO-566
+	 */
 	@Test
 	public void findAllAndRemoveShouldNotTriggerRemoveIfFindResultIsEmpty() {
 

spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/AbstractPersonRepositoryIntegrationTests.java

@@ -781,6 +781,7 @@ public abstract class AbstractPersonRepositoryIntegrationTests {
 		repository.save(alicia);
 
 		Page<Person> result = repository.findByHavingCreator(new PageRequest(0, 100));
+
 		assertThat(result.getNumberOfElements(), is(1));
 		assertThat(result.getContent().get(0), is(alicia));
 	}
@@ -792,6 +793,7 @@ public abstract class AbstractPersonRepositoryIntegrationTests {
 	public void deleteByShouldReturnListOfDeletedElementsWhenRetunTypeIsCollectionLike() {
 
 		List<Person> result = repository.deleteByLastname("Beauford");
+
 		assertThat(result, hasItem(carter));
 		assertThat(result, hasSize(1));
 	}
@@ -803,6 +805,7 @@ public abstract class AbstractPersonRepositoryIntegrationTests {
 	public void deleteByShouldRemoveElementsMatchingDerivedQuery() {
 
 		repository.deleteByLastname("Beauford");
+
 		assertThat(operations.count(new BasicQuery("{'lastname':'Beauford'}"), Person.class), is(0L));
 	}
 
@@ -837,6 +840,7 @@ public abstract class AbstractPersonRepositoryIntegrationTests {
 	public void deleteByUsingAnnotatedQueryShouldReturnListOfDeletedElementsWhenRetunTypeIsCollectionLike() {
 
 		List<Person> result = repository.removeByLastnameUsingAnnotatedQuery("Beauford");
+
 		assertThat(result, hasItem(carter));
 		assertThat(result, hasSize(1));
 	}
@@ -848,6 +852,7 @@ public abstract class AbstractPersonRepositoryIntegrationTests {
 	public void deleteByUsingAnnotatedQueryShouldRemoveElementsMatchingDerivedQuery() {
 
 		repository.removeByLastnameUsingAnnotatedQuery("Beauford");
+
 		assertThat(operations.count(new BasicQuery("{'lastname':'Beauford'}"), Person.class), is(0L));
 	}
 
@@ -858,5 +863,4 @@ public abstract class AbstractPersonRepositoryIntegrationTests {
 	public void deleteByUsingAnnotatedQueryShouldReturnNumberOfDocumentsRemovedIfReturnTypeIsLong() {
 		assertThat(repository.removePersonByLastnameUsingAnnotatedQuery("Beauford"), is(1L));
 	}
-
 }

spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/query/AbstracMongoQueryUnitTests.java

@@ -15,6 +15,7 @@
  */
 package org.springframework.data.mongodb.repository.query;
 
+import static org.hamcrest.CoreMatchers.*;
 import static org.junit.Assert.*;
 import static org.mockito.Mockito.*;
 
@@ -47,33 +48,30 @@ import org.springframework.data.repository.core.RepositoryMetadata;
 import com.mongodb.WriteResult;
 
 /**
+ * Unit tests for {@link AbstractMongoQuery}.
+ * 
  * @author Christoph Strobl
+ * @author Oliver Gierke
  */
 @RunWith(MockitoJUnitRunner.class)
-public class MongoQueryExecutionUnitTests {
+public class AbstracMongoQueryUnitTests {
-
-	private @Mock RepositoryMetadata metadataMock;
-
-	private @Mock MongoOperations mongoOperationsMock;
-
-	@SuppressWarnings("rawtypes")//
-	private @Mock BasicMongoPersistentEntity persitentEntityMock;
 
-	private @Mock MongoMappingContext mappingContextMock;
+	@Mock RepositoryMetadata metadataMock;
+	@Mock MongoOperations mongoOperationsMock;
+	@Mock @SuppressWarnings("rawtypes") BasicMongoPersistentEntity persitentEntityMock;
+	@Mock MongoMappingContext mappingContextMock;
+	@Mock WriteResult writeResultMock;
 
-	private @Mock WriteResult writeResultMock;
-
-	@SuppressWarnings({ "unchecked", "rawtypes" })
 	@Before
+	@SuppressWarnings({ "unchecked", "rawtypes" })
 	public void setUp() {
 
 		when(metadataMock.getDomainType()).thenReturn((Class) Person.class);
 		when(metadataMock.getReturnedDomainClass(Matchers.any(Method.class))).thenReturn((Class) Person.class);
 		when(persitentEntityMock.getCollection()).thenReturn("persons");
-
 		when(mappingContextMock.getPersistentEntity(Matchers.any(Class.class))).thenReturn(persitentEntityMock);
-
 		when(persitentEntityMock.getType()).thenReturn(Person.class);
+
 		DbRefResolver dbRefResolver = new DefaultDbRefResolver(mock(MongoDbFactory.class));
 		MappingMongoConverter converter = new MappingMongoConverter(dbRefResolver, mappingContextMock);
 		converter.afterPropertiesSet();
@@ -89,6 +87,7 @@ public class MongoQueryExecutionUnitTests {
 	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("persons"));
 		verify(this.mongoOperationsMock, times(0)).find(Matchers.any(Query.class), Matchers.any(Class.class),
 				Matchers.anyString());
@@ -134,14 +133,19 @@ public class MongoQueryExecutionUnitTests {
 		MongoQueryFake query = createQueryForMethod("deletePersonByLastname", String.class);
 		query.setDeleteQuery(true);
 
-		assertThat(query.execute(new Object[] { "fake" }), Is.<Object> is(100L));
+		assertThat(query.execute(new Object[] { "fake" }), is((Object) 100L));
-
 		verify(this.mongoOperationsMock, times(1)).remove(Matchers.any(Query.class), Matchers.eq("persons"));
 	}
 
 	private MongoQueryFake createQueryForMethod(String methodName, Class<?>... paramTypes) {
+
 		try {
-			return this.createQueryForMethod(Repo.class.getMethod(methodName, paramTypes));
+
+			Method method = Repo.class.getMethod(methodName, paramTypes);
+			MongoQueryMethod queryMethod = new MongoQueryMethod(method, metadataMock, mappingContextMock);
+
+			return new MongoQueryFake(queryMethod, mongoOperationsMock);
+
 		} catch (NoSuchMethodException e) {
 			throw new IllegalArgumentException(e.getMessage(), e);
 		} catch (SecurityException e) {
@@ -149,23 +153,15 @@ public class MongoQueryExecutionUnitTests {
 		}
 	}
 
-	private MongoQueryFake createQueryForMethod(Method method) {
+	private static class MongoQueryFake extends AbstractMongoQuery {
-		return new MongoQueryFake(createMongoQueryMethodFrom(method), this.mongoOperationsMock);
-	}
 
-	private MongoQueryMethod createMongoQueryMethodFrom(Method method) {
+		private boolean isCountQuery;
-		return new MongoQueryMethod(method, metadataMock, this.mappingContextMock);
+		private boolean isDeleteQuery;
-	}
-
-	class MongoQueryFake extends AbstractMongoQuery {
 
 		public MongoQueryFake(MongoQueryMethod method, MongoOperations operations) {
 			super(method, operations);
 		}
 
-		private boolean isCountQuery;
-		private boolean isDeleteQuery;
-
 		@Override
 		protected Query createQuery(ConvertingParameterAccessor accessor) {
 			return new BasicQuery("{'foo':'bar'}");
@@ -181,11 +177,6 @@ public class MongoQueryExecutionUnitTests {
 			return isDeleteQuery;
 		}
 
-		public MongoQueryFake setCountQuery(boolean isCountQuery) {
-			this.isCountQuery = isCountQuery;
-			return this;
-		}
-
 		public MongoQueryFake setDeleteQuery(boolean isDeleteQuery) {
 			this.isDeleteQuery = isDeleteQuery;
 			return this;
@@ -198,5 +189,4 @@ public class MongoQueryExecutionUnitTests {
 
 		Long deletePersonByLastname(String lastname);
 	}
-
 }

spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/query/StringBasedMongoQueryUnitTests.java

@@ -150,6 +150,14 @@ public class StringBasedMongoQueryUnitTests {
 		assertThat(mongoQuery.isDeleteQuery(), is(true));
 	}
 
+	/**
+	 * @see DATAMONGO-566
+	 */
+	@Test(expected = IllegalArgumentException.class)
+	public void preventsDeleteAndCountFlagAtTheSameTime() throws Exception {
+		createQueryForMethod("invalidMethod", String.class);
+	}
+
 	private StringBasedMongoQuery createQueryForMethod(String name, Class<?>... parameters) throws Exception {
 
 		Method method = SampleRepository.class.getMethod(name, parameters);
@@ -174,5 +182,7 @@ public class StringBasedMongoQueryUnitTests {
 		@Query(value = "{ 'lastname' : ?0 }", delete = true)
 		void removeByLastname(String lastname);
 
+		@Query(value = "{ 'lastname' : ?0 }", delete = true, count = true)
+		void invalidMethod(String lastname);
 	}
 }