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.

Closes: #4098
Original pull request: #4133.

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

@@ -38,7 +38,6 @@ import org.bson.codecs.DecoderContext;
 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;
@@ -187,7 +186,7 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 
 		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);
 	}
 
@@ -396,7 +395,46 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 		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> S convert(Object source, TypeInformation<? extends S> 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;
 
@@ -412,12 +450,13 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 		}
 
 		@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,
@@ -425,7 +464,7 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 		}
 
 		@Override
-		public ConversionContext withPath(ObjectPath currentPath) {
+		public DefaultConversionContext withPath(ObjectPath currentPath) {
 			return new ProjectingConversionContext(sourceConverter, conversions, currentPath, collectionConverter,
 					mapConverter, dbRefConverter, elementConverter, returnedTypeDescriptor);
 		}
@@ -544,7 +583,7 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 		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;
@@ -647,7 +686,7 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 				continue;
 			}
 
-			ConversionContext propertyContext = context.forProperty(prop.getName());
+			ConversionContext propertyContext = context.forProperty(prop);
 			MongoDbPropertyValueProvider valueProviderToUse = valueProvider.withContext(propertyContext);
 
 			if (prop.isAssociation() && !entity.isConstructorArgument(prop)) {
@@ -721,7 +760,7 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 				accessor.setProperty(property,
 						dbRefResolver.resolveReference(property,
 								new DocumentReferenceSource(documentAccessor.getDocument(), documentAccessor.get(property)),
-								referenceLookupDelegate, context::convert));
+								referenceLookupDelegate, context.forProperty(property)::convert));
 			}
 			return;
 		}
@@ -1971,13 +2010,13 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 				return null;
 			}
 
-			CustomConversions conversions = context.conversions;
+			CustomConversions conversions = context.getCustomConversions();
 			if (conversions.getPropertyValueConversions().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());
 		}
@@ -2201,13 +2240,49 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 		}
 	}
 
+	interface ConversionContext {
+
+		default <S extends Object> S convert(Object source, TypeInformation<? extends S> typeHint) {
+			return convert(source, typeHint, this);
+		}
+
+		<S extends Object> S convert(Object source, TypeInformation<? extends S> 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;
@@ -2218,7 +2293,7 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 		final ContainerValueConverter<DBRef> dbRefConverter;
 		final ValueConverter<Object> elementConverter;
 
-		ConversionContext(MongoConverter sourceConverter,
+		DefaultConversionContext(MongoConverter sourceConverter,
 				org.springframework.data.convert.CustomConversions customConversions, ObjectPath path,
 				ContainerValueConverter<Bson> documentConverter, ContainerValueConverter<Collection<?>> collectionConverter,
 				ContainerValueConverter<Bson> mapConverter, ContainerValueConverter<DBRef> dbRefConverter,
@@ -2242,7 +2317,8 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 		 * @return the converted object.
 		 */
 		@SuppressWarnings("unchecked")
-		public <S extends Object> S convert(Object source, TypeInformation<? extends S> typeHint) {
+		public <S extends Object> S convert(Object source, TypeInformation<? extends S> typeHint,
+				ConversionContext context) {
 
 			Assert.notNull(source, "Source must not be null");
 			Assert.notNull(typeHint, "TypeInformation must not be null");
@@ -2262,18 +2338,18 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 				}
 
 				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(
@@ -2281,7 +2357,7 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 			}
 
 			if (source instanceof DBRef) {
-				return (S) dbRefConverter.convert(this, (DBRef) source, typeHint);
+				return (S) dbRefConverter.convert(context, (DBRef) source, typeHint);
 			}
 
 			if (source instanceof Collection) {
@@ -2290,31 +2366,41 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 			}
 
 			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;
 		}
 

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

@@ -44,7 +44,6 @@ import org.junit.jupiter.api.extension.ExtendWith;
 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;
@@ -2850,6 +2849,15 @@ class MappingMongoConverterUnitTests {
 		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> {
 		T content;
 	}
@@ -3573,12 +3581,10 @@ class MappingMongoConverterUnitTests {
 		@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;
 
 	}
@@ -3692,4 +3698,12 @@ class MappingMongoConverterUnitTests {
 
 	}
 
+	@Data
+	static class Cyclic {
+
+		@Id String id;
+		String value;
+		Cyclic cycle;
+	}
+
 }