DATAMONGO-2155 - Bypass mapping for already mapped updates.

We now make sure that mapped updates (as in doSaveVersioned and doUpdate) are mapped only once as mapping is required only once. Mapping already mapped query/update objects comes with undesired side-effects such as following invalid property paths or reduction of type information availability.

We now make sure to map key/value pairs of Map like properties to the values domain type, and apply potentially registered custom converters to the keys.
Fixed invalid test for DATAMONGO-1423 as this one did not check the application of the registered converter.

Original pull request: #625.

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

@@ -82,7 +82,21 @@ public class MappedDocument {
 		return Filters.eq(ID_FIELD, document.get(ID_FIELD));
 	}
 
-	public Update updateWithoutId() {
+	public MappedUpdate updateWithoutId() {
-		return Update.fromDocument(document, ID_FIELD);
+		return new MappedUpdate(Update.fromDocument(document, ID_FIELD));
+	}
+
+	public class MappedUpdate extends Update {
+
+		private final Update delegate;
+
+		public MappedUpdate(Update delegate) {
+			this.delegate = delegate;
+		}
+
+		@Override
+		public Document getUpdateObject() {
+			return delegate.getUpdateObject();
+		}
 	}
 }

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

@@ -60,6 +60,7 @@ import org.springframework.data.mongodb.SessionSynchronization;
 import org.springframework.data.mongodb.core.BulkOperations.BulkMode;
 import org.springframework.data.mongodb.core.DefaultBulkOperations.BulkOperationContext;
 import org.springframework.data.mongodb.core.EntityOperations.AdaptibleEntity;
+import org.springframework.data.mongodb.core.MappedDocument.MappedUpdate;
 import org.springframework.data.mongodb.core.aggregation.Aggregation;
 import org.springframework.data.mongodb.core.aggregation.AggregationOperationContext;
 import org.springframework.data.mongodb.core.aggregation.AggregationOptions;
@@ -1384,7 +1385,7 @@ public class MongoTemplate implements MongoOperations, ApplicationContextAware,
 		MappedDocument mapped = source.toMappedDocument(mongoConverter);
 
 		maybeEmitEvent(new BeforeSaveEvent<>(toSave, mapped.getDocument(), collectionName));
-		Update update = mapped.updateWithoutId();
+		MappedUpdate update = mapped.updateWithoutId();
 
 		UpdateResult result = doUpdate(collectionName, query, update, toSave.getClass(), false, false);
 
@@ -1580,7 +1581,7 @@ public class MongoTemplate implements MongoOperations, ApplicationContextAware,
 					query.getCollation().map(Collation::toMongoCollation).ifPresent(opts::collation);
 				}
 
-				Document updateObj = updateMapper.getMappedObject(update.getUpdateObject(), entity);
+				Document updateObj =  update instanceof MappedUpdate ? update.getUpdateObject() : updateMapper.getMappedObject(update.getUpdateObject(), entity);
 
 				if (multi && update.isIsolated() && !queryObj.containsKey("$isolated")) {
 					queryObj.put("$isolated", 1);

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

@@ -20,7 +20,9 @@ import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.Iterator;
+import java.util.LinkedHashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Optional;
 import java.util.Set;
@@ -35,6 +37,7 @@ import org.springframework.data.domain.Example;
 import org.springframework.data.mapping.Association;
 import org.springframework.data.mapping.MappingException;
 import org.springframework.data.mapping.PersistentEntity;
+import org.springframework.data.mapping.PersistentProperty;
 import org.springframework.data.mapping.PersistentPropertyPath;
 import org.springframework.data.mapping.PropertyPath;
 import org.springframework.data.mapping.PropertyReferenceException;
@@ -50,6 +53,7 @@ import org.springframework.data.util.ClassTypeInformation;
 import org.springframework.data.util.TypeInformation;
 import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
+import org.springframework.util.ObjectUtils;
 import org.springframework.util.StringUtils;
 
 import com.mongodb.BasicDBList;
@@ -449,6 +453,23 @@ public class QueryMapper {
 			return source;
 		}
 
+		if (source instanceof Map) {
+
+			LinkedHashMap<String, Object> map = new LinkedHashMap<>();
+
+			((Map<String, Object>) source).entrySet().forEach(it -> {
+
+				String key = ObjectUtils.nullSafeToString(converter.convertToMongoType(it.getKey()));
+
+				if (it.getValue() instanceof Document) {
+					map.put(key, getMappedObject((Document) it.getValue(), entity));
+				} else {
+					map.put(key, delegateConvertToMongoType(it.getValue(), entity));
+				}
+			});
+			return map;
+		}
+
 		return delegateConvertToMongoType(source, entity);
 	}
 
