DATAMONGO-990 - Polishing.

Removed EvaluationExpressionContext from all AbstractMongoQuery implementations that don't actually need it and from AbstractMongoQuery itself, too. Cleaned up test cases after that.

Moved SpEL related tests into AbstractPersonRepositoryIntegrationTests to make sure they're executed for all sub-types. JavaDoc and assertion polishes.

Original pull request: spring-projects#285.

Author: odrotbohm

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

@@ -31,7 +31,6 @@
 import org.springframework.data.mongodb.core.MongoOperations;
 import org.springframework.data.mongodb.core.query.NearQuery;
 import org.springframework.data.mongodb.core.query.Query;
-import org.springframework.data.repository.query.EvaluationContextProvider;
 import org.springframework.data.repository.query.ParameterAccessor;
 import org.springframework.data.repository.query.RepositoryQuery;
 import org.springframework.data.util.CloseableIterator;
@@ -52,25 +51,20 @@ public abstract class AbstractMongoQuery implements RepositoryQuery {
 
 	private final MongoQueryMethod method;
 	private final MongoOperations operations;
-	private final EvaluationContextProvider evaluationContextProvider;
 
 	/**
 	 * Creates a new {@link AbstractMongoQuery} from the given {@link MongoQueryMethod} and {@link MongoOperations}.
 	 * 
 	 * @param method must not be {@literal null}.
 	 * @param operations must not be {@literal null}.
-	 * @param evaluationContextProvider must not be {@literal null}.
 	 */
-	public AbstractMongoQuery(MongoQueryMethod method, MongoOperations operations,
-			EvaluationContextProvider evaluationContextProvider) {
+	public AbstractMongoQuery(MongoQueryMethod method, MongoOperations operations) {
 
-		Assert.notNull(operations);
-		Assert.notNull(method);
-		Assert.notNull(evaluationContextProvider, "ExpressionEvaluationContextProvider must not be null!");
+		Assert.notNull(operations, "MongoOperations must not be null!");
+		Assert.notNull(method, "MongoQueryMethod must not be null!");
 
 		this.method = method;
 		this.operations = operations;
-		this.evaluationContextProvider = evaluationContextProvider;
 	}
 
 	/* 
@@ -164,10 +158,6 @@ protected Query createCountQuery(ConvertingParameterAccessor accessor) {
 	 */
 	protected abstract boolean isDeleteQuery();
 
-	public EvaluationContextProvider getEvaluationContextProvider() {
-		return evaluationContextProvider;
-	}
-
 	private abstract class Execution {
 
 		abstract Object execute(Query query);
@@ -318,8 +308,8 @@ private SingleEntityExecution(boolean countProjection) {
 		Object execute(Query query) {
 
 			MongoEntityMetadata<?> metadata = method.getEntityInformation();
-			return countProjection ? operations.count(query, metadata.getJavaType()) : operations.findOne(query,
-					metadata.getJavaType(), metadata.getCollectionName());
+			return countProjection ? operations.count(query, metadata.getJavaType())
+					: operations.findOne(query, metadata.getJavaType(), metadata.getCollectionName());
 		}
 	}
 

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

@@ -217,7 +217,7 @@ public void remove() {
 	/**
 	 * Returns the given object as {@link Collection}. Will do a copy of it if it implements {@link Iterable} or is an
 	 * array. Will return an empty {@link Collection} in case {@literal null} is given. Will wrap all other types into a
-	 * single-element collction
+	 * single-element collection.
 	 * 
 	 * @param source
 	 * @return
@@ -240,8 +240,9 @@ private static Collection<?> asCollection(Object source) {
 
 		return source.getClass().isArray() ? CollectionUtils.arrayToList(source) : Collections.singleton(source);
 	}
-	
-	/* (non-Javadoc)
+
+	/*
+	 * (non-Javadoc)
 	 * @see org.springframework.data.mongodb.repository.query.MongoParameterAccessor#getValues()
 	 */
 	@Override

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

@@ -120,12 +120,13 @@ protected TextCriteria potentiallyConvertFullText(Object fullText) {
 			return ((TextCriteria) fullText);
 		}
 
-		throw new IllegalArgumentException(String.format(
-				"Expected full text parameter to be one of String, Term or TextCriteria but found %s.",
-				ClassUtils.getShortName(fullText.getClass())));
+		throw new IllegalArgumentException(
+				String.format("Expected full text parameter to be one of String, Term or TextCriteria but found %s.",
+						ClassUtils.getShortName(fullText.getClass())));
 	}
-	
-	/* (non-Javadoc)
+
+	/* 
+	 * (non-Javadoc)
 	 * @see org.springframework.data.mongodb.repository.query.MongoParameterAccessor#getValues()
 	 */
 	@Override

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

@@ -22,7 +22,6 @@
 import org.springframework.data.mongodb.core.query.BasicQuery;
 import org.springframework.data.mongodb.core.query.Query;
 import org.springframework.data.mongodb.core.query.TextCriteria;
-import org.springframework.data.repository.query.EvaluationContextProvider;
 import org.springframework.data.repository.query.QueryMethod;
 import org.springframework.data.repository.query.RepositoryQuery;
 import org.springframework.data.repository.query.parser.PartTree;
@@ -48,12 +47,10 @@ public class PartTreeMongoQuery extends AbstractMongoQuery {
 	 * 
 	 * @param method must not be {@literal null}.
 	 * @param mongoOperations must not be {@literal null}.
-	 * @param evaluationContextProvider must not be {@literal null}.
 	 */
-	public PartTreeMongoQuery(MongoQueryMethod method, MongoOperations mongoOperations,
-			EvaluationContextProvider evaluationContextProvider) {
+	public PartTreeMongoQuery(MongoQueryMethod method, MongoOperations mongoOperations) {
 
-		super(method, mongoOperations, evaluationContextProvider);
+		super(method, mongoOperations);
 		this.tree = new PartTree(method.getName(), method.getEntityInformation().getJavaType());
 		this.isGeoNearQuery = method.isGeoNearQuery();
 		this.context = mongoOperations.getConverter().getMappingContext();

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

@@ -57,36 +57,40 @@ public class StringBasedMongoQuery extends AbstractMongoQuery {
 	private final List<ParameterBinding> queryParameterBindings;
 	private final List<ParameterBinding> fieldSpecParameterBindings;
 	private final SpelExpressionParser expressionParser;
+	private final EvaluationContextProvider evaluationContextProvider;
 
 	/**
 	 * 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}.
-	 * @param evaluationContextProvider must not be {@literal null}.
 	 * @param expressionParser must not be {@literal null}.
+	 * @param evaluationContextProvider must not be {@literal null}.
 	 */
 	public StringBasedMongoQuery(MongoQueryMethod method, MongoOperations mongoOperations,
-			EvaluationContextProvider evaluationContextProvider, SpelExpressionParser expressionParser) {
-		this(method.getAnnotatedQuery(), method, mongoOperations, evaluationContextProvider, expressionParser);
+			SpelExpressionParser expressionParser, EvaluationContextProvider evaluationContextProvider) {
+		this(method.getAnnotatedQuery(), method, mongoOperations, expressionParser, evaluationContextProvider);
 	}
 
 	/**
-	 * Creates a new {@link StringBasedMongoQuery} for the given {@link String}, {@link MongoQueryMethod} and
-	 * {@link MongoOperations}.
-	 * 
+	 * Creates a new {@link StringBasedMongoQuery} for the given {@link String}, {@link MongoQueryMethod},
+	 * {@link MongoOperations}, {@link SpelExpressionParser} and {@link EvaluationContextProvider}.
+	 *
+	 * @param query must not be {@literal null}.
 	 * @param method must not be {@literal null}.
+	 * @param mongoOperations must not be {@literal null}.
 	 * @param expressionParser must not be {@literal null}.
-	 * @param template must not be {@literal null}.
 	 */
 	public StringBasedMongoQuery(String query, MongoQueryMethod method, MongoOperations mongoOperations,
-			EvaluationContextProvider evaluationContextProvider, SpelExpressionParser expressionParser) {
+			SpelExpressionParser expressionParser, EvaluationContextProvider evaluationContextProvider) {
 
-		super(method, mongoOperations, evaluationContextProvider);
+		super(method, mongoOperations);
 
+		Assert.notNull(query, "Query must not be null!");
 		Assert.notNull(expressionParser, "SpelExpressionParser must not be null!");
 
 		this.expressionParser = expressionParser;
+		this.evaluationContextProvider = evaluationContextProvider;
 
 		this.queryParameterBindings = new ArrayList<ParameterBinding>();
 		this.query = BINDING_PARSER.parseAndCollectParameterBindingsFromQueryIntoBindings(query,
@@ -157,7 +161,8 @@ protected boolean isDeleteQuery() {
 	 * @param bindings
 	 * @return
 	 */
-	private String replacePlaceholders(String input, ConvertingParameterAccessor accessor, List<ParameterBinding> bindings) {
+	private String replacePlaceholders(String input, ConvertingParameterAccessor accessor,
+			List<ParameterBinding> bindings) {
 
 		if (bindings.isEmpty()) {
 			return input;
@@ -187,12 +192,8 @@ private String replacePlaceholders(String input, ConvertingParameterAccessor acc
 	 */
 	private String getParameterValueForBinding(ConvertingParameterAccessor accessor, ParameterBinding binding) {
 
-		Object value = null;
-		if (binding.isExpression()) {
-			value = evaluateExpression(binding.getExpression(), accessor.getValues());
-		} else {
-			value = accessor.getBindableValue(binding.getParameterIndex());
-		}
+		Object value = binding.isExpression() ? evaluateExpression(binding.getExpression(), accessor.getValues())
+				: accessor.getBindableValue(binding.getParameterIndex());
 
 		if (value instanceof String && binding.isQuoted()) {
 			return (String) value;
@@ -210,8 +211,8 @@ private String getParameterValueForBinding(ConvertingParameterAccessor accessor,
 	 */
 	private Object evaluateExpression(String expressionString, Object[] parameterValues) {
 
-		EvaluationContext evaluationContext = getEvaluationContextProvider().getEvaluationContext(
-				getQueryMethod().getParameters(), parameterValues);
+		EvaluationContext evaluationContext = evaluationContextProvider
+				.getEvaluationContext(getQueryMethod().getParameters(), parameterValues);
 		Expression expression = expressionParser.parseExpression(expressionString);
 		return expression.getValue(evaluationContext, Object.class);
 	}
@@ -237,8 +238,8 @@ private static enum ParameterBindingParser {
 		 * Returns a list of {@link ParameterBinding}s found in the given {@code input} or an
 		 * {@link Collections#emptyList()}.
 		 * 
-		 * @param input
-		 * @param bindings
+		 * @param input can be {@literal null} or empty.
+		 * @param bindings must not be {@literal null}.
 		 * @return
 		 */
 		public String parseAndCollectParameterBindingsFromQueryIntoBindings(String input, List<ParameterBinding> bindings) {
@@ -247,37 +248,36 @@ public String parseAndCollectParameterBindingsFromQueryIntoBindings(String input
 				return input;
 			}
 
-			String transformedInput = transformQueryAndCollectExpressionParametersIntoBindings(bindings, input);
+			Assert.notNull(bindings, "Parameter bindings must not be null!");
 
+			String transformedInput = transformQueryAndCollectExpressionParametersIntoBindings(input, bindings);
 			String parseableInput = makeParameterReferencesParseable(transformedInput);
 
 			collectParameterReferencesIntoBindings(bindings, JSON.parse(parseableInput));
 
 			return transformedInput;
 		}
 
-		private String transformQueryAndCollectExpressionParametersIntoBindings(List<ParameterBinding> bindings,
-				String input) {
+		private String transformQueryAndCollectExpressionParametersIntoBindings(String input,
+				List<ParameterBinding> bindings) {
 
 			Matcher matcher = PARAMETER_EXPRESSION_PATTERN.matcher(input);
 
 			StringBuilder result = new StringBuilder();
 
 			int lastPos = 0;
-
 			int exprIndex = 0;
+
 			while (matcher.find()) {
 
 				int startOffSet = matcher.start();
-				result.append(input.subSequence(lastPos, startOffSet));
-
-				String expression = matcher.group(3);
 
+				result.append(input.subSequence(lastPos, startOffSet));
 				result.append("'?expr").append(exprIndex).append("'");
 
 				lastPos = matcher.end();
 
-				bindings.add(new ParameterBinding(exprIndex, true, expression));
+				bindings.add(new ParameterBinding(exprIndex, true, matcher.group(3)));
 
 				exprIndex++;
 			}
@@ -305,9 +305,10 @@ private void collectParameterReferencesIntoBindings(List<ParameterBinding> bindi
 			} else if (value instanceof Pattern) {
 
 				String string = ((Pattern) value).toString().trim();
-
 				Matcher valueMatcher = PARSEABLE_BINDING_PATTERN.matcher(string);
+
 				while (valueMatcher.find()) {
+
 					int paramIndex = Integer.parseInt(valueMatcher.group(PARAMETER_INDEX_GROUP));
 
 					/*

spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/MongoRepositoryFactory.java

@@ -15,7 +15,7 @@
  */
 package org.springframework.data.mongodb.repository.support;
 
-import static org.springframework.data.querydsl.QueryDslUtils.QUERY_DSL_PRESENT;
+import static org.springframework.data.querydsl.QueryDslUtils.*;
 
 import java.io.Serializable;
 import java.lang.reflect.Method;
@@ -51,7 +51,7 @@
 public class MongoRepositoryFactory extends RepositoryFactorySupport {
 
 	private static final SpelExpressionParser EXPRESSION_PARSER = new SpelExpressionParser();
-	
+
 	private final MongoOperations mongoOperations;
 	private final MappingContext<? extends MongoPersistentEntity<?>, MongoPersistentProperty> mappingContext;
 
@@ -92,8 +92,8 @@ protected Object getTargetRepository(RepositoryInformation information) {
 		return getTargetRepositoryViaReflection(information, entityInformation, mongoOperations);
 	}
 
-	
-	/* (non-Javadoc)
+	/* 
+	 * (non-Javadoc)
 	 * @see org.springframework.data.repository.core.support.RepositoryFactorySupport#getQueryLookupStrategy(org.springframework.data.repository.query.QueryLookupStrategy.Key, org.springframework.data.repository.query.EvaluationContextProvider)
 	 */
 	@Override
@@ -112,8 +112,8 @@ public <T, ID extends Serializable> MongoEntityInformation<T, ID> getEntityInfor
 		MongoPersistentEntity<?> entity = mappingContext.getPersistentEntity(domainClass);
 
 		if (entity == null) {
-			throw new MappingException(String.format("Could not lookup mapping metadata for domain class %s!",
-					domainClass.getName()));
+			throw new MappingException(
+					String.format("Could not lookup mapping metadata for domain class %s!", domainClass.getName()));
 		}
 
 		return new MappingMongoEntityInformation<T, ID>((MongoPersistentEntity<T>) entity);
@@ -144,11 +144,12 @@ public RepositoryQuery resolveQuery(Method method, RepositoryMetadata metadata,
 
 			if (namedQueries.hasQuery(namedQueryName)) {
 				String namedQuery = namedQueries.getQuery(namedQueryName);
-				return new StringBasedMongoQuery(namedQuery, queryMethod, mongoOperations, evaluationContextProvider, EXPRESSION_PARSER);
+				return new StringBasedMongoQuery(namedQuery, queryMethod, mongoOperations, EXPRESSION_PARSER,
+						evaluationContextProvider);
 			} else if (queryMethod.hasAnnotatedQuery()) {
-				return new StringBasedMongoQuery(queryMethod, mongoOperations, evaluationContextProvider, EXPRESSION_PARSER);
+				return new StringBasedMongoQuery(queryMethod, mongoOperations, EXPRESSION_PARSER, evaluationContextProvider);
 			} else {
-				return new PartTreeMongoQuery(queryMethod, mongoOperations, evaluationContextProvider);
+				return new PartTreeMongoQuery(queryMethod, mongoOperations);
 			}
 		}
 	}