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

(cherry picked from commit c1ac689d114e6681587ea24b009d61551653fee0)
(cherry picked from commit 157d39a0aea2b2c1dab68f2193fb6cad631e4a65)
(cherry picked from commit 7ce44276e1be8ad8dcac555a894cb90d72de62fe)

4 files changed, 66 insertions, 29 deletions

diff --git a/jstests/core/fts_querylang.js b/jstests/core/fts_querylang.js
index de27b65ba5b..f5778735101 100644
--- a/jstests/core/fts_querylang.js
+++ b/jstests/core/fts_querylang.js
@@ -1,5 +1,5 @@
 // Test the $text query operator.
-// @tags: [requires_non_retryable_writes]
+// @tags: [requires_fcv_42, requires_non_retryable_writes]
 (function() {
     "use strict";
 
@@ -79,8 +79,20 @@
     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.BadValue);
+    assert.commandFailedWithCode(
+        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 47be833f125..ef2ff2f8519 100644
--- a/src/mongo/db/matcher/expression_optimize_test.cpp
+++ b/src/mongo/db/matcher/expression_optimize_test.cpp
@@ -74,7 +74,11 @@ Status isValid(const std::string& queryStr, const QueryRequest& qrRaw) {
     BSONObj queryObj = fromjson(queryStr);
     std::unique_ptr<MatchExpression> me(parseMatchExpression(queryObj));
     me = MatchExpression::optimize(std::move(me));
-    return CanonicalQuery::isValid(me.get(), qrRaw);
+    auto status = CanonicalQuery::isValid(me.get(), qrRaw);
+    if (!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 3d15afafe6c..bcbbaf2acb5 100644
--- a/src/mongo/db/query/canonical_query.cpp
+++ b/src/mongo/db/query/canonical_query.cpp
@@ -224,13 +224,19 @@ Status CanonicalQuery::init(OperationContext* opCtx,
 
     _canHaveNoopMatchNodes = canHaveNoopMatchNodes;
 
-    // Normalize, sort and validate tree.
-    _root = MatchExpression::optimize(std::move(root));
-    sortTree(_root.get());
-    Status validStatus = isValid(_root.get(), *_qr);
+    // Validate, normalize, and sort tree.
+    auto validStatus = isValid(root.get(), *_qr);
     if (!validStatus.isOK()) {
         return validStatus;
     }
+    _root = MatchExpression::optimize(std::move(root));
+    sortTree(_root.get());
+    // The tree must always be valid after normalization.
+    dassert(isValid(_root.get(), *_qr).isOK());
+    auto status = isValidNormalized(_root.get());
+    if (!status.isOK()) {
+        return status;
+    }
 
     // Validate the projection if there is one.
     if (!_qr->getProj().isEmpty()) {
@@ -314,7 +320,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()) {
@@ -329,9 +335,7 @@ bool hasNodeInSubtree(MatchExpression* root,
 }
 
 // static
-Status CanonicalQuery::isValid(MatchExpression* root, const QueryRequest& parsed) {
-    // Analysis below should be done after squashing the tree to make it clearer.
-
+Status CanonicalQuery::isValid(const MatchExpression* root, const QueryRequest& parsed) {
     // There can only be one TEXT.  If there is a TEXT, it cannot appear inside a NOR.
     //
     // Note that the query grammar (as enforced by the MatchExpression parser) forbids TEXT
@@ -351,20 +355,7 @@ Status CanonicalQuery::isValid(MatchExpression* root, const QueryRequest& parsed
     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.
     }
 
     // NEAR cannot have a $natural sort or $natural hint.
@@ -426,6 +417,31 @@ Status CanonicalQuery::isValid(MatchExpression* root, const QueryRequest& parsed
     return Status::OK();
 }
 
+Status CanonicalQuery::isValidNormalized(const MatchExpression* root) {
+    auto numGeoNear = countNodes(root, MatchExpression::GEO_NEAR);
+    if (numGeoNear > 0) {
+        invariant(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();
+}
+
 std::string CanonicalQuery::toString() const {
     str::stream ss;
     ss << "ns=" << _qr->nss().ns();
diff --git a/src/mongo/db/query/canonical_query.h b/src/mongo/db/query/canonical_query.h
index b94b310a00d..b88308698f5 100644
--- a/src/mongo/db/query/canonical_query.h
+++ b/src/mongo/db/query/canonical_query.h
@@ -146,7 +146,12 @@ public:
      *
      * TODO: Move this to query_validator.cpp
      */
-    static Status isValid(MatchExpression* root, const QueryRequest& parsed);
+    static Status isValid(const MatchExpression* root, const QueryRequest& parsed);
+
+    /**
+     * Perform additional validation checks on the normalized 'root'.
+     */
+    static Status isValidNormalized(const MatchExpression* root);
 
     /**
      * Traverses expression tree post-order.