@@ -816,13 +837,24 @@ public class QueryMapper {
 			return null;
 		}
 
+		/**
+		 * Returns whether the field references a {@link java.util.Map}.
+		 * 
+		 * @return {@literal true} if property information is available and references a {@link java.util.Map}.
+		 * @see PersistentProperty#isMap()
+		 * @since 2.2
+		 */
+		public boolean isMap() {
+			return getProperty() != null && getProperty().isMap();
+		}
+
 		public TypeInformation<?> getTypeHint() {
 			return ClassTypeInformation.OBJECT;
 		}
 	}
 
 	/**
-	 * Extension of {@link DocumentField} to be backed with mapping metadata.
+	 * Extension of {@link Field} to be backed with mapping metadata.
 	 *
 	 * @author Oliver Gierke
 	 * @author Thomas Darimont

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

@@ -16,6 +16,7 @@
 package org.springframework.data.mongodb.core.convert;
 
 import java.util.Collection;
+import java.util.LinkedHashMap;
 import java.util.Map.Entry;
 
 import org.bson.Document;
@@ -143,7 +144,9 @@ public class UpdateMapper extends QueryMapper {
 	protected Entry<String, Object> getMappedObjectForField(Field field, Object rawValue) {
 
 		if (isDocument(rawValue)) {
-			return createMapEntry(field, convertSimpleOrDocument(rawValue, field.getPropertyEntity()));
+
+			Object val = field.isMap() ? new LinkedHashMap<>((Document) rawValue) : rawValue; // unwrap to preserve field type
+			return createMapEntry(field, convertSimpleOrDocument(val, field.getPropertyEntity()));
 		}
 
 		if (isQuery(rawValue)) {

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

@@ -22,7 +22,6 @@ import static org.mockito.Mockito.any;
 import static org.springframework.data.mongodb.core.aggregation.Aggregation.*;
 import static org.springframework.data.mongodb.test.util.IsBsonObject.*;
 
-import com.mongodb.MongoNamespace;
 import lombok.Data;
 
 import java.math.BigInteger;
@@ -82,6 +81,7 @@ import org.springframework.test.util.ReflectionTestUtils;
 import com.mongodb.DB;
 import com.mongodb.MongoClient;
 import com.mongodb.MongoException;
+import com.mongodb.MongoNamespace;
 import com.mongodb.ReadPreference;
 import com.mongodb.client.AggregateIterable;
 import com.mongodb.client.FindIterable;
@@ -118,6 +118,7 @@ public class MongoTemplateUnitTests extends MongoOperationsUnitTests {
 	@Mock FindIterable<Document> findIterable;
 	@Mock AggregateIterable aggregateIterable;
 	@Mock MapReduceIterable mapReduceIterable;
+	@Mock UpdateResult updateResult;
 
 	Document commandResultDocument = new Document();
 
@@ -139,6 +140,7 @@ public class MongoTemplateUnitTests extends MongoOperationsUnitTests {
 		when(collection.getNamespace()).thenReturn(new MongoNamespace("db.mock-collection"));
 		when(collection.aggregate(any(List.class), any())).thenReturn(aggregateIterable);
 		when(collection.withReadPreference(any())).thenReturn(collection);
+		when(collection.replaceOne(any(), any(), any(ReplaceOptions.class))).thenReturn(updateResult);
 		when(findIterable.projection(any())).thenReturn(findIterable);
 		when(findIterable.sort(any(org.bson.Document.class))).thenReturn(findIterable);
 		when(findIterable.collation(any())).thenReturn(findIterable);
@@ -157,7 +159,7 @@ public class MongoTemplateUnitTests extends MongoOperationsUnitTests {
 		when(aggregateIterable.into(any())).thenReturn(Collections.emptyList());
 
 		this.mappingContext = new MongoMappingContext();
-		this.converter = new MappingMongoConverter(new DefaultDbRefResolver(factory), mappingContext);
+		this.converter = spy(new MappingMongoConverter(new DefaultDbRefResolver(factory), mappingContext));
 		this.template = new MongoTemplate(factory, converter);
 	}
 
@@ -951,6 +953,27 @@ public class MongoTemplateUnitTests extends MongoOperationsUnitTests {
 		verify(findIterable).projection(eq(new Document()));
 	}
 
+	@Test // DATAMONGO-2155
+	public void saveVersionedEntityShouldCallUpdateCorrectly() {
+
+		when(updateResult.getModifiedCount()).thenReturn(1L);
+
+		VersionedEntity entity = new VersionedEntity();
+		entity.id = 1;
+		entity.version = 10;
+
+		ArgumentCaptor<org.bson.Document> queryCaptor = ArgumentCaptor.forClass(org.bson.Document.class);
+		ArgumentCaptor<org.bson.Document> updateCaptor = ArgumentCaptor.forClass(org.bson.Document.class);
+
+		template.save(entity);
+
+		verify(collection, times(1)).replaceOne(queryCaptor.capture(), updateCaptor.capture(), any(ReplaceOptions.class));
+
+		assertThat(queryCaptor.getValue(), is(equalTo(new Document("_id", 1).append("version", 10))));
+		assertThat(updateCaptor.getValue(),
+				is(equalTo(new Document("version", 11).append("_class", VersionedEntity.class.getName()))));
+	}
+
 	class AutogenerateableId {
 
 		@Id BigInteger id;

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

@@ -874,7 +874,7 @@ public class UpdateMapperUnitTests {
 		assertThat(mappedUpdate).containsEntry("$max", new Document("maxfield", 999));
 	}
 
-	@Test // DATAMONGO-1423
+	@Test // DATAMONGO-1423, DATAMONGO-2155
 	@SuppressWarnings("unchecked")
 	public void mappingShouldConsiderCustomConvertersForEnumMapKeys() {
 
@@ -898,8 +898,8 @@ public class UpdateMapperUnitTests {
 		Document $set = DocumentTestUtils.getAsDocument(mappedUpdate, "$set");
 		assertThat($set.containsKey("enumAsMapKey")).isTrue();
 
-		Document enumAsMapKey = $set.get("enumAsMapKey", Document.class);
+		Map enumAsMapKey = $set.get("enumAsMapKey", Map.class);
-		assertThat(enumAsMapKey.get("AVAILABLE")).isEqualTo(100);
+		assertThat(enumAsMapKey.get("V")).isEqualTo(100);
 	}
 
 	@Test // DATAMONGO-1176
@@ -938,15 +938,17 @@ public class UpdateMapperUnitTests {
 		assertThat(mappedObject).hasSize(2);
 	}
 
-	@Test // DATAMONGO-1486
+	@Test // DATAMONGO-1486, DATAMONGO-2155
 	public void mappingShouldConvertMapKeysToString() {
 
 		Update update = new Update().set("map", Collections.singletonMap(25, "#StarTrek50"));
 		Document mappedUpdate = mapper.getMappedObject(update.getUpdateObject(),
 				context.getPersistentEntity(EntityWithObjectMap.class));
 
-		Document mapToSet = getAsDocument(getAsDocument(mappedUpdate, "$set"), "map");
+		Document $set = DocumentTestUtils.getAsDocument(mappedUpdate, "$set");
+		assertThat($set.containsKey("map")).isTrue();
 
+		Map mapToSet = $set.get("map", Map.class);
 		for (Object key : mapToSet.keySet()) {
 			assertThat(key).isInstanceOf(String.class);
 		}
@@ -980,6 +982,44 @@ public class UpdateMapperUnitTests {
 				.doesNotContainKey("$set.concreteInnerList.[0]._class");
 	}
 
+	@Test // DATAMONGO-2155
+	public void shouldPreserveFieldNamesOfMapProperties() {
+
+		Update update = Update
+				.fromDocument(new Document("concreteMap", new Document("Name", new Document("name", "fooo"))));
+
+		Document mappedUpdate = mapper.getMappedObject(update.getUpdateObject(),
+				context.getPersistentEntity(EntityWithObjectMap.class));
+
+		assertThat(mappedUpdate).isEqualTo(new Document("concreteMap", new Document("Name", new Document("name", "fooo"))));
+	}
+
+	@Test // DATAMONGO-2155
+	public void shouldPreserveExplicitFieldNamesInsideMapProperties() {
+
+		Update update = Update
+				.fromDocument(new Document("map", new Document("Value", new Document("renamed-value", "fooo"))));
+
+		Document mappedUpdate = mapper.getMappedObject(update.getUpdateObject(),
+				context.getPersistentEntity(EntityWithMapOfAliased.class));
+
+		assertThat(mappedUpdate)
+				.isEqualTo(new Document("map", new Document("Value", new Document("renamed-value", "fooo"))));
+	}
+
+	@Test // DATAMONGO-2155
+	public void shouldMapAliasedFieldNamesInMapsCorrectly() {
+
+		Update update = Update
+				.fromDocument(new Document("map", Collections.singletonMap("Value", new Document("value", "fooo"))));
+
+		Document mappedUpdate = mapper.getMappedObject(update.getUpdateObject(),
+				context.getPersistentEntity(EntityWithMapOfAliased.class));
+
+		assertThat(mappedUpdate)
+				.isEqualTo(new Document("map", new Document("Value", new Document("renamed-value", "fooo"))));
+	}
+
 	@Test // DATAMONGO-2174
 	public void mappingUpdateDocumentWithExplicitFieldNameShouldBePossible() {
 
@@ -1211,6 +1251,10 @@ public class UpdateMapperUnitTests {
 		Object field;
 	}
 
+	static class EntityWithMapOfAliased {
+		Map<String, EntityWithAliasedObject> map;
+	}
+
 	static class EntityWithObjectMap {
 
 		Map<Object, Object> map;

spring-data-mongodb/src/test/java/org/springframework/data/mongodb/test/util/DocumentAssert.java

@@ -288,7 +288,7 @@ public class DocumentAssert extends AbstractMapAssert<DocumentAssert, Map<String
 
 			String key = it.next().replace("\\.", ".");
 
-			if (!(current instanceof Bson) && !key.startsWith("[")) {
+			if ((!(current instanceof Bson) && !(current instanceof Map)) && !key.startsWith("[")) {
 				return Lookup.found(null);
 			}
 
@@ -316,6 +316,17 @@ public class DocumentAssert extends AbstractMapAssert<DocumentAssert, Map<String
 					current = document.get(key);
 				}
 
+				else if (current instanceof Map) {
+
+					Map document = (Map) current;
+
+					if (!it.hasNext() && !document.containsKey(key)) {
+						return Lookup.notFound();
+					}
+
+					current = document.get(key);
+				}
+
 				if (!it.hasNext()) {
 					return Lookup.found((T) current);
 				}