SERVER-51082 Make MatchExpression lifetime independent of CST

7 files changed, 126 insertions, 97 deletions

diff --git a/src/mongo/db/cst/cst_parser.h b/src/mongo/db/cst/cst_parser.h
new file mode 100755
index 00000000000..f96df0ec8ca
--- /dev/null
+++ b/src/mongo/db/cst/cst_parser.h
@@ -0,0 +1,67 @@
+/**

+ *    Copyright (C) 2020-present MongoDB, Inc.

+ *

+ *    This program is free software: you can redistribute it and/or modify

+ *    it under the terms of the Server Side Public License, version 1,

+ *    as published by MongoDB, Inc.

+ *

+ *    This program is distributed in the hope that it will be useful,

+ *    but WITHOUT ANY WARRANTY; without even the implied warranty of

+ *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the

+ *    Server Side Public License for more details.

+ *

+ *    You should have received a copy of the Server Side Public License

+ *    along with this program. If not, see

+ *    <http://www.mongodb.com/licensing/server-side-public-license>.

+ *

+ *    As a special exception, the copyright holders give permission to link the

+ *    code of portions of this program with the OpenSSL library under certain

+ *    conditions as described in each individual source file and distribute

+ *    linked combinations including the program with the OpenSSL library. You

+ *    must comply with the Server Side Public License in all respects for

+ *    all of the code used other than as permitted herein. If you modify file(s)

+ *    with this exception, you may extend this exception to your version of the

+ *    file(s), but you are not obligated to do so. If you do not wish to do so,

+ *    delete this exception statement from your version. If you delete this

+ *    exception statement from all source files in the program, then also delete

+ *    it in the license file.

+ */

+

+#pragma once

+

+#include "mongo/platform/basic.h"

+

+#include "mongo/db/cst/bson_lexer.h"

+#include "mongo/db/cst/c_node.h"

+#include "mongo/db/cst/cst_match_translation.h"

+#include "mongo/db/cst/cst_sort_translation.h"

+#include "mongo/db/matcher/expression.h"

+#include "mongo/db/pipeline/expression_context.h"

+#include "mongo/db/query/sort_pattern.h"

+

+namespace mongo::cst {

+

+/**

+ * Parses the given 'filter' to a MatchExpression. Throws an exception if the filter fails to parse.

+ */

+std::unique_ptr<MatchExpression> parseToMatchExpression(

+    BSONObj filter, const boost::intrusive_ptr<ExpressionContext>& expCtx) {

+    BSONLexer lexer{filter, ParserGen::token::START_MATCH};

+    CNode cst;

+    ParserGen(lexer, &cst).parse();

+    return cst_match_translation::translateMatchExpression(cst, expCtx);

+}

+

+/**

+ * Parses the given 'sort' object into a SortPattern. Throws an exception if the sort object fails

+ * to parse.

+ */

+SortPattern parseToSortPattern(BSONObj sort,

+                               const boost::intrusive_ptr<ExpressionContext>& expCtx) {

+    BSONLexer lexer{sort, ParserGen::token::START_SORT};

+    CNode cst;

+    ParserGen(lexer, &cst).parse();

+    return cst_sort_translation::translateSortSpec(cst, expCtx);

+}

+

+}  // namespace mongo::cst

diff --git a/src/mongo/db/matcher/expression_path.h b/src/mongo/db/matcher/expression_path.h
index 1ae09636e8d..a9b6ec6ca80 100644
--- a/src/mongo/db/matcher/expression_path.h
+++ b/src/mongo/db/matcher/expression_path.h
@@ -47,11 +47,8 @@ public:
                         ElementPath::LeafArrayBehavior leafArrBehavior,
                         ElementPath::NonLeafArrayBehavior nonLeafArrayBehavior,
                         clonable_ptr<ErrorAnnotation> annotation = nullptr)
