From e2397450b065346341dc78702ac0968741e47191 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Tue, 19 Mar 2024 14:36:26 +0100 Subject: [PATCH] Polishing. Wrap no results predicate in dedicated lookup filter and make sure to omit loading attempt also for map properties. See #4612 Original pull request: #4613 --- .../mongodb/core/convert/ReferenceLoader.java | 26 ++++++++++ .../core/convert/ReferenceLookupDelegate.java | 48 +++++++++++-------- .../ReferenceLookupDelegateUnitTests.java | 42 ++++++++++------ 3 files changed, 80 insertions(+), 36 deletions(-) diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/ReferenceLoader.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/ReferenceLoader.java index 10da55773..cf7444e56 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/ReferenceLoader.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/ReferenceLoader.java @@ -21,6 +21,7 @@ import java.util.Iterator; import org.bson.Document; import org.bson.conversions.Bson; import org.springframework.data.mongodb.core.convert.ReferenceResolver.ReferenceCollection; +import org.springframework.data.mongodb.core.mapping.FieldName; import org.springframework.lang.Nullable; import com.mongodb.client.MongoCollection; @@ -126,5 +127,30 @@ public interface ReferenceLoader { } }; } + + /** + * @return a {@link DocumentReferenceQuery} that will not match any documents. + * @since 4.3 + */ + static DocumentReferenceQuery forNoResult() { + return NoResultsFilter.INSTANCE; + } + } + + /** + * A dedicated {@link DocumentReferenceQuery} that will not match any documents. + * + * @since 4.3 + */ + enum NoResultsFilter implements DocumentReferenceQuery { + INSTANCE; + + private static final Document NO_RESULTS_PREDICATE = new Document(FieldName.ID.name(), + new Document("$exists", false)); + + @Override + public Bson getQuery() { + return NO_RESULTS_PREDICATE; + } } } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/ReferenceLookupDelegate.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/ReferenceLookupDelegate.java index 8c628799b..3cd5f0e98 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/ReferenceLookupDelegate.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/ReferenceLookupDelegate.java @@ -34,10 +34,10 @@ import org.bson.conversions.Bson; import org.springframework.data.mapping.context.MappingContext; import org.springframework.data.mapping.model.SpELContext; import org.springframework.data.mongodb.core.convert.ReferenceLoader.DocumentReferenceQuery; +import org.springframework.data.mongodb.core.convert.ReferenceLoader.NoResultsFilter; import org.springframework.data.mongodb.core.convert.ReferenceResolver.MongoEntityReader; import org.springframework.data.mongodb.core.convert.ReferenceResolver.ReferenceCollection; import org.springframework.data.mongodb.core.mapping.DocumentReference; -import org.springframework.data.mongodb.core.mapping.FieldName; import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity; import org.springframework.data.mongodb.core.mapping.MongoPersistentProperty; import org.springframework.data.mongodb.util.BsonUtils; @@ -49,6 +49,7 @@ import org.springframework.data.util.Streamable; import org.springframework.expression.EvaluationContext; import org.springframework.lang.Nullable; import org.springframework.util.Assert; +import org.springframework.util.ObjectUtils; import org.springframework.util.StringUtils; import com.mongodb.DBRef; @@ -65,9 +66,6 @@ import com.mongodb.client.MongoCollection; */ public final class ReferenceLookupDelegate { - private static final Document NO_RESULTS_PREDICATE = new Document(FieldName.ID.name(), - new Document("$exists", false)); - private final MappingContext, MongoPersistentProperty> mappingContext; private final SpELContext spELContext; private final ParameterBindingDocumentCodec codec; @@ -104,18 +102,15 @@ public final class ReferenceLookupDelegate { public Object readReference(MongoPersistentProperty property, Object source, LookupFunction lookupFunction, MongoEntityReader entityReader) { - Object value = source instanceof DocumentReferenceSource documentReferenceSource ? documentReferenceSource.getTargetSource() + Object value = source instanceof DocumentReferenceSource documentReferenceSource + ? documentReferenceSource.getTargetSource() : source; - // GH-4612 no need to query database if target collection is empty - if (value != null && property.isCollectionLike() && (value instanceof Collection c) && c.isEmpty()) { - return new ArrayList<>(); - } - - DocumentReferenceQuery filter = computeFilter(property, source, spELContext); - ReferenceCollection referenceCollection = computeReferenceContext(property, value, spELContext); + Iterable result = retrieveRawDocuments(property, source, lookupFunction, value); - Iterable result = lookupFunction.apply(filter, referenceCollection); + if (result == null) { + return null; + } if (property.isCollectionLike()) { return entityReader.read(result, property.getTypeInformation()); @@ -129,6 +124,19 @@ public final class ReferenceLookupDelegate { return resultValue != null ? entityReader.read(resultValue, property.getTypeInformation()) : null; } + @Nullable + private Iterable retrieveRawDocuments(MongoPersistentProperty property, Object source, + LookupFunction lookupFunction, Object value) { + + DocumentReferenceQuery filter = computeFilter(property, source, spELContext); + if (filter instanceof NoResultsFilter) { + return Collections.emptyList(); + } + + ReferenceCollection referenceCollection = computeReferenceContext(property, value, spELContext); + return lookupFunction.apply(filter, referenceCollection); + } + private ReferenceCollection computeReferenceContext(MongoPersistentProperty property, Object value, SpELContext spELContext) { @@ -278,7 +286,7 @@ public final class ReferenceLookupDelegate { Collection objects = (Collection) value; if (objects.isEmpty()) { - return new ListDocumentReferenceQuery(NO_RESULTS_PREDICATE, sort); + return DocumentReferenceQuery.forNoResult(); } List ors = new ArrayList<>(objects.size()); @@ -293,11 +301,11 @@ public final class ReferenceLookupDelegate { if (property.isMap() && value instanceof Map) { - Set> entries = ((Map) value).entrySet(); - if (entries.isEmpty()) { - return new MapDocumentReferenceQuery(NO_RESULTS_PREDICATE, sort, Collections.emptyMap()); + if(ObjectUtils.isEmpty(value)) { + return DocumentReferenceQuery.forNoResult(); } + Set> entries = ((Map) value).entrySet(); Map filterMap = new LinkedHashMap<>(entries.size()); for (Entry entry : entries) { @@ -411,8 +419,7 @@ public final class ReferenceLookupDelegate { public Iterable restoreOrder(Iterable documents) { Map targetMap = new LinkedHashMap<>(); - List collected = documents instanceof List list ? list - : Streamable.of(documents).toList(); + List collected = documents instanceof List list ? list : Streamable.of(documents).toList(); for (Entry filterMapping : filterOrderMap.entrySet()) { @@ -444,8 +451,7 @@ public final class ReferenceLookupDelegate { @Override public Iterable restoreOrder(Iterable documents) { - List target = documents instanceof List list ? list - : Streamable.of(documents).toList(); + List target = documents instanceof List list ? list : Streamable.of(documents).toList(); if (!sort.isEmpty() || !query.containsKey("$or")) { return target; diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/ReferenceLookupDelegateUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/ReferenceLookupDelegateUnitTests.java index fd24a7e5c..bb5094a12 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/ReferenceLookupDelegateUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/ReferenceLookupDelegateUnitTests.java @@ -15,6 +15,11 @@ */ package org.springframework.data.mongodb.core.convert; +import static org.assertj.core.api.Assertions.*; +import static org.mockito.Mockito.*; + +import java.util.Collections; + import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -28,11 +33,6 @@ import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity; import org.springframework.data.mongodb.core.mapping.MongoPersistentProperty; import org.springframework.expression.EvaluationContext; -import java.util.Collections; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.*; - /** * Unit tests for {@link ReferenceLookupDelegate}. * @@ -53,16 +53,6 @@ class ReferenceLookupDelegateUnitTests { lookupDelegate = new ReferenceLookupDelegate(mappingContext, spELContext); } - @Test // GH-4612 - void shouldResolveEmptyListOnEmptyTargetCollection() { - MongoPersistentProperty property = mock(MongoPersistentProperty.class); - ReferenceLookupDelegate.LookupFunction lookupFunction = mock(ReferenceLookupDelegate.LookupFunction.class); - - when(property.isCollectionLike()).thenReturn(true); - lookupDelegate.readReference(property, Collections.emptyList(), lookupFunction, entityReader); - verify(lookupFunction, never()).apply(any(), any()); - } - @Test // GH-3842 void shouldComputePlainStringTargetCollection() { @@ -82,4 +72,26 @@ class ReferenceLookupDelegateUnitTests { return Collections.emptyList(); }, entityReader); } + + @Test // GH-4612 + void shouldResolveEmptyListOnEmptyTargetCollection() { + + MongoPersistentProperty property = mock(MongoPersistentProperty.class); + ReferenceLookupDelegate.LookupFunction lookupFunction = mock(ReferenceLookupDelegate.LookupFunction.class); + + when(property.isCollectionLike()).thenReturn(true); + lookupDelegate.readReference(property, Collections.emptyList(), lookupFunction, entityReader); + verify(lookupFunction, never()).apply(any(), any()); + } + + @Test // GH-4612 + void shouldResolveEmptyMapOnEmptyTargetCollection() { + + MongoPersistentProperty property = mock(MongoPersistentProperty.class); + ReferenceLookupDelegate.LookupFunction lookupFunction = mock(ReferenceLookupDelegate.LookupFunction.class); + + when(property.isMap()).thenReturn(true); + lookupDelegate.readReference(property, Collections.emptyMap(), lookupFunction, entityReader); + verify(lookupFunction, never()).apply(any(), any()); + } }