From ef7870a3a43d03d314067a3178f88f7250211671 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 27 Jul 2023 11:42:36 +0200 Subject: [PATCH] Polishing. Use previous context instead of root for mapping objects within an Inheriting context. This avoids accidental mapping of fields against the root entity after eg. a projection stage. Add missing tests for AggregationOperationRenderer to ensure intended context propagation. Original Pull Request: #4459 --- .../AggregationOperationRenderer.java | 2 +- ...osedFieldsAggregationOperationContext.java | 5 - ...osedFieldsAggregationOperationContext.java | 7 ++ ...AggregationOperationRendererUnitTests.java | 118 ++++++++++++++++++ .../core/aggregation/AggregationTests.java | 19 +++ 5 files changed, 145 insertions(+), 6 deletions(-) create mode 100644 spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/aggregation/AggregationOperationRendererUnitTests.java diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/AggregationOperationRenderer.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/AggregationOperationRenderer.java index d62d376f0..13a2fe813 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/AggregationOperationRenderer.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/AggregationOperationRenderer.java @@ -64,7 +64,7 @@ class AggregationOperationRenderer { contextToUse = new InheritingExposedFieldsAggregationOperationContext(fields, contextToUse); } else { contextToUse = fields.exposesNoFields() ? DEFAULT_CONTEXT - : new ExposedFieldsAggregationOperationContext(exposedFieldsOperation.getFields(), contextToUse); + : new ExposedFieldsAggregationOperationContext(fields, contextToUse); } } } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/ExposedFieldsAggregationOperationContext.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/ExposedFieldsAggregationOperationContext.java index 82a4845b1..2877a5375 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/ExposedFieldsAggregationOperationContext.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/ExposedFieldsAggregationOperationContext.java @@ -67,11 +67,6 @@ class ExposedFieldsAggregationOperationContext implements AggregationOperationCo * (non-Javadoc) * @see org.springframework.data.mongodb.core.aggregation.AggregationOperationContext#getReference(org.springframework.data.mongodb.core.aggregation.ExposedFields.AvailableField) */ - @Override - public Document getMappedObject(Document document) { - return rootContext.getMappedObject(document); - } - @Override public FieldReference getReference(Field field) { diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/InheritingExposedFieldsAggregationOperationContext.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/InheritingExposedFieldsAggregationOperationContext.java index b5f28ec4b..a4e6a5d78 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/InheritingExposedFieldsAggregationOperationContext.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/InheritingExposedFieldsAggregationOperationContext.java @@ -15,6 +15,7 @@ */ package org.springframework.data.mongodb.core.aggregation; +import org.bson.Document; import org.springframework.data.mongodb.core.aggregation.ExposedFields.FieldReference; /** @@ -22,6 +23,7 @@ import org.springframework.data.mongodb.core.aggregation.ExposedFields.FieldRefe * {@link AggregationOperationContext}. * * @author Mark Paluch + * @author Christoph Strobl * @since 1.9 */ class InheritingExposedFieldsAggregationOperationContext extends ExposedFieldsAggregationOperationContext { @@ -47,6 +49,11 @@ class InheritingExposedFieldsAggregationOperationContext extends ExposedFieldsAg * (non-Javadoc) * @see org.springframework.data.mongodb.core.aggregation.ExposedFieldsAggregationOperationContext#resolveExposedField(org.springframework.data.mongodb.core.aggregation.Field, java.lang.String) */ + @Override + public Document getMappedObject(Document document) { + return previousContext.getMappedObject(document); + } + @Override protected FieldReference resolveExposedField(Field field, String name) { diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/aggregation/AggregationOperationRendererUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/aggregation/AggregationOperationRendererUnitTests.java new file mode 100644 index 000000000..54b018b12 --- /dev/null +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/aggregation/AggregationOperationRendererUnitTests.java @@ -0,0 +1,118 @@ +/* + * Copyright 2023 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 + * + * https://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.aggregation; + +import static org.assertj.core.api.Assertions.*; +import static org.mockito.Mockito.*; + +import java.util.List; + +import org.assertj.core.api.InstanceOfAssertFactories; +import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; +import org.springframework.data.mongodb.core.aggregation.FieldsExposingAggregationOperation.InheritsFieldsAggregationOperation; + +/** + * @author Christoph Strobl + */ +public class AggregationOperationRendererUnitTests { + + @Test // GH-4443 + void nonFieldsExposingAggregationOperationContinuesWithSameContextForNextStage() { + + AggregationOperationContext rootContext = mock(AggregationOperationContext.class); + AggregationOperation stage1 = mock(AggregationOperation.class); + AggregationOperation stage2 = mock(AggregationOperation.class); + + AggregationOperationRenderer.toDocument(List.of(stage1, stage2), rootContext); + + verify(stage1).toPipelineStages(eq(rootContext)); + verify(stage2).toPipelineStages(eq(rootContext)); + } + + @Test // GH-4443 + void fieldsExposingAggregationOperationNotExposingFieldsForcesUseOfDefaultContextForNextStage() { + + AggregationOperationContext rootContext = mock(AggregationOperationContext.class); + FieldsExposingAggregationOperation stage1 = mock(FieldsExposingAggregationOperation.class); + ExposedFields stage1fields = mock(ExposedFields.class); + AggregationOperation stage2 = mock(AggregationOperation.class); + + when(stage1.getFields()).thenReturn(stage1fields); + when(stage1fields.exposesNoFields()).thenReturn(true); + + AggregationOperationRenderer.toDocument(List.of(stage1, stage2), rootContext); + + verify(stage1).toPipelineStages(eq(rootContext)); + verify(stage2).toPipelineStages(eq(AggregationOperationRenderer.DEFAULT_CONTEXT)); + } + + @Test // GH-4443 + void fieldsExposingAggregationOperationForcesNewContextForNextStage() { + + AggregationOperationContext rootContext = mock(AggregationOperationContext.class); + FieldsExposingAggregationOperation stage1 = mock(FieldsExposingAggregationOperation.class); + ExposedFields stage1fields = mock(ExposedFields.class); + AggregationOperation stage2 = mock(AggregationOperation.class); + + when(stage1.getFields()).thenReturn(stage1fields); + when(stage1fields.exposesNoFields()).thenReturn(false); + + ArgumentCaptor captor = ArgumentCaptor.forClass(AggregationOperationContext.class); + + AggregationOperationRenderer.toDocument(List.of(stage1, stage2), rootContext); + + verify(stage1).toPipelineStages(eq(rootContext)); + verify(stage2).toPipelineStages(captor.capture()); + + assertThat(captor.getValue()).isInstanceOf(ExposedFieldsAggregationOperationContext.class) + .isNotInstanceOf(InheritingExposedFieldsAggregationOperationContext.class); + } + + @Test // GH-4443 + void inheritingFieldsExposingAggregationOperationForcesNewContextForNextStageKeepingReferenceToPreviousContext() { + + AggregationOperationContext rootContext = mock(AggregationOperationContext.class); + InheritsFieldsAggregationOperation stage1 = mock(InheritsFieldsAggregationOperation.class); + InheritsFieldsAggregationOperation stage2 = mock(InheritsFieldsAggregationOperation.class); + InheritsFieldsAggregationOperation stage3 = mock(InheritsFieldsAggregationOperation.class); + + ExposedFields exposedFields = mock(ExposedFields.class); + when(exposedFields.exposesNoFields()).thenReturn(false); + when(stage1.getFields()).thenReturn(exposedFields); + when(stage2.getFields()).thenReturn(exposedFields); + when(stage3.getFields()).thenReturn(exposedFields); + + ArgumentCaptor captor = ArgumentCaptor.forClass(AggregationOperationContext.class); + + AggregationOperationRenderer.toDocument(List.of(stage1, stage2, stage3), rootContext); + + verify(stage1).toPipelineStages(captor.capture()); + verify(stage2).toPipelineStages(captor.capture()); + verify(stage3).toPipelineStages(captor.capture()); + + assertThat(captor.getAllValues().get(0)).isEqualTo(rootContext); + + assertThat(captor.getAllValues().get(1)) + .asInstanceOf(InstanceOfAssertFactories.type(InheritingExposedFieldsAggregationOperationContext.class)) + .extracting("previousContext").isSameAs(captor.getAllValues().get(0)); + + assertThat(captor.getAllValues().get(2)) + .asInstanceOf(InstanceOfAssertFactories.type(InheritingExposedFieldsAggregationOperationContext.class)) + .extracting("previousContext").isSameAs(captor.getAllValues().get(1)); + } + +} diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/aggregation/AggregationTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/aggregation/AggregationTests.java index 6b594eef4..f67b8d109 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/aggregation/AggregationTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/aggregation/AggregationTests.java @@ -1980,6 +1980,25 @@ public class AggregationTests { assertThat(mappedResults.get(0)).containsEntry("item_id", "1"); } + @Test // GH-4443 + void projectShouldResetContextToAvoidMappingFieldsAgainstANoLongerExistingTarget() { + + Item item1 = Item.builder().itemId("1").tags(Arrays.asList("a", "b")).build(); + Item item2 = Item.builder().itemId("1").tags(Arrays.asList("a", "c")).build(); + mongoTemplate.insert(Arrays.asList(item1, item2), Item.class); + + TypedAggregation aggregation = newAggregation(Item.class, + match(where("itemId").is("1")), + unwind("tags"), + project().and("itemId").as("itemId").and("tags").as("tags"), + match(where("itemId").is("1").and("tags").is("c"))); + + AggregationResults results = mongoTemplate.aggregate(aggregation, Document.class); + List mappedResults = results.getMappedResults(); + assertThat(mappedResults).hasSize(1); + assertThat(mappedResults.get(0)).containsEntry("itemId", "1"); + } + private void createUsersWithReferencedPersons() { mongoTemplate.dropCollection(User.class);