From 361f9daa45f9ba4a2da264c4540b280bdad336d5 Mon Sep 17 00:00:00 2001 From: Thomas Darimont Date: Thu, 19 Sep 2013 16:58:14 +0200 Subject: [PATCH] DATAMONGO-753 - Add support for nested field references in aggregation operations. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Aggregation pipelines now correctly handle nested field references in aggregation operations. We introduced FieldsExposingAggregationOperation to mark AggregationOperations that change the set of exposed fields available for processing by later AggregationOperations. Extracted context state out of AggregationOperation to ExposedFieldsAggregationContext for better separation of concerns. Modified toDbObject(…) in Aggregation to only replace the aggregation context when the current AggregationOperation is a FieldExposingAggregationOperation. Original pull request: #74. --- .../mongodb/core/aggregation/Aggregation.java | 5 +- ...osedFieldsAggregationOperationContext.java | 27 +++++-- .../FieldsExposingAggregationOperation.java | 32 ++++++++ .../core/aggregation/GroupOperation.java | 2 +- .../core/aggregation/ProjectionOperation.java | 6 +- .../TypeBasedAggregationOperationContext.java | 4 +- .../core/aggregation/UnwindOperation.java | 11 +-- .../core/aggregation/AggregationTests.java | 75 ++++++++++++++++++- .../aggregation/AggregationUnitTests.java | 59 ++++++++++++++- 9 files changed, 191 insertions(+), 30 deletions(-) create mode 100644 spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/FieldsExposingAggregationOperation.java diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/Aggregation.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/Aggregation.java index 3dbc20299..f352c5728 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/Aggregation.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/Aggregation.java @@ -227,8 +227,9 @@ public class Aggregation { operationDocuments.add(operation.toDBObject(context)); - if (operation instanceof AggregationOperationContext) { - context = (AggregationOperationContext) operation; + if (operation instanceof FieldsExposingAggregationOperation) { + FieldsExposingAggregationOperation exposedFieldsOperation = (FieldsExposingAggregationOperation) operation; + context = new ExposedFieldsAggregationOperationContext(exposedFieldsOperation.getFields()); } } 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 8b6c517a2..88590bed8 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 @@ -17,17 +17,32 @@ package org.springframework.data.mongodb.core.aggregation; import org.springframework.data.mongodb.core.aggregation.ExposedFields.ExposedField; import org.springframework.data.mongodb.core.aggregation.ExposedFields.FieldReference; +import org.springframework.util.Assert; import com.mongodb.DBObject; /** - * Support class to implement {@link AggregationOperation}s that will become an {@link AggregationOperationContext} as - * well defining {@link ExposedFields}. + * {@link AggregationOperationContext} that combines the available field references from a given + * {@code AggregationOperationContext} and an {@link FieldsExposingAggregationOperation}. * + * @author Thomas Darimont * @author Oliver Gierke - * @since 1.3 + * @since 1.4 */ -public abstract class ExposedFieldsAggregationOperationContext implements AggregationOperationContext { +class ExposedFieldsAggregationOperationContext implements AggregationOperationContext { + + private final ExposedFields exposedFields; + + /** + * Creates a new {@link ExposedFieldsAggregationOperationContext} from the given {@link ExposedFields}. + * + * @param exposedFields must not be {@literal null}. + */ + public ExposedFieldsAggregationOperationContext(ExposedFields exposedFields) { + + Assert.notNull(exposedFields, "ExposedFields must not be null!"); + this.exposedFields = exposedFields; + } /* * (non-Javadoc) @@ -54,7 +69,7 @@ public abstract class ExposedFieldsAggregationOperationContext implements Aggreg @Override public FieldReference getReference(String name) { - ExposedField field = getFields().getField(name); + ExposedField field = exposedFields.getField(name); if (field != null) { return new FieldReference(field); @@ -62,6 +77,4 @@ public abstract class ExposedFieldsAggregationOperationContext implements Aggreg throw new IllegalArgumentException(String.format("Invalid reference '%s'!", name)); } - - protected abstract ExposedFields getFields(); } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/FieldsExposingAggregationOperation.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/FieldsExposingAggregationOperation.java new file mode 100644 index 000000000..c5addfe33 --- /dev/null +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/FieldsExposingAggregationOperation.java @@ -0,0 +1,32 @@ +/* + * Copyright 2013 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.aggregation; + +/** + * {@link AggregationOperation} that exposes new {@link ExposedFields} that can be used for later aggregation pipeline + * {@code AggregationOperation}s. + * + * @author Thomas Darimont + */ +public interface FieldsExposingAggregationOperation extends AggregationOperation { + + /** + * Returns the fields exposed by the {@link AggregationOperation}. + * + * @return will never be {@literal null}. + */ + ExposedFields getFields(); +} diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/GroupOperation.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/GroupOperation.java index e3d8f1f3a..9d16ee1bb 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/GroupOperation.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/GroupOperation.java @@ -38,7 +38,7 @@ import com.mongodb.DBObject; * @author Oliver Gierke * @since 1.3 */ -public class GroupOperation extends ExposedFieldsAggregationOperationContext implements AggregationOperation { +public class GroupOperation implements FieldsExposingAggregationOperation { private final ExposedFields nonSynthecticFields; private final List operations; diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/ProjectionOperation.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/ProjectionOperation.java index 5baf2c834..81b122535 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/ProjectionOperation.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/ProjectionOperation.java @@ -41,7 +41,7 @@ import com.mongodb.DBObject; * @author Oliver Gierke * @since 1.3 */ -public class ProjectionOperation extends ExposedFieldsAggregationOperationContext implements AggregationOperation { +public class ProjectionOperation implements FieldsExposingAggregationOperation { private static final List NONE = Collections.emptyList(); @@ -149,10 +149,10 @@ public class ProjectionOperation extends ExposedFieldsAggregationOperationContex /* * (non-Javadoc) - * @see org.springframework.data.mongodb.core.aggregation.ExposedFieldsAggregationOperationContext#getFields() + * @see org.springframework.data.mongodb.core.aggregation.AggregationOperationContextSupport#getFields() */ @Override - protected ExposedFields getFields() { + public ExposedFields getFields() { ExposedFields fields = null; diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/TypeBasedAggregationOperationContext.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/TypeBasedAggregationOperationContext.java index 16b0a6c0a..7441b8958 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/TypeBasedAggregationOperationContext.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/TypeBasedAggregationOperationContext.java @@ -71,9 +71,9 @@ public class TypeBasedAggregationOperationContext implements AggregationOperatio return mapper.getMappedObject(dbObject, mappingContext.getPersistentEntity(type)); } - /* + /* * (non-Javadoc) - * @see org.springframework.data.mongodb.core.aggregation.AggregationOperationContext#getReference(org.springframework.data.mongodb.core.aggregation.ExposedFields.AvailableField) + * @see org.springframework.data.mongodb.core.aggregation.AggregationOperationContext#getReference(org.springframework.data.mongodb.core.aggregation.Field) */ @Override public FieldReference getReference(Field field) { diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/UnwindOperation.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/UnwindOperation.java index 110bbd190..5410f79f3 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/UnwindOperation.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/UnwindOperation.java @@ -29,7 +29,7 @@ import com.mongodb.DBObject; * @author Oliver Gierke * @since 1.3 */ -public class UnwindOperation extends ExposedFieldsAggregationOperationContext implements AggregationOperation { +public class UnwindOperation implements AggregationOperation { private final ExposedField field; @@ -44,15 +44,6 @@ public class UnwindOperation extends ExposedFieldsAggregationOperationContext im this.field = new ExposedField(field, true); } - /* - * (non-Javadoc) - * @see org.springframework.data.mongodb.core.aggregation.ExposedFieldsAggregationOperationContext#getFields() - */ - @Override - protected ExposedFields getFields() { - return ExposedFields.from(field); - } - /* * (non-Javadoc) * @see org.springframework.data.mongodb.core.aggregation.AggregationOperation#toDBObject(org.springframework.data.mongodb.core.aggregation.AggregationOperationContext) 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 e017bfb2d..bd632c3ae 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 @@ -15,7 +15,7 @@ */ package org.springframework.data.mongodb.core.aggregation; -import static org.hamcrest.CoreMatchers.*; +import static org.hamcrest.Matchers.*; import static org.junit.Assert.*; import static org.springframework.data.domain.Sort.Direction.*; import static org.springframework.data.mongodb.core.aggregation.Aggregation.*; @@ -84,6 +84,7 @@ public class AggregationTests { mongoTemplate.dropCollection(INPUT_COLLECTION); mongoTemplate.dropCollection(Product.class); mongoTemplate.dropCollection(UserWithLikes.class); + mongoTemplate.dropCollection(DATAMONGO753.class); } /** @@ -452,7 +453,61 @@ public class AggregationTests { assertThat((Double) resultList.get(0).get("netPriceMul2"), is(netPrice * 2)); assertThat((Double) resultList.get(0).get("netPriceDiv119"), is(netPrice / 1.19)); assertThat((Integer) resultList.get(0).get("spaceUnitsMod2"), is(spaceUnits % 2)); + } + + /** + * @see DATAMONGO-753 + * @see http + * ://stackoverflow.com/questions/18653574/spring-data-mongodb-aggregation-framework-invalid-reference-in-group + * -operati + */ + @Test + public void allowsNestedFieldReferencesAsGroupIdsInGroupExpressions() { + + mongoTemplate.insert(new DATAMONGO753().withPDs(new PD("A", 1), new PD("B", 1), new PD("C", 1))); + mongoTemplate.insert(new DATAMONGO753().withPDs(new PD("B", 1), new PD("B", 1), new PD("C", 1))); + + TypedAggregation agg = newAggregation(DATAMONGO753.class, // + unwind("pd"), // + group("pd.pDch") // the nested field expression + .sum("pd.up").as("uplift"), // + project("_id", "uplift")); + AggregationResults result = mongoTemplate.aggregate(agg, DBObject.class); + List stats = result.getMappedResults(); + + assertThat(stats.size(), is(3)); + assertThat(stats.get(0).get("_id").toString(), is("C")); + assertThat((Integer) stats.get(0).get("uplift"), is(2)); + assertThat(stats.get(1).get("_id").toString(), is("B")); + assertThat((Integer) stats.get(1).get("uplift"), is(3)); + assertThat(stats.get(2).get("_id").toString(), is("A")); + assertThat((Integer) stats.get(2).get("uplift"), is(1)); + } + + /** + * @see DATAMONGO-753 + * @see http + * ://stackoverflow.com/questions/18653574/spring-data-mongodb-aggregation-framework-invalid-reference-in-group + * -operati + */ + @Test + public void aliasesNestedFieldInProjectionImmediately() { + + mongoTemplate.insert(new DATAMONGO753().withPDs(new PD("A", 1), new PD("B", 1), new PD("C", 1))); + mongoTemplate.insert(new DATAMONGO753().withPDs(new PD("B", 1), new PD("B", 1), new PD("C", 1))); + + TypedAggregation agg = newAggregation(DATAMONGO753.class, // + unwind("pd"), // + project().and("pd.up").as("up")); + + AggregationResults results = mongoTemplate.aggregate(agg, DBObject.class); + List mappedResults = results.getMappedResults(); + + assertThat(mappedResults, hasSize(6)); + for (DBObject element : mappedResults) { + assertThat(element.get("up"), is((Object) 1)); + } } private void assertLikeStats(LikeStats like, String id, long count) { @@ -502,4 +557,22 @@ public class AggregationTests { assertThat(tagCount.getN(), is(n)); } + static class DATAMONGO753 { + PD[] pd; + + DATAMONGO753 withPDs(PD... pds) { + this.pd = pds; + return this; + } + } + + static class PD { + String pDch; + @org.springframework.data.mongodb.core.mapping.Field("alias") int up; + + public PD(String pDch, int up) { + this.pDch = pDch; + this.up = up; + } + } } diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/aggregation/AggregationUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/aggregation/AggregationUnitTests.java index 6ae0ce90a..31251b16a 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/aggregation/AggregationUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/aggregation/AggregationUnitTests.java @@ -15,32 +15,83 @@ */ package org.springframework.data.mongodb.core.aggregation; +import static org.springframework.data.mongodb.core.aggregation.Aggregation.*; +import static org.springframework.data.mongodb.core.query.Criteria.*; + +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; /** * Unit tests for {@link Aggregation}. * * @author Oliver Gierke + * @author Thomas Darimont */ public class AggregationUnitTests { + public @Rule ExpectedException exception = ExpectedException.none(); + @Test(expected = IllegalArgumentException.class) public void rejectsNullAggregationOperation() { - Aggregation.newAggregation((AggregationOperation[]) null); + newAggregation((AggregationOperation[]) null); } @Test(expected = IllegalArgumentException.class) public void rejectsNullTypedAggregationOperation() { - Aggregation.newAggregation(String.class, (AggregationOperation[]) null); + newAggregation(String.class, (AggregationOperation[]) null); } @Test(expected = IllegalArgumentException.class) public void rejectsNoAggregationOperation() { - Aggregation.newAggregation(new AggregationOperation[0]); + newAggregation(new AggregationOperation[0]); } @Test(expected = IllegalArgumentException.class) public void rejectsNoTypedAggregationOperation() { - Aggregation.newAggregation(String.class, new AggregationOperation[0]); + newAggregation(String.class, new AggregationOperation[0]); + } + + /** + * @see DATAMONGO-753 + */ + @Test + public void checkForCorrectFieldScopeTransfer() { + + exception.expect(IllegalArgumentException.class); + exception.expectMessage("Invalid reference"); + exception.expectMessage("'b'"); + + newAggregation( // + project("a", "b"), // + group("a").count().as("cnt"), // a was introduced to the context by the project operation + project("cnt", "b") // b was removed from the context by the group operation + ).toDbObject("foo", Aggregation.DEFAULT_CONTEXT); // -> triggers IllegalArgumentException + } + + /** + * @see DATAMONGO-753 + */ + @Test + public void unwindOperationShouldNotChangeAvailableFields() { + + newAggregation( // + project("a", "b"), // + unwind("a"), // + project("a", "b") // b should still be available + ).toDbObject("foo", Aggregation.DEFAULT_CONTEXT); + } + + /** + * @see DATAMONGO-753 + */ + @Test + public void matchOperationShouldNotChangeAvailableFields() { + + newAggregation( // + project("a", "b"), // + match(where("a").gte(1)), // + project("a", "b") // b should still be available + ).toDbObject("foo", Aggregation.DEFAULT_CONTEXT); } }