-        : MatchExpression(matchType, std::move(annotation)), _path(path) {
-        _elementPath.init(_path);
-        _elementPath.setLeafArrayBehavior(leafArrBehavior);
-        _elementPath.setNonLeafArrayBehavior(nonLeafArrayBehavior);
-    }
+        : MatchExpression(matchType, std::move(annotation)),
+          _elementPath(path, leafArrBehavior, nonLeafArrayBehavior) {}
 
     virtual ~PathMatchExpression() {}
 
@@ -71,12 +68,15 @@ public:
     }
 
     const StringData path() const final {
-        return _path;
+        return _elementPath.fieldRef().dottedField();
     }
 
+    /**
+     * Resets the path for this expression. Note that this method will make a copy of 'path' such
+     * that there's no lifetime requirements for the string which 'path' points into.
+     */
     void setPath(StringData path) {
-        _path = path;
-        _elementPath.init(_path);
+        _elementPath.reset(path);
     }
 
     /**
@@ -86,17 +86,17 @@ public:
      * element).
      */
     void applyRename(const StringMap<std::string>& renameList) {
-        FieldRef pathFieldRef(_path);
-
         size_t renamesFound = 0u;
+        std::string rewrittenPath;
         for (auto rename : renameList) {
-            if (rename.first == _path) {
-                _rewrittenPath = rename.second;
+            if (rename.first == path()) {
+                rewrittenPath = rename.second;
 
                 ++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.
@@ -104,7 +104,7 @@ public:
                                                              pathFieldRef.numParts());
                 // Replace the chopped off components with the component names resulting from the
                 // rename.
-                _rewrittenPath = str::stream() << rename.second << "." << pathTail.toString();
+                rewrittenPath = str::stream() << rename.second << "." << pathTail.toString();
 
                 ++renamesFound;
             }
@@ -115,7 +115,7 @@ public:
         if (renamesFound == 1u) {
             // There is an applicable rename. Modify the path of this expression to use the new
             // name.
-            setPath(_rewrittenPath);
+            setPath(rewrittenPath);
         }
     }
 
@@ -137,17 +137,13 @@ public:
 
 protected:
     void _doAddDependencies(DepsTracker* deps) const final {
-        if (!_path.empty()) {
-            deps->fields.insert(_path.toString());
+        if (!path().empty()) {
+            deps->fields.insert(path().toString());
         }
     }
 
 private:
-    StringData _path;
+    // ElementPath holds a FieldRef, which owns the underlying path string.
     ElementPath _elementPath;
-
-    // We use this when we rewrite the value in '_path' and we need a backing store for the
-    // rewritten string.
-    std::string _rewrittenPath;
 };
 }  // namespace mongo
