From ff72150518f21ab5619dbc9077b46c772c705a8b Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Mon, 9 Dec 2013 16:24:54 +0100 Subject: [PATCH] DATAMONGO-806 - Fixed invalid rendering of id field references. Tweaked the rendering of projection operations to always use the field based reference lookup to make sure the reference gets rendered aliased. Moved value calculation logic into FieldReference. Original pull request: #101. --- .../core/aggregation/ExposedFields.java | 17 +++-- .../core/aggregation/ProjectionOperation.java | 4 +- .../TypeBasedAggregationOperationContext.java | 16 +++-- .../core/aggregation/AggregationTests.java | 70 +++++++++++++++++++ 4 files changed, 90 insertions(+), 17 deletions(-) diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/ExposedFields.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/ExposedFields.java index ab07a506c..934128d9d 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/ExposedFields.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/ExposedFields.java @@ -343,13 +343,6 @@ public class ExposedFields implements Iterable { this.field = field; } - /** - * @return - */ - public boolean isSynthetic() { - return field.synthetic; - } - /** * Returns the raw, unqualified reference, i.e. the field reference without a {@literal $} prefix. * @@ -360,6 +353,16 @@ public class ExposedFields implements Iterable { return field.synthetic ? target : String.format("%s.%s", Fields.UNDERSCORE_ID, target); } + /** + * Returns the referenve value for the given field reference. Will return 1 for a synthetic, unaliased field or the + * raw rendering of the reference otherwise. + * + * @return + */ + public Object getReferenceValue() { + return field.synthetic && !field.isAliased() ? 1 : toString(); + } + /* * (non-Javadoc) * @see java.lang.Object#toString() 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 5ebb27b32..e055984f5 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 @@ -21,7 +21,6 @@ import java.util.Collections; import java.util.List; import org.springframework.data.mongodb.core.aggregation.ExposedFields.ExposedField; -import org.springframework.data.mongodb.core.aggregation.ExposedFields.FieldReference; import org.springframework.data.mongodb.core.aggregation.ProjectionOperation.ProjectionOperationBuilder.FieldProjection; import org.springframework.util.Assert; @@ -506,8 +505,7 @@ public class ProjectionOperation implements FieldsExposingAggregationOperation { if (value == null || Boolean.TRUE.equals(value)) { // check whether referenced field exists in the context - FieldReference reference = context.getReference(field.getTarget()); - return reference.isSynthetic() && !field.isAliased() ? 1 : reference.toString(); + return context.getReference(field).getReferenceValue(); } else if (Boolean.FALSE.equals(value)) { 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 7441b8958..44c1928b4 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 @@ -88,15 +88,17 @@ public class TypeBasedAggregationOperationContext implements AggregationOperatio */ @Override public FieldReference getReference(String name) { - PropertyPath path = PropertyPath.from(name, type); - - PersistentPropertyPath propertyPath = mappingContext.getPersistentPropertyPath(path); - - return getReferenceFor(field(path.getLeafProperty().getSegment(), - propertyPath.toDotPath(MongoPersistentProperty.PropertyToFieldNameConverter.INSTANCE))); + return getReferenceFor(field(name)); } private FieldReference getReferenceFor(Field field) { - return new FieldReference(new ExposedField(field, true)); + + PropertyPath path = PropertyPath.from(field.getTarget(), type); + + PersistentPropertyPath propertyPath = mappingContext.getPersistentPropertyPath(path); + Field mappedField = field(propertyPath.getLeafProperty().getName(), + propertyPath.toDotPath(MongoPersistentProperty.PropertyToFieldNameConverter.INSTANCE)); + + return new FieldReference(new ExposedField(mappedField, true)); } } 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 0777d68e4..01af34c71 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 @@ -23,10 +23,12 @@ import static org.springframework.data.mongodb.core.query.Criteria.*; import java.io.BufferedInputStream; import java.util.ArrayList; +import java.util.Arrays; import java.util.Date; import java.util.List; import java.util.Scanner; +import org.joda.time.LocalDateTime; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -36,6 +38,7 @@ import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.core.io.ClassPathResource; import org.springframework.dao.DataAccessException; +import org.springframework.data.annotation.Id; import org.springframework.data.mongodb.core.CollectionCallback; import org.springframework.data.mongodb.core.MongoTemplate; import org.springframework.data.mongodb.core.query.Query; @@ -551,6 +554,40 @@ public class AggregationTests { assertThat((Integer) items.get(1).get("y"), is(1)); } + /** + * @see DATAMONGO-806 + */ + @Test + public void shouldAllowGroupByIdFields() { + + mongoTemplate.dropCollection(User.class); + + LocalDateTime now = new LocalDateTime(); + + User user1 = new User("u1", new PushMessage("1", "aaa", now.toDate())); + User user2 = new User("u2", new PushMessage("2", "bbb", now.minusDays(2).toDate())); + User user3 = new User("u3", new PushMessage("3", "ccc", now.minusDays(1).toDate())); + + mongoTemplate.save(user1); + mongoTemplate.save(user2); + mongoTemplate.save(user3); + + Aggregation agg = newAggregation( // + project("id", "msgs"), // + unwind("msgs"), // + match(where("msgs.createDate").gt(now.minusDays(1).toDate())), // + group("id").push("msgs").as("msgs") // + ); + + AggregationResults results = mongoTemplate.aggregate(agg, User.class, DBObject.class); + + List mappedResults = results.getMappedResults(); + + DBObject firstItem = mappedResults.get(0); + assertThat(firstItem.get("_id"), is(notNullValue())); + assertThat(String.valueOf(firstItem.get("_id")), is("u1")); + } + private void assertLikeStats(LikeStats like, String id, long count) { assertThat(like, is(notNullValue())); @@ -634,4 +671,37 @@ public class AggregationTests { } } + /** + * @see DATAMONGO-806 + */ + static class User { + + @Id String id; + List msgs; + + public User() {} + + public User(String id, PushMessage... msgs) { + this.id = id; + this.msgs = Arrays.asList(msgs); + } + } + + /** + * @see DATAMONGO-806 + */ + static class PushMessage { + + @Id String id; + String content; + Date createDate; + + public PushMessage() {} + + public PushMessage(String id, String content, Date createDate) { + this.id = id; + this.content = content; + this.createDate = createDate; + } + } }