diff options
author | Anton Korshunov <anton.korshunov@mongodb.com> | 2021-06-02 12:20:39 +0100 |
---|---|---|
committer | Evergreen Agent <no-reply@evergreen.mongodb.com> | 2021-06-02 14:58:28 +0000 |
commit | 157d39a0aea2b2c1dab68f2193fb6cad631e4a65 (patch) | |
tree | 2fbaabc317b531aa029ac10e30417212e36095a7 | |
parent | 6c317bc5fa081cd800ccdc865e5775fab31b3d88 (diff) | |
download | mongo-157d39a0aea2b2c1dab68f2193fb6cad631e4a65.tar.gz |
SERVER-57053 Split CanonicalQuery validation on pre- and post-normalization
(cherry picked from commit c1ac689d114e6681587ea24b009d61551653fee0)
-rw-r--r-- | jstests/core/fts_querylang.js | 21 | ||||
-rw-r--r-- | src/mongo/db/matcher/expression_optimize_test.cpp | 5 | ||||
-rw-r--r-- | src/mongo/db/query/canonical_query.cpp | 56 | ||||
-rw-r--r-- | src/mongo/db/query/canonical_query.h | 11 |
4 files changed, 66 insertions, 27 deletions
diff --git a/jstests/core/fts_querylang.js b/jstests/core/fts_querylang.js index 87a5e1d760b..ad4bd2d3d49 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_44, requires_non_retryable_writes] (function() { "use strict"; @@ -84,8 +84,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 699d547c091..232f7139d13 100644 --- a/src/mongo/db/matcher/expression_optimize_test.cpp +++ b/src/mongo/db/matcher/expression_optimize_test.cpp @@ -73,7 +73,10 @@ 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).getStatus(); + if (auto status = CanonicalQuery::isValid(me.get(), qrRaw).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 3ddf8b17a36..b23dc90013c 100644 --- a/src/mongo/db/query/canonical_query.cpp +++ b/src/mongo/db/query/canonical_query.cpp @@ -225,14 +225,19 @@ Status CanonicalQuery::init(OperationContext* opCtx, _canHaveNoopMatchNodes = canHaveNoopMatchNodes; - // Normalize, sort and validate tree. - _root = MatchExpression::optimize(std::move(root)); - sortTree(_root.get()); - auto validStatus = isValid(_root.get(), *_qr); + // Validate, normalize, and sort tree. + auto validStatus = isValid(root.get(), *_qr); if (!validStatus.isOK()) { return validStatus.getStatus(); } auto unavailableMetadata = validStatus.getValue(); + _root = MatchExpression::optimize(std::move(root)); + sortTree(_root.get()); + // The tree must always be valid after normalization. + dassert(isValid(_root.get(), *_qr).isOK()); + if (auto status = isValidNormalized(_root.get()); !status.isOK()) { + return status; + } // Validate the projection if there is one. if (!_qr->getProj().isEmpty()) { @@ -360,7 +365,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()) { @@ -374,7 +379,7 @@ bool hasNodeInSubtree(MatchExpression* root, return false; } -StatusWith<QueryMetadataBitSet> CanonicalQuery::isValid(MatchExpression* root, +StatusWith<QueryMetadataBitSet> CanonicalQuery::isValid(const MatchExpression* root, const QueryRequest& request) { QueryMetadataBitSet unavailableMetadata{}; @@ -400,20 +405,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; @@ -489,6 +481,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) { + 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 e1261805e20..e6ff4685ab9 100644 --- a/src/mongo/db/query/canonical_query.h +++ b/src/mongo/db/query/canonical_query.h @@ -109,15 +109,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 QueryRequest& request); + /** + * Perform additional validation checks on the normalized 'root'. + */ + static Status isValidNormalized(const MatchExpression* root); + const NamespaceString& nss() const { return _qr->nss(); } |