SERVER-89611 Fix $group with empty object expressionr7.3.2-rc1r7.3.2

GitOrigin-RevId: cb58a79ea634ff332b6b6f3c145571db23a09c95

5 files changed, 73 insertions, 20 deletions

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);
     }
 }