From 2ec0f933254fc18b5e3161ae200edabd452d2b83 Mon Sep 17 00:00:00 2001
From: Oliver Drotbohm <ogierke@pivotal.io>
Date: Thu, 15 Nov 2018 15:26:36 +0100
Subject: [PATCH] DATAMONGO-2135 - Default to intermediate List for properties
 typed to Collection.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We now defensively create a List rather than a LinkedHashSet (which Spring's CollectionFactory.createCollection(…) defaults to) to make sure we're not accidentally dropping values that are considered equal according to their Java class definition.
---
 .../core/convert/MappingMongoConverter.java   | 19 ++++++++++-----
 .../MappingMongoConverterUnitTests.java       | 24 +++++++++++++++++++
 2 files changed, 37 insertions(+), 6 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 ca379e375..a239ae919 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
@@ -578,7 +578,7 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 			return;
 		}
 
-		MongoPersistentEntity<?> entity = isSubtype(prop.getType(), obj.getClass())
+		MongoPersistentEntity<?> entity = isSubTypeOf(obj.getClass(), prop.getType())
 				? mappingContext.getRequiredPersistentEntity(obj.getClass())
 				: mappingContext.getRequiredPersistentEntity(type);
 
@@ -590,10 +590,6 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 		accessor.put(prop, document);
 	}
 
-	private boolean isSubtype(Class<?> left, Class<?> right) {
-		return left.isAssignableFrom(right) && !left.equals(right);
-	}
-
 	/**
 	 * Returns given object as {@link Collection}. Will return the {@link Collection} as is if the source is a
 	 * {@link Collection} already, will convert an array into a {@link Collection} or simply create a single element
@@ -970,7 +966,7 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 		Assert.notNull(path, "Object path must not be null!");
 
 		Class<?> collectionType = targetType.getType();
-		collectionType = Collection.class.isAssignableFrom(collectionType) //
+		collectionType = isSubTypeOf(collectionType, Collection.class) //
 				? collectionType //
 				: List.class;
 
@@ -1544,6 +1540,17 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App
 		return true;
 	}
 
+	/**
+	 * Returns whether the given type is a sub type of the given reference, i.e. assignable but not the exact same type.
+	 * 
+	 * @param type must not be {@literal null}.
+	 * @param reference must not be {@literal null}.
+	 * @return
+	 */
+	private static boolean isSubTypeOf(Class<?> type, Class<?> reference) {
+		return !type.equals(reference) && reference.isAssignableFrom(type);
+	}
+
 	/**
 	 * Marker class used to indicate we have a non root document object here that might be used within an update - so we
 	 * need to preserve type hints for potential nested elements but need to remove it on top level.
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 ab93d548a..c9f2413fb 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
@@ -26,6 +26,7 @@ import static org.junit.Assert.fail;
 import static org.mockito.Mockito.*;
 import static org.springframework.data.mongodb.core.DocumentTestUtils.*;
 
+import lombok.EqualsAndHashCode;
 import lombok.RequiredArgsConstructor;
 
 import java.math.BigDecimal;
@@ -1904,6 +1905,18 @@ public class MappingMongoConverterUnitTests {
 		assertThat(target).doesNotContainKeys("_class");
 	}
 
+	@Test // DATAMONGO-2135
+	public void addsEqualObjectsToCollection() {
+
+		org.bson.Document itemDocument = new org.bson.Document("itemKey", "123");
+		org.bson.Document orderDocument = new org.bson.Document("items",
+				Arrays.asList(itemDocument, itemDocument, itemDocument));
+
+		Order order = converter.read(Order.class, orderDocument);
+
+		assertThat(order.items).hasSize(3);
+	}
+
 	static class GenericType<T> {
 		T content;
 	}
@@ -2284,4 +2297,15 @@ public class MappingMongoConverterUnitTests {
 	static class WithNestedLists {
 		float[][][] nestedFloats;
 	}
+
+	// DATAMONGO-2135
+
+	@EqualsAndHashCode // equality check by fields
+	static class SomeItem {
+		String itemKey;
+	}
+
+	static class Order {
+		Collection<SomeItem> items = new ArrayList<>();
+	}
 }