diff --git a/src/mongo/db/matcher/path.cpp b/src/mongo/db/matcher/path.cpp
index beed8706527..dbb0379f038 100644
--- a/src/mongo/db/matcher/path.cpp
+++ b/src/mongo/db/matcher/path.cpp
@@ -34,12 +34,6 @@
 
 namespace mongo {
 
-void ElementPath::init(StringData path) {
-    _nonLeafArrayBehavior = NonLeafArrayBehavior::kTraverse;
-    _leafArrayBehavior = LeafArrayBehavior::kTraverse;
-    _fieldRef.parse(path);
-}
-
 // -----
 
 ElementIterator::~ElementIterator() {}
@@ -191,10 +185,10 @@ bool BSONElementIterator::subCursorHasMore() {
                 return true;
             }
 
-            _subCursorPath.reset(new ElementPath());
-            _subCursorPath->init(_arrayIterationState.restOfPath.substr(
-                _arrayIterationState.nextPieceOfPath.size() + 1));
-            _subCursorPath->setLeafArrayBehavior(_path->leafArrayBehavior());
+            _subCursorPath.reset(
+                new ElementPath(_arrayIterationState.restOfPath.substr(
+                                    _arrayIterationState.nextPieceOfPath.size() + 1),
+                                _path->leafArrayBehavior()));
 
             // If we're here, we must be able to traverse nonleaf arrays.
             dassert(_path->nonLeafArrayBehavior() == ElementPath::NonLeafArrayBehavior::kTraverse);
@@ -281,9 +275,8 @@ bool BSONElementIterator::more() {
             if (eltInArray.type() == Object) {
                 // The current array element is a subdocument.  See if the subdocument generates
                 // any elements matching the remaining subpath.
-                _subCursorPath.reset(new ElementPath());
-                _subCursorPath->init(_arrayIterationState.restOfPath);
-                _subCursorPath->setLeafArrayBehavior(_path->leafArrayBehavior());
+                _subCursorPath.reset(
+                    new ElementPath(_arrayIterationState.restOfPath, _path->leafArrayBehavior()));
 
                 _subCursor.reset(new BSONElementIterator(_subCursorPath.get(), eltInArray.Obj()));
                 if (subCursorHasMore()) {
@@ -307,10 +300,10 @@ bool BSONElementIterator::more() {
                 if (eltInArray.type() == Array) {
                     // The current array element is itself an array.  See if the nested array
                     // has any elements matching the remainihng.
-                    _subCursorPath.reset(new ElementPath());
-                    _subCursorPath->init(_arrayIterationState.restOfPath.substr(
-                        _arrayIterationState.nextPieceOfPath.size() + 1));
-                    _subCursorPath->setLeafArrayBehavior(_path->leafArrayBehavior());
+                    _subCursorPath.reset(
+                        new ElementPath(_arrayIterationState.restOfPath.substr(
+                                            _arrayIterationState.nextPieceOfPath.size() + 1),
+                                        _path->leafArrayBehavior()));
                     BSONElementIterator* real = new BSONElementIterator(
                         _subCursorPath.get(), _arrayIterationState._current.Obj());
                     _subCursor.reset(real);
diff --git a/src/mongo/db/matcher/path.h b/src/mongo/db/matcher/path.h
index 88d759462f5..ee2e67f026b 100644
--- a/src/mongo/db/matcher/path.h
+++ b/src/mongo/db/matcher/path.h
@@ -80,9 +80,13 @@ public:
           _nonLeafArrayBehavior(nonLeafArrayBehavior),
           _fieldRef(path) {}
 
-    // TODO: replace uses of members below with regular construction.
-    ElementPath() {}
-    void init(StringData path);
+    /**
+     * Resets this ElementPath to 'newPath'. Note that this method will make a copy of 'newPath'
+     * such that there's no lifetime requirements for the string which 'newPath' points into.
+     */
+    void reset(StringData newPath) {
+        _fieldRef.parse(newPath);
+    }
 
     void setLeafArrayBehavior(LeafArrayBehavior leafArrBehavior) {
         _leafArrayBehavior = leafArrBehavior;
diff --git a/src/mongo/db/matcher/path_test.cpp b/src/mongo/db/matcher/path_test.cpp
index dd0d7314ca9..02eb4579be7 100644
--- a/src/mongo/db/matcher/path_test.cpp
+++ b/src/mongo/db/matcher/path_test.cpp
@@ -38,8 +38,7 @@ namespace mongo {
 using std::string;
 
 TEST(Path, Root1) {
-    ElementPath p;
-    p.init("a");
+    ElementPath p{"a"};
 
     BSONObj doc = BSON("x" << 4 << "a" << 5);
 
@@ -52,8 +51,7 @@ TEST(Path, Root1) {
 }
 
 TEST(Path, RootArray1) {
-    ElementPath p;
-    p.init("a");
+    ElementPath p{"a"};
 
     BSONObj doc = BSON("x" << 4 << "a" << BSON_ARRAY(5 << 6));
 
@@ -75,8 +73,7 @@ TEST(Path, RootArray1) {
 }
 
 TEST(Path, RootArray2) {
-    ElementPath p;
-    p.init("a");
+    ElementPath p{"a"};
     p.setLeafArrayBehavior(ElementPath::LeafArrayBehavior::kNoTraversal);
 
     BSONObj doc = BSON("x" << 4 << "a" << BSON_ARRAY(5 << 6));
@@ -91,8 +88,7 @@ TEST(Path, RootArray2) {
 }
 
 TEST(Path, Nested1) {
-    ElementPath p;
-    p.init("a.b");
+    ElementPath p{"a.b"};
 
     BSONObj doc =
         BSON("a" << BSON_ARRAY(BSON("b" << 5) << 3 << BSONObj() << BSON("b" << BSON_ARRAY(9 << 11))
@@ -130,8 +126,7 @@ TEST(Path, Nested1) {
 }
 
 TEST(Path, NestedPartialMatchScalar) {
-    ElementPath p;
-    p.init("a.b");
+    ElementPath p{"a.b"};
 
     BSONObj doc = BSON("a" << 4);
 
@@ -149,8 +144,7 @@ TEST(Path, NestedPartialMatchScalar) {
 // the iteration logic does not return an EOO.
 // what we want ideally.
 TEST(Path, NestedPartialMatchArray) {
-    ElementPath p;
-    p.init("a.b");
+    ElementPath p{"a.b"};
 
     BSONObj doc = BSON("a" << BSON_ARRAY(4));
 
@@ -161,8 +155,7 @@ TEST(Path, NestedPartialMatchArray) {
 
 // Note that this describes existing behavior and not necessarily
 TEST(Path, NestedEmptyArray) {
-    ElementPath p;
-    p.init("a.b");
+    ElementPath p{"a.b"};
 
     BSONObj doc = BSON("a" << BSON("b" << BSONArray()));
 
@@ -177,8 +170,7 @@ TEST(Path, NestedEmptyArray) {
 }
 
 TEST(Path, NestedNoLeaf1) {
-    ElementPath p;
-    p.init("a.b");
+    ElementPath p{"a.b"};
     p.setLeafArrayBehavior(ElementPath::LeafArrayBehavior::kNoTraversal);
 
     BSONObj doc =
@@ -209,8 +201,7 @@ TEST(Path, NestedNoLeaf1) {
 }
 
 TEST(Path, MatchSubpathReturnsArrayOnSubpath) {
-    ElementPath path;
-    path.init("a.b.c");
+    ElementPath path{"a.b.c"};
     path.setLeafArrayBehavior(ElementPath::LeafArrayBehavior::kNoTraversal);
     path.setNonLeafArrayBehavior(ElementPath::NonLeafArrayBehavior::kMatchSubpath);
 
@@ -226,8 +217,7 @@ TEST(Path, MatchSubpathReturnsArrayOnSubpath) {
 }
 
 TEST(Path, MatchSubpathWithTraverseLeafFalseReturnsLeafArrayOnPath) {
-    ElementPath path;
-    path.init("a.b.c");
+    ElementPath path{"a.b.c"};
     path.setLeafArrayBehavior(ElementPath::LeafArrayBehavior::kNoTraversal);
     path.setNonLeafArrayBehavior(ElementPath::NonLeafArrayBehavior::kMatchSubpath);
 
@@ -243,8 +233,7 @@ TEST(Path, MatchSubpathWithTraverseLeafFalseReturnsLeafArrayOnPath) {
 }
 
 TEST(Path, MatchSubpathWithTraverseLeafTrueReturnsLeafArrayAndValuesOnPath) {
-    ElementPath path;
-    path.init("a.b.c");
+    ElementPath path{"a.b.c"};
     path.setLeafArrayBehavior(ElementPath::LeafArrayBehavior::kTraverse);
     path.setNonLeafArrayBehavior(ElementPath::NonLeafArrayBehavior::kMatchSubpath);
 
@@ -268,8 +257,7 @@ TEST(Path, MatchSubpathWithTraverseLeafTrueReturnsLeafArrayAndValuesOnPath) {
 }
 
 TEST(Path, MatchSubpathWithMultipleArraysReturnsOutermostArray) {
-    ElementPath path;
-    path.init("a.b.c");
+    ElementPath path{"a.b.c"};
     path.setLeafArrayBehavior(ElementPath::LeafArrayBehavior::kTraverse);
     path.setNonLeafArrayBehavior(ElementPath::NonLeafArrayBehavior::kMatchSubpath);
 
@@ -285,8 +273,7 @@ TEST(Path, MatchSubpathWithMultipleArraysReturnsOutermostArray) {
 }
 
 TEST(Path, NoTraversalOfNonLeafArrayReturnsNothingWithNonLeafArrayInDoc) {
-    ElementPath path;
-    path.init("a.b");
+    ElementPath path{"a.b"};
     path.setLeafArrayBehavior(ElementPath::LeafArrayBehavior::kTraverse);
     path.setNonLeafArrayBehavior(ElementPath::NonLeafArrayBehavior::kNoTraversal);
 
@@ -297,8 +284,7 @@ TEST(Path, NoTraversalOfNonLeafArrayReturnsNothingWithNonLeafArrayInDoc) {
 }
 
 TEST(Path, MatchSubpathWithNumericalPathComponentReturnsEntireArray) {
-    ElementPath path;
-    path.init("a.0.b");
+    ElementPath path{"a.0.b"};
     path.setLeafArrayBehavior(ElementPath::LeafArrayBehavior::kTraverse);
     path.setNonLeafArrayBehavior(ElementPath::NonLeafArrayBehavior::kMatchSubpath);
 
@@ -314,8 +300,7 @@ TEST(Path, MatchSubpathWithNumericalPathComponentReturnsEntireArray) {
 }
 
 TEST(Path, ArrayIndex1) {
-    ElementPath p;
-    p.init("a.1");
+    ElementPath p{"a.1"};
 
     BSONObj doc = BSON("a" << BSON_ARRAY(5 << 7 << 3));
 
@@ -329,8 +314,7 @@ TEST(Path, ArrayIndex1) {
 }
 
 TEST(Path, ArrayIndex2) {
-    ElementPath p;
-    p.init("a.1");
+    ElementPath p{"a.1"};
 
     BSONObj doc = BSON("a" << BSON_ARRAY(5 << BSON_ARRAY(2 << 4) << 3));
 
@@ -344,8 +328,7 @@ TEST(Path, ArrayIndex2) {
 }
 
 TEST(Path, ArrayIndex3) {
-    ElementPath p;
-    p.init("a.1");
+    ElementPath p{"a.1"};
 
     BSONObj doc = BSON("a" << BSON_ARRAY(5 << BSON("1" << 4) << 3));
 
@@ -363,8 +346,7 @@ TEST(Path, ArrayIndex3) {
 }
 
 TEST(Path, ArrayIndexNested1) {
-    ElementPath p;
-    p.init("a.1.b");
+    ElementPath p{"a.1.b"};
 
     BSONObj doc = BSON("a" << BSON_ARRAY(5 << BSON("b" << 4) << 3));
 
@@ -383,8 +365,7 @@ TEST(Path, ArrayIndexNested1) {
 }
 
 TEST(Path, ArrayIndexNested2) {
-    ElementPath p;
-    p.init("a.1.b");
+    ElementPath p{"a.1.b"};
 
     BSONObj doc = BSON("a" << BSON_ARRAY(5 << BSON_ARRAY(BSON("b" << 4)) << 3));
 
@@ -401,8 +382,7 @@ TEST(Path, ArrayIndexNested2) {
 // SERVER-15899: test iteration using a path that generates no elements, but traverses a long
 // array containing subdocuments with nested arrays.
 TEST(Path, NonMatchingLongArrayOfSubdocumentsWithNestedArrays) {
-    ElementPath p;
-    p.init("a.b.x");
+    ElementPath p{"a.b.x"};
 
     // Build the document {a: [{b: []}, {b: []}, {b: []}, ...]}.
     BSONObj subdoc = BSON("b" << BSONArray());
@@ -422,8 +402,7 @@ TEST(Path, NonMatchingLongArrayOfSubdocumentsWithNestedArrays) {
 // ElementIterator::Context::arrayOffset() should always refer to the current offset of the
 // outermost array that is implicitly traversed.
 TEST(Path, NestedArrayImplicitTraversal) {
-    ElementPath p;
-    p.init("a.b");
+    ElementPath p{"a.b"};
     BSONObj doc = fromjson("{a: [{b: [2, 3]}, {b: [4, 5]}]}");
     BSONElementIterator cursor(&p, doc);
 
@@ -470,8 +449,7 @@ TEST(Path, NestedArrayImplicitTraversal) {
 // is being traversed implicitly, ElementIterator::Context::arrayOffset() should return the
 // current offset of the array being implicitly traversed.
 TEST(Path, ArrayOffsetWithImplicitAndExplicitTraversal) {
-    ElementPath p;
-    p.init("a.0.b");
+    ElementPath p{"a.0.b"};
     BSONObj doc = fromjson("{a: [{b: [2, 3]}, {b: [4, 5]}]}");
     BSONElementIterator cursor(&p, doc);
 
diff --git a/src/mongo/db/query/canonical_query.cpp b/src/mongo/db/query/canonical_query.cpp
index 6cae65c2ed6..0e2945eae21 100644
--- a/src/mongo/db/query/canonical_query.cpp
+++ b/src/mongo/db/query/canonical_query.cpp
@@ -35,10 +35,7 @@
 
 #include "mongo/db/catalog/collection.h"
 #include "mongo/db/commands/test_commands_enabled.h"
-#include "mongo/db/cst/bson_lexer.h"
-#include "mongo/db/cst/c_node.h"
-#include "mongo/db/cst/cst_match_translation.h"
-#include "mongo/db/cst/cst_sort_translation.h"
+#include "mongo/db/cst/cst_parser.h"
 #include "mongo/db/jsobj.h"
 #include "mongo/db/matcher/expression_array.h"
 #include "mongo/db/namespace_string.h"
@@ -125,9 +122,7 @@ StatusWith<std::unique_ptr<CanonicalQuery>> CanonicalQuery::canonicalize(
     StatusWithMatchExpression statusWithMatcher = [&]() -> StatusWithMatchExpression {
         if (getTestCommandsEnabled() && internalQueryEnableCSTParser.load()) {
             try {
-                BSONLexer lexer{qr->getFilter(), ParserGen::token::START_MATCH};
-                ParserGen(lexer, &cq->_filterCst).parse();
-                return cst_match_translation::translateMatchExpression(cq->_filterCst, newExpCtx);
+                return cst::parseToMatchExpression(qr->getFilter(), newExpCtx);
             } catch (const DBException& ex) {
                 return ex.toStatus();
             }
@@ -257,9 +252,7 @@ void CanonicalQuery::initSortPattern(QueryMetadataBitSet unavailableMetadata) {
     }
 
     if (getTestCommandsEnabled() && internalQueryEnableCSTParser.load()) {
-        BSONLexer lexer{_qr->getSort(), ParserGen::token::START_SORT};
-        ParserGen(lexer, &_sortCst).parse();
-        _sortPattern = cst_sort_translation::translateSortSpec(_sortCst, _expCtx);
+        _sortPattern = cst::parseToSortPattern(_qr->getSort(), _expCtx);
     } else {
         _sortPattern = SortPattern{_qr->getSort(), _expCtx};
     }
diff --git a/src/mongo/db/query/canonical_query.h b/src/mongo/db/query/canonical_query.h
index dd6de95e624..1e4ac425c25 100644
--- a/src/mongo/db/query/canonical_query.h
+++ b/src/mongo/db/query/canonical_query.h
@@ -237,12 +237,10 @@ private:
 
     std::unique_ptr<QueryRequest> _qr;
 
-    CNode _filterCst;
     std::unique_ptr<MatchExpression> _root;
 
     boost::optional<projection_ast::Projection> _proj;
 
-    CNode _sortCst;
     boost::optional<SortPattern> _sortPattern;
 
     // Keeps track of what metadata has been explicitly requested.