SERVER-57053 Split CanonicalQuery validation on pre- and post-normalization

(cherry picked from commit c1ac689d114e6681587ea24b009d61551653fee0)

4 files changed, 64 insertions, 25 deletions

diff --git a/jstests/core/fts_querylang.js b/jstests/core/fts_querylang.js
index a4bdc3981b5..e745be8f788 100644
--- a/jstests/core/fts_querylang.js
+++ b/jstests/core/fts_querylang.js
@@ -2,6 +2,7 @@
 // @tags: [
 //   requires_multi_updates,
 //   requires_non_retryable_writes,
+//   requires_fcv_50,
 // ]
 (function() {
 "use strict";
@@ -87,8 +88,19 @@ assert.eq(2, coll.find({b: 1}).itcount(), 'incorrect number of documents updated
 
 // $text cannot be contained within a $nor.
 assert.commandFailedWithCode(
-    assert.throws(function() {
-                     coll.find({$nor: [{$text: {$search: 'a'}}]}).itcount();
-                 }),
-                 ErrorCodes.NoQueryExecutionPlans);
+    assert.throws(() => coll.find({$nor: [{$text: {$search: 'a'}}]}).itcount()),
+                 ErrorCodes.BadValue);
+assert.commandFailedWithCode(
+    assert.throws(() => coll.find({$nor: [{a: 1}, {$text: {$search: 'a'}}]}).itcount()),
+                 ErrorCodes.BadValue);
+assert.commandFailedWithCode(
+    assert.throws(() =>
+                      coll.find({
+                              $and: [
+                                  {a: 2},
+                                  {$nor: [{$or: [{c: {$not: {$ne: 10}}}, {$text: {$search: 'a'}}]}]}
+                              ]
+                          })
+                          .itcount()),
+                 ErrorCodes.BadValue);
 }());
