summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--jstests/aggregation/sources/group/group_by_empty_object.js19
-rw-r--r--jstests/aggregation/sources/group/group_by_variable.js5
-rw-r--r--src/mongo/db/pipeline/document_source_group_test.cpp37
-rw-r--r--src/mongo/db/pipeline/expression.cpp2
-rw-r--r--src/mongo/db/pipeline/expression_javascript_test.cpp6
-rw-r--r--src/mongo/db/pipeline/expression_test.cpp14
-rw-r--r--src/mongo/db/pipeline/group_processor_base.cpp34
7 files changed, 94 insertions, 23 deletions
diff --git a/jstests/aggregation/sources/group/group_by_empty_object.js b/jstests/aggregation/sources/group/group_by_empty_object.js
new file mode 100644
index 00000000000..1dc64a6ee1c
--- /dev/null
+++ b/jstests/aggregation/sources/group/group_by_empty_object.js
@@ -0,0 +1,19 @@
+/*
+ * Test that $group works when an empty object is passed for _id. This is intended to reproduce
+ * SERVER-89611.
+ */
+const coll = db.group_by_empty_obj;
+coll.drop();
+
+assert.commandWorked(coll.insert([{_id: 1, x: 1}, {_id: 2, x: 2}]));
+
+function assertIsEmptyObjId(groupSpec) {
+ assert.eq([{_id: {}}], coll.aggregate([groupSpec]).toArray());
+}
+assertIsEmptyObjId({$group: {_id: {}}});
+assertIsEmptyObjId({$group: {_id: {$expr: {}}}});
+assertIsEmptyObjId({$group: {_id: {$expr: {$const: {}}}}});
+assertIsEmptyObjId({$group: {_id: {$expr: {$expr: {}}}}});
+
+// The original fuzzer failure involved a $sortByCount query
+assert.eq([{_id: {}, count: 2}], coll.aggregate([{$sortByCount: {$expr: {}}}]).toArray());
diff --git a/jstests/aggregation/sources/group/group_by_variable.js b/jstests/aggregation/sources/group/group_by_variable.js
index ef08d7bb91b..f306a8eabd0 100644
--- a/jstests/aggregation/sources/group/group_by_variable.js
+++ b/jstests/aggregation/sources/group/group_by_variable.js
@@ -5,8 +5,7 @@
const coll = db.group_by_system_var;
coll.drop();
-assert.commandWorked(coll.insert({_id: 1, x: 1}));
-assert.commandWorked(coll.insert({_id: 2, x: 2}));
+assert.commandWorked(coll.insert([{_id: 1, x: 1}, {_id: 2, x: 2}]));
function checkPipeline(pipeline, expectedResults) {
const res = coll.aggregate(pipeline).toArray();
@@ -19,4 +18,4 @@ checkPipeline([{$group: {_id: "$$CURRENT"}}, {$sort: {"_id": 1}}], wholeCollUnde
const collIds = [{_id: 1}, {_id: 2}];
checkPipeline([{$group: {_id: "$$ROOT.x"}}, {$sort: {"_id": 1}}], collIds);
-checkPipeline([{$group: {_id: "$$CURRENT.x"}}, {$sort: {"_id": 1}}], collIds); \ No newline at end of file
+checkPipeline([{$group: {_id: "$$CURRENT.x"}}, {$sort: {"_id": 1}}], collIds);
diff --git a/src/mongo/db/pipeline/document_source_group_test.cpp b/src/mongo/db/pipeline/document_source_group_test.cpp
index 62fbfaebb1b..b861dc64025 100644
--- a/src/mongo/db/pipeline/document_source_group_test.cpp
+++ b/src/mongo/db/pipeline/document_source_group_test.cpp
@@ -358,6 +358,21 @@ TEST_F(DocumentSourceGroupTest, StreamingGroupRedactsCorrectly) {
redact(*docSource));
}
+TEST_F(DocumentSourceGroupTest, CanHandleEmptyExpressionObject) {
+ // Test the case where the _id expression is an empty object. This is handled as a special case,
+ // but is not directly reachable from the parser, so we will manually instantiate an
+ // ExpressionObject here.
+ auto idExpression = ExpressionObject::create(getExpCtx().get(), {});
+ std::vector<AccumulationStatement> accumulationStatements;
+ auto group = DocumentSourceGroup::create(getExpCtx(), idExpression, accumulationStatements);
+ auto mock = DocumentSourceMock::createForTest({Document{{"_id"_sd, 0}}}, getExpCtx());
+ group->setSource(mock.get());
+ auto next = group->getNext();
+ ASSERT(next.isAdvanced());
+ // The constant _id value from the $group spec is passed through.
+ ASSERT_DOCUMENT_EQ((Document{{"_id", Document{}}}), next.getDocument());
+}
+
BSONObj toBson(const boost::intrusive_ptr<DocumentSource>& source) {
std::vector<Value> arr;
source->serializeToArray(arr);
@@ -526,6 +541,26 @@ class IdEmptyObject : public IdConstantBase {
}
};
+/** $group _id is an empty object surrounded by a no-op $expr */
+class IdEmptyObjectWithExpr : public IdConstantBase {
+ BSONObj spec() override {
+ return BSON("_id" << BSON("$expr" << BSONObj()));
+ }
+ BSONObj expected() override {
+ return BSON("_id" << BSONObj());
+ }
+};
+
+/** $group _id is an empty object surrounded by multiple no-op $exprs */
+class IdEmptyObjectWithManyExpr : public IdConstantBase {
+ BSONObj spec() override {
+ return BSON("_id" << BSON("$expr" << BSON("$expr" << BSON("$expr" << BSONObj()))));
+ }
+ BSONObj expected() override {
+ return BSON("_id" << BSONObj());
+ }
+};
+
/** $group _id is computed from an object expression. */
class IdObjectExpression : public ExpressionBase {
BSONObj doc() {
@@ -1308,6 +1343,8 @@ public:
add<NonObject>();
add<EmptySpec>();
add<IdEmptyObject>();
+ add<IdEmptyObjectWithExpr>();
+ add<IdEmptyObjectWithManyExpr>();
add<IdObjectExpression>();
add<IdInvalidObjectExpression>();
add<TwoIdSpecs>();
diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp
index 1920e268aac..6859842fb52 100644
--- a/src/mongo/db/pipeline/expression.cpp
+++ b/src/mongo/db/pipeline/expression.cpp
@@ -154,7 +154,7 @@ intrusive_ptr<Expression> Expression::parseObject(ExpressionContext* const expCt
BSONObj obj,
const VariablesParseState& vps) {
if (obj.isEmpty()) {
- return ExpressionObject::create(expCtx, {});
+ return ExpressionConstant::create(expCtx, Value(Document{}));
}
if (obj.firstElementFieldName()[0] == '$') {
diff --git a/src/mongo/db/pipeline/expression_javascript_test.cpp b/src/mongo/db/pipeline/expression_javascript_test.cpp
index 51a8be82a20..d6cdf887071 100644
--- a/src/mongo/db/pipeline/expression_javascript_test.cpp
+++ b/src/mongo/db/pipeline/expression_javascript_test.cpp
@@ -164,8 +164,10 @@ TEST_F(MapReduceFixture, ExpressionFunctionFailsIfBodyNotSpecified) {
}
TEST_F(MapReduceFixture, ExpressionFunctionFailsIfBodyIsNotConstantExpression) {
- auto bsonExpr = BSON("expr" << BSON("body" << BSONObj() << "args" << BSON_ARRAY(1 << 2)
- << "lang" << ExpressionFunction::kJavaScript));
+ auto bsonExpr = BSON("expr" << BSON("body"
+ << "$a"
+ << "args" << BSON_ARRAY(1 << 2) << "lang"
+ << ExpressionFunction::kJavaScript));
ASSERT_THROWS_CODE(ExpressionFunction::parse(getExpCtxRaw(), bsonExpr.firstElement(), getVPS()),
AssertionException,
31432);
diff --git a/src/mongo/db/pipeline/expression_test.cpp b/src/mongo/db/pipeline/expression_test.cpp
index 743f46ee6cd..8ff42966922 100644
--- a/src/mongo/db/pipeline/expression_test.cpp
+++ b/src/mongo/db/pipeline/expression_test.cpp
@@ -1602,11 +1602,10 @@ boost::intrusive_ptr<Expression> parseObject(BSONObj specification) {
TEST(ParseObject, ShouldAcceptEmptyObject) {
auto resultExpression = parseObject(BSONObj());
- // Should return an empty ExpressionObject.
- auto resultObject = dynamic_cast<ExpressionObject*>(resultExpression.get());
+ // Should return an empty object.
+ auto resultObject = dynamic_cast<ExpressionConstant*>(resultExpression.get());
ASSERT_TRUE(resultObject);
-
- ASSERT_EQ(resultObject->getChildExpressions().size(), 0UL);
+ ASSERT_VALUE_EQ(resultObject->getValue(), Value(Document{}));
}
TEST(ParseObject, ShouldRecognizeKnownExpression) {
@@ -5126,6 +5125,13 @@ TEST(ExpressionParseParenthesisExpressionObjTest, SingleExprSimplification) {
ASSERT_EQ(expr->serialize().toString(), "[{$const: 123}]");
}
+TEST(ExpressionParseParenthesisExpressionObjTest, EmptyObject) {
+ auto expCtx = ExpressionContextForTest{};
+ auto specObject = fromjson("{$expr: {}}");
+ auto expr = Expression::parseObject(&expCtx, specObject, expCtx.variablesParseState);
+ ASSERT_EQ(expr->serialize().toString(), "{$const: {}}");
+}
+
/**
* Test case for round-trip conversion of random double using $convert.
*
diff --git a/src/mongo/db/pipeline/group_processor_base.cpp b/src/mongo/db/pipeline/group_processor_base.cpp
index 1f09cd0fcf8..5c5ae8c153b 100644
--- a/src/mongo/db/pipeline/group_processor_base.cpp
+++ b/src/mongo/db/pipeline/group_processor_base.cpp
@@ -76,20 +76,28 @@ void GroupProcessorBase::freeMemory() {
void GroupProcessorBase::setIdExpression(const boost::intrusive_ptr<Expression> idExpression) {
tassert(7801001, "Can't mutate _id fields after initialization", !_executionStarted);
- if (auto object = dynamic_cast<ExpressionObject*>(idExpression.get())) {
- auto& childExpressions = object->getChildExpressions();
- invariant(!childExpressions.empty()); // We expect to have converted an empty object into a
- // constant expression.
-
- // grouping on an "artificial" object. Rather than create the object for each input
- // in initialize(), instead group on the output of the raw expressions. The artificial
- // object will be created at the end in makeDocument() while outputting results.
- for (auto&& childExpPair : childExpressions) {
- _idFieldNames.push_back(childExpPair.first);
- _idExpressions.push_back(childExpPair.second);
- }
- } else {
+
+
+ auto object = dynamic_cast<ExpressionObject*>(idExpression.get());
+ if (!object || object->getChildExpressions().empty()) {
+ // Any single expression (including an empty object) can be directly computed and output
+ // without any custom handling or transformations. Note that we don't expect the parser to
+ // produce an empty object ExpressionObject (it should produce an ExpressionConstant
+ // instead), but we have been mistaken about that before in SERVER-89611, so we will handle
+ // this special case as well.
_idExpressions.push_back(idExpression);
+ return;
+ }
+
+ // For objects (e.g. from parsing {$group: {_id: {a: "$a", b: "$b"}}}), we do the following
+ // optimization to perform grouping on an "artificial" object. Rather than create the object for
+ // each input in initialize(), instead group on the output of the raw expressions. The
+ // artificial object will be created at the end in makeDocument() while outputting results.
+ auto& childExpressions = object->getChildExpressions();
+
+ for (auto&& childExpPair : childExpressions) {
+ _idFieldNames.push_back(childExpPair.first);
+ _idExpressions.push_back(childExpPair.second);
}
}
generated by cgit v1.2.3 (git 2.39.1) at 2026-09-11 11:41:14 +0000