From babab54ffd8731de1fe6ebcd5609a3db3b60580b Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Mon, 11 Jul 2016 15:55:39 +0200 Subject: [PATCH] DATAMONGO-1194 - Improve DBRef resolution for collections. We now bulk load collections of referenced objects as long as they are stored in the same collection. This reduces database roundtrips and network traffic. Original pull request: #377. --- .../mongodb/core/convert/DbRefResolver.java | 14 +- .../core/convert/DefaultDbRefResolver.java | 63 +++++++++ .../core/convert/MappingMongoConverter.java | 73 ++++++++-- .../data/mongodb/core/MongoTemplateTests.java | 58 +++++++- .../DbRefMappingMongoConverterUnitTests.java | 64 +++++++++ .../DefaultDbRefResolverUnitTests.java | 129 ++++++++++++++++++ 6 files changed, 388 insertions(+), 13 deletions(-) create mode 100644 spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/DefaultDbRefResolverUnitTests.java diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/DbRefResolver.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/DbRefResolver.java index a43b742c6..8efedbee5 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/DbRefResolver.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/DbRefResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2013-2015 the original author or authors. + * Copyright 2013-2016 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -15,6 +15,8 @@ */ package org.springframework.data.mongodb.core.convert; +import java.util.List; + import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity; import org.springframework.data.mongodb.core.mapping.MongoPersistentProperty; @@ -64,4 +66,14 @@ public interface DbRefResolver { * @since 1.7 */ DBObject fetch(DBRef dbRef); + + /** + * Loads a given {@link List} of {@link DBRef}s from the datasource in one batch.
+ * The {@link DBRef} elements in the list must not reference different collections. + * + * @param dbRefs must not be {@literal null}. + * @return never {@literal null}. + * @since 1.10 + */ + List bulkFetch(List dbRefs); } 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 cbffcf1f5..850a8c7ba 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 @@ -22,6 +22,10 @@ import java.io.ObjectInputStream; import java.io.ObjectOutputStream; import java.io.Serializable; import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; +import java.util.List; import org.aopalliance.intercept.MethodInterceptor; import org.aopalliance.intercept.MethodInvocation; @@ -31,6 +35,7 @@ import org.springframework.cglib.proxy.Enhancer; import org.springframework.cglib.proxy.Factory; import org.springframework.cglib.proxy.MethodProxy; import org.springframework.dao.DataAccessException; +import org.springframework.dao.InvalidDataAccessApiUsageException; import org.springframework.dao.support.PersistenceExceptionTranslator; import org.springframework.data.mongodb.LazyLoadingException; import org.springframework.data.mongodb.MongoDbFactory; @@ -40,6 +45,9 @@ import org.springframework.objenesis.ObjenesisStd; import org.springframework.util.Assert; import org.springframework.util.ReflectionUtils; +import com.mongodb.BasicDBObject; +import com.mongodb.BasicDBObjectBuilder; +import com.mongodb.DB; import com.mongodb.DBObject; import com.mongodb.DBRef; @@ -109,6 +117,40 @@ public class DefaultDbRefResolver implements DbRefResolver { return ReflectiveDBRefResolver.fetch(mongoDbFactory, dbRef); } + /* + * (non-Javadoc) + * @see org.springframework.data.mongodb.core.convert.DbRefResolver#bulkFetch(java.util.List) + */ + @Override + public List bulkFetch(List refs) { + + Assert.notNull(mongoDbFactory, "Factory must not be null!"); + Assert.notNull(refs, "DBRef to fetch must not be null!"); + + if (refs.isEmpty()) { + return Collections.emptyList(); + } + + String collection = refs.iterator().next().getCollectionName(); + + List ids = new ArrayList(refs.size()); + for (DBRef ref : refs) { + + if (!collection.equals(ref.getCollectionName())) { + throw new InvalidDataAccessApiUsageException( + "DBRefs must all target the same collection for bulk fetch operation."); + } + + ids.add(ref.getId()); + } + + DB db = mongoDbFactory.getDb(); + List result = db.getCollection(collection) + .find(new BasicDBObjectBuilder().add("_id", new BasicDBObject("$in", ids)).get()).toArray(); + Collections.sort(result, new DbRefByReferencePositionComperator(ids)); + return result; + } + /** * Creates a proxy for the given {@link MongoPersistentProperty} using the given {@link DbRefResolverCallback} to * eventually resolve the value of the property. @@ -395,4 +437,25 @@ public class DefaultDbRefResolver implements DbRefResolver { return result; } } + + /** + * {@link Comparator} for sorting {@link DBObject} that have been loaded in random order by a predefined list of + * reference ids. + * + * @author Christoph Strobl + * @since 1.10 + */ + private static class DbRefByReferencePositionComperator implements Comparator { + + List reference; + + public DbRefByReferencePositionComperator(List referenceIds) { + reference = new ArrayList(referenceIds); + } + + @Override + public int compare(DBObject o1, DBObject o2) { + return Integer.compare(reference.indexOf(o1.get("_id")), reference.indexOf(o2.get("_id"))); + } + } } 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 a8df88a04..ac220a2fc 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 @@ -901,9 +901,14 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App Collection items = targetType.getType().isArray() ? new ArrayList() : CollectionFactory.createCollection(collectionType, rawComponentType, sourceValue.size()); + if (isCollectionOfDbRefWhereBulkFetchIsPossible(sourceValue) && !DBRef.class.equals(rawComponentType)) { + return bulkReadAndConvertDBRefs((List) (ArrayList) sourceValue, componentType, path, rawComponentType); + } + for (Object dbObjItem : sourceValue) { if (dbObjItem instanceof DBRef) { + items.add(DBRef.class.equals(rawComponentType) ? dbObjItem : readAndConvertDBRef((DBRef) dbObjItem, componentType, path, rawComponentType)); } else if (dbObjItem instanceof DBObject) { @@ -916,6 +921,27 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App return getPotentiallyConvertedSimpleRead(items, targetType.getType()); } + private boolean isCollectionOfDbRefWhereBulkFetchIsPossible(Collection source) { + + String collection = null; + + for (Object dbObjItem : source) { + + if (!(dbObjItem instanceof DBRef)) { + return false; + } + + DBRef ref = (DBRef) dbObjItem; + + if (collection != null && !collection.equals(ref.getCollectionName())) { + return false; + } + collection = ref.getCollectionName(); + } + + return true; + } + /** * Reads the given {@link DBObject} into a {@link Map}. will recursively resolve nested {@link Map}s as well. * @@ -1215,23 +1241,41 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App return (T) (object != null ? object : readAndConvertDBRef(dbref, type, path, rawType)); } - @SuppressWarnings("unchecked") private T readAndConvertDBRef(DBRef dbref, TypeInformation type, ObjectPath path, final Class rawType) { - final DBObject readRef = readRef(dbref); - final String collectionName = dbref.getCollectionName(); + List result = bulkReadAndConvertDBRefs(Collections.singletonList(dbref), type, path, rawType); + return CollectionUtils.isEmpty(result) ? null : result.iterator().next(); + } - if (readRef != null) { - maybeEmitEvent(new AfterLoadEvent(readRef, (Class) rawType, collectionName)); + @SuppressWarnings("unchecked") + private List bulkReadAndConvertDBRefs(List dbrefs, TypeInformation type, ObjectPath path, + final Class rawType) { + + if (CollectionUtils.isEmpty(dbrefs)) { + return Collections.emptyList(); } - final T target = (T) read(type, readRef, path); + final List referencedRawDocuments = dbrefs.size() == 1 + ? Collections.singletonList(readRef(dbrefs.iterator().next())) : bulkReadRefs(dbrefs); + final String collectionName = dbrefs.iterator().next().getCollectionName(); - if (target != null) { - maybeEmitEvent(new AfterConvertEvent(readRef, target, collectionName)); + List targeList = new ArrayList(dbrefs.size()); + + for (DBObject document : referencedRawDocuments) { + + if (document != null) { + maybeEmitEvent(new AfterLoadEvent(document, (Class) rawType, collectionName)); + } + + final T target = (T) read(type, document, path); + targeList.add(target); + + if (target != null) { + maybeEmitEvent(new AfterConvertEvent(document, target, collectionName)); + } } - return target; + return targeList; } private void maybeEmitEvent(MongoMappingEvent event) { @@ -1255,6 +1299,17 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App return dbRefResolver.fetch(ref); } + /** + * Performs a bulk fetch operation for the given {@link DBRef}s. + * + * @param references must not be {@literal null}. + * @return never {@literal null}. + * @since 1.10 + */ + List bulkReadRefs(List references) { + return dbRefResolver.bulkFetch(references); + } + /** * 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/MongoTemplateTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateTests.java index ebb99b174..979f6c45b 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateTests.java @@ -95,6 +95,10 @@ import com.mongodb.ReadPreference; import com.mongodb.WriteConcern; import com.mongodb.WriteResult; +import lombok.Data; +import lombok.EqualsAndHashCode; +import lombok.NoArgsConstructor; + /** * Integration test for {@link MongoTemplate}. * @@ -3368,6 +3372,50 @@ public class MongoTemplateTests { assertThat(stream.hasNext(), is(false)); } + /** + * @see DATAMONGO-1194 + */ + @Test + public void shouldFetchListOfReferencesCorrectly() { + + Sample one = new Sample("1", "jon snow"); + Sample two = new Sample("2", "tyrion lannister"); + + template.save(one); + template.save(two); + + DocumentWithDBRefCollection source = new DocumentWithDBRefCollection(); + source.dbRefAnnotatedList = Arrays.asList(two, one); + + template.save(source); + + assertThat(template.findOne(query(where("id").is(source.id)), DocumentWithDBRefCollection.class), is(source)); + } + + /** + * @see DATAMONGO-1194 + */ + @Test + public void shouldFetchListOfLazyReferencesCorrectly() { + + Sample one = new Sample("1", "jon snow"); + Sample two = new Sample("2", "tyrion lannister"); + + template.save(one); + template.save(two); + + DocumentWithDBRefCollection source = new DocumentWithDBRefCollection(); + source.lazyDbRefAnnotatedList = Arrays.asList(two, one); + + template.save(source); + + DocumentWithDBRefCollection target = template.findOne(query(where("id").is(source.id)), + DocumentWithDBRefCollection.class); + + assertThat(target.lazyDbRefAnnotatedList, instanceOf(LazyLoadingProxy.class)); + assertThat(target.getLazyDbRefAnnotatedList(), contains(two, one)); + } + static class TypeWithNumbers { @Id String id; @@ -3427,6 +3475,7 @@ public class MongoTemplateTests { } + @Data static class DocumentWithDBRefCollection { @Id public String id; @@ -3437,6 +3486,10 @@ public class MongoTemplateTests { @org.springframework.data.mongodb.core.mapping.DBRef // public Sample dbRefProperty; + + @Field("lazy_db_ref_list") /** @see DATAMONGO-1194 */ + @org.springframework.data.mongodb.core.mapping.DBRef(lazy = true) // + public List lazyDbRefAnnotatedList; } static class DocumentWithCollection { @@ -3528,13 +3581,13 @@ public class MongoTemplateTests { @Id MyId id; } + @EqualsAndHashCode + @NoArgsConstructor static class Sample { @Id String id; String field; - public Sample() {} - public Sample(String id, String field) { this.id = id; this.field = field; @@ -3729,5 +3782,4 @@ public class MongoTemplateTests { String description; GeoJsonPoint point; } - } diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/DbRefMappingMongoConverterUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/DbRefMappingMongoConverterUnitTests.java index bc549004d..9cf3619b1 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/DbRefMappingMongoConverterUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/DbRefMappingMongoConverterUnitTests.java @@ -18,6 +18,7 @@ package org.springframework.data.mongodb.core.convert; import static org.hamcrest.CoreMatchers.*; import static org.junit.Assert.*; import static org.mockito.Matchers.*; +import static org.mockito.Matchers.any; import static org.mockito.Mockito.*; import static org.springframework.data.mongodb.core.convert.LazyLoadingTestUtils.*; @@ -35,6 +36,7 @@ import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mock; +import org.mockito.Mockito; import org.mockito.runners.MockitoJUnitRunner; import org.springframework.data.annotation.AccessType; import org.springframework.data.annotation.AccessType.Type; @@ -577,6 +579,68 @@ public class DbRefMappingMongoConverterUnitTests { assertProxyIsResolved(result.dbRefToPlainObject, false); } + /** + * @see DATAMONGO-1194 + */ + @Test + public void shouldBulkFetchListOfReferences() { + + String id1 = "1"; + String id2 = "2"; + String value = "val"; + + MappingMongoConverter converterSpy = spy(converter); + doReturn(Arrays.asList(new BasicDBObject("_id", id1).append("value", value), + new BasicDBObject("_id", id2).append("value", value))).when(converterSpy).bulkReadRefs(anyListOf(DBRef.class)); + + BasicDBObject dbo = new BasicDBObject(); + ClassWithLazyDbRefs lazyDbRefs = new ClassWithLazyDbRefs(); + lazyDbRefs.dbRefToConcreteCollection = new ArrayList( + Arrays.asList(new LazyDbRefTarget(id1, value), new LazyDbRefTarget(id2, value))); + converterSpy.write(lazyDbRefs, dbo); + + ClassWithLazyDbRefs result = converterSpy.read(ClassWithLazyDbRefs.class, dbo); + + assertProxyIsResolved(result.dbRefToConcreteCollection, false); + assertThat(result.dbRefToConcreteCollection.get(0).getId(), is(id1)); + assertProxyIsResolved(result.dbRefToConcreteCollection, true); + assertThat(result.dbRefToConcreteCollection.get(1).getId(), is(id2)); + + verify(converterSpy, never()).readRef(Mockito.any(DBRef.class)); + } + + /** + * @see DATAMONGO-1194 + */ + @Test + public void shouldFallbackToOneByOneFetchingWhenElementsInListOfReferencesPointToDifferentCollections() { + + String id1 = "1"; + String id2 = "2"; + String value = "val"; + + MappingMongoConverter converterSpy = spy(converter); + doReturn(new BasicDBObject("_id", id1).append("value", value)) + .doReturn(new BasicDBObject("_id", id2).append("value", value)).when(converterSpy) + .readRef(Mockito.any(DBRef.class)); + + BasicDBObject dbo = new BasicDBObject(); + ClassWithLazyDbRefs lazyDbRefs = new ClassWithLazyDbRefs(); + lazyDbRefs.dbRefToConcreteCollection = new ArrayList( + Arrays.asList(new LazyDbRefTarget(id1, value), new SerializableLazyDbRefTarget(id2, value))); + converterSpy.write(lazyDbRefs, dbo); + + ClassWithLazyDbRefs result = converterSpy.read(ClassWithLazyDbRefs.class, dbo); + + assertProxyIsResolved(result.dbRefToConcreteCollection, false); + assertThat(result.dbRefToConcreteCollection.get(0).getId(), is(id1)); + assertProxyIsResolved(result.dbRefToConcreteCollection, true); + assertThat(result.dbRefToConcreteCollection.get(1).getId(), is(id2)); + + verify(converterSpy, times(2)).readRef(Mockito.any(DBRef.class)); + verify(converterSpy, never()).bulkReadRefs(anyListOf(DBRef.class)); + } + private Object transport(Object result) { return SerializationUtils.deserialize(SerializationUtils.serialize(result)); } 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 new file mode 100644 index 000000000..1dda3b89f --- /dev/null +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/DefaultDbRefResolverUnitTests.java @@ -0,0 +1,129 @@ +/* + * Copyright 2016 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.mongodb.core.convert; + +import static org.hamcrest.collection.IsIterableContainingInOrder.contains; +import static org.hamcrest.collection.IsIterableWithSize.*; +import static org.junit.Assert.*; +import static org.mockito.Matchers.*; +import static org.mockito.Mockito.*; + +import java.util.Arrays; +import java.util.Collections; + +import org.bson.types.ObjectId; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; +import org.springframework.dao.InvalidDataAccessApiUsageException; +import org.springframework.data.mongodb.MongoDbFactory; +import org.springframework.data.mongodb.core.DBObjectTestUtils; + +import com.mongodb.BasicDBObject; +import com.mongodb.DB; +import com.mongodb.DBCollection; +import com.mongodb.DBCursor; +import com.mongodb.DBObject; +import com.mongodb.DBRef; + +/** + * @author Christoph Strobl + */ +@RunWith(MockitoJUnitRunner.class) +public class DefaultDbRefResolverUnitTests { + + @Mock MongoDbFactory factoryMock; + @Mock DB dbMock; + @Mock DBCollection collectionMock; + @Mock DBCursor cursorMock; + DefaultDbRefResolver resolver; + + @Before + public void setUp() { + + when(factoryMock.getDb()).thenReturn(dbMock); + when(dbMock.getCollection(anyString())).thenReturn(collectionMock); + when(collectionMock.find(any(DBObject.class))).thenReturn(cursorMock); + when(cursorMock.toArray()).thenReturn(Collections. emptyList()); + + resolver = new DefaultDbRefResolver(factoryMock); + } + + /** + * @see DATAMONGO-1194 + */ + @Test + @SuppressWarnings("unchecked") + public void bulkFetchShouldLoadDbRefsCorrectly() { + + DBRef ref1 = new DBRef("collection-1", new ObjectId()); + DBRef ref2 = new DBRef("collection-1", new ObjectId()); + + resolver.bulkFetch(Arrays.asList(ref1, ref2)); + + ArgumentCaptor captor = ArgumentCaptor.forClass(DBObject.class); + + verify(collectionMock, times(1)).find(captor.capture()); + + DBObject _id = DBObjectTestUtils.getAsDBObject(captor.getValue(), "_id"); + Iterable $in = DBObjectTestUtils.getTypedValue(_id, "$in", Iterable.class); + + assertThat($in, iterableWithSize(2)); + } + + /** + * @see DATAMONGO-1194 + */ + @Test(expected = InvalidDataAccessApiUsageException.class) + public void bulkFetchShouldThrowExceptionWhenUsingDifferntCollectionsWithinSetOfReferences() { + + DBRef ref1 = new DBRef("collection-1", new ObjectId()); + DBRef ref2 = new DBRef("collection-2", new ObjectId()); + + resolver.bulkFetch(Arrays.asList(ref1, ref2)); + } + + /** + * @see DATAMONGO-1194 + */ + @Test + public void bulkFetchShouldReturnEarlyForEmptyLists() { + + resolver.bulkFetch(Collections. emptyList()); + + verify(collectionMock, never()).find(any(DBObject.class)); + } + + /** + * @see DATAMONGO-1194 + */ + @Test + public void bulkFetchShouldRestoreOriginalOrder() { + + DBObject o1 = new BasicDBObject("_id", new ObjectId()); + DBObject o2 = new BasicDBObject("_id", new ObjectId()); + + DBRef ref1 = new DBRef("collection-1", o1.get("_id")); + DBRef ref2 = new DBRef("collection-1", o2.get("_id")); + + when(cursorMock.toArray()).thenReturn(Arrays.asList(o2, o1)); + + assertThat(resolver.bulkFetch(Arrays.asList(ref1, ref2)), contains(o1, o2)); + } +}