SERVER-90869 Disallow dotted full-path renames for '$elemMatch' expressions (#23108) (#23252)r7.0.12-rc0

GitOrigin-RevId: c2ec0b501cd9f0a5b380d081c6e063af87cf98bb

5 files changed, 204 insertions, 56 deletions

diff --git a/etc/backports_required_for_multiversion_tests.yml b/etc/backports_required_for_multiversion_tests.yml
index 69d25e315e4..952f839449f 100644
--- a/etc/backports_required_for_multiversion_tests.yml
+++ b/etc/backports_required_for_multiversion_tests.yml
@@ -505,6 +505,8 @@ last-continuous:
     ticket: SERVER-90485
   - test_file: jstests/aggregation/group_by_objectid.js
     ticket: SERVER-90173
+  - test_file: jstests/aggregation/match_swapping_renamed_fields.js
+    ticket: SERVER-90869
   - test_file: jstests/replsets/catchup_ignores_old_heartbeats.js
     ticket: SERVER-86674
   - test_file: jstests/replsets/snapshot_read_drop_collection.js
@@ -1068,6 +1070,8 @@ last-lts:
     ticket: SERVER-90485
   - test_file: jstests/aggregation/group_by_objectid.js
     ticket: SERVER-90173
+  - test_file: jstests/aggregation/match_swapping_renamed_fields.js
+    ticket: SERVER-90869
   - test_file: jstests/replsets/catchup_ignores_old_heartbeats.js
     ticket: SERVER-86674
   - test_file: jstests/replsets/snapshot_read_drop_collection.js
diff --git a/jstests/aggregation/match_swapping_renamed_fields.js b/jstests/aggregation/match_swapping_renamed_fields.js
index 1657282e735..20f81b306ab 100644
--- a/jstests/aggregation/match_swapping_renamed_fields.js
+++ b/jstests/aggregation/match_swapping_renamed_fields.js
@@ -11,6 +11,14 @@
 
 load("jstests/libs/analyze_plan.js");
 
+/**
+ * Utillity for inhibiting aggregate stage pushdowns to find-land queries. It doesn't prohibit other
+ * pipeline optimisation techniques such as stage reordering & renames from being applied.
+ */
+function pipelineWithoutOptimizations(pipeline) {
+    return [{$_internalInhibitOptimization: {}}, ...pipeline];
+}
+
 let coll = db.match_swapping_renamed_fields;
 coll.drop();
 
@@ -196,12 +204,107 @@ assert.neq(null, ixscan, tojson(explain));
 assert.eq({"a.b.c": 1}, ixscan.keyPattern, tojson(ixscan));
 
 // Test multiple renames. Designed to reproduce SERVER-32690.
-pipeline =
-    [{$_internalInhibitOptimization: {}}, {$project: {x: "$x", y: "$x"}}, {$match: {y: 1, w: 1}}];
+pipeline = [{$project: {x: "$x", y: "$x"}}, {$match: {y: 1, w: 1}}];
 assert.eq([], coll.aggregate(pipeline).toArray());
-explain = coll.explain().aggregate(pipeline);
+explain = coll.explain().aggregate(pipelineWithoutOptimizations(pipeline));
 // We expect that the $match stage has been split into two, since one predicate has an
 // applicable rename that allows swapping, while the other does not.
 let matchStages = getAggPlanStages(explain, "$match");
 assert.eq(2, matchStages.length);
+
+// Test that we correctly match using the '$elemMatch' expression on renamed subfields. Designed to
+// reproduce HELP-59485.
+coll.drop();
+assert.commandWorked(coll.insertMany([
+    {
+        _id: 0,
+        otherField: "same-string",
+        outer: undefined,
+    },
+    {
+        _id: 1,
+        otherField: "same-string",
+        outer: [{inner: true}],
+    },
+    {
+        _id: 2,
+        otherField: "same-string",
+        outer: [[], [[{inner: true}]]],
+    },
+    {
+        _id: 3,
+        otherField: "same-string",
+        outer: [[], [[]], [[[{inner: true}]]]],
+    },
+    {
+        _id: 4,
+        otherField: "same-string",
+        outer: [{inner: [true]}],
+    },
+]));
+
+function runElemMatchTest({pipeline, expectedDocumentIds}) {
+    const extractDocumentId = ({_id}) => _id;
+    const actualIds = coll.aggregate(pipeline).toArray().map(extractDocumentId);
+    assert.eq(actualIds, expectedDocumentIds);
+    // Expect the '$match' expression to be split into two parts, since the 'otherField' rename is
+    // still a valid rewrite.
+    const explain = coll.explain().aggregate(pipelineWithoutOptimizations(pipeline));
+    const matchStages = getAggPlanStages(explain, "$match");
+    assert.eq(2, matchStages.length);
+}
+
+runElemMatchTest({
+    pipeline: [
+        {
+            $addFields: {
+                flattened: {
+                    $map: {input: '$outer', as: "iter", in : "$$iter.inner"},
+                },
+                renamedOtherField: "$otherField"
+            },
+        },
+        {
+            $match: {flattened: {$elemMatch: {$eq: true}}, renamedOtherField: "same-string"},
+        }
+    ],
+    expectedDocumentIds: [1]
+});
+
+// Repeat the previous test case, but this time with a $project stage targeting a deeply nested
+// transform.
+runElemMatchTest({
+    pipeline: [
+        {
+            $project: {
+                a: {
+                    b: {
+                        c: {
+                            $map: {input: '$outer', as: "iter", in : "$$iter.inner"},
+                        }
+                    }
+                },
+                renamedOtherField: "$otherField"
+            }
+        },
+        {
+            $match: {"a.b.c": {$elemMatch: {$eq: true}}, renamedOtherField: "same-string"},
+        }
+    ],
+    expectedDocumentIds: [1],
+});
+
+// Similarly, ensure that we match on the correct documents when using $elemMatch expressions on
+// simple dot-syntax renamed fields.
+runElemMatchTest({
+    pipeline: [
+        {
+            $project: {flattened: "$outer.inner", renamedOtherField: "$otherField"},
+        },
+        {
+            $match: {flattened: {$elemMatch: {$eq: true}}, renamedOtherField: "same-string"},
+        }
+    ],
+    expectedDocumentIds: [1]
+});
 }());
