Skip to content

Fix BucketOperationSupport _id field exposure. #5047

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
*
* @author Mark Paluch
* @author Christoph Strobl
* @author Amine Jaoui
* @since 1.10
*/
public abstract class BucketOperationSupport<T extends BucketOperationSupport<T, B>, B extends OutputBuilder<B, T>>
Expand Down Expand Up @@ -421,12 +422,13 @@ private Outputs(Collection<Output> current, Output output) {
*/
protected ExposedFields asExposedFields() {

// The _id is always present after the bucket operation
ExposedFields fields = ExposedFields.from(new ExposedField(Fields.UNDERSCORE_ID, true));
// The count field is included by default when the output is not specified.
if (isEmpty()) {
return ExposedFields.from(new ExposedField("count", true));
fields = fields.and(new ExposedField("count", true));
}

ExposedFields fields = ExposedFields.from();

for (Output output : outputs) {
fields = fields.and(output.getExposedField());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
* @author Oliver Gierke
* @author Christoph Strobl
* @author Mark Paluch
* @author Amine Jaoui
* @since 1.3
* @see <a href="https://docs.mongodb.com/manual/reference/operator/aggregation/project/">MongoDB Aggregation Framework:
* $project</a>
Expand Down Expand Up @@ -1404,14 +1405,6 @@ private Object renderFieldValue(AggregationOperationContext context) {
return field.getTarget();
}

if (field.getTarget().equals(Fields.UNDERSCORE_ID)) {
try {
return context.getReference(field).getReferenceValue();
} catch (java.lang.IllegalArgumentException e) {
return Fields.UNDERSCORE_ID_REF;
}
}

// check whether referenced field exists in the context
return context.getReference(field).getReferenceValue();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package org.springframework.data.mongodb.core.aggregation;

import static org.springframework.data.mongodb.core.DocumentTestUtils.getAsDocument;
import static org.springframework.data.mongodb.core.aggregation.AddFieldsOperation.addField;
import static org.springframework.data.mongodb.core.aggregation.Aggregation.DEFAULT_CONTEXT;
import static org.springframework.data.mongodb.core.aggregation.Aggregation.bucket;
import static org.springframework.data.mongodb.core.aggregation.Aggregation.group;
Expand Down Expand Up @@ -43,7 +44,6 @@
import org.springframework.data.domain.Sort;
import org.springframework.data.domain.Sort.Direction;
import org.springframework.data.mongodb.core.aggregation.ConditionalOperators.Cond;
import org.springframework.data.mongodb.core.aggregation.ProjectionOperationUnitTests.BookWithFieldAnnotation;
import org.springframework.data.mongodb.core.convert.MappingMongoConverter;
import org.springframework.data.mongodb.core.convert.NoOpDbRefResolver;
import org.springframework.data.mongodb.core.convert.QueryMapper;
Expand All @@ -61,6 +61,7 @@
* @author Christoph Strobl
* @author Mark Paluch
* @author Julia Lee
* @author Amine Jaoui
*/
public class AggregationUnitTests {

Expand Down Expand Up @@ -605,18 +606,22 @@ void shouldAllowInternalThisAndValueReferences() {
"{\"attributeRecordArrays\": {\"$reduce\": {\"input\": \"$attributeRecordArrays\", \"initialValue\": [], \"in\": {\"$concatArrays\": [\"$$value\", \"$$this\"]}}}}"));
}

@Test // DATAMONGO-2644
void projectOnIdIsAlwaysValid() {

MongoMappingContext mappingContext = new MongoMappingContext();
Document target = new Aggregation(bucket("start"), project("_id")).toDocument("collection-1",
new TypeBasedAggregationOperationContext(BookWithFieldAnnotation.class, mappingContext,
new QueryMapper(new MappingMongoConverter(NoOpDbRefResolver.INSTANCE, mappingContext)),
FieldLookupPolicy.relaxed()));
@Test // GH-5046
void projectOnBucketIntervalId() {

Document target = new Aggregation(bucket("start"), project("_id")).toDocument("collection-1", DEFAULT_CONTEXT);
assertThat(extractPipelineElement(target, 1, "$project")).isEqualTo(Document.parse(" { \"_id\" : 1 }"));
}

@Test // GH-5046
void addFieldFromBucketIntervalId() {

Document target = new Aggregation(bucket("start"), addField("bucketLabel").withValueOf("_id").build())
.toDocument("collection-1", DEFAULT_CONTEXT);
assertThat(extractPipelineElement(target, 1, "$addFields"))
.isEqualTo(Document.parse(" { \"bucketLabel\" : \"$_id\" }"));
}

@Test // GH-3898
void shouldNotConvertIncludeExcludeValuesForProjectOperation() {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@
*/
package org.springframework.data.mongodb.core.aggregation;

import static org.assertj.core.api.Assertions.*;
import static org.springframework.data.mongodb.core.aggregation.Aggregation.*;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
import static org.springframework.data.mongodb.core.aggregation.Aggregation.bucket;

import org.bson.Document;
import org.junit.jupiter.api.Test;
Expand All @@ -25,6 +27,7 @@
* Unit tests for {@link BucketOperation}.
*
* @author Mark Paluch
* @author Amine Jaoui
*/
public class BucketOperationUnitTests {

Expand Down Expand Up @@ -194,15 +197,26 @@ public void shouldRenderSumWithOwnOutputExpression() {
.isEqualTo(Document.parse("{ total : { $multiply: [ {$add : [\"$netPrice\", \"$tax\"]}, 5] } }"));
}

@Test // DATAMONGO-1552
public void shouldExposeDefaultCountField() {
@Test // GH-5046
public void shouldExposeDefaultFields() {

BucketOperation operation = bucket("field");

assertThat(operation.getFields().exposesSingleFieldOnly()).isTrue();
assertThat(operation.getFields().exposesNoNonSyntheticFields()).isTrue();
assertThat(operation.getFields().getField(Fields.UNDERSCORE_ID)).isNotNull();
assertThat(operation.getFields().getField("count")).isNotNull();
}

@Test // GH-5046
public void shouldExposeIDWhenCustomOutputField() {

BucketOperation operation = bucket("field").andOutputCount().as("score");

assertThat(operation.getFields().getField(Fields.UNDERSCORE_ID)).isNotNull();
assertThat(operation.getFields().getField("score")).isNotNull();
assertThat(operation.getFields().getField("count")).isNull();
}

private static Document extractOutput(Document fromBucketClause) {
return (Document) ((Document) fromBucketClause.get("$bucket")).get("output");
}
Expand Down