DATAMONGO-759 - Improved rendering of GroupOperation.

GroupOperation gets the _id field now rendered as null if no group fields were added to the operation. Previously it was rendered as empty document (i.e. { }). While this was technically correct as well, we're now closer to what the MongoDB reference documentation describes.

Original pull request: #73.

spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/ExposedFields.java

@@ -29,6 +29,7 @@ import org.springframework.util.CompositeIterator;
  * Value object to capture the fields exposed by an {@link AggregationOperation}.
  * 
  * @author Oliver Gierke
+ * @author Thomas Darimont
  * @since 1.3
  */
 public class ExposedFields implements Iterable<ExposedField> {
@@ -151,13 +152,47 @@ public class ExposedFields implements Iterable<ExposedField> {
 		return null;
 	}
 
+	/**
+	 * Returns whether the {@link ExposedFields} exposes no non-synthetic fields at all.
+	 * 
+	 * @return
+	 */
+	boolean exposesNoNonSyntheticFields() {
+		return originalFields.isEmpty();
+	}
+
+	/**
+	 * Returns whether the {@link ExposedFields} exposes a single non-synthetic field only.
+	 * 
+	 * @return
+	 */
+	boolean exposesSingleNonSyntheticFieldOnly() {
+		return originalFields.size() == 1;
+	}
+
+	/**
+	 * Returns whether the {@link ExposedFields} exposes no fields at all.
+	 * 
+	 * @return
+	 */
+	boolean exposesNoFields() {
+		return exposedFieldsCount() == 0;
+	}
+
 	/**
 	 * Returns whether the {@link ExposedFields} exposes a single field only.
 	 * 
 	 * @return
 	 */
-	public boolean exposesSingleFieldOnly() {
-		return originalFields.size() + syntheticFields.size() == 1;
+	boolean exposesSingleFieldOnly() {
+		return exposedFieldsCount() == 1;
+	}
+
+	/**
+	 * @return
+	 */
+	private int exposedFieldsCount() {
+		return originalFields.size() + syntheticFields.size();
 	}
 
 	/* 

spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/GroupOperation.java

@@ -40,7 +40,11 @@ import com.mongodb.DBObject;
  */
 public class GroupOperation implements FieldsExposingAggregationOperation {
 
-	private final ExposedFields nonSynthecticFields;
+	/**
+	 * Holds the non-synthetic fields which are the fields of the group-id structure.
+	 */
+	private final ExposedFields idFields;
+
 	private final List<Operation> operations;
 
 	/**
@@ -50,7 +54,7 @@ public class GroupOperation implements FieldsExposingAggregationOperation {
 	 */
 	public GroupOperation(Fields fields) {
 
-		this.nonSynthecticFields = ExposedFields.nonSynthetic(fields);
+		this.idFields = ExposedFields.nonSynthetic(fields);
 		this.operations = new ArrayList<Operation>();
 	}
 
@@ -74,7 +78,7 @@ public class GroupOperation implements FieldsExposingAggregationOperation {
 		Assert.notNull(groupOperation, "GroupOperation must not be null!");
 		Assert.notNull(nextOperations, "NextOperations must not be null!");
 
-		this.nonSynthecticFields = groupOperation.nonSynthecticFields;
+		this.idFields = groupOperation.idFields;
 		this.operations = new ArrayList<Operation>(nextOperations.size() + 1);
 		this.operations.addAll(groupOperation.operations);
 		this.operations.addAll(nextOperations);
@@ -261,7 +265,7 @@ public class GroupOperation implements FieldsExposingAggregationOperation {
 	@Override
 	public ExposedFields getFields() {
 
-		ExposedFields fields = this.nonSynthecticFields.and(new ExposedField(Fields.UNDERSCORE_ID, true));
+		ExposedFields fields = this.idFields.and(new ExposedField(Fields.UNDERSCORE_ID, true));
 
 		for (Operation operation : operations) {
 			fields = fields.and(operation.asField());
@@ -279,16 +283,20 @@ public class GroupOperation implements FieldsExposingAggregationOperation {
 
 		BasicDBObject operationObject = new BasicDBObject();
 
-		if (nonSynthecticFields.exposesSingleFieldOnly()) {
+		if (idFields.exposesNoNonSyntheticFields()) {
 
-			FieldReference reference = context.getReference(nonSynthecticFields.iterator().next());
+			operationObject.put(Fields.UNDERSCORE_ID, null);
+
+		} else if (idFields.exposesSingleNonSyntheticFieldOnly()) {
+
+			FieldReference reference = context.getReference(idFields.iterator().next());
 			operationObject.put(Fields.UNDERSCORE_ID, reference.toString());
 
 		} else {
 
 			BasicDBObject inner = new BasicDBObject();
 
-			for (ExposedField field : nonSynthecticFields) {
+			for (ExposedField field : idFields) {
 				FieldReference reference = context.getReference(field);
 				inner.put(field.getName(), reference.toString());
 			}

spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/ProjectionOperation.java

@@ -155,9 +155,9 @@ public class ProjectionOperation implements FieldsExposingAggregationOperation {
 		return new ProjectionOperation(this.projections, FieldProjection.from(fields, true));
 	}
 
-	/* 
+	/*
 	 * (non-Javadoc)
-	 * @see org.springframework.data.mongodb.core.aggregation.AggregationOperationContextSupport#getFields()
+	 * @see org.springframework.data.mongodb.core.aggregation.FieldsExposingAggregationOperation#getFields()
 	 */
 	@Override
 	public ExposedFields getFields() {

spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/aggregation/GroupOperationUnitTests.java

@@ -37,6 +37,38 @@ public class GroupOperationUnitTests {
 		new GroupOperation((Fields) null);
 	}
 
+	/**
+	 * @see DATAMONGO-759
+	 */
+	@Test
+	public void groupOperationWithNoGroupIdFieldsShouldGenerateNullAsGroupId() {
+
+		GroupOperation operation = new GroupOperation(Fields.from());
+		ExposedFields fields = operation.getFields();
+		DBObject groupClause = extractDbObjectFromGroupOperation(operation);
+
+		assertThat(fields.exposesSingleFieldOnly(), is(true));
+		assertThat(fields.exposesNoFields(), is(false));
+		assertThat(groupClause.get(UNDERSCORE_ID), is(nullValue()));
+	}
+
+	/**
+	 * @see DATAMONGO-759
+	 */
+	@Test
+	public void groupOperationWithNoGroupIdFieldsButAdditionalFieldsShouldGenerateNullAsGroupId() {
+
+		GroupOperation operation = new GroupOperation(Fields.from()).count().as("cnt").last("foo").as("foo");
+		ExposedFields fields = operation.getFields();
+		DBObject groupClause = extractDbObjectFromGroupOperation(operation);
+
+		assertThat(fields.exposesSingleFieldOnly(), is(false));
+		assertThat(fields.exposesNoFields(), is(false));
+		assertThat(groupClause.get(UNDERSCORE_ID), is(nullValue()));
+		assertThat((BasicDBObject) groupClause.get("cnt"), is(new BasicDBObject("$sum", 1)));
+		assertThat((BasicDBObject) groupClause.get("foo"), is(new BasicDBObject("$last", "$foo")));
+	}
+
 	@Test
 	public void createsGroupOperationWithSingleField() {