diff --git a/jstests/change_streams/oplog_rewrite/change_stream_match_pushdown_fullDocument_rewrite.js b/jstests/change_streams/oplog_rewrite/change_stream_match_pushdown_fullDocument_rewrite.js
index b149335a069..f209220bc6c 100644
--- a/jstests/change_streams/oplog_rewrite/change_stream_match_pushdown_fullDocument_rewrite.js
+++ b/jstests/change_streams/oplog_rewrite/change_stream_match_pushdown_fullDocument_rewrite.js
@@ -55,14 +55,18 @@ function verifyOnWholeCluster(userMatchExpr,
 
 // These operations will create oplog events. The change stream will apply several filters on these
 // series of events and ensure that the '$match' expressions are rewritten correctly.
-assert.commandWorked(coll.insert({_id: 2, shard: 0}));
-assert.commandWorked(coll.insert({_id: 3, shard: 0}));
-assert.commandWorked(coll.insert({_id: 2, shard: 1}));
-assert.commandWorked(coll.insert({_id: 3, shard: 1}));
-assert.commandWorked(coll.replaceOne({_id: 2, shard: 0}, {_id: 2, shard: 0, foo: "a"}));
-assert.commandWorked(coll.replaceOne({_id: 3, shard: 0}, {_id: 3, shard: 0, foo: "a"}));
-assert.commandWorked(coll.replaceOne({_id: 2, shard: 1}, {_id: 2, shard: 1, foo: "a"}));
-assert.commandWorked(coll.replaceOne({_id: 3, shard: 1}, {_id: 3, shard: 1, foo: "a"}));
+assert.commandWorked(coll.insert({_id: 2, shard: 0, arr: [0, 2]}));
+assert.commandWorked(coll.insert({_id: 3, shard: 0, arr: [1, 3]}));
+assert.commandWorked(coll.insert({_id: 2, shard: 1, arr: [0, 2]}));
+assert.commandWorked(coll.insert({_id: 3, shard: 1, arr: [1, 3]}));
+assert.commandWorked(
+    coll.replaceOne({_id: 2, shard: 0}, {_id: 2, shard: 0, arr: [0, 2], foo: "a"}));
+assert.commandWorked(
+    coll.replaceOne({_id: 3, shard: 0}, {_id: 3, shard: 0, arr: [1, 3], foo: "a"}));
+assert.commandWorked(
+    coll.replaceOne({_id: 2, shard: 1}, {_id: 2, shard: 1, arr: [0, 2], foo: "a"}));
+assert.commandWorked(
+    coll.replaceOne({_id: 3, shard: 1}, {_id: 3, shard: 1, arr: [1, 3], foo: "a"}));
 assert.commandWorked(coll.update({_id: 2, shard: 0}, {$set: {foo: "b"}}));
 assert.commandWorked(coll.update({_id: 3, shard: 0}, {$set: {foo: "b"}}));
 assert.commandWorked(coll.update({_id: 2, shard: 1}, {$set: {foo: "b"}}));
@@ -190,7 +194,7 @@ const runVerifyOpsTestcases = (op) => {
     // Initialize 'doc' so that it matches the 'fullDocument' field of one of the events where
     // operationType == 'op'. For all 'insert' events, 'fullDocument' only has the '_id' field and
     // the 'shard' field. For 'replace' and 'update' events, 'fullDocument' also has a 'foo' field.
-    const doc = {_id: 2, shard: 0};
+    const doc = {_id: 2, shard: 0, arr: [0, 2]};
     if (op != "insert") {
         doc.foo = (op == "replace" ? "a" : "b");
     }
@@ -218,6 +222,14 @@ const runVerifyOpsTestcases = (op) => {
                          op != "update" ? [0, 2] : [2, 2] /* expectedOplogRetDocsForEachShard */,
                          [0, 2] /* expectedChangeStreamDocsForEachShard */);
 
+    // Test out a $elemMatch predicate on 'fullDocument.arr'.
+    const isEvenExpr = {$mod: [2, 0]};
+    verifyOnWholeCluster(
+        {$match: {operationType: op, "fullDocument.arr": {$elemMatch: isEvenExpr}}},
+        {[collName]: {[op]: [2, 2]}},
+        op != "update" ? [1, 1] : [2, 2] /* expectedOplogRetDocsForEachShard */,
+        [1, 1] /* expectedChangeStreamDocsForEachShard */);
+
     // Test out a negated predicate on the full 'fullDocument' field.
     verifyOnWholeCluster({$match: {operationType: op, fullDocument: {$not: {$eq: doc}}}},
                          {[collName]: {[op]: [3, 2, 3]}},
diff --git a/src/mongo/db/matcher/expression_path.h b/src/mongo/db/matcher/expression_path.h
index 3c4f88269ab..6069d817c98 100644
--- a/src/mongo/db/matcher/expression_path.h
+++ b/src/mongo/db/matcher/expression_path.h
@@ -151,28 +151,46 @@ public:
         const StringMap<std::string>& renameList) const {
         invariant(_elementPath);
 
+        const bool isElemMatch = matchType() == MatchType::ELEM_MATCH_VALUE ||
+            matchType() == MatchType::ELEM_MATCH_OBJECT;
         size_t renamesFound = 0u;
-        std::string rewrittenPath;
-        for (const auto& rename : renameList) {
-            if (rename.first == path()) {
-                rewrittenPath = rename.second;
-
+        FieldRef rewrittenPathRef;
+        for (const auto& [src, dst] : renameList) {
+            if (src == path()) {
+                rewrittenPathRef.parse(dst);
+                if (isElemMatch && rewrittenPathRef.numParts() > 1) {
+                    // Inhibit full-path renames for '$elemMatch' field paths containing multiple
+                    // components. Doing so might alter the semantics of the original expression by
+                    // replacing array re-shaping operations with non-compatible implicit array
+                    // traversals.
+                    //
+                    // Consider expr = {flattened: {$elemMatch: {$eq: true}}} and renames =
+                    // {{"flattened", "outer.inner"}} where the underlying document is {outer:
+                    // [{inner: true}]}:
+                    // - The original non-optimised expression would succeed matching {$eq: true}
+                    // over each element of the reshaped 'outer.inner' array: [{flattened: [true]}].
+                    // - The renamed expression would be incorrect due to implicit array traversal.
+                    // The
+                    // {$elemMatch: {$eq: true}} predicate would be applied to each element of
+                    // 'outer.inner', and then subsequently return false because 'true' has a
+                    // non-array type.
+                    return {false, boost::none};
+                }
                 ++renamesFound;
             }
 
-            FieldRef prefixToRename(rename.first);
-            const auto& pathFieldRef = _elementPath->fieldRef();
-            if (prefixToRename.isPrefixOf(pathFieldRef)) {
-                // Get the 'pathTail' by chopping off the 'prefixToRename' path components from the
-                // beginning of the 'pathFieldRef' path.
-                auto pathTail = pathFieldRef.dottedSubstring(prefixToRename.numParts(),
-                                                             pathFieldRef.numParts());
-                // Replace the chopped off components with the component names resulting from the
-                // rename.
-                rewrittenPath = str::stream() << rename.second << "." << pathTail.toString();
-
+            FieldRef prefixToRename(src);
+            const auto& currFieldPathRef = _elementPath->fieldRef();
+            if (prefixToRename.isPrefixOf(currFieldPathRef)) {
+                // Perform a partial prefix rewrite. 'src' is a prefix for the current field path
+                // and we should substitute it with 'dst'.
+                rewrittenPathRef.parse(dst);
+                for (size_t it = prefixToRename.numParts(); it < currFieldPathRef.numParts();
+                     it++) {
+                    rewrittenPathRef.appendPart(currFieldPathRef.getPart(it));
+                }
                 ++renamesFound;
-            } else if (pathFieldRef.isPrefixOf(prefixToRename)) {
+            } else if (currFieldPathRef.isPrefixOf(prefixToRename)) {
                 // TODO SERVER-74298 Implement renaming by each path component instead of returning
                 // the pair of 'false' and boost::none. We can traverse subexpressions with the
                 // remaining path suffix of 'prefixToRename' to see if we can rename each path
@@ -185,11 +203,14 @@ public:
         }
 
         // There should never be multiple applicable renames.
-        invariant(renamesFound <= 1u);
+        tassert(9086900,
+                str::stream() << "expected at most one applicable rename, but found "
+                              << renamesFound << " for path " << path(),
+                renamesFound <= 1u);
         if (renamesFound == 1u) {
             // There is an applicable rename. Modify the path of this expression to use the new
             // name.
-            return {true, rewrittenPath};
+            return {true, rewrittenPathRef.dottedField().toString()};
         }
 
         return {true, boost::none};
diff --git a/src/mongo/db/pipeline/pipeline_test.cpp b/src/mongo/db/pipeline/pipeline_test.cpp
index 00fb0e097d7..f0862a6a563 100644
--- a/src/mongo/db/pipeline/pipeline_test.cpp
+++ b/src/mongo/db/pipeline/pipeline_test.cpp
@@ -2419,38 +2419,46 @@ TEST(PipelineOptimizationTest, MatchOnArrayFieldCanSplitAcrossRenameWithMapAndPr
 }
 
 TEST(PipelineOptimizationTest,
-     MatchElemMatchValueOnArrayFieldCanSplitAcrossRenameWithMapAndProject) {
-    // The $project simply renames 'a.b' & 'a.c' to 'd.e' & 'd.f' and the $match with $elemMatch on
-    // the leaf value can be swapped with $project.
-    string inputPipe = R"(
+     MatchElemMatchValueOnArrayFieldCanSplitAcrossRenameWithSimpleProject) {
+    // The $project simply renames 'a' to 'b', and the $match with $elemMatch on
+    // the value can be swapped with $project.
+    std::string inputPipe =
+        "[{$project: {b: '$a', _id: false}},{$match: {b: {$elemMatch: {$eq: 1}}}}]";
+    std::string outputPipe =
+        "[{$match: {a: {$elemMatch: {$eq: 1}}}},{$project: {b: '$a', _id: false}}]";
+    assertPipelineOptimizesTo(inputPipe, outputPipe);
+}
+
+TEST(PipelineOptimizationTest,
+     MatchElemMatchValueOnArrayFieldCanNotSplitAcrossRenameWithMapAndAddFields) {
+    // The $addFields simply maps an array of objects to one containing their inner 'elementField'
+    // scalar values . The $match stage on the reshaped array should not be swapped with $project to
+    // preserve the original $elemMatch semantics.
+    std::string pipeline = R"(
 [
     {
-        $project: {
-            d: {
-                $map: {input: '$a', as: 'iter', in : {e: '$$iter.b', f: '$$iter.c'}}
-            }
+        $addFields: {
+            "reshapedArray": {
+                $map: {input: '$arrayField', as: 'iter', in : "$$iter.elementField"}
+            },
+            _id: { "$const": false }
         }
     },
-    {$match: {"d.e": {$elemMatch: {$eq: 1}}, "d.f": {$elemMatch: {$eq: 1}}}}
-]
-        )";
-    string outputPipe = R"(
-[
-    {
-        $match: {$and: [{"a.b": {$elemMatch: {$eq: 1}}}, {"a.c": {$elemMatch: {$eq: 1}}}]}
-    },
-    {
-        $project: {
-            _id: true,
-            d: {
-                $map: {input: '$a', as: 'iter', in : {e: '$$iter.b', f: '$$iter.c'}}
-            }
-        }
-    }
+    {$match: {"reshapedArray": {$elemMatch: {$eq: 1}}}}
 ]
         )";
+    assertPipelineOptimizesTo(pipeline, pipeline);
+}
 
-    assertPipelineOptimizesTo(inputPipe, outputPipe);
+TEST(PipelineOptimizationTest,
+     MatchElemMatchValueOnArrayFieldCanNotSplitAcrossRenameWithDottedProject) {
+    // The $project stage maps a dotted field path to a simple non-dotted one which is then matched
+    // upon. Expect no swap to be happen as it might affect the result of the query due to
+    // $elemMatch.
+    std::string pipeline =
+        "[{$project: {reshaped: '$document.array.element.deeply.nested.field', _id: false}},"
+        "{$match: {reshaped: {$elemMatch: {$eq: 1}}}}]";
+    assertPipelineOptimizesTo(pipeline, pipeline);
 }
 
 // TODO SERVER-74298 The $match can be swapped with $project after renaming.