From 70653ac4fc329663f9e03c17fb2dad4ba96e2434 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Fri, 1 Sep 2023 12:01:01 +0200 Subject: [PATCH 1/4] Prepare issue branch. --- pom.xml | 2 +- spring-data-mongodb-benchmarks/pom.xml | 2 +- spring-data-mongodb-distribution/pom.xml | 2 +- spring-data-mongodb/pom.xml | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pom.xml b/pom.xml index 366786fc6d..3ef61861eb 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-mongodb-parent - 4.2.0-SNAPSHOT + 4.2.0-GH-4491-SNAPSHOT pom Spring Data MongoDB diff --git a/spring-data-mongodb-benchmarks/pom.xml b/spring-data-mongodb-benchmarks/pom.xml index 2de4b6b635..4cf395104a 100644 --- a/spring-data-mongodb-benchmarks/pom.xml +++ b/spring-data-mongodb-benchmarks/pom.xml @@ -7,7 +7,7 @@ org.springframework.data spring-data-mongodb-parent - 4.2.0-SNAPSHOT + 4.2.0-GH-4491-SNAPSHOT ../pom.xml diff --git a/spring-data-mongodb-distribution/pom.xml b/spring-data-mongodb-distribution/pom.xml index 060a6d0dd9..e41d524181 100644 --- a/spring-data-mongodb-distribution/pom.xml +++ b/spring-data-mongodb-distribution/pom.xml @@ -15,7 +15,7 @@ org.springframework.data spring-data-mongodb-parent - 4.2.0-SNAPSHOT + 4.2.0-GH-4491-SNAPSHOT ../pom.xml diff --git a/spring-data-mongodb/pom.xml b/spring-data-mongodb/pom.xml index dc07f13ccc..ef508ed175 100644 --- a/spring-data-mongodb/pom.xml +++ b/spring-data-mongodb/pom.xml @@ -13,7 +13,7 @@ org.springframework.data spring-data-mongodb-parent - 4.2.0-SNAPSHOT + 4.2.0-GH-4491-SNAPSHOT ../pom.xml From 4f3471973ebe205f96413a1ff44ce85b290ecc5f Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Fri, 1 Sep 2023 12:01:42 +0200 Subject: [PATCH 2/4] Correctly read unwrapped properties during constructor creation. Closes #4491 --- .../core/convert/MappingMongoConverter.java | 32 ++++++++++++++++--- .../MappingMongoConverterUnitTests.java | 30 +++++++++++++++++ 2 files changed, 58 insertions(+), 4 deletions(-) diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/MappingMongoConverter.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/MappingMongoConverter.java index 6aeb08e240..3610d65532 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/MappingMongoConverter.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/MappingMongoConverter.java @@ -17,7 +17,16 @@ import java.lang.reflect.Constructor; import java.lang.reflect.Method; -import java.util.*; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; import java.util.function.Predicate; import java.util.stream.Collectors; @@ -41,7 +50,13 @@ import org.springframework.data.annotation.Reference; import org.springframework.data.convert.CustomConversions; import org.springframework.data.convert.TypeMapper; -import org.springframework.data.mapping.*; +import org.springframework.data.mapping.Association; +import org.springframework.data.mapping.InstanceCreatorMetadata; +import org.springframework.data.mapping.MappingException; +import org.springframework.data.mapping.Parameter; +import org.springframework.data.mapping.PersistentEntity; +import org.springframework.data.mapping.PersistentProperty; +import org.springframework.data.mapping.PersistentPropertyAccessor; import org.springframework.data.mapping.callback.EntityCallbacks; import org.springframework.data.mapping.context.MappingContext; import org.springframework.data.mapping.model.ConvertingPropertyAccessor; @@ -1961,6 +1976,8 @@ class AssociationAwareMongoDbPropertyValueProvider extends MongoDbPropertyValueP @SuppressWarnings("unchecked") public T getPropertyValue(MongoPersistentProperty property) { + ConversionContext propertyContext = context.forProperty(property); + if (property.isDbReference() && property.getDBRef().lazy()) { Object rawRefValue = accessor.get(property); @@ -1977,9 +1994,16 @@ public T getPropertyValue(MongoPersistentProperty property) { } if (property.isDocumentReference()) { + return (T) dbRefResolver.resolveReference(property, - new DocumentReferenceSource(accessor.getDocument(), accessor.get(property)), - referenceLookupDelegate, context::convert); + new DocumentReferenceSource(accessor.getDocument(), accessor.get(property)), referenceLookupDelegate, + context::convert); + } + + if (property.isUnwrapped()) { + + return (T) readUnwrapped(propertyContext, accessor, property, + mappingContext.getRequiredPersistentEntity(property)); } return super.getPropertyValue(property); diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/MappingMongoConverterUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/MappingMongoConverterUnitTests.java index 8a3fac7797..a1eabc33ba 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/MappingMongoConverterUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/MappingMongoConverterUnitTests.java @@ -2346,6 +2346,24 @@ void readUnwrappedTypeWithComplexValue() { .isEqualTo(expected); } + @Test // GH-4491 + void readUnwrappedTypeWithComplexValueUsingConstructor() { + + org.bson.Document source = new org.bson.Document("_id", "id-1").append("stringValue", "hello").append("address", + new org.bson.Document("s", "1007 Mountain Drive").append("city", "Gotham")); + + WithUnwrappedConstructor target = converter.read(WithUnwrappedConstructor.class, source); + + Address expected = new Address(); + expected.city = "Gotham"; + expected.street = "1007 Mountain Drive"; + + assertThat(target.embeddableValue.stringValue) // + .isEqualTo("hello"); + assertThat(target.embeddableValue.address) // + .isEqualTo(expected); + } + @Test // DATAMONGO-1902 void writeUnwrappedTypeWithComplexValue() { @@ -3422,6 +3440,18 @@ static class WithNullableUnwrapped { @Unwrapped.Nullable EmbeddableType embeddableValue; } + static class WithUnwrappedConstructor { + + private final String id; + + private final @Unwrapped.Empty EmbeddableType embeddableValue; + + public WithUnwrappedConstructor(String id, EmbeddableType embeddableValue) { + this.id = id; + this.embeddableValue = embeddableValue; + } + } + static class WithPrefixedNullableUnwrapped { String id; From 4d82856c0414d5596abab1a4c2fdc7b6ee08fdbd Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Fri, 1 Sep 2023 12:19:59 +0200 Subject: [PATCH 3/4] Consistently use the same reading strategies to read associations. Return the value to set instead of calling the accessor directly. Remove duplicate calls to resolve associations. See #4491 --- .../core/convert/MappingMongoConverter.java | 74 +++++++------------ 1 file changed, 28 insertions(+), 46 deletions(-) diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/MappingMongoConverter.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/MappingMongoConverter.java index 3610d65532..ec5d866461 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/MappingMongoConverter.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/MappingMongoConverter.java @@ -507,7 +507,6 @@ private S read(ConversionContext context, MongoPersistentEntity entity, D S instance = instantiator.createInstance(entity, provider); if (entity.requiresPropertyPopulation()) { - return populateProperties(context, entity, documentAccessor, evaluator, instance); } @@ -586,14 +585,18 @@ private void readProperties(ConversionContext context, MongoPersistentEntity ConversionContext propertyContext = context.forProperty(prop); MongoDbPropertyValueProvider valueProviderToUse = valueProvider.withContext(propertyContext); - if (prop.isAssociation() && !entity.isCreatorArgument(prop)) { + if (prop.isAssociation()) { if (callback == null) { callback = getDbRefResolverCallback(propertyContext, documentAccessor, evaluator); } - readAssociation(prop.getRequiredAssociation(), accessor, documentAccessor, dbRefProxyHandler, callback, - propertyContext, evaluator); + Object value = readAssociation(prop.getRequiredAssociation(), documentAccessor, dbRefProxyHandler, callback, + propertyContext); + + if (value != null) { + accessor.setProperty(prop, value); + } continue; } @@ -608,17 +611,6 @@ private void readProperties(ConversionContext context, MongoPersistentEntity continue; } - if (prop.isAssociation()) { - - if (callback == null) { - callback = getDbRefResolverCallback(propertyContext, documentAccessor, evaluator); - } - - readAssociation(prop.getRequiredAssociation(), accessor, documentAccessor, dbRefProxyHandler, callback, - propertyContext, evaluator); - continue; - } - accessor.setProperty(prop, valueProviderToUse.getPropertyValue(prop)); } } @@ -630,9 +622,10 @@ private DbRefResolverCallback getDbRefResolverCallback(ConversionContext context (prop, bson, e, path) -> MappingMongoConverter.this.getValueInternal(context, prop, bson, e)); } - private void readAssociation(Association association, PersistentPropertyAccessor accessor, + @Nullable + private Object readAssociation(Association association, DocumentAccessor documentAccessor, DbRefProxyHandler handler, DbRefResolverCallback callback, - ConversionContext context, SpELExpressionEvaluator evaluator) { + ConversionContext context) { MongoPersistentProperty property = association.getInverse(); Object value = documentAccessor.get(property); @@ -645,30 +638,27 @@ private void readAssociation(Association association, P if (conversionService.canConvert(DocumentPointer.class, property.getActualType())) { if (value == null) { - return; + return null; } DocumentPointer pointer = () -> value; // collection like special treatment - accessor.setProperty(property, conversionService.convert(pointer, property.getActualType())); + return conversionService.convert(pointer, property.getActualType()); } else { - accessor.setProperty(property, - dbRefResolver.resolveReference(property, + return dbRefResolver.resolveReference(property, new DocumentReferenceSource(documentAccessor.getDocument(), documentAccessor.get(property)), - referenceLookupDelegate, context.forProperty(property)::convert)); + referenceLookupDelegate, context.forProperty(property)::convert); } - return; } if (value == null) { - return; + return null; } if (value instanceof DBRef dbref) { - accessor.setProperty(property, dbRefResolver.resolveDbRef(property, dbref, callback, handler)); - return; + return dbRefResolver.resolveDbRef(property, dbref, callback, handler); } /* @@ -679,18 +669,18 @@ private void readAssociation(Association association, P if (value instanceof Document document) { if (property.isMap()) { if (document.isEmpty() || peek(document.values()) instanceof DBRef) { - accessor.setProperty(property, dbRefResolver.resolveDbRef(property, null, callback, handler)); + return dbRefResolver.resolveDbRef(property, null, callback, handler); } else { - accessor.setProperty(property, readMap(context, document, property.getTypeInformation())); + return readMap(context, document, property.getTypeInformation()); } } else { - accessor.setProperty(property, read(property.getActualType(), document)); + return read(property.getActualType(), document); } } else if (value instanceof Collection collection && !collection.isEmpty() && peek(collection) instanceof Document) { - accessor.setProperty(property, readCollectionOrArray(context, collection, property.getTypeInformation())); + return readCollectionOrArray(context, collection, property.getTypeInformation()); } else { - accessor.setProperty(property, dbRefResolver.resolveDbRef(property, null, callback, handler)); + return dbRefResolver.resolveDbRef(property, null, callback, handler); } } @@ -1978,26 +1968,14 @@ public T getPropertyValue(MongoPersistentProperty property) { ConversionContext propertyContext = context.forProperty(property); - if (property.isDbReference() && property.getDBRef().lazy()) { - - Object rawRefValue = accessor.get(property); - if (rawRefValue == null) { - return null; - } + if (property.isAssociation()) { DbRefResolverCallback callback = new DefaultDbRefResolverCallback(accessor.getDocument(), context.getPath(), evaluator, (prop, bson, evaluator, path) -> MappingMongoConverter.this.getValueInternal(context, prop, bson, evaluator)); - DBRef dbref = rawRefValue instanceof DBRef dbRef ? dbRef : null; - return (T) dbRefResolver.resolveDbRef(property, dbref, callback, dbRefProxyHandler); - } - - if (property.isDocumentReference()) { - - return (T) dbRefResolver.resolveReference(property, - new DocumentReferenceSource(accessor.getDocument(), accessor.get(property)), referenceLookupDelegate, - context::convert); + return (T) readAssociation(property.getRequiredAssociation(), accessor, dbRefProxyHandler, callback, + propertyContext); } if (property.isUnwrapped()) { @@ -2006,6 +1984,10 @@ public T getPropertyValue(MongoPersistentProperty property) { mappingContext.getRequiredPersistentEntity(property)); } + if (!accessor.hasValue(property)) { + return null; + } + return super.getPropertyValue(property); } } From a9b2b8b1981b926cf0419b7dd97ea3d2985173da Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Fri, 1 Sep 2023 12:20:30 +0200 Subject: [PATCH 4/4] Polishing. Remove sysout from tests. See #4491 --- .../mongodb/core/encryption/AbstractEncryptionTestBase.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/encryption/AbstractEncryptionTestBase.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/encryption/AbstractEncryptionTestBase.java index 393a0d12f2..87f3009cbe 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/encryption/AbstractEncryptionTestBase.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/encryption/AbstractEncryptionTestBase.java @@ -353,7 +353,7 @@ void altKeyDetection(@Autowired CachingMongoClientEncryption mongoClientEncrypti template.save(p3); template.execute(Person.class, collection -> { - collection.find(new Document()).forEach(it -> System.out.println(it.toJson())); + collection.find(new Document()); return null; });