From 5d50155d816d326554039bb11513465784ce87e7 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Wed, 13 Jul 2016 13:46:55 +0200 Subject: [PATCH] DATAMONGO-1194 - Improve DBRef resolution for maps. We bulk load maps 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 | 5 +- .../core/convert/DefaultDbRefResolver.java | 6 +- .../core/convert/MappingMongoConverter.java | 67 +++++++++++------ .../data/mongodb/core/MongoTemplateTests.java | 29 ++++++++ .../DbRefMappingMongoConverterUnitTests.java | 72 +++++++++++++++++++ 5 files changed, 152 insertions(+), 27 deletions(-) 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 8efedbee5..49cd715e3 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 @@ -17,6 +17,7 @@ package org.springframework.data.mongodb.core.convert; import java.util.List; +import org.springframework.dao.InvalidDataAccessApiUsageException; import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity; import org.springframework.data.mongodb.core.mapping.MongoPersistentProperty; @@ -68,11 +69,13 @@ public interface DbRefResolver { DBObject fetch(DBRef dbRef); /** - * Loads a given {@link List} of {@link DBRef}s from the datasource in one batch.
+ * Loads a given {@link List} of {@link DBRef}s from the datasource in one batch. The resulting {@link List} of + * {@link DBObject} will reflect the ordering of the {@link DBRef} passed in.
* The {@link DBRef} elements in the list must not reference different collections. * * @param dbRefs must not be {@literal null}. * @return never {@literal null}. + * @throws InvalidDataAccessApiUsageException in case not all {@link DBRef} target the same collection. * @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 850a8c7ba..346eaea97 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 @@ -147,7 +147,7 @@ public class DefaultDbRefResolver implements DbRefResolver { 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)); + Collections.sort(result, new DbRefByReferencePositionComparator(ids)); return result; } @@ -445,11 +445,11 @@ public class DefaultDbRefResolver implements DbRefResolver { * @author Christoph Strobl * @since 1.10 */ - private static class DbRefByReferencePositionComperator implements Comparator { + private static class DbRefByReferencePositionComparator implements Comparator { List reference; - public DbRefByReferencePositionComperator(List referenceIds) { + public DbRefByReferencePositionComparator(List referenceIds) { reference = new ArrayList(referenceIds); } 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 ac220a2fc..dff32f66e 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 @@ -883,6 +883,7 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App * @param path must not be {@literal null}. * @return the converted {@link Collection} or array, will never be {@literal null}. */ + @SuppressWarnings({ "rawtypes", "unchecked" }) private Object readCollectionOrArray(TypeInformation targetType, BasicDBList sourceValue, ObjectPath path) { Assert.notNull(targetType, "Target type must not be null!"); @@ -901,8 +902,8 @@ 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); + if (!DBRef.class.equals(rawComponentType) && isCollectionOfDbRefWhereBulkFetchIsPossible(sourceValue)) { + return bulkReadAndConvertDBRefs((List) (List) (sourceValue), componentType, path, rawComponentType); } for (Object dbObjItem : sourceValue) { @@ -921,27 +922,6 @@ 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. * @@ -967,6 +947,11 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App Map map = CollectionFactory.createMap(mapType, rawKeyType, dbObject.keySet().size()); Map sourceMap = dbObject.toMap(); + if (!DBRef.class.equals(rawValueType) && isCollectionOfDbRefWhereBulkFetchIsPossible(sourceMap.values())) { + bulkReadAndConvertDBRefMapIntoTarget(valueType, rawValueType, sourceMap, map); + return map; + } + for (Entry entry : sourceMap.entrySet()) { if (typeMapper.isTypeKey(entry.getKey())) { continue; @@ -1247,6 +1232,21 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App return CollectionUtils.isEmpty(result) ? null : result.iterator().next(); } + @SuppressWarnings({ "unchecked", "rawtypes" }) + private void bulkReadAndConvertDBRefMapIntoTarget(TypeInformation valueType, Class rawValueType, + Map sourceMap, Map targetMap) { + + LinkedHashMap referenceMap = new LinkedHashMap(sourceMap); + List convertedObjects = bulkReadAndConvertDBRefs((List) new ArrayList(referenceMap.values()), + valueType, ObjectPath.ROOT, rawValueType); + + int index = 0; + for (String key : referenceMap.keySet()) { + targetMap.put(key, convertedObjects.get(index)); + index++; + } + } + @SuppressWarnings("unchecked") private List bulkReadAndConvertDBRefs(List dbrefs, TypeInformation type, ObjectPath path, final Class rawType) { @@ -1278,6 +1278,27 @@ public class MappingMongoConverter extends AbstractMongoConverter implements App return targeList; } + 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; + } + private void maybeEmitEvent(MongoMappingEvent event) { if (canPublishEvent()) { 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 979f6c45b..e6d2bd049 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 @@ -33,6 +33,7 @@ import java.util.Collections; import java.util.Date; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.List; import java.util.Locale; import java.util.Map; @@ -3416,6 +3417,31 @@ public class MongoTemplateTests { assertThat(target.getLazyDbRefAnnotatedList(), contains(two, one)); } + /** + * @see DATAMONGO-1194 + */ + @Test + public void shouldFetchMapOfLazyReferencesCorrectly() { + + 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.lazyDbRefAnnotatedMap = new LinkedHashMap(); + source.lazyDbRefAnnotatedMap.put("tyrion", two); + source.lazyDbRefAnnotatedMap.put("jon", one); + template.save(source); + + DocumentWithDBRefCollection target = template.findOne(query(where("id").is(source.id)), + DocumentWithDBRefCollection.class); + + assertThat(target.lazyDbRefAnnotatedMap, instanceOf(LazyLoadingProxy.class)); + assertThat(target.lazyDbRefAnnotatedMap.values(), contains(two, one)); + } + static class TypeWithNumbers { @Id String id; @@ -3490,6 +3516,9 @@ public class MongoTemplateTests { @Field("lazy_db_ref_list") /** @see DATAMONGO-1194 */ @org.springframework.data.mongodb.core.mapping.DBRef(lazy = true) // public List lazyDbRefAnnotatedList; + + @Field("lazy_db_ref_map") /** @see DATAMONGO-1194 */ + @org.springframework.data.mongodb.core.mapping.DBRef(lazy = true) public Map lazyDbRefAnnotatedMap; } static class DocumentWithCollection { 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 9cf3619b1..75a033214 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 @@ -27,6 +27,7 @@ import java.math.BigInteger; import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -641,12 +642,83 @@ public class DbRefMappingMongoConverterUnitTests { verify(converterSpy, never()).bulkReadRefs(anyListOf(DBRef.class)); } + /** + * @see DATAMONGO-1194 + */ + @Test + public void shouldBulkFetchMapOfReferences() { + + MapDBRefVal val1 = new MapDBRefVal(); + val1.id = BigInteger.ONE; + + MapDBRefVal val2 = new MapDBRefVal(); + val2.id = BigInteger.ZERO; + + MappingMongoConverter converterSpy = spy(converter); + doReturn(Arrays.asList(new BasicDBObject("_id", val1.id), new BasicDBObject("_id", val2.id))).when(converterSpy) + .bulkReadRefs(anyListOf(DBRef.class)); + + BasicDBObject dbo = new BasicDBObject(); + MapDBRef mapDBRef = new MapDBRef(); + mapDBRef.map = new LinkedHashMap(); + mapDBRef.map.put("one", val1); + mapDBRef.map.put("two", val2); + + converterSpy.write(mapDBRef, dbo); + + MapDBRef result = converterSpy.read(MapDBRef.class, dbo); + + // assertProxyIsResolved(result.map, false); + assertThat(result.map.get("one").id, is(val1.id)); + // assertProxyIsResolved(result.map, true); + assertThat(result.map.get("two").id, is(val2.id)); + + verify(converterSpy, times(1)).bulkReadRefs(anyListOf(DBRef.class)); + verify(converterSpy, never()).readRef(Mockito.any(DBRef.class)); + } + + /** + * @see DATAMONGO-1194 + */ + @Test + public void shouldBulkFetchLazyMapOfReferences() { + + MapDBRefVal val1 = new MapDBRefVal(); + val1.id = BigInteger.ONE; + + MapDBRefVal val2 = new MapDBRefVal(); + val2.id = BigInteger.ZERO; + + MappingMongoConverter converterSpy = spy(converter); + doReturn(Arrays.asList(new BasicDBObject("_id", val1.id), new BasicDBObject("_id", val2.id))).when(converterSpy) + .bulkReadRefs(anyListOf(DBRef.class)); + + BasicDBObject dbo = new BasicDBObject(); + MapDBRef mapDBRef = new MapDBRef(); + mapDBRef.lazyMap = new LinkedHashMap(); + mapDBRef.lazyMap.put("one", val1); + mapDBRef.lazyMap.put("two", val2); + + converterSpy.write(mapDBRef, dbo); + + MapDBRef result = converterSpy.read(MapDBRef.class, dbo); + + assertProxyIsResolved(result.lazyMap, false); + assertThat(result.lazyMap.get("one").id, is(val1.id)); + assertProxyIsResolved(result.lazyMap, true); + assertThat(result.lazyMap.get("two").id, is(val2.id)); + + verify(converterSpy, times(1)).bulkReadRefs(anyListOf(DBRef.class)); + verify(converterSpy, never()).readRef(Mockito.any(DBRef.class)); + } + private Object transport(Object result) { return SerializationUtils.deserialize(SerializationUtils.serialize(result)); } class MapDBRef { @org.springframework.data.mongodb.core.mapping.DBRef Map map; + @org.springframework.data.mongodb.core.mapping.DBRef(lazy = true) Map lazyMap; } class MapDBRefVal {