From 630da43b690ec0b71e773f153d98a65c5a1a0ddc Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Mon, 3 Dec 2018 15:40:32 +0100 Subject: [PATCH] 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. --- .../data/mongodb/core/MappedDocument.java | 18 ++++++- .../data/mongodb/core/MongoTemplate.java | 5 +- .../mongodb/core/convert/QueryMapper.java | 34 +++++++++++- .../mongodb/core/convert/UpdateMapper.java | 5 +- .../mongodb/core/MongoTemplateUnitTests.java | 27 +++++++++- .../core/convert/UpdateMapperUnitTests.java | 54 +++++++++++++++++-- .../mongodb/test/util/DocumentAssert.java | 13 ++++- 7 files changed, 142 insertions(+), 14 deletions(-) diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MappedDocument.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MappedDocument.java index 006a74327..cb7eac8ed 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MappedDocument.java +++ b/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() { - return Update.fromDocument(document, ID_FIELD); + public MappedUpdate updateWithoutId() { + 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(); + } } } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java index ecfb07dd6..f7ccc4eb1 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java +++ b/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); diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java index 3a10a6c9a..7bf59bcf7 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java +++ b/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 map = new LinkedHashMap<>(); + + ((Map) 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 diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/UpdateMapper.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/UpdateMapper.java index c8bfd137a..6eee6776f 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/UpdateMapper.java +++ b/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 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)) { diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateUnitTests.java index c7f1f359d..71da4c9c3 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateUnitTests.java +++ b/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 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 queryCaptor = ArgumentCaptor.forClass(org.bson.Document.class); + ArgumentCaptor 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; diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/UpdateMapperUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/UpdateMapperUnitTests.java index 36306042c..52643287c 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/UpdateMapperUnitTests.java +++ b/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); - assertThat(enumAsMapKey.get("AVAILABLE")).isEqualTo(100); + Map enumAsMapKey = $set.get("enumAsMapKey", Map.class); + 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 map; + } + static class EntityWithObjectMap { Map map; diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/test/util/DocumentAssert.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/test/util/DocumentAssert.java index 717ef5b60..1ced3c9fa 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/test/util/DocumentAssert.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/test/util/DocumentAssert.java @@ -288,7 +288,7 @@ public class DocumentAssert extends AbstractMapAssert