DATAMONGO-517 - Fixed complex keyword handling.

Introduced intermediate getMappedKeyword(Keyword keyword, MongoPersistentProperty property) to correctly return a DBObject for keyword plus converted value. A few refactorings and improvements in the implementation of QueryMapper (Keyword value object etc.).

spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011 by the original author(s).
+ * Copyright 2011-2012 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -19,7 +19,6 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 
-import org.bson.types.BasicBSONList;
 import org.bson.types.ObjectId;
 import org.springframework.core.convert.ConversionException;
 import org.springframework.core.convert.ConversionService;
@@ -35,11 +34,12 @@ import org.springframework.util.Assert;
 import com.mongodb.BasicDBList;
 import com.mongodb.BasicDBObject;
 import com.mongodb.DBObject;
+import com.mongodb.DBRef;
 
 /**
  * A helper class to encapsulate any modifications of a Query object before it gets submitted to the database.
  * 
- * @author Jon Brisbin <jbrisbin@vmware.com>
+ * @author Jon Brisbin
  * @author Oliver Gierke
  */
 public class QueryMapper {
@@ -75,8 +75,8 @@ public class QueryMapper {
 	 */
 	public DBObject getMappedObject(DBObject query, MongoPersistentEntity<?> entity) {
 
-		if (isKeyWord(query)) {
+		if (Keyword.isKeyword(query)) {
-			return getMappedKeyword(query, entity);
+			return getMappedKeyword(new Keyword(query), entity);
 		}
 
 		DBObject result = new BasicDBObject();
@@ -100,25 +100,38 @@ public class QueryMapper {
 	 * @param entity
 	 * @return
 	 */
-	private DBObject getMappedKeyword(DBObject query, MongoPersistentEntity<?> entity) {
+	private DBObject getMappedKeyword(Keyword query, MongoPersistentEntity<?> entity) {
-
-		String newKey = query.keySet().iterator().next();
-		Object value = query.get(newKey);
 
 		// $or/$nor
-		if (newKey.matches(N_OR_PATTERN)) {
+		if (query.key.matches(N_OR_PATTERN)) {
 
-			Iterable<?> conditions = (Iterable<?>) value;
+			Iterable<?> conditions = (Iterable<?>) query.value;
 			BasicDBList newConditions = new BasicDBList();
 
 			for (Object condition : conditions) {
 				newConditions.add(getMappedObject((DBObject) condition, entity));
 			}
 
-			return new BasicDBObject(newKey, newConditions);
+			return new BasicDBObject(query.key, newConditions);
+		}
+
+		return new BasicDBObject(query.key, convertSimpleOrDBObject(query.value, entity));
+	}
+
+	/**
+	 * Returns the mapped keyword considered defining a criteria for the given property.
+	 * 
+	 * @param keyword
+	 * @param property
+	 * @return
+	 */
+	public DBObject getMappedKeyword(Keyword keyword, MongoPersistentProperty property) {
+
+		if (property.isAssociation()) {
+			convertAssociation(keyword.value, property);
 		}
 
-		return new BasicDBObject(newKey, convertSimpleOrDBObject(value, entity));
+		return new BasicDBObject(keyword.key, getMappedValue(keyword.value, property, keyword.key));
 	}
 
 	/**
@@ -161,7 +174,7 @@ public class QueryMapper {
 		}
 
 		if (property.isAssociation()) {
-			return isKeyWord(source) ? getMappedValue(getKeywordValue(source), property, newKey) : convertAssociation(source,
+			return Keyword.isKeyword(source) ? getMappedKeyword(new Keyword(source), property) : convertAssociation(source,
 					property);
 		}
 
@@ -243,14 +256,14 @@ public class QueryMapper {
 		}
 
 		if (source instanceof Iterable) {
-			BasicBSONList result = new BasicBSONList();
+			BasicDBList result = new BasicDBList();
 			for (Object element : (Iterable<?>) source) {
-				result.add(converter.toDBRef(element, property));
+				result.add(element instanceof DBRef ? element : converter.toDBRef(element, property));
 			}
 			return result;
 		}
 
-		return converter.toDBRef(source, property);
+		return source instanceof DBRef ? source : converter.toDBRef(source, property);
 	}
 
 	/**
@@ -276,47 +289,59 @@ public class QueryMapper {
 	}
 
 	/**
-	 * Returns whether the given value is representing a query keyword.
+	 * Converts the given raw id value into either {@link ObjectId} or {@link String}.
 	 * 
-	 * @param value
+	 * @param id
 	 * @return
 	 */
-	private static boolean isKeyWord(Object value) {
+	public Object convertId(Object id) {
 
-		if (!(value instanceof DBObject) || value instanceof BasicDBList) {
+		try {
-			return false;
+			return conversionService.convert(id, ObjectId.class);
+		} catch (ConversionException e) {
+			// Ignore
 		}
 
-		DBObject dbObject = (DBObject) value;
+		return converter.convertToMongoType(id);
-		return dbObject.keySet().size() == 1 && dbObject.keySet().iterator().next().startsWith("$");
 	}
 
 	/**
-	 * Returns the value of the given source assuming it's a query keyword.
+	 * Value object to capture a query keyword representation.
 	 * 
-	 * @param source
+	 * @author Oliver Gierke
-	 * @return
 	 */
-	private static Object getKeywordValue(Object source) {
+	private static class Keyword {
+
+		String key;
+		Object value;
+
+		Keyword(Object source) {
 
-		DBObject dbObject = (DBObject) source;
+			Assert.isInstanceOf(DBObject.class, source);
-		return dbObject.get(dbObject.keySet().iterator().next());
+
+			DBObject value = (DBObject) source;
+
+			Assert.isTrue(value.keySet().size() == 1, "Keyword must have a single key only!");
+
+			this.key = value.keySet().iterator().next();
+			this.value = value.get(key);
 		}
 
 		/**
-	 * Converts the given raw id value into either {@link ObjectId} or {@link String}.
+		 * Returns whether the given value actually represents a keyword. If this returns {@literal true} it's safe to call
+		 * the constructor.
 		 * 
-	 * @param id
+		 * @param value
 		 * @return
 		 */
-	public Object convertId(Object id) {
+		static boolean isKeyword(Object value) {
 
-		try {
+			if (!(value instanceof DBObject)) {
-			return conversionService.convert(id, ObjectId.class);
+				return false;
-		} catch (ConversionException e) {
-			// Ignore
 			}
 
-		return converter.convertToMongoType(id);
+			DBObject dbObject = (DBObject) value;
+			return dbObject.keySet().size() == 1 && dbObject.keySet().iterator().next().startsWith("$");
+		}
 	}
 }

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

@@ -34,6 +34,7 @@ import org.mockito.Mock;
 import org.mockito.runners.MockitoJUnitRunner;
 import org.springframework.data.annotation.Id;
 import org.springframework.data.mongodb.MongoDbFactory;
+import org.springframework.data.mongodb.core.DBObjectUtils;
 import org.springframework.data.mongodb.core.Person;
 import org.springframework.data.mongodb.core.mapping.DBRef;
 import org.springframework.data.mongodb.core.mapping.Field;
@@ -318,6 +319,22 @@ public class QueryMapperUnitTests {
 		assertThat(referenceObject, is(instanceOf(com.mongodb.DBRef.class)));
 	}
 
+	@Test
+	public void convertsInKeywordCorrectly() {
+
+		Reference first = new Reference();
+		first.id = 5L;
+
+		Reference second = new Reference();
+		second.id = 6L;
+
+		Query query = query(where("reference").in(first, second));
+		DBObject result = mapper.getMappedObject(query.getQueryObject(), context.getPersistentEntity(WithDBRef.class));
+
+		DBObject reference = DBObjectUtils.getAsDBObject(result, "reference");
+		assertThat(reference.containsField("$in"), is(true));
+	}
+
 	class IdWrapper {
 		Object id;
 	}