DATADOC-114 - Fix bug in Update class that wasn't converting POJOs properly when being used from updateFirst/Multi

spring-data-document-core/template.mf

@@ -30,6 +30,7 @@ Import-Template:
  org.springframework.expression.spel.support.*;version="[3.0.0, 4.0.0)",
  org.springframework.validation.*;version="[3.0.0, 4.0.0)",
  org.springframework.data.core.*;version="[1.0.0, 2.0.0)",
+ org.springframework.context.support.*;version="[1.0.0, 2.0.0)",
  org.aopalliance.*;version="[1.0.0, 2.0.0)";resolution:=optional,
  org.apache.commons.logging.*;version="[1.1.1, 2.0.0)",
  org.w3c.dom.*;version="0",

spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/MongoTemplate.java

@@ -25,10 +25,12 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
+import com.mongodb.BasicDBList;
 import com.mongodb.BasicDBObject;
 import com.mongodb.CommandResult;
 import com.mongodb.DB;
@@ -155,6 +157,7 @@ public class MongoTemplate implements InitializingBean, MongoOperations, Applica
 	 * @param writeConcern
 	 * @param writeResultChecking
 	 */
+	@SuppressWarnings({"unchecked"})
 	MongoTemplate(Mongo mongo, String databaseName, String defaultCollectionName, MongoConverter mongoConverter, WriteConcern writeConcern, WriteResultChecking writeResultChecking) {
 
 		Assert.notNull(mongo);
@@ -165,24 +168,24 @@ public class MongoTemplate implements InitializingBean, MongoOperations, Applica
 		this.databaseName = databaseName;
 		this.writeConcern = writeConcern;
 		this.mongoConverter = mongoConverter == null ? getDefaultMongoConverter() : mongoConverter;
-		
+
 		if (this.mongoConverter instanceof MappingMongoConverter) {
-		  initializeMappingMongoConverter((MappingMongoConverter) this.mongoConverter);
+			initializeMappingMongoConverter((MappingMongoConverter) this.mongoConverter);
 		}
-		
+
 		this.mappingContext = this.mongoConverter.getMappingContext();
 		this.mapper = new QueryMapper(this.mongoConverter);
-		
+
 		if (writeResultChecking != null) {
 			this.writeResultChecking = writeResultChecking;
 		}
 	}
