From a92221279e52cada51744bfb1c412f0018b91c30 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 13 Jun 2024 13:26:50 +0200 Subject: [PATCH 1/3] Prepare issue branch. --- pom.xml | 2 +- spring-data-mongodb-benchmarks/pom.xml | 2 +- spring-data-mongodb-distribution/pom.xml | 2 +- spring-data-mongodb/pom.xml | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pom.xml b/pom.xml index de66da1866..7a5ae7e1b5 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-mongodb-parent - 4.4.0-SNAPSHOT + 4.4.x-GH-4722-SNAPSHOT pom Spring Data MongoDB diff --git a/spring-data-mongodb-benchmarks/pom.xml b/spring-data-mongodb-benchmarks/pom.xml index a3dc49f892..d50e04dd55 100644 --- a/spring-data-mongodb-benchmarks/pom.xml +++ b/spring-data-mongodb-benchmarks/pom.xml @@ -7,7 +7,7 @@ org.springframework.data spring-data-mongodb-parent - 4.4.0-SNAPSHOT + 4.4.x-GH-4722-SNAPSHOT ../pom.xml diff --git a/spring-data-mongodb-distribution/pom.xml b/spring-data-mongodb-distribution/pom.xml index e33930bfd2..3e03f8ddef 100644 --- a/spring-data-mongodb-distribution/pom.xml +++ b/spring-data-mongodb-distribution/pom.xml @@ -15,7 +15,7 @@ org.springframework.data spring-data-mongodb-parent - 4.4.0-SNAPSHOT + 4.4.x-GH-4722-SNAPSHOT ../pom.xml diff --git a/spring-data-mongodb/pom.xml b/spring-data-mongodb/pom.xml index fafe9c8793..67dee32271 100644 --- a/spring-data-mongodb/pom.xml +++ b/spring-data-mongodb/pom.xml @@ -13,7 +13,7 @@ org.springframework.data spring-data-mongodb-parent - 4.4.0-SNAPSHOT + 4.4.x-GH-4722-SNAPSHOT ../pom.xml From da656e007d4c7cf3bc7e61365585ed819e94d5a6 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 13 Jun 2024 15:20:48 +0200 Subject: [PATCH 2/3] Fix conversion of types when mapping Aggregation pipeline. This change makes sure to apply conversion to non native mongo types when the context does not expose fields. --- .../AggregationOperationRenderer.java | 35 ++++++++- ...AggregationOperationRendererUnitTests.java | 75 ++++++++++++++----- 2 files changed, 89 insertions(+), 21 deletions(-) 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 ea29f751de..c15eaaf85a 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 @@ -62,7 +62,7 @@ static List toDocument(List operations, Aggregat if (operation instanceof InheritsFieldsAggregationOperation || exposedFieldsOperation.inheritsFields()) { contextToUse = contextToUse.inheritAndExpose(fields); } else { - contextToUse = fields.exposesNoFields() ? DEFAULT_CONTEXT + contextToUse = fields.exposesNoFields() ? ConverterAwareNoOpContext.instance(rootContext) : contextToUse.expose(fields); } } @@ -72,6 +72,39 @@ static List toDocument(List operations, Aggregat return operationDocuments; } + private static class ConverterAwareNoOpContext implements AggregationOperationContext { + + AggregationOperationContext ctx; + + static ConverterAwareNoOpContext instance(AggregationOperationContext ctx) { + + if(ctx instanceof ConverterAwareNoOpContext noOpContext) { + return noOpContext; + } + + return new ConverterAwareNoOpContext(ctx); + } + + ConverterAwareNoOpContext(AggregationOperationContext ctx) { + this.ctx = ctx; + } + + @Override + public Document getMappedObject(Document document, @Nullable Class type) { + return ctx.getMappedObject(document, null); + } + + @Override + public FieldReference getReference(Field field) { + return new DirectFieldReference(new ExposedField(field, true)); + } + + @Override + public FieldReference getReference(String name) { + return new DirectFieldReference(new ExposedField(new AggregationField(name), true)); + } + } + /** * Simple {@link AggregationOperationContext} that just returns {@link FieldReference}s as is. * 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 index a8b32f957e..aab644702e 100644 --- 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 @@ -15,18 +15,29 @@ */ package org.springframework.data.mongodb.core.aggregation; -import static org.mockito.Mockito.*; -import static org.springframework.data.domain.Sort.Direction.*; -import static org.springframework.data.mongodb.core.aggregation.Aggregation.*; - +import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.springframework.data.domain.Sort.Direction.DESC; +import static org.springframework.data.mongodb.core.aggregation.Aggregation.project; +import static org.springframework.data.mongodb.core.aggregation.Aggregation.sort; + +import java.time.ZonedDateTime; import java.util.List; +import java.util.Set; +import org.assertj.core.api.Assertions; +import org.bson.Document; import org.junit.jupiter.api.Test; - import org.springframework.data.annotation.Id; +import org.springframework.data.convert.ConverterBuilder; +import org.springframework.data.convert.CustomConversions; +import org.springframework.data.convert.CustomConversions.StoreConversions; +import org.springframework.data.domain.Sort.Direction; import org.springframework.data.mongodb.core.convert.MappingMongoConverter; import org.springframework.data.mongodb.core.convert.NoOpDbRefResolver; import org.springframework.data.mongodb.core.convert.QueryMapper; +import org.springframework.data.mongodb.core.query.Criteria; import org.springframework.data.mongodb.test.util.MongoTestMappingContext; /** @@ -47,32 +58,56 @@ void nonFieldsExposingAggregationOperationContinuesWithSameContextForNextStage() verify(stage2).toPipelineStages(eq(rootContext)); } - record TestRecord(@Id String field1, String field2, LayerOne layerOne) { - record LayerOne(List layerTwo) { - } + @Test + void contextShouldCarryOnRelaxedFieldMapping() { - record LayerTwo(LayerThree layerThree) { - } + MongoTestMappingContext ctx = new MongoTestMappingContext(cfg -> { + cfg.initialEntitySet(TestRecord.class); + }); + + MappingMongoConverter mongoConverter = new MappingMongoConverter(NoOpDbRefResolver.INSTANCE, ctx); - record LayerThree(int fieldA, int fieldB) - {} + Aggregation agg = Aggregation.newAggregation(Aggregation.unwind("layerOne.layerTwo"), + project().and("layerOne.layerTwo.layerThree").as("layerOne.layerThree"), + sort(DESC, "layerOne.layerThree.fieldA")); + + AggregationOperationRenderer.toDocument(agg.getPipeline().getOperations(), + new RelaxedTypeBasedAggregationOperationContext(TestRecord.class, ctx, new QueryMapper(mongoConverter))); } - @Test - void xxx() { + @Test // GH-4722 + void appliesConversionToValuesUsedInAggregation() { MongoTestMappingContext ctx = new MongoTestMappingContext(cfg -> { cfg.initialEntitySet(TestRecord.class); }); MappingMongoConverter mongoConverter = new MappingMongoConverter(NoOpDbRefResolver.INSTANCE, ctx); - - Aggregation agg = Aggregation.newAggregation( - Aggregation.unwind("layerOne.layerTwo"), - project().and("layerOne.layerTwo.layerThree").as("layerOne.layerThree"), - sort(DESC, "layerOne.layerThree.fieldA") + mongoConverter.setCustomConversions(new CustomConversions(StoreConversions.NONE, + Set.copyOf(ConverterBuilder.writing(ZonedDateTime.class, String.class, ZonedDateTime::toString) + .andReading(it -> ZonedDateTime.parse(it)).getConverters()))); + mongoConverter.afterPropertiesSet(); + + var agg = Aggregation.newAggregation(Aggregation.sort(Direction.DESC, "version"), + Aggregation.group("entityId").first(Aggregation.ROOT).as("value"), Aggregation.replaceRoot("value"), + Aggregation.match(Criteria.where("createdDate").lt(ZonedDateTime.now())) // here is the problem ); - AggregationOperationRenderer.toDocument(agg.getPipeline().getOperations(), new RelaxedTypeBasedAggregationOperationContext(TestRecord.class, ctx, new QueryMapper(mongoConverter))); + List document = AggregationOperationRenderer.toDocument(agg.getPipeline().getOperations(), + new RelaxedTypeBasedAggregationOperationContext(TestRecord.class, ctx, new QueryMapper(mongoConverter))); + Assertions.assertThat(document).last() + .extracting(it -> it.getEmbedded(List.of("$match", "createdDate", "$lt"), Object.class)) + .isInstanceOf(String.class); + } + + record TestRecord(@Id String field1, String field2, LayerOne layerOne) { + record LayerOne(List layerTwo) { + } + + record LayerTwo(LayerThree layerThree) { + } + + record LayerThree(int fieldA, int fieldB) { + } } } From 153bd6548048f1a082f108451d19860dfda93fd0 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Fri, 14 Jun 2024 13:50:11 +0200 Subject: [PATCH 3/3] Additional test --- ...AggregationOperationRendererUnitTests.java | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) 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 index aab644702e..792a86249a 100644 --- 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 @@ -19,16 +19,21 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.springframework.data.domain.Sort.Direction.DESC; +import static org.springframework.data.mongodb.core.aggregation.Aggregation.newAggregation; import static org.springframework.data.mongodb.core.aggregation.Aggregation.project; import static org.springframework.data.mongodb.core.aggregation.Aggregation.sort; import java.time.ZonedDateTime; import java.util.List; import java.util.Set; +import java.util.stream.Stream; import org.assertj.core.api.Assertions; import org.bson.Document; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.springframework.data.annotation.Id; import org.springframework.data.convert.ConverterBuilder; import org.springframework.data.convert.CustomConversions; @@ -37,6 +42,7 @@ import org.springframework.data.mongodb.core.convert.MappingMongoConverter; import org.springframework.data.mongodb.core.convert.NoOpDbRefResolver; import org.springframework.data.mongodb.core.convert.QueryMapper; +import org.springframework.data.mongodb.core.mapping.Field; import org.springframework.data.mongodb.core.query.Criteria; import org.springframework.data.mongodb.test.util.MongoTestMappingContext; @@ -100,6 +106,37 @@ void appliesConversionToValuesUsedInAggregation() { .isInstanceOf(String.class); } + @ParameterizedTest // GH-4722 + @MethodSource("studentAggregationContexts") + void mapsOperationThatDoesNotExposeDedicatedFieldsCorrectly(AggregationOperationContext aggregationContext) { + + var agg = newAggregation(Student.class, Aggregation.unwind("grades"), Aggregation.replaceRoot("grades"), + Aggregation.project("grades")); + + List mappedPipeline = AggregationOperationRenderer.toDocument(agg.getPipeline().getOperations(), + aggregationContext); + + Assertions.assertThat(mappedPipeline).last().isEqualTo(Document.parse("{\"$project\": {\"grades\": 1}}")); + } + + private static Stream studentAggregationContexts() { + + MongoTestMappingContext ctx = new MongoTestMappingContext(cfg -> { + cfg.initialEntitySet(Student.class); + }); + + MappingMongoConverter mongoConverter = new MappingMongoConverter(NoOpDbRefResolver.INSTANCE, ctx); + mongoConverter.afterPropertiesSet(); + + QueryMapper queryMapper = new QueryMapper(mongoConverter); + + return Stream.of( + Arguments + .of(new TypeBasedAggregationOperationContext(Student.class, ctx, queryMapper, FieldLookupPolicy.strict())), + Arguments.of( + new TypeBasedAggregationOperationContext(Student.class, ctx, queryMapper, FieldLookupPolicy.relaxed()))); + } + record TestRecord(@Id String field1, String field2, LayerOne layerOne) { record LayerOne(List layerTwo) { } @@ -110,4 +147,17 @@ record LayerTwo(LayerThree layerThree) { record LayerThree(int fieldA, int fieldB) { } } + + static class Student { + + @Field("mark") List grades; + + } + + static class Grade { + + int points; + String grades; + } + }