diff --git a/src/mongo/db/matcher/expression_optimize_test.cpp b/src/mongo/db/matcher/expression_optimize_test.cpp
index 6a41ad33643..ab9e91cb30f 100644
--- a/src/mongo/db/matcher/expression_optimize_test.cpp
+++ b/src/mongo/db/matcher/expression_optimize_test.cpp
@@ -75,7 +75,10 @@ Status isValid(const std::string& queryStr, const FindCommandRequest& findComman
     BSONObj queryObj = fromjson(queryStr);
     std::unique_ptr<MatchExpression> me(parseMatchExpression(queryObj));
     me = MatchExpression::optimize(std::move(me));
-    return CanonicalQuery::isValid(me.get(), findCommand).getStatus();
+    if (auto status = CanonicalQuery::isValid(me.get(), findCommand).getStatus(); !status.isOK()) {
+        return status;
+    }
+    return CanonicalQuery::isValidNormalized(me.get());
 }
 
 TEST(ExpressionOptimizeTest, IsValidText) {
diff --git a/src/mongo/db/query/canonical_query.cpp b/src/mongo/db/query/canonical_query.cpp
index 0c857b68afd..602a1a8dcb1 100644
--- a/src/mongo/db/query/canonical_query.cpp
+++ b/src/mongo/db/query/canonical_query.cpp
@@ -198,13 +198,17 @@ Status CanonicalQuery::init(OperationContext* opCtx,
     _canHaveNoopMatchNodes = canHaveNoopMatchNodes;
     _forceClassicEngine = internalQueryForceClassicEngine.load();
 
-    // Normalize and validate tree.
-    _root = MatchExpression::normalize(std::move(root));
-    auto validStatus = isValid(_root.get(), *_findCommand);
+    auto validStatus = isValid(root.get(), *_findCommand);
     if (!validStatus.isOK()) {
         return validStatus.getStatus();
     }
     auto unavailableMetadata = validStatus.getValue();
+    _root = MatchExpression::normalize(std::move(root));
+    // The tree must always be valid after normalization.
+    dassert(isValid(_root.get(), *_findCommand).isOK());
+    if (auto status = isValidNormalized(_root.get()); !status.isOK()) {
+        return status;
+    }
 
     // Validate the projection if there is one.
     if (!_findCommand->getProjection().isEmpty()) {
@@ -328,7 +332,7 @@ size_t CanonicalQuery::countNodes(const MatchExpression* root, MatchExpression::
 /**
  * Does 'root' have a subtree of type 'subtreeType' with a node of type 'childType' inside?
  */
-bool hasNodeInSubtree(MatchExpression* root,
+bool hasNodeInSubtree(const MatchExpression* root,
                       MatchExpression::MatchType childType,
                       MatchExpression::MatchType subtreeType) {
     if (subtreeType == root->matchType()) {
@@ -342,7 +346,7 @@ bool hasNodeInSubtree(MatchExpression* root,
     return false;
 }
 
-StatusWith<QueryMetadataBitSet> CanonicalQuery::isValid(MatchExpression* root,
+StatusWith<QueryMetadataBitSet> CanonicalQuery::isValid(const MatchExpression* root,
                                                         const FindCommandRequest& findCommand) {
     QueryMetadataBitSet unavailableMetadata{};
 
@@ -368,20 +372,7 @@ StatusWith<QueryMetadataBitSet> CanonicalQuery::isValid(MatchExpression* root,
     if (numGeoNear > 1) {
         return Status(ErrorCodes::BadValue, "Too many geoNear expressions");
     } else if (1 == numGeoNear) {
-        bool topLevel = false;
-        if (MatchExpression::GEO_NEAR == root->matchType()) {
-            topLevel = true;
-        } else if (MatchExpression::AND == root->matchType()) {
-            for (size_t i = 0; i < root->numChildren(); ++i) {
-                if (MatchExpression::GEO_NEAR == root->getChild(i)->matchType()) {
-                    topLevel = true;
-                    break;
-                }
-            }
-        }
-        if (!topLevel) {
-            return Status(ErrorCodes::BadValue, "geoNear must be top-level expr");
-        }
+        // Do nothing, we will perform extra checks in CanonicalQuery::isValidNormalized.
     } else {
         // Geo distance and geo point metadata are unavailable.
         unavailableMetadata |= DepsTracker::kAllGeoNearData;
@@ -457,6 +448,30 @@ StatusWith<QueryMetadataBitSet> CanonicalQuery::isValid(MatchExpression* root,
     return unavailableMetadata;
 }
 
+Status CanonicalQuery::isValidNormalized(const MatchExpression* root) {
+    if (auto numGeoNear = countNodes(root, MatchExpression::GEO_NEAR); numGeoNear > 0) {
+        tassert(5705300, "Only one $getNear expression is expected", numGeoNear == 1);
+
+        auto topLevel = false;
+        if (MatchExpression::GEO_NEAR == root->matchType()) {
+            topLevel = true;
+        } else if (MatchExpression::AND == root->matchType()) {
+            for (size_t i = 0; i < root->numChildren(); ++i) {
+                if (MatchExpression::GEO_NEAR == root->getChild(i)->matchType()) {
+                    topLevel = true;
+                    break;
+                }
+            }
+        }
+
+        if (!topLevel) {
+            return Status(ErrorCodes::BadValue, "geoNear must be top-level expr");
+        }
+    }
+
+    return Status::OK();
+}
+
 int CanonicalQuery::getOptions() const {
     int options = 0;
     if (_findCommand->getTailable()) {
diff --git a/src/mongo/db/query/canonical_query.h b/src/mongo/db/query/canonical_query.h
index 5b7dcdf191f..66877058389 100644
--- a/src/mongo/db/query/canonical_query.h
+++ b/src/mongo/db/query/canonical_query.h
@@ -111,15 +111,24 @@ public:
      * for illegal combinations of operators. Returns a non-OK status if any such illegal
      * combination is found.
      *
+     * This method can be called both on normalized and non-normalized 'root'. However, some checks
+     * can only be performed once the match expressions is normalized. To perform these checks one
+     * can call 'isValidNormalized()'.
+     *
      * On success, returns a bitset indicating which types of metadata are *unavailable*. For
      * example, if 'root' does not contain a $text predicate, then the returned metadata bitset will
      * indicate that text score metadata is unavailable. This means that if subsequent
      * $meta:"textScore" expressions are found during analysis of the query, we should raise in an
      * error.
      */
-    static StatusWith<QueryMetadataBitSet> isValid(MatchExpression* root,
+    static StatusWith<QueryMetadataBitSet> isValid(const MatchExpression* root,
                                                    const FindCommandRequest& findCommand);
 
+    /**
+     * Perform additional validation checks on the normalized 'root'.
+     */
+    static Status isValidNormalized(const MatchExpression* root);
+
     const NamespaceString nss() const {
         invariant(_findCommand->getNamespaceOrUUID().nss());
         return *_findCommand->getNamespaceOrUUID().nss();