-	
+
 	private final MongoConverter getDefaultMongoConverter() {
-	  
-	  SimpleMongoConverter converter = new SimpleMongoConverter();
-	  converter.afterPropertiesSet();
-	  return converter;
+
+		SimpleMongoConverter converter = new SimpleMongoConverter();
+		converter.afterPropertiesSet();
+		return converter;
 	}
 
 	public void setApplicationEventPublisher(ApplicationEventPublisher applicationEventPublisher) {
@@ -612,21 +615,21 @@ public class MongoTemplate implements InitializingBean, MongoOperations, Applica
 			*/
 	public <T> void insertList(List<? extends T> listToSave, MongoWriter<T> writer) {
 		Map<String, List<Object>> objs = new HashMap<String, List<Object>>();
-		
+
 		for (Object o : listToSave) {
-		  
+
 			MongoPersistentEntity<?> entity = mappingContext.getPersistentEntity(o.getClass());
 			String collection = entity == null ? getDefaultCollectionName() : entity.getCollection();
-			
+
 			List<Object> objList = objs.get(collection);
 			if (null == objList) {
 				objList = new ArrayList<Object>();
 				objs.put(collection, objList);
 			}
 			objList.add(o);
-			
+
 		}
-		
+
 		for (Map.Entry<String, List<Object>> entry : objs.entrySet()) {
 			insertList(entry.getKey(), entry.getValue());
 		}
@@ -814,17 +817,19 @@ public class MongoTemplate implements InitializingBean, MongoOperations, Applica
 			*/
 	public WriteResult updateFirst(String collectionName, final Query query, final Update update) {
 		if (LOGGER.isDebugEnabled()) {
-			LOGGER.debug("calling update using query: " + query.getQueryObject() + " and update: " + update.getUpdateObject() + " in collecion: " + collectionName);
+			LOGGER.debug("calling update using query: " + query.getQueryObject() + " and update: " + update.getUpdateObject(mongoConverter) + " in collecion: " + collectionName);
 		}
 		return execute(collectionName, new CollectionCallback<WriteResult>() {
 			public WriteResult doInCollection(DBCollection collection) throws MongoException, DataAccessException {
+				DBObject updateObj = update.getUpdateObject(mongoConverter);
+
 				WriteResult wr;
 				if (writeConcern == null) {
-					wr = collection.update(query.getQueryObject(), update.getUpdateObject());
+					wr = collection.update(query.getQueryObject(), updateObj);
 				} else {
-					wr = collection.update(query.getQueryObject(), update.getUpdateObject(), false, false, writeConcern);
+					wr = collection.update(query.getQueryObject(), updateObj, false, false, writeConcern);
 				}
-				handleAnyWriteResultErrors(wr, query.getQueryObject(), "update with '" + update.getUpdateObject() + "'");
+				handleAnyWriteResultErrors(wr, query.getQueryObject(), "update with '" + updateObj + "'");
 				return wr;
 			}
 		});
@@ -842,17 +847,18 @@ public class MongoTemplate implements InitializingBean, MongoOperations, Applica
 			*/
 	public WriteResult updateMulti(String collectionName, final Query query, final Update update) {
 		if (LOGGER.isDebugEnabled()) {
-			LOGGER.debug("calling updateMulti using query: " + query.getQueryObject() + " and update: " + update.getUpdateObject() + " in collecion: " + collectionName);
+			LOGGER.debug("calling updateMulti using query: " + query.getQueryObject() + " and update: " + update.getUpdateObject(mongoConverter) + " in collecion: " + collectionName);
 		}
 		return execute(collectionName, new CollectionCallback<WriteResult>() {
 			public WriteResult doInCollection(DBCollection collection) throws MongoException, DataAccessException {
-				WriteResult wr = null;
+				DBObject updateObj = update.getUpdateObject(mongoConverter);
+				WriteResult wr;
 				if (writeConcern == null) {
-					wr = collection.updateMulti(query.getQueryObject(), update.getUpdateObject());
+					wr = collection.updateMulti(query.getQueryObject(), updateObj);
 				} else {
-					wr = collection.update(query.getQueryObject(), update.getUpdateObject(), false, true, writeConcern);
+					wr = collection.update(query.getQueryObject(), updateObj, false, true, writeConcern);
 				}
-				handleAnyWriteResultErrors(wr, query.getQueryObject(), "update with '" + update.getUpdateObject() + "'");
+				handleAnyWriteResultErrors(wr, query.getQueryObject(), "update with '" + updateObj + "'");
 				return wr;
 			}
 		});
@@ -871,12 +877,12 @@ public class MongoTemplate implements InitializingBean, MongoOperations, Applica
 	}
 
 	public <T> void remove(Query query, Class<T> targetClass) {
-	  Assert.notNull(query);
+		Assert.notNull(query);
 		remove(getEntityCollection(targetClass), query, targetClass);
 	}
-	
+
 	private PersistentEntity<?> getPersistentEntity(Class<?> type) {
-	  return type == null ? null : mappingContext.getPersistentEntity(type);
+		return type == null ? null : mappingContext.getPersistentEntity(type);
 	}
 
 	public <T> void remove(String collectionName, final Query query, Class<T> targetClass) {
@@ -983,7 +989,7 @@ public class MongoTemplate implements InitializingBean, MongoOperations, Applica
 		}
 		PersistentEntity<?> entity = mappingContext.getPersistentEntity(targetClass);
 		DBObject mappedQuery = mapper.getMappedObject(query, entity);
-		
+
 		return execute(new FindOneCallback(mappedQuery, fields),
 				new ReadDbObjectCallback<T>(readerToUse, targetClass),
 				collectionName);
@@ -1085,14 +1091,14 @@ public class MongoTemplate implements InitializingBean, MongoOperations, Applica
 	}
 
 	protected Object getIdValue(Object object) {
-		
+
 		PersistentEntity<?> entity = mappingContext.getPersistentEntity(object.getClass());
 		PersistentProperty idProp = entity.getIdProperty();
-		
+
 		if (idProp == null) {
-		  throw new MappingException("No id property found for object of type " + entity.getType().getName());
+			throw new MappingException("No id property found for object of type " + entity.getType().getName());
 		}
-		
+
 		try {
 			return MappingBeanHelper.getProperty(object, idProp, Object.class, true);
 		} catch (IllegalAccessException e) {
@@ -1117,9 +1123,9 @@ public class MongoTemplate implements InitializingBean, MongoOperations, Applica
 		PersistentProperty idProp = getIdPropertyFor(savedObject.getClass());
 
 		if (idProp == null) {
-		  return;
+			return;
 		}
-		
+
 		try {
 			MappingBeanHelper.setProperty(savedObject, idProp, id);
 			return;
@@ -1130,9 +1136,9 @@ public class MongoTemplate implements InitializingBean, MongoOperations, Applica
 		}
 	}
 
-  private PersistentProperty getIdPropertyFor(Class<?> type) {
-    return mappingContext.getPersistentEntity(type).getIdProperty();
-  }
+	private PersistentProperty getIdPropertyFor(Class<?> type) {
+		return mappingContext.getPersistentEntity(type).getIdProperty();
+	}
 
 	/**
 	 * Substitutes the id key if it is found in he query. Any 'id' keys will be replaced with '_id' and the value converted
@@ -1147,11 +1153,9 @@ public class MongoTemplate implements InitializingBean, MongoOperations, Applica
 		MongoConverter converter = null;
 		if (reader instanceof SimpleMongoConverter) {
 			converter = (MongoConverter) reader;
-		}
-		else if (reader instanceof MappingMongoConverter) {
+		} else if (reader instanceof MappingMongoConverter) {
 			converter = (MappingMongoConverter) reader;
-		}
-		else {
+		} else {
 			return;
 		}
 		String idKey = null;
@@ -1187,7 +1191,7 @@ public class MongoTemplate implements InitializingBean, MongoOperations, Applica
 				if (dbo.containsField("$in")) {
 					List<Object> ids = new ArrayList<Object>();
 					int count = 0;
-					for (Object o : (Object[])dbo.get("$in")) {
+					for (Object o : (Object[]) dbo.get("$in")) {
 						count++;
 						ObjectId newValue = convertIdValue(converter, o);
 						if (newValue != null) {
@@ -1196,7 +1200,7 @@ public class MongoTemplate implements InitializingBean, MongoOperations, Applica
 					}
 					if (ids.size() > 0 && ids.size() != count) {
 						throw new InvalidDataAccessApiUsageException("Inconsistent set of id values provided " +
-								Arrays.asList((Object[])dbo.get("$in")));
+								Arrays.asList((Object[]) dbo.get("$in")));
 					}
 					if (ids.size() > 0) {
 						dbo.removeField("$in");
@@ -1205,8 +1209,7 @@ public class MongoTemplate implements InitializingBean, MongoOperations, Applica
 				}
 				query.removeField(idKey);
 				query.put(MongoPropertyDescriptor.ID_KEY, value);
-			}
-			else {
+			} else {
 				ObjectId newValue = convertIdValue(converter, value);
 				query.removeField(idKey);
 				if (newValue != null) {
@@ -1248,11 +1251,11 @@ public class MongoTemplate implements InitializingBean, MongoOperations, Applica
 	}
 
 	private String getEntityCollection(Class<?> clazz) {
-	  
-	  if (clazz == null) {
-	    return getDefaultCollectionName();
-	  }
-		
+
+		if (clazz == null) {
+			return getDefaultCollectionName();
+		}
+
 		MongoPersistentEntity<?> entity = mappingContext.getPersistentEntity(clazz);
 		return entity.getCollection();
 	}

spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/query/BasicUpdate.java

@@ -20,6 +20,7 @@ import java.util.Collections;
 import com.mongodb.BasicDBObject;
 import com.mongodb.DBObject;
 import com.mongodb.util.JSON;
+import org.springframework.data.document.mongodb.convert.MongoConverter;
 
 public class BasicUpdate extends Update {
 

spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/query/Query.java

@@ -22,101 +22,96 @@ import com.mongodb.DBObject;
 
 public class Query {
 
-  private LinkedHashMap<String, CriteriaDefinition> criteria = new LinkedHashMap<String, CriteriaDefinition>();
+	private LinkedHashMap<String, CriteriaDefinition> criteria = new LinkedHashMap<String, CriteriaDefinition>();
+	private Field fieldSpec;
+	private Sort sort;
+	private int skip;
+	private int limit;
 
-  private Field fieldSpec;
+	/**
+	 * Static factory method to create a Query using the provided criteria
+	 *
+	 * @param critera
+	 * @return
+	 */
+	public static Query query(Criteria critera) {
+		return new Query(critera);
+	}
 
-  private Sort sort;
+	public Query() {
+	}
 
-  private int skip;
+	public Query(Criteria criteria) {
+		addCriteria(criteria);
+	}
 
-  private int limit;
+	public Query addCriteria(Criteria criteria) {
+		this.criteria.put(criteria.getKey(), criteria);
+		return this;
+	}
 
+	public Query or(Query... queries) {
+		this.criteria.put("$or", new OrCriteria(queries));
+		return this;
+	}
 
-  /**
-   * Static factory method to create a Query using the provided criteria
-   *
-   * @param key
-   * @return
-   */
-  public static Query query(Criteria critera) {
-    return new Query(critera);
-  }
-  
-  public Query() {
-  }
+	public Field fields() {
+		synchronized (this) {
+			if (fieldSpec == null) {
+				this.fieldSpec = new Field();
+			}
+		}
+		return this.fieldSpec;
+	}
 
-  public Query(Criteria criteria) {
-    addCriteria(criteria);
-  }
+	public Query skip(int skip) {
+		this.skip = skip;
+		return this;
+	}
 
-  public Query addCriteria(Criteria criteria) {
-    this.criteria.put(criteria.getKey(), criteria);
-    return this;
-  }
+	public Query limit(int limit) {
+		this.limit = limit;
+		return this;
+	}
 
-  public Query or(Query... queries) {
-    this.criteria.put("$or", new OrCriteria(queries));
-    return this;
-  }
+	public Sort sort() {
+		synchronized (this) {
+			if (this.sort == null) {
+				this.sort = new Sort();
+			}
+		}
+		return this.sort;
+	}
 
-  public Field fields() {
-    synchronized (this) {
-      if (fieldSpec == null) {
-        this.fieldSpec = new Field();
-      }
-    }
-    return this.fieldSpec;
-  }
+	public DBObject getQueryObject() {
+		DBObject dbo = new BasicDBObject();
+		for (String k : criteria.keySet()) {
+			CriteriaDefinition c = criteria.get(k);
+			DBObject cl = c.getCriteriaObject();
+			dbo.putAll(cl);
+		}
+		return dbo;
+	}
 
-  public Query skip(int skip) {
-    this.skip = skip;
-    return this;
-  }
+	public DBObject getFieldsObject() {
+		if (this.fieldSpec == null) {
+			return null;
+		}
+		return fieldSpec.getFieldsObject();
+	}
 
-  public Query limit(int limit) {
-    this.limit = limit;
-    return this;
-  }
+	public DBObject getSortObject() {
+		if (this.sort == null) {
+			return null;
+		}
+		return this.sort.getSortObject();
+	}
 
-  public Sort sort() {
-    synchronized (this) {
-      if (this.sort == null) {
-        this.sort = new Sort();
-      }
-    }
-    return this.sort;
-  }
+	public int getSkip() {
+		return this.skip;
+	}
 
-  public DBObject getQueryObject() {
-    DBObject dbo = new BasicDBObject();
-    for (String k : criteria.keySet()) {
-      CriteriaDefinition c = criteria.get(k);
-      DBObject cl = c.getCriteriaObject();
-      dbo.putAll(cl);
-    }
-    return dbo;
-  }
-
-  public DBObject getFieldsObject() {
-    if (this.fieldSpec == null) {
-      return null;
-    }
-    return fieldSpec.getFieldsObject();
-  }
-
-  public DBObject getSortObject() {
-    if (this.sort == null) {
-      return null;
-    }
-    return this.sort.getSortObject();
-  }
-
-  public int getSkip() {
-    return this.skip;
-  }
-
-  public int getLimit() {
-    return this.limit;
-  }
+	public int getLimit() {
+		return this.limit;
+	}
 }

spring-data-mongodb/src/main/java/org/springframework/data/document/mongodb/query/Update.java

@@ -16,12 +16,16 @@
 package org.springframework.data.document.mongodb.query;
 
 import java.util.HashMap;
+import java.util.Iterator;
 import java.util.LinkedHashMap;
+import java.util.Map;
 
-import org.springframework.dao.InvalidDataAccessApiUsageException;
-
+import com.mongodb.BasicDBList;
 import com.mongodb.BasicDBObject;
 import com.mongodb.DBObject;
+import org.springframework.dao.InvalidDataAccessApiUsageException;
+import org.springframework.data.document.mongodb.convert.MongoConverter;
+import org.springframework.data.mapping.MappingBeanHelper;
 
 public class Update {
 
@@ -33,7 +37,7 @@ public class Update {
 
 	/**
 	 * Static factory method to create an Update using the provided key
-	 * 
+	 *
 	 * @param key
 	 * @return
 	 */
@@ -43,7 +47,7 @@ public class Update {
 
 	/**
 	 * Update using the $set update modifier
-	 * 
+	 *
 	 * @param key
 	 * @param value
 	 * @return
@@ -55,7 +59,7 @@ public class Update {
 
 	/**
 	 * Update using the $unset update modifier
-	 * 
+	 *
 	 * @param key
 	 * @return
 	 */
@@ -66,7 +70,7 @@ public class Update {
 
 	/**
 	 * Update using the $inc update modifier
-	 * 
+	 *
 	 * @param key
 	 * @param inc
 	 * @return
@@ -78,7 +82,7 @@ public class Update {
 
 	/**
 	 * Update using the $push update modifier
-	 * 
+	 *
 	 * @param key
 	 * @param value
 	 * @return
@@ -90,7 +94,7 @@ public class Update {
 
 	/**
 	 * Update using the $pushAll update modifier
-	 * 
+	 *
 	 * @param key
 	 * @param values
 	 * @return
@@ -108,7 +112,7 @@ public class Update {
 
 	/**
 	 * Update using the $addToSet update modifier
-	 * 
+	 *
 	 * @param key
 	 * @param value
 	 * @return
@@ -120,20 +124,20 @@ public class Update {
 
 	/**
 	 * Update using the $pop update modifier
-	 * 
+	 *
 	 * @param key
 	 * @param pos
 	 * @return
 	 */
 	public Update pop(String key, Position pos) {
-		addMultiFieldOperation("$pop", key, 
-						(pos == Position.FIRST ? -1 : 1));
+		addMultiFieldOperation("$pop", key,
+				(pos == Position.FIRST ? -1 : 1));
 		return this;
 	}
 
 	/**
 	 * Update using the $pull update modifier
-	 * 
+	 *
 	 * @param key
 	 * @param value
 	 * @return
@@ -145,7 +149,7 @@ public class Update {
 
 	/**
 	 * Update using the $pullAll update modifier
-	 * 
+	 *
 	 * @param key
 	 * @param values
 	 * @return
@@ -163,7 +167,7 @@ public class Update {
 
 	/**
 	 * Update using the $rename update modifier
-	 * 
+	 *
 	 * @param oldName
 	 * @param newName
 	 * @return
@@ -173,17 +177,26 @@ public class Update {
 		return this;
 	}
 
-	public DBObject getUpdateObject() {
+	public DBObject getUpdateObject(MongoConverter converter) {
 		DBObject dbo = new BasicDBObject();
 		for (String k : modifierOps.keySet()) {
-			dbo.put(k, modifierOps.get(k));
+			Object o = modifierOps.get(k);
+			if (null != converter) {
+				dbo.put(k, maybeConvertObject(o, converter));
+			} else {
+				dbo.put(k, o);
+			}
 		}
 		return dbo;
 	}
 
+	public DBObject getUpdateObject() {
+		return getUpdateObject(null);
+	}
+
 	@SuppressWarnings("unchecked")
 	protected void addMultiFieldOperation(String operator, String key,
-			Object value) {
+																				Object value) {
 		Object existingValue = this.modifierOps.get(operator);
 		LinkedHashMap<String, Object> keyValueMap;
 		if (existingValue == null) {
@@ -192,9 +205,8 @@ public class Update {
 		} else {
 			if (existingValue instanceof LinkedHashMap) {
 				keyValueMap = (LinkedHashMap<String, Object>) existingValue;
-			}
-			else {
-				throw new InvalidDataAccessApiUsageException("Modifier Operations should be a LinkedHashMap but was " + 
+			} else {
+				throw new InvalidDataAccessApiUsageException("Modifier Operations should be a LinkedHashMap but was " +
 						existingValue.getClass());
 			}
 		}
@@ -208,4 +220,47 @@ public class Update {
 		return value;
 	}
 
+	@SuppressWarnings({"unchecked"})
+	protected Object maybeConvertObject(Object obj, MongoConverter converter) {
+		if (null != obj && MappingBeanHelper.isSimpleType(obj.getClass())) {
+			// Doesn't need conversion
+			return obj;
+		}
+
+		if (obj instanceof Map) {
+			Map<Object, Object> m = new HashMap<Object, Object>();
+			for (Map.Entry<Object, Object> entry : ((Map<Object, Object>) obj).entrySet()) {
+				m.put(entry.getKey(), maybeConvertObject(entry.getValue(), converter));
+			}
+			return m;
+		}
+
+		if (obj instanceof BasicDBList) {
+			return maybeConvertList((BasicDBList) obj, converter);
+		}
+
+		if (obj instanceof DBObject) {
+			DBObject newValueDbo = new BasicDBObject();
+			for (String vk : ((DBObject) obj).keySet()) {
+				Object o = ((DBObject) obj).get(vk);
+				newValueDbo.put(vk, maybeConvertObject(o, converter));
+			}
+			return newValueDbo;
+		}
+
+		DBObject newDbo = new BasicDBObject();
+		converter.write(obj, newDbo);
+		return newDbo;
+	}
+
+	protected BasicDBList maybeConvertList(BasicDBList dbl, MongoConverter converter) {
+		BasicDBList newDbl = new BasicDBList();
+		Iterator iter = dbl.iterator();
+		while (iter.hasNext()) {
+			Object o = iter.next();
+			newDbl.add(maybeConvertObject(o, converter));
+		}
+		return newDbl;
+	}
+
 }

spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/Address.java

@@ -19,7 +19,7 @@ package org.springframework.data.document.mongodb.mapping;
 /**
  * @author Jon Brisbin <jbrisbin@vmware.com>
  */
-public class Address {
+public class Address implements Comparable<Address> {
 
   private String id;
   private String[] lines;
@@ -67,4 +67,8 @@ public class Address {
   public void setCountry(String country) {
     this.country = country;
   }
+
+	public int compareTo(Address address) {
+		return 0;
+	}
 }

spring-data-mongodb/src/test/java/org/springframework/data/document/mongodb/mapping/MappingTests.java

@@ -18,6 +18,9 @@ package org.springframework.data.document.mongodb.mapping;
 
 import static org.hamcrest.Matchers.*;
 import static org.junit.Assert.*;
+import static org.springframework.data.document.mongodb.query.Criteria.*;
+import static org.springframework.data.document.mongodb.query.Query.*;
+import static org.springframework.data.document.mongodb.query.Update.*;
 
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -318,4 +321,23 @@ public class MappingTests {
 		assertNotNull(p.getId());
 	}
 
+	@SuppressWarnings({"unchecked"})
+	@Test
+	public void testQueryUpdate() {
+		Address addr = new Address();
+		addr.setLines(new String[]{"1234 W. 1st Street", "Apt. 12"});
+		addr.setCity("Anytown");
+		addr.setPostalCode(12345);
+		addr.setCountry("USA");
+
+		Person p = new Person(1111, "Query", "Update", 37, addr);
+		template.insert(p);
+
+		addr.setCity("New Town");
+		template.updateFirst(query(where("ssn").is(1111)), update("address", addr));
+
+		Person p2 = template.findOne(query(where("ssn").is(1111)), Person.class);
+		assertThat(p2.getAddress().getCity(), is("New Town"));
+	}
+
 }