From a95f77245ed8c5100c82c62e9e28a7c49808086a Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Mon, 7 Aug 2017 16:51:42 +0200 Subject: [PATCH] DATAMONGO-1765 - DefaultDbRefResolver now maps duplicate references correctly. On bulk resolution of a DBRef array we now map the resulting documents back to their ids to make sure that reoccurring identifiers are mapped to the corresponding documents. --- .../core/convert/DefaultDbRefResolver.java | 65 ++++++++----------- .../DefaultDbRefResolverUnitTests.java | 27 ++++++-- 2 files changed, 48 insertions(+), 44 deletions(-) diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/DefaultDbRefResolver.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/DefaultDbRefResolver.java index 3379470cd..4be8f1c56 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/DefaultDbRefResolver.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/DefaultDbRefResolver.java @@ -23,9 +23,11 @@ import java.io.ObjectOutputStream; import java.io.Serializable; import java.lang.reflect.Method; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; -import java.util.Comparator; import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; import org.aopalliance.intercept.MethodInterceptor; import org.aopalliance.intercept.MethodInvocation; @@ -142,8 +144,8 @@ public class DefaultDbRefResolver implements DbRefResolver { } String collection = refs.iterator().next().getCollectionName(); + List ids = new ArrayList<>(refs.size()); - List ids = new ArrayList(refs.size()); for (DBRef ref : refs) { if (!collection.equals(ref.getCollectionName())) { @@ -155,10 +157,14 @@ public class DefaultDbRefResolver implements DbRefResolver { } MongoDatabase db = mongoDbFactory.getDb(); - List result = new ArrayList<>(); - db.getCollection(collection).find(new Document("_id", new Document("$in", ids))).into(result); - result.sort(new DbRefByReferencePositionComparator(ids)); - return result; + + List result = db.getCollection(collection) // + .find(new Document("_id", new Document("$in", ids))) // + .into(new ArrayList<>()); + + return ids.stream() // + .flatMap(id -> documentWithId(id, result)) // + .collect(Collectors.toList()); } /** @@ -223,6 +229,20 @@ public class DefaultDbRefResolver implements DbRefResolver { return property.getDBRef() != null && property.getDBRef().lazy(); } + /** + * Returns document with the given identifier from the given list of {@link Document}s. + * + * @param identifier + * @param documents + * @return + */ + private static Stream documentWithId(Object identifier, Collection documents) { + + return documents.stream() // + .filter(it -> it.get("_id").equals(identifier)) // + .limit(1); + } + /** * A {@link MethodInterceptor} that is used within a lazy loading proxy. The property resolving is delegated to a * {@link DbRefResolverCallback}. The resolving process is triggered by a method invocation on the proxy and is @@ -449,37 +469,4 @@ public class DefaultDbRefResolver implements DbRefResolver { return result; } } - - /** - * {@link Comparator} for sorting {@link Document} that have been loaded in random order by a predefined list of - * reference identifiers. - * - * @author Christoph Strobl - * @author Oliver Gierke - * @since 1.10 - */ - private static class DbRefByReferencePositionComparator implements Comparator { - - private final List reference; - - /** - * Creates a new {@link DbRefByReferencePositionComparator} for the given list of reference identifiers. - * - * @param referenceIds must not be {@literal null}. - */ - public DbRefByReferencePositionComparator(List referenceIds) { - - Assert.notNull(referenceIds, "Reference identifiers must not be null!"); - this.reference = new ArrayList(referenceIds); - } - - /* - * (non-Javadoc) - * @see java.util.Comparator#compare(java.lang.Object, java.lang.Object) - */ - @Override - public int compare(Document o1, Document o2) { - return Integer.compare(reference.indexOf(o1.get("_id")), reference.indexOf(o2.get("_id"))); - } - } } diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/DefaultDbRefResolverUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/DefaultDbRefResolverUnitTests.java index 7ff61250d..456f7519f 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/DefaultDbRefResolverUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/DefaultDbRefResolverUnitTests.java @@ -15,18 +15,18 @@ */ package org.springframework.data.mongodb.core.convert; +import static org.assertj.core.api.Assertions.assertThat; import static org.hamcrest.Matchers.*; import static org.hamcrest.Matchers.contains; -import static org.junit.Assert.*; +import static org.junit.Assert.assertThat; +import static org.mockito.ArgumentMatchers.*; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.*; import java.util.Arrays; import java.util.Collection; import java.util.Collections; -import com.mongodb.client.FindIterable; -import com.mongodb.client.MongoCollection; -import com.mongodb.client.MongoDatabase; import org.bson.Document; import org.bson.types.ObjectId; import org.junit.Before; @@ -43,6 +43,9 @@ import org.springframework.data.mongodb.MongoDbFactory; import org.springframework.data.mongodb.core.DocumentTestUtils; import com.mongodb.DBRef; +import com.mongodb.client.FindIterable; +import com.mongodb.client.MongoCollection; +import com.mongodb.client.MongoDatabase; /** * Unit tests for {@link DefaultDbRefResolver}. @@ -100,7 +103,7 @@ public class DefaultDbRefResolverUnitTests { @Test // DATAMONGO-1194 public void bulkFetchShouldReturnEarlyForEmptyLists() { - resolver.bulkFetch(Collections.emptyList()); + resolver.bulkFetch(Collections. emptyList()); verify(collectionMock, never()).find(Mockito.any(Document.class)); } @@ -115,6 +118,7 @@ public class DefaultDbRefResolverUnitTests { DBRef ref2 = new DBRef("collection-1", o2.get("_id")); when(cursorMock.into(any())).then(new Answer() { + @Override public Object answer(InvocationOnMock invocation) throws Throwable { @@ -127,4 +131,17 @@ public class DefaultDbRefResolverUnitTests { assertThat(resolver.bulkFetch(Arrays.asList(ref1, ref2)), contains(o1, o2)); } + + @Test // DATAMONGO-1765 + public void bulkFetchContainsDuplicates() { + + Document document = new Document("_id", new ObjectId()); + + DBRef ref1 = new DBRef("collection-1", document.get("_id")); + DBRef ref2 = new DBRef("collection-1", document.get("_id")); + + when(cursorMock.into(any())).then(invocation -> Arrays.asList(document)); + + assertThat(resolver.bulkFetch(Arrays.asList(ref1, ref2))).containsExactly(document, document); + } }