Skip to content

DATAMONGO-759 - Render group operation without non synthetic fields corr... #73

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

DATAMONGO-759 - Render group operation without non synthetic fields corr... #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 54 additions & 1 deletion ...ongodb/src/main/java/org/springframework/data/mongodb/core/aggregation/ExposedFields.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
* 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> {
Expand Down Expand Up @@ -151,13 +152,65 @@ public ExposedField getField(String name) {
return null;
}

/**
* Returns whether the {@link ExposedFields} exposes no non-synthetic fields at all.
*
* @return
*/
public boolean exposesNoNonSyntheticFields() {
return originalFields.isEmpty();
}

/**
* Returns whether the {@link ExposedFields} exposes a single non-synthetic field only.
*
* @return
*/
public boolean exposesSingleNonSyntheticFieldOnly() {
return originalFields.size() == 1;
}

/**
* Returns whether the {@link ExposedFields} exposes no synthetic fields at all.
*
* @return
*/
public boolean exposesNoSyntheticFields() {
return syntheticFields.isEmpty();
}

/**
* Returns whether the {@link ExposedFields} exposes a single synthetic field only.
*
* @return
*/
public boolean exposesSingleSytheticFieldOnly() {
return syntheticFields.size() == 1;
}

/**
* Returns whether the {@link ExposedFields} exposes no fields at all.
*
* @return
*/
public 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;
return exposedFieldsCount() == 1;
}

/**
* @return
*/
private int exposedFieldsCount() {
return originalFields.size() + syntheticFields.size();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have to look for the original fields only. Otherwise we eventually render a {} again as soon as someone adds a synthetic field to the group operation. Could be that we have to find a better name for what's currently isEmpty().

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? ExposesField combines "synthetic" as well as "original" fields hence a method like exposedFieldsCount should consider synthetic as well as original fields.

In addition to that I added a test case to verify that this is appropriate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not arguing about the name. If that needs to be different, fine. Maybe the issue I see is more driven from the variable name used in GroupOperation. In this class we hold an instance of ExposedFields (which can hold both synthetic and non-synthetic fields) but only store synthetic fields in it. Without realizing the latter, simply calling ….isEmpty() creates the impression we check for whether there are no synthetic and non-synthetic fields present. So we're using a method here that semantically only makes sense, because we use the type in a certain way. I'd argue adding a hasNonSyntheticFields() method only checking the non-synthetic fields is semantically clearer - both on the type and on the client side using the type in a very special way.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I choose to use exposesNoNonSyntheticFields() instead of hasNonSyntheticFields() in order to stay consistent with the other methods. Changed the name of nonSyntheticFields in GroupOperation to idFields to make their purpose more clear.

}

/*
Expand Down
22 changes: 15 additions & 7 deletions ...ngodb/src/main/java/org/springframework/data/mongodb/core/aggregation/GroupOperation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,11 @@
*/
public class GroupOperation extends ExposedFieldsAggregationOperationContext implements AggregationOperation {

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;

/**
Expand All @@ -50,7 +54,7 @@ public class GroupOperation extends ExposedFieldsAggregationOperationContext imp
*/
public GroupOperation(Fields fields) {

this.nonSynthecticFields = ExposedFields.nonSynthetic(fields);
this.idFields = ExposedFields.nonSynthetic(fields);
this.operations = new ArrayList<Operation>();
}

Expand All @@ -74,7 +78,7 @@ private GroupOperation(GroupOperation groupOperation, List<Operation> nextOperat
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);
Expand Down Expand Up @@ -261,7 +265,7 @@ private GroupOperationBuilder newBuilder(Keyword keyword, String reference, Obje
@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());
Expand All @@ -279,16 +283,20 @@ public com.mongodb.DBObject toDBObject(AggregationOperationContext context) {

BasicDBObject operationObject = new BasicDBObject();

if (nonSynthecticFields.exposesSingleFieldOnly()) {
if (idFields.exposesNoNonSyntheticFields()) {

operationObject.put(Fields.UNDERSCORE_ID, null);

} else if (idFields.exposesSingleNonSyntheticFieldOnly()) {

FieldReference reference = context.getReference(nonSynthecticFields.iterator().next());
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());
}
Expand Down
32 changes: 32 additions & 0 deletions .../test/java/org/springframework/data/mongodb/core/aggregation/GroupOperationUnitTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,38 @@ public void rejectsNullFields() {
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() {

Expand Down