From c01802c82f0a8cdf16966ec1b21c29a67ec8a007 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Tue, 2 Aug 2022 13:06:58 +0200 Subject: [PATCH 1/2] 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 9cc6d6e9b7..af6be81333 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-mongodb-parent - 4.0.0-SNAPSHOT + 4.0.x-GH-4098-SNAPSHOT pom Spring Data MongoDB diff --git a/spring-data-mongodb-benchmarks/pom.xml b/spring-data-mongodb-benchmarks/pom.xml index c28a240d2c..f063592c6f 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.0.0-SNAPSHOT + 4.0.x-GH-4098-SNAPSHOT ../pom.xml diff --git a/spring-data-mongodb-distribution/pom.xml b/spring-data-mongodb-distribution/pom.xml index 5dedcf81ed..af7b89df0c 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.0.0-SNAPSHOT + 4.0.x-GH-4098-SNAPSHOT ../pom.xml diff --git a/spring-data-mongodb/pom.xml b/spring-data-mongodb/pom.xml index 9cd2578964..02438c440d 100644 --- a/spring-data-mongodb/pom.xml +++ b/spring-data-mongodb/pom.xml @@ -12,7 +12,7 @@ org.springframework.data spring-data-mongodb-parent - 4.0.0-SNAPSHOT + 4.0.x-GH-4098-SNAPSHOT ../pom.xml From 7c81633128d54469172f35789987c1dc0273689f Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Wed, 3 Aug 2022 12:19:17 +0200 Subject: [PATCH 2/2] Fix non association mapping when id value matches already resolved instance of same type. This commit ensures to fully resolve non association values from the given source document instead of trying attempt a by id lookup in already resolved instances. --- .../core/convert/MappingMongoConverter.java | 140 ++++++++++++++---- .../MappingMongoConverterUnitTests.java | 24 ++- 2 files changed, 132 insertions(+), 32 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 af84e780c6..36030ec5a6 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 @@ -39,7 +39,6 @@ import org.bson.conversions.Bson; import org.bson.json.JsonReader; import org.bson.types.ObjectId; - import org.springframework.beans.BeansException; import org.springframework.beans.factory.BeanClassLoaderAware; import org.springframework.context.ApplicationContext; @@ -188,7 +187,7 @@ protected ConversionContext getConversionContext(ObjectPath path) { Assert.notNull(path, "ObjectPath must not be null"); - return new ConversionContext(this, conversions, path, this::readDocument, this::readCollectionOrArray, + return new DefaultConversionContext(this, conversions, path, this::readDocument, this::readCollectionOrArray, this::readMap, this::readDBRef, this::getPotentiallyConvertedSimpleRead); } @@ -385,7 +384,46 @@ private Object doReadOrProject(ConversionContext context, Bson source, TypeInfor return readDocument(context, source, typeHint); } - class ProjectingConversionContext extends ConversionContext { + static class AssociationConversionContext implements ConversionContext { + + private final ConversionContext delegate; + + public AssociationConversionContext(ConversionContext delegate) { + this.delegate = delegate; + } + + @Override + public S convert(Object source, TypeInformation typeHint, ConversionContext context) { + return delegate.convert(source, typeHint, context); + } + + @Override + public ConversionContext withPath(ObjectPath currentPath) { + return new AssociationConversionContext(delegate.withPath(currentPath)); + } + + @Override + public ObjectPath getPath() { + return delegate.getPath(); + } + + @Override + public CustomConversions getCustomConversions() { + return delegate.getCustomConversions(); + } + + @Override + public MongoConverter getSourceConverter() { + return delegate.getSourceConverter(); + } + + @Override + public boolean resolveIdsInContext() { + return true; + } + } + + class ProjectingConversionContext extends DefaultConversionContext { private final EntityProjection returnedTypeDescriptor; @@ -401,12 +439,13 @@ class ProjectingConversionContext extends ConversionContext { } @Override - public ConversionContext forProperty(String name) { + public DefaultConversionContext forProperty(String name) { EntityProjection property = returnedTypeDescriptor.findProperty(name); if (property == null) { - return new ConversionContext(sourceConverter, conversions, path, MappingMongoConverter.this::readDocument, - collectionConverter, mapConverter, dbRefConverter, elementConverter); + return new DefaultConversionContext(sourceConverter, conversions, path, + MappingMongoConverter.this::readDocument, collectionConverter, mapConverter, dbRefConverter, + elementConverter); } return new ProjectingConversionContext(sourceConverter, conversions, path, collectionConverter, mapConverter, @@ -414,7 +453,7 @@ public ConversionContext forProperty(String name) { } @Override - public ConversionContext withPath(ObjectPath currentPath) { + public DefaultConversionContext withPath(ObjectPath currentPath) { return new ProjectingConversionContext(sourceConverter, conversions, currentPath, collectionConverter, mapConverter, dbRefConverter, elementConverter, returnedTypeDescriptor); } @@ -529,7 +568,7 @@ private S read(ConversionContext context, MongoPersistentEntity entity, D SpELExpressionEvaluator evaluator = new DefaultSpELExpressionEvaluator(bson, spELContext); DocumentAccessor documentAccessor = new DocumentAccessor(bson); - if (hasIdentifier(bson)) { + if (context.resolveIdsInContext() && hasIdentifier(bson)) { S existing = findContextualEntity(context, entity, bson); if (existing != null) { return existing; @@ -632,7 +671,7 @@ private void readProperties(ConversionContext context, MongoPersistentEntity continue; } - ConversionContext propertyContext = context.forProperty(prop.getName()); + ConversionContext propertyContext = context.forProperty(prop); MongoDbPropertyValueProvider valueProviderToUse = valueProvider.withContext(propertyContext); if (prop.isAssociation() && !entity.isConstructorArgument(prop)) { @@ -706,7 +745,7 @@ private void readAssociation(Association association, P accessor.setProperty(property, dbRefResolver.resolveReference(property, new DocumentReferenceSource(documentAccessor.getDocument(), documentAccessor.get(property)), - referenceLookupDelegate, context::convert)); + referenceLookupDelegate, context.forProperty(property)::convert)); } return; } @@ -1935,13 +1974,13 @@ public T getPropertyValue(MongoPersistentProperty property) { return null; } - CustomConversions conversions = context.conversions; + CustomConversions conversions = context.getCustomConversions(); if (conversions.hasValueConverter(property)) { return (T) conversions.getPropertyValueConversions().getValueConverter(property).read(value, - new MongoConversionContext(property, context.sourceConverter)); + new MongoConversionContext(property, context.getSourceConverter())); } - ConversionContext contextToUse = context.forProperty(property.getName()); + ConversionContext contextToUse = context.forProperty(property); return (T) contextToUse.convert(value, property.getTypeInformation()); } @@ -2158,13 +2197,49 @@ public TypeDescriptor toTypeDescriptor() { } } + interface ConversionContext { + + default S convert(Object source, TypeInformation typeHint) { + return convert(source, typeHint, this); + } + + S convert(Object source, TypeInformation typeHint, ConversionContext context); + + ConversionContext withPath(ObjectPath currentPath); + + ObjectPath getPath(); + + default ConversionContext forProperty(String name) { + return this; + } + + default ConversionContext forProperty(@Nullable PersistentProperty property) { + + if (property != null) { + if (property.isAssociation()) { + return new AssociationConversionContext(forProperty(property.getName())); + } + return forProperty(property.getName()); + } + return this; + } + + default boolean resolveIdsInContext() { + return false; + } + + CustomConversions getCustomConversions(); + + MongoConverter getSourceConverter(); + } + /** * Conversion context holding references to simple {@link ValueConverter} and {@link ContainerValueConverter}. * Entrypoint for recursive conversion of {@link Document} and other types. * * @since 3.2 */ - protected static class ConversionContext { + protected static class DefaultConversionContext implements ConversionContext { final MongoConverter sourceConverter; final org.springframework.data.convert.CustomConversions conversions; @@ -2175,7 +2250,7 @@ protected static class ConversionContext { final ContainerValueConverter dbRefConverter; final ValueConverter elementConverter; - ConversionContext(MongoConverter sourceConverter, + DefaultConversionContext(MongoConverter sourceConverter, org.springframework.data.convert.CustomConversions customConversions, ObjectPath path, ContainerValueConverter documentConverter, ContainerValueConverter> collectionConverter, ContainerValueConverter mapConverter, ContainerValueConverter dbRefConverter, @@ -2199,7 +2274,8 @@ protected static class ConversionContext { * @return the converted object. */ @SuppressWarnings("unchecked") - public S convert(Object source, TypeInformation typeHint) { + public S convert(Object source, TypeInformation typeHint, + ConversionContext context) { Assert.notNull(source, "Source must not be null"); Assert.notNull(typeHint, "TypeInformation must not be null"); @@ -2219,18 +2295,18 @@ public S convert(Object source, TypeInformation } if (typeHint.isCollectionLike() || typeHint.getType().isAssignableFrom(Collection.class)) { - return (S) collectionConverter.convert(this, (Collection) source, typeHint); + return (S) collectionConverter.convert(context, (Collection) source, typeHint); } } if (typeHint.isMap()) { if (ClassUtils.isAssignable(Document.class, typeHint.getType())) { - return (S) documentConverter.convert(this, BsonUtils.asBson(source), typeHint); + return (S) documentConverter.convert(context, BsonUtils.asBson(source), typeHint); } if (BsonUtils.supportsBson(source)) { - return (S) mapConverter.convert(this, BsonUtils.asBson(source), typeHint); + return (S) mapConverter.convert(context, BsonUtils.asBson(source), typeHint); } throw new IllegalArgumentException( @@ -2238,7 +2314,7 @@ public S convert(Object source, TypeInformation } if (source instanceof DBRef) { - return (S) dbRefConverter.convert(this, (DBRef) source, typeHint); + return (S) dbRefConverter.convert(context, (DBRef) source, typeHint); } if (source instanceof Collection) { @@ -2247,31 +2323,41 @@ public S convert(Object source, TypeInformation } if (BsonUtils.supportsBson(source)) { - return (S) documentConverter.convert(this, BsonUtils.asBson(source), typeHint); + return (S) documentConverter.convert(context, BsonUtils.asBson(source), typeHint); } return (S) elementConverter.convert(source, typeHint); } + @Override + public CustomConversions getCustomConversions() { + return conversions; + } + + @Override + public MongoConverter getSourceConverter() { + return sourceConverter; + } + /** - * Create a new {@link ConversionContext} with {@link ObjectPath currentPath} applied. + * Create a new {@link DefaultConversionContext} with {@link ObjectPath currentPath} applied. * * @param currentPath must not be {@literal null}. - * @return a new {@link ConversionContext} with {@link ObjectPath currentPath} applied. + * @return a new {@link DefaultConversionContext} with {@link ObjectPath currentPath} applied. */ - public ConversionContext withPath(ObjectPath currentPath) { + public DefaultConversionContext withPath(ObjectPath currentPath) { Assert.notNull(currentPath, "ObjectPath must not be null"); - return new ConversionContext(sourceConverter, conversions, currentPath, documentConverter, collectionConverter, - mapConverter, dbRefConverter, elementConverter); + return new DefaultConversionContext(sourceConverter, conversions, currentPath, documentConverter, + collectionConverter, mapConverter, dbRefConverter, elementConverter); } public ObjectPath getPath() { return path; } - public ConversionContext forProperty(String name) { + public DefaultConversionContext forProperty(String name) { return this; } 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 44f6d700da..3f8c65d637 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 @@ -45,7 +45,6 @@ import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; - import org.springframework.aop.framework.ProxyFactory; import org.springframework.beans.ConversionNotSupportedException; import org.springframework.beans.factory.annotation.Autowired; @@ -2823,6 +2822,15 @@ public org.bson.Document write(@Nullable String domainValue, MongoConversionCont assertThat(read.viaRegisteredConverter).isEqualTo("spring"); } + @Test // GH-4098 + void resolvesCyclicNonAssociationValueFromSource/* and does not attempt to be smart and look up id values in context */() { + + org.bson.Document source = new org.bson.Document("_id", "id-1").append("value", "v1").append("cycle", + new org.bson.Document("_id", "id-1").append("value", "v2")); + + assertThat(converter.read(Cyclic.class, source).cycle.value).isEqualTo("v2"); + } + static class GenericType { T content; } @@ -3546,12 +3554,10 @@ static class WithFieldWrite { @org.springframework.data.mongodb.core.mapping.Field( write = org.springframework.data.mongodb.core.mapping.Field.Write.ALWAYS) Integer writeAlways; - @org.springframework.data.mongodb.core.mapping.DBRef - @org.springframework.data.mongodb.core.mapping.Field( + @org.springframework.data.mongodb.core.mapping.DBRef @org.springframework.data.mongodb.core.mapping.Field( write = org.springframework.data.mongodb.core.mapping.Field.Write.NON_NULL) Person writeNonNullPerson; - @org.springframework.data.mongodb.core.mapping.DBRef - @org.springframework.data.mongodb.core.mapping.Field( + @org.springframework.data.mongodb.core.mapping.DBRef @org.springframework.data.mongodb.core.mapping.Field( write = org.springframework.data.mongodb.core.mapping.Field.Write.ALWAYS) Person writeAlwaysPerson; } @@ -3665,4 +3671,12 @@ static class Author { } + @Data + static class Cyclic { + + @Id String id; + String value; + Cyclic cycle; + } + }