diff options
| author | Tess Avitabile <tess.avitabile@mongodb.com> | 2017-10-02 17:24:03 -0400 |
|---|---|---|
| committer | Tess Avitabile <tess.avitabile@mongodb.com> | 2017-10-05 16:47:26 -0400 |
| commit | cb9f7cdcb7eb6ad9077ac8af3a4c0d7275c7e34f (patch) | |
| tree | 24fea4c596a4652a457bc496d34a5590b7882190 | |
| parent | e02285559b5f15be6afbf876f169874bd9008b0b (diff) | |
| download | mongo-cb9f7cdcb7eb6ad9077ac8af3a4c0d7275c7e34f.tar.gz | |
SERVER-30731 Add expr support in MatchExpression outside of aggregation
54 files changed, 807 insertions, 391 deletions
diff --git a/buildscripts/resmokeconfig/suites/aggregation_sharded_collections_passthrough.yml b/buildscripts/resmokeconfig/suites/aggregation_sharded_collections_passthrough.yml index 69460671b4d..de8b568eee6 100644 --- a/buildscripts/resmokeconfig/suites/aggregation_sharded_collections_passthrough.yml +++ b/buildscripts/resmokeconfig/suites/aggregation_sharded_collections_passthrough.yml @@ -30,8 +30,6 @@ selector: - jstests/aggregation/bugs/lookup_unwind_killcursor.js # TODO: Remove after SERVER-23229 is fixed. - jstests/aggregation/bugs/groupMissing.js - # TODO SERVER-30731: Remove once ChunkManager::getShardIdsForQuery() supports query with $expr. - - jstests/aggregation/sources/match/expr_match.js exclude_with_any_tags: # Tests tagged with the following will fail because they assume collections are not sharded. - assumes_no_implicit_collection_creation_after_drop diff --git a/jstests/aggregation/sources/graphLookup/error.js b/jstests/aggregation/sources/graphLookup/error.js index 143ea5bc31f..20dad85b842 100644 --- a/jstests/aggregation/sources/graphLookup/error.js +++ b/jstests/aggregation/sources/graphLookup/error.js @@ -302,10 +302,10 @@ load("jstests/aggregation/extras/utils.js"); // For "assertErrorCode". connectToField: "a", connectFromField: "b", as: "output", - restrictSearchWithMatch: {$expr: {$eq: ["$x", 5]}} + restrictSearchWithMatch: {$expr: {$eq: ["$x", "$$unbound"]}} } }; - assertErrorCode(local, pipeline, 40186, "cannot use $expr inside $graphLookup"); + assertErrorCode(local, pipeline, 17276, "cannot use $expr with unbound variable"); // $graphLookup can only consume at most 100MB of memory. var foreign = db.foreign; diff --git a/jstests/aggregation/sources/graphLookup/filter.js b/jstests/aggregation/sources/graphLookup/filter.js index 15ff6d4a4ae..b6eaff41b25 100644 --- a/jstests/aggregation/sources/graphLookup/filter.js +++ b/jstests/aggregation/sources/graphLookup/filter.js @@ -75,4 +75,20 @@ .toArray()[0]; assert.eq(res.results.length, 1); + + // $expr is allowed inside the 'restrictSearchWithMatch' match expression. + // TODO SERVER-31029: This should not throw once support is added for top-level $expr within $or + // and $and. + assert.throws(function() { + local.aggregate({ + $graphLookup: { + from: "foreign", + startWith: "$starting", + connectFromField: "to", + connectToField: "from", + as: "results", + restrictSearchWithMatch: {$expr: {$eq: ["$shouldBeIncluded", true]}} + } + }); + }); })(); diff --git a/jstests/core/collation.js b/jstests/core/collation.js index ac89db598b6..98da1ba8ebf 100644 --- a/jstests/core/collation.js +++ b/jstests/core/collation.js @@ -646,6 +646,14 @@ .collation({locale: "en_US", strength: 3}) .sort({a: 1}); assert.eq(res.toArray(), [{a: "a"}, {a: "A"}, {a: "b"}, {a: "B"}]); + + // Find should return correct results when collation specified and query contains $expr. + coll.drop(); + assert.writeOK(coll.insert([{a: "A"}, {a: "B"}])); + assert.eq(1, + coll.find({$expr: {$eq: ["$a", "a"]}}) + .collation({locale: "en_US", strength: 2}) + .itcount()); } // Find should return correct results when no collation specified and collection has a default @@ -692,6 +700,14 @@ .addOption(DBQuery.Option.oplogReplay) .itcount()); + // Find should return correct results for query containing $expr when no collation specified and + // collection has a default collation. + coll.drop(); + assert.commandWorked( + db.createCollection(coll.getName(), {collation: {locale: "en_US", strength: 2}})); + assert.writeOK(coll.insert([{a: "A"}, {a: "B"}])); + assert.eq(1, coll.find({$expr: {$eq: ["$a", "a"]}}).itcount()); + if (db.getMongo().useReadCommands()) { // Find should return correct results when "simple" collation specified and collection has a // default collation. diff --git a/jstests/core/doc_validation.js b/jstests/core/doc_validation.js index 7a3dc5f038d..e1d02bdba9b 100644 --- a/jstests/core/doc_validation.js +++ b/jstests/core/doc_validation.js @@ -98,4 +98,22 @@ assert.writeOK(coll.update({_id: 'invalid2'}, {$set: {a: 1}})); coll.drop(); + // The validator is allowed to contain $expr. + assert.commandWorked(db.createCollection(collName, {validator: {$expr: {$eq: ["$a", 5]}}})); + assert.writeOK(coll.insert({a: 5})); + assertFailsValidation(coll.insert({a: 4})); + assert.commandWorked( + db.runCommand({"collMod": collName, "validator": {$expr: {$eq: ["$a", 4]}}})); + assert.writeOK(coll.insert({a: 4})); + assertFailsValidation(coll.insert({a: 5})); + + // The validator can contain an $expr that may throw at runtime. + coll.drop(); + assert.commandWorked( + db.createCollection(collName, {validator: {$expr: {$eq: ["$a", {$divide: [1, "$b"]}]}}})); + assert.writeOK(coll.insert({a: 1, b: 1})); + let res = coll.insert({a: 1, b: 0}); + assert.writeError(res); + assert.eq(res.getWriteError().code, 16608); + assert.writeOK(coll.insert({a: -1, b: -1})); })(); diff --git a/jstests/core/doc_validation_invalid_validators.js b/jstests/core/doc_validation_invalid_validators.js index e56977ac8e1..3cedaf0d537 100644 --- a/jstests/core/doc_validation_invalid_validators.js +++ b/jstests/core/doc_validation_invalid_validators.js @@ -22,6 +22,8 @@ assert.commandFailed(db.createCollection(collName, {validator: {$geoNear: {place: "holder"}}})); assert.commandFailed( db.createCollection(collName, {validator: {$nearSphere: {place: "holder"}}})); + assert.commandFailed( + db.createCollection(collName, {validator: {$expr: {$eq: ["$a", "$$unbound"]}}})); // Verify we fail on admin, local and config databases. assert.commandFailed( @@ -47,9 +49,8 @@ db.runCommand({"collMod": collName, "validator": {$geoNear: {place: "holder"}}})); assert.commandFailed( db.runCommand({"collMod": collName, "validator": {$nearSphere: {place: "holder"}}})); - assert.commandFailed( - db.runCommand({"collMod": collName, "validator": {$expr: {$eq: ["$a", 5]}}})); + db.runCommand({"collMod": collName, "validator": {$expr: {$eq: ["$a", "$$unbound"]}}})); coll.drop(); diff --git a/jstests/core/expr.js b/jstests/core/expr.js new file mode 100644 index 00000000000..d2c6d9216ff --- /dev/null +++ b/jstests/core/expr.js @@ -0,0 +1,239 @@ +// Tests for $expr in the CRUD commands. +(function() { + "use strict"; + + const coll = db.expr; + + const isMaster = db.runCommand("ismaster"); + assert.commandWorked(isMaster); + const isMongos = (isMaster.msg === "isdbgrid"); + + // + // $expr in aggregate. + // + + coll.drop(); + assert.writeOK(coll.insert({a: 5})); + assert.eq(1, coll.aggregate([{$match: {$expr: {$eq: ["$a", 5]}}}]).itcount()); + assert.throws(function() { + coll.aggregate([{$match: {$expr: {$eq: ["$a", "$$unbound"]}}}]); + }); + + // + // $expr in count. + // + + coll.drop(); + assert.writeOK(coll.insert({a: 5})); + assert.eq(1, coll.find({$expr: {$eq: ["$a", 5]}}).count()); + assert.throws(function() { + coll.find({$expr: {$eq: ["$a", "$$unbound"]}}).count(); + }); + + // + // $expr in distinct. + // + + coll.drop(); + assert.writeOK(coll.insert({a: 5})); + assert.eq(1, coll.distinct("a", {$expr: {$eq: ["$a", 5]}}).length); + assert.throws(function() { + coll.distinct("a", {$expr: {$eq: ["$a", "$$unbound"]}}); + }); + + // + // $expr in find. + // + + // $expr is allowed in query. + coll.drop(); + assert.writeOK(coll.insert({a: 5})); + assert.eq(1, coll.find({$expr: {$eq: ["$a", 5]}}).itcount()); + + // $expr with unbound variable throws. + assert.throws(function() { + coll.find({$expr: {$eq: ["$a", "$$unbound"]}}).itcount(); + }); + + // $expr is allowed in find with explain. + assert.commandWorked(coll.find({$expr: {$eq: ["$a", 5]}}).explain()); + + // $expr with unbound variable in find with explain throws. + assert.throws(function() { + coll.find({$expr: {$eq: ["$a", "$$unbound"]}}).explain(); + }); + + // $expr is not allowed in $elemMatch projection. + coll.drop(); + assert.writeOK(coll.insert({a: [{b: 5}]})); + assert.throws(function() { + coll.find({}, {a: {$elemMatch: {$expr: {$eq: ["$b", 5]}}}}).itcount(); + }); + + // + // $expr in findAndModify. + // + + // $expr is allowed in the query when upsert=false. + coll.drop(); + assert.writeOK(coll.insert({_id: 0, a: 5})); + assert.eq({_id: 0, a: 5, b: 6}, + coll.findAndModify( + {query: {_id: 0, $expr: {$eq: ["$a", 5]}}, update: {$set: {b: 6}}, new: true})); + + // $expr with unbound variable throws. + assert.throws(function() { + coll.findAndModify( + {query: {_id: 0, $expr: {$eq: ["$a", "$$unbound"]}}, update: {$set: {b: 6}}}); + }); + + // $expr is not allowed in the query when upsert=true. + coll.drop(); + assert.writeOK(coll.insert({_id: 0, a: 5})); + assert.throws(function() { + coll.findAndModify( + {query: {_id: 0, $expr: {$eq: ["$a", 5]}}, update: {$set: {b: 6}}, upsert: true}); + }); + + // $expr is not allowed in $pull filter. + coll.drop(); + assert.writeOK(coll.insert({_id: 0, a: [{b: 5}]})); + assert.throws(function() { + coll.findAndModify({query: {_id: 0}, update: {$pull: {a: {$expr: {$eq: ["$b", 5]}}}}}); + }); + + // $expr is not allowed in arrayFilters. + if (db.getMongo().writeMode() === "commands") { + coll.drop(); + assert.writeOK(coll.insert({_id: 0, a: [{b: 5}]})); + assert.throws(function() { + coll.findAndModify({ + query: {_id: 0}, + update: {$set: {"a.$[i].b": 6}}, + arrayFilters: [{"i.b": 5, $expr: {$eq: ["$i.b", 5]}}] + }); + }); + } + + // + // $expr in geoNear. + // + + coll.drop(); + assert.writeOK(coll.insert({geo: {type: "Point", coordinates: [0, 0]}, a: 5})); + assert.commandWorked(coll.ensureIndex({geo: "2dsphere"})); + assert.eq(1, + assert + .commandWorked(db.runCommand({ + geoNear: coll.getName(), + near: {type: "Point", coordinates: [0, 0]}, + spherical: true, + query: {$expr: {$eq: ["$a", 5]}} + })) + .results.length); + assert.commandFailed(db.runCommand({ + geoNear: coll.getName(), + near: {type: "Point", coordinates: [0, 0]}, + spherical: true, + query: {$expr: {$eq: ["$a", "$$unbound"]}} + })); + + // + // $expr in group. + // + + // The group command is not permitted in sharded collections. + if (!isMongos) { + coll.drop(); + assert.writeOK(coll.insert({a: 5})); + assert.eq([{a: 5, count: 1}], coll.group({ + cond: {$expr: {$eq: ["$a", 5]}}, + key: {a: 1}, + initial: {count: 0}, + reduce: function(curr, result) { + result.count += 1; + } + })); + assert.throws(function() { + coll.group({ + cond: {$expr: {$eq: ["$a", "$$unbound"]}}, + key: {a: 1}, + initial: {count: 0}, + reduce: function(curr, result) { + result.count += 1; + } + }); + }); + } + + // + // $expr in mapReduce. + // + + coll.drop(); + assert.writeOK(coll.insert({a: 5})); + let mapReduceOut = coll.mapReduce( + function() { + emit(this.a, 1); + }, + function(key, values) { + return Array.sum(values); + }, + {out: {inline: 1}, query: {$expr: {$eq: ["$a", 5]}}}); + assert.commandWorked(mapReduceOut); + assert.eq(mapReduceOut.results.length, 1, tojson(mapReduceOut)); + assert.throws(function() { + coll.mapReduce( + function() { + emit(this.a, 1); + }, + function(key, values) { + return Array.sum(values); + }, + {out: {inline: 1}, query: {$expr: {$eq: ["$a", "$$unbound"]}}}); + }); + + // + // $expr in remove. + // + + coll.drop(); + assert.writeOK(coll.insert({_id: 0, a: 5})); + let writeRes = coll.remove({_id: 0, $expr: {$eq: ["$a", 5]}}); + assert.writeOK(writeRes); + assert.eq(1, writeRes.nRemoved); + assert.writeError(coll.remove({_id: 0, $expr: {$eq: ["$a", "$$unbound"]}})); + + // + // $expr in update. + // + + // $expr is allowed in the query when upsert=false. + coll.drop(); + assert.writeOK(coll.insert({_id: 0, a: 5})); + assert.writeOK(coll.update({_id: 0, $expr: {$eq: ["$a", 5]}}, {$set: {b: 6}})); + assert.eq({_id: 0, a: 5, b: 6}, coll.findOne({_id: 0})); + + // $expr with unbound variable fails. + assert.writeError(coll.update({_id: 0, $expr: {$eq: ["$a", "$$unbound"]}}, {$set: {b: 6}})); + + // $expr is not allowed in the query when upsert=true. + coll.drop(); + assert.writeOK(coll.insert({_id: 0, a: 5})); + assert.writeError( + coll.update({_id: 0, $expr: {$eq: ["$a", 5]}}, {$set: {b: 6}}, {upsert: true})); + + // $expr is not allowed in $pull filter. + coll.drop(); + assert.writeOK(coll.insert({_id: 0, a: [{b: 5}]})); + assert.writeError(coll.update({_id: 0}, {$pull: {a: {$expr: {$eq: ["$b", 5]}}}})); + + // $expr is not allowed in arrayFilters. + if (db.getMongo().writeMode() === "commands") { + coll.drop(); + assert.writeOK(coll.insert({_id: 0, a: [{b: 5}]})); + assert.writeError(coll.update({_id: 0}, + {$set: {"a.$[i].b": 6}}, + {arrayFilters: [{"i.b": 5, $expr: {$eq: ["$i.b", 5]}}]})); + } +})(); diff --git a/jstests/core/index_filter_commands.js b/jstests/core/index_filter_commands.js index d0973aacd69..644707fa199 100644 --- a/jstests/core/index_filter_commands.js +++ b/jstests/core/index_filter_commands.js @@ -234,3 +234,42 @@ assert.commandWorked( explain = t.find(queryAA).collation(collationEN).explain(); assert(isCollscan(explain.queryPlanner.winningPlan), "Expected collscan: " + tojson(explain)); + +// +// Test that planCacheSetFilter and planCacheClearFilters allow queries containing $expr. +// + +t.drop(); +assert.writeOK(t.insert({a: "a"})); +assert.commandWorked(t.createIndex(indexA1, {name: "a_1"})); + +assert.commandWorked(t.runCommand( + "planCacheSetFilter", {query: {a: "a", $expr: {$eq: ["$a", "a"]}}, indexes: [indexA1]})); +filters = getFilters(); +assert.eq(1, filters.length, tojson(filters)); +assert.eq({a: "a", $expr: {$eq: ["$a", "a"]}}, filters[0].query, tojson(filters[0])); + +assert.commandWorked( + t.runCommand("planCacheClearFilters", {query: {a: "a", $expr: {$eq: ["$a", "a"]}}})); +filters = getFilters(); +assert.eq(0, filters.length, tojson(filters)); + +// +// Test that planCacheSetFilter and planCacheClearFilters do not allow queries containing $expr with +// unbound variables. +// + +t.drop(); +assert.writeOK(t.insert({a: "a"})); +assert.commandWorked(t.createIndex(indexA1, {name: "a_1"})); + +assert.commandFailed( + t.runCommand("planCacheSetFilter", + {query: {a: "a", $expr: {$eq: ["$a", "$$unbound"]}}, indexes: [indexA1]})); +filters = getFilters(); +assert.eq(0, filters.length, tojson(filters)); + +assert.commandFailed( + t.runCommand("planCacheClearFilters", {query: {a: "a", $expr: {$eq: ["$a", "$$unbound"]}}})); +filters = getFilters(); +assert.eq(0, filters.length, tojson(filters)); diff --git a/jstests/core/index_partial_create_drop.js b/jstests/core/index_partial_create_drop.js index 5875093c706..f3bc37efbd9 100644 --- a/jstests/core/index_partial_create_drop.js +++ b/jstests/core/index_partial_create_drop.js @@ -32,6 +32,8 @@ partialFilterExpression: {$and: [{$and: [{x: {$lt: 2}}, {x: {$gt: 0}}]}, {x: {$exists: true}}]} })); + assert.commandFailed( + coll.createIndex({x: 1}, {partialFilterExpression: {$expr: {$eq: ["$x", 5]}}})); for (var i = 0; i < 10; i++) { assert.writeOK(coll.insert({x: i, a: i})); diff --git a/jstests/core/list_collections_filter.js b/jstests/core/list_collections_filter.js index 41d80d0a344..defd5c62bc0 100644 --- a/jstests/core/list_collections_filter.js +++ b/jstests/core/list_collections_filter.js @@ -84,6 +84,14 @@ }, []); + // Filter with $expr. + testListCollections({$expr: {$eq: ["$name", "lists"]}}, ["lists"]); + + // Filter with $expr with an unbound variable. + assert.throws(function() { + mydb.getCollectionInfos({$expr: {$eq: ["$name", "$$unbound"]}}); + }); + // No extensions are allowed in filters. assert.throws(function() { mydb.getCollectionInfos({$text: {$search: "str"}}); @@ -99,8 +107,4 @@ mydb.getCollectionInfos( {a: {$nearSphere: {$geometry: {type: "Point", coordinates: [0, 0]}}}}); }); - - assert.throws(function() { - mydb.getCollectionInfos({$expr: {$eq: ["$a", 5]}}); - }); }()); diff --git a/jstests/core/list_databases.js b/jstests/core/list_databases.js index ecc746ce775..a52cfec656b 100644 --- a/jstests/core/list_databases.js +++ b/jstests/core/list_databases.js @@ -62,6 +62,16 @@ assert.eq(1, cmdRes.databases.length, tojson(cmdRes)); verifyNameOnly(cmdRes); + // $expr in filter. + cmdRes = assert.commandWorked(db.adminCommand( + {listDatabases: 1, filter: {$expr: {$eq: ["$name", "jstest_list_databases_zap"]}}})); + assert.eq(1, cmdRes.databases.length, tojson(cmdRes)); + assert.eq("jstest_list_databases_zap", cmdRes.databases[0].name, tojson(cmdRes)); + + // $expr with an unbound variable in filter. + assert.commandFailed( + db.adminCommand({listDatabases: 1, filter: {$expr: {$eq: ["$name", "$$unbound"]}}})); + // No extensions are allowed in filters. assert.commandFailed(db.adminCommand({listDatabases: 1, filter: {$text: {$search: "str"}}})); assert.commandFailed(db.adminCommand({ @@ -76,6 +86,4 @@ listDatabases: 1, filter: {a: {$nearSphere: {$geometry: {type: "Point", coordinates: [0, 0]}}}} })); - - assert.commandFailed(db.adminCommand({listDatabases: 1, filter: {$expr: {$eq: ["$a", 5]}}})); }()); diff --git a/jstests/core/plan_cache_clear.js b/jstests/core/plan_cache_clear.js index 8f9cf0ea302..bb44ecab549 100644 --- a/jstests/core/plan_cache_clear.js +++ b/jstests/core/plan_cache_clear.js @@ -37,6 +37,17 @@ assert.eq(1, getShapes().length, 'unexpected cache size after running 2nd query' assert.commandWorked(t.runCommand('planCacheClear', {query: {a: 1, b: 1}})); assert.eq(0, getShapes().length, 'unexpected cache size after dropping 2nd query from cache'); +// planCacheClear can clear $expr queries. +assert.eq(1, t.find({a: 1, b: 1, $expr: {$eq: ['$a', 1]}}).itcount(), 'unexpected document count'); +assert.eq(1, getShapes().length, 'unexpected cache size after running 2nd query'); +assert.commandWorked( + t.runCommand('planCacheClear', {query: {a: 1, b: 1, $expr: {$eq: ['$a', 1]}}})); +assert.eq(0, getShapes().length, 'unexpected cache size after dropping 2nd query from cache'); + +// planCacheClear fails with an $expr query with an unbound variable. +assert.commandFailed( + t.runCommand('planCacheClear', {query: {a: 1, b: 1, $expr: {$eq: ['$a', '$$unbound']}}})); + // Insert two more shapes into the cache. assert.eq(1, t.find({a: 1, b: 1}).itcount(), 'unexpected document count'); assert.eq(1, t.find({a: 1, b: 1}, {_id: 0, a: 1}).itcount(), 'unexpected document count'); diff --git a/jstests/multiVersion/collection_validator_feature_compatibility_version.js b/jstests/multiVersion/collection_validator_feature_compatibility_version.js new file mode 100644 index 00000000000..f7bb6b17a80 --- /dev/null +++ b/jstests/multiVersion/collection_validator_feature_compatibility_version.js @@ -0,0 +1,154 @@ +/** + * Test that mongod will not allow creation of collection validators using 3.6 query features when + * the feature compatibility version is older than 3.6. + * + * We restart mongod during the test and expect it to have the same data after restarting. + * @tags: [requires_persistence] + */ + +(function() { + "use strict"; + + const testName = "collection_validator_feature_compatibility_version"; + const dbpath = MongoRunner.dataPath + testName; + + /** + * Tests the correct behavior of a collection validator using 3.6 query features with different + * binary versions and feature compatibility versions. 'validator' should be a collection + * validator using a new 3.6 query feature, and 'nonMatchingDocument' should be a document that + * does not match 'validator'. + */ + function testValidator(validator, nonMatchingDocument) { + resetDbpath(dbpath); + + let conn = MongoRunner.runMongod({dbpath: dbpath, binVersion: "latest"}); + assert.neq(null, conn, "mongod was unable to start up"); + + let testDB = conn.getDB(testName); + + let adminDB = conn.getDB("admin"); + + // Explicitly set feature compatibility version 3.6. + assert.commandWorked(adminDB.runCommand({setFeatureCompatibilityVersion: "3.6"})); + + // Create a collection with a validator using 3.6 query features. + assert.commandWorked(testDB.createCollection("coll", {validator: validator})); + let coll = testDB.coll; + + // The validator should cause this insert to fail. + assert.writeError(coll.insert(nonMatchingDocument), ErrorCodes.DocumentValidationFailure); + + // Set a validator using 3.6 query features on an existing collection. + coll.drop(); + assert.commandWorked(testDB.createCollection("coll")); + assert.commandWorked(testDB.runCommand({collMod: "coll", validator: validator})); + + // Another failing update. + assert.writeError(coll.insert(nonMatchingDocument), ErrorCodes.DocumentValidationFailure); + + // Set the feature compatibility version to 3.4. + assert.commandWorked(adminDB.runCommand({setFeatureCompatibilityVersion: "3.4"})); + + // The validator is already in place, so it should still cause this insert to fail. + assert.writeError(coll.insert(nonMatchingDocument), ErrorCodes.DocumentValidationFailure); + + // Trying to create a new collection with a validator using 3.6 query features should fail + // while feature compatibility version is 3.4. + let res = testDB.createCollection("coll2", {validator: validator}); + assert.commandFailedWithCode(res, ErrorCodes.QueryFeatureNotAllowed); + assert(res.errmsg.match(/featureCompatibilityVersion/), + "Expected error message from createCollection referencing " + + "'featureCompatibilityVersion' but instead got: " + res.errmsg); + + // Trying to update a collection with a validator using 3.6 query features should also fail. + res = testDB.runCommand({collMod: "coll", validator: validator}); + assert.commandFailedWithCode(res, ErrorCodes.QueryFeatureNotAllowed); + assert( + res.errmsg.match(/featureCompatibilityVersion/), + "Expected error message from collMod referencing 'featureCompatibilityVersion' but " + + "instead got: " + res.errmsg); + + MongoRunner.stopMongod(conn); + + // If we try to start up a 3.4 mongod, it will fail, because it will not be able to parse + // the validator using 3.6 query features. + conn = MongoRunner.runMongod({dbpath: dbpath, binVersion: "3.4", noCleanData: true}); + assert.eq(null, + conn, + "mongod 3.4 started, even with a validator using 3.6 query features in place."); + + // Starting up a 3.6 mongod, however, should succeed, even though the feature compatibility + // version is still set to 3.4. + conn = MongoRunner.runMongod({dbpath: dbpath, binVersion: "latest", noCleanData: true}); + assert.neq(null, conn, "mongod was unable to start up"); + + adminDB = conn.getDB("admin"); + testDB = conn.getDB(testName); + coll = testDB.coll; + + // And the validator should still work. + assert.writeError(coll.insert(nonMatchingDocument), ErrorCodes.DocumentValidationFailure); + + // Remove the validator. + assert.commandWorked(testDB.runCommand({collMod: "coll", validator: {}})); + + MongoRunner.stopMongod(conn); + + // Now, we should be able to start up a 3.4 mongod. + conn = MongoRunner.runMongod({dbpath: dbpath, binVersion: "3.4", noCleanData: true}); + assert.neq( + null, + conn, + "mongod 3.4 failed to start, even after we removed the validator using 3.6 query features"); + + MongoRunner.stopMongod(conn); + + // The rest of the test uses mongod 3.6. + conn = MongoRunner.runMongod({dbpath: dbpath, binVersion: "latest", noCleanData: true}); + assert.neq(null, conn, "mongod was unable to start up"); + + adminDB = conn.getDB("admin"); + testDB = conn.getDB(testName); + coll = testDB.coll; + + // Set the feature compatibility version back to 3.6. + assert.commandWorked(adminDB.runCommand({setFeatureCompatibilityVersion: "3.6"})); + + // Now we should be able to create a collection with a validator using 3.6 query features + // again. + assert.commandWorked(testDB.createCollection("coll2", {validator: validator})); + + // And we should be able to modify a collection to have a validator using 3.6 query + // features. + assert.commandWorked(testDB.runCommand({collMod: "coll", validator: validator})); + + // Set the feature compatibility version to 3.4 and then restart with + // internalValidateFeaturesAsMaster=false. + assert.commandWorked(adminDB.runCommand({setFeatureCompatibilityVersion: "3.4"})); + MongoRunner.stopMongod(conn); + conn = MongoRunner.runMongod({ + dbpath: dbpath, + binVersion: "latest", + noCleanData: true, + setParameter: "internalValidateFeaturesAsMaster=false" + }); + assert.neq(null, conn, "mongod was unable to start up"); + + testDB = conn.getDB(testName); + + // Even though the feature compatibility version is 3.4, we should still be able to add a + // validator using 3.6 query features, because internalValidateFeaturesAsMaster is false. + assert.commandWorked(testDB.createCollection("coll3", {validator: validator})); + + // We should also be able to modify a collection to have a validator using 3.6 query + // features. + testDB.coll3.drop(); + assert.commandWorked(testDB.createCollection("coll3")); + assert.commandWorked(testDB.runCommand({collMod: "coll3", validator: validator})); + + MongoRunner.stopMongod(conn); + } + + testValidator({$jsonSchema: {properties: {foo: {type: "string"}}}}, {foo: 1.0}); + testValidator({$expr: {$eq: ["$a", "good"]}}, {a: "bad"}); +}()); diff --git a/jstests/multiVersion/json_schema_feature_compatibility_version.js b/jstests/multiVersion/json_schema_feature_compatibility_version.js deleted file mode 100644 index e17a3cdc96b..00000000000 --- a/jstests/multiVersion/json_schema_feature_compatibility_version.js +++ /dev/null @@ -1,136 +0,0 @@ -/** - * Test that mongod will not allow creationg of JSON schema validators when the feature - * compatibility version is older than 3.6. - * - * We restart mongod during the test and expect it to have the same data after restarting. - * @tags: [requires_persistence] - */ - -(function() { - "use strict"; - - const testName = "json_schema_feature_compatibility_on_startup"; - let dbpath = MongoRunner.dataPath + testName; - resetDbpath(dbpath); - - let conn = MongoRunner.runMongod({dbpath: dbpath, binVersion: "latest"}); - assert.neq(null, conn, "mongod was unable to start up"); - - let testDB = conn.getDB(testName); - assert.commandWorked(testDB.dropDatabase()); - - let adminDB = conn.getDB("admin"); - - // Explicitly set feature compatibility version 3.6. - assert.commandWorked(adminDB.runCommand({setFeatureCompatibilityVersion: "3.6"})); - - // Create a collection with a JSON Schema validator. - assert.commandWorked(testDB.createCollection( - "coll", {validator: {$jsonSchema: {properties: {foo: {type: "string"}}}}})); - let coll = testDB.coll; - - // The validator should cause this insert to fail. - assert.writeError(coll.insert({foo: 1.0}), ErrorCodes.DocumentValidationFailure); - - // Set a JSON Schema validator on an existing collection. - assert.commandWorked(testDB.runCommand( - {collMod: "coll", validator: {$jsonSchema: {properties: {bar: {type: "string"}}}}})); - - // Another failing update. - assert.writeError(coll.insert({bar: 1.0}), ErrorCodes.DocumentValidationFailure); - - // Set the feature compatibility version to 3.4. - assert.commandWorked(adminDB.runCommand({setFeatureCompatibilityVersion: "3.4"})); - - // The validator is already in place, so it should still cause this insert to fail. - assert.writeError(coll.insert({bar: 1.0}), ErrorCodes.DocumentValidationFailure); - - // Trying to create a new collection with a JSON Schema validator should fail while feature - // compatibility version is 3.4. - let res = testDB.createCollection("coll2", {validator: {$jsonSchema: {}}}); - assert.commandFailedWithCode(res, ErrorCodes.JSONSchemaNotAllowed); - assert( - res.errmsg.match(/featureCompatibilityVersion/), - "Expected error message from createCollection referencing 'featureCompatibilityVersion' but instead got: " + - res.errmsg); - - // Trying to update a collection with a JSON Schema validator should also fail. - res = testDB.runCommand({collMod: "coll", validator: {$jsonSchema: {}}}); - assert.commandFailedWithCode(res, ErrorCodes.JSONSchemaNotAllowed); - assert( - res.errmsg.match(/featureCompatibilityVersion/), - "Expected error message from collMod referencing 'featureCompatibilityVersion' but instead got: " + - res.errmsg); - - MongoRunner.stopMongod(conn); - - // If we try to start up a 3.4 mongod, it will fail, because it will not be able to parse the - // $jsonSchema validator. - conn = MongoRunner.runMongod({dbpath: dbpath, binVersion: "3.4", noCleanData: true}); - assert.eq(null, conn, "mongod 3.4 started, even with a $jsonSchema validator in place."); - - // Starting up a 3.6 mongod, however, should succeed, even though the feature compatibility - // version is still set to 3.4. - conn = MongoRunner.runMongod({dbpath: dbpath, binVersion: "latest", noCleanData: true}); - assert.neq(null, conn, "mongod was unable to start up"); - - adminDB = conn.getDB("admin"); - testDB = conn.getDB(testName); - coll = testDB.coll; - - // And the validator should still work. - assert.writeError(coll.insert({bar: 1.0}), ErrorCodes.DocumentValidationFailure); - - // Remove the validator. - assert.commandWorked(testDB.runCommand({collMod: "coll", validator: {}})); - - MongoRunner.stopMongod(conn); - - // Now, we should be able to start up a 3.4 mongod. - conn = MongoRunner.runMongod({dbpath: dbpath, binVersion: "3.4", noCleanData: true}); - assert.neq( - null, conn, "mongod 3.4 failed to start, even after we removed the $jsonSchema validator"); - - MongoRunner.stopMongod(conn); - - // The rest of the test uses mongod 3.6. - conn = MongoRunner.runMongod({dbpath: dbpath, binVersion: "latest", noCleanData: true}); - assert.neq(null, conn, "mongod was unable to start up"); - - adminDB = conn.getDB("admin"); - testDB = conn.getDB(testName); - coll = testDB.coll; - - // Set the feature compatibility version back to 3.6. - assert.commandWorked(adminDB.runCommand({setFeatureCompatibilityVersion: "3.6"})); - - // Now we should be able to create a collection with a JSON Schema validator again. - assert.commandWorked(testDB.createCollection("coll2", {validator: {$jsonSchema: {}}})); - - // And we should be able to modify a collection to have a JSON Schema validator. - assert.commandWorked(testDB.runCommand({collMod: "coll", validator: {$jsonSchema: {}}})); - - // Set the feature compatibility version to 3.4 and then restart with - // internalValidateFeaturesAsMaster=false. - assert.commandWorked(adminDB.runCommand({setFeatureCompatibilityVersion: "3.4"})); - MongoRunner.stopMongod(conn); - conn = MongoRunner.runMongod({ - dbpath: dbpath, - binVersion: "latest", - noCleanData: true, - setParameter: "internalValidateFeaturesAsMaster=false" - }); - assert.neq(null, conn, "mongod was unable to start up"); - - testDB = conn.getDB(testName); - - // Even though the feature compatibility version is 3.4, we should still be able to add a - // $jsonSchema validator, because internalValidateFeaturesAsMaster is false. - assert.commandWorked(testDB.createCollection("coll3", {validator: {$jsonSchema: {}}})); - - // We should also be able to modify a collection to have a $jsonSchema validator. - assert.commandWorked( - testDB.runCommand({collMod: "coll3", validator: {properties: {str: {type: "string"}}}})); - - MongoRunner.stopMongod(conn); -}()); diff --git a/jstests/replsets/collection_validator_initial_sync_with_feature_compatibility.js b/jstests/replsets/collection_validator_initial_sync_with_feature_compatibility.js new file mode 100644 index 00000000000..014ee8daf7b --- /dev/null +++ b/jstests/replsets/collection_validator_initial_sync_with_feature_compatibility.js @@ -0,0 +1,87 @@ +/** + * Test that a new replica set member can successfully sync a collection with a validator using 3.6 + * query features, even when the replica set was downgraded to feature compatibility version 3.4. + * + * We restart the secondary as part of this test with the expecation that it still has the same + * data after the restart. + * @tags: [requires_persistence] + */ + +load("jstests/replsets/rslib.js"); + +(function() { + "use strict"; + const testName = "collection_validator_initial_sync_with_feature_compatibility"; + + function testValidator(validator, nonMatchingDocument) { + // + // Create a single-node replica set. + // + let replTest = new ReplSetTest({name: testName, nodes: 1}); + + replTest.startSet(); + replTest.initiate(); + + let primary = replTest.getPrimary(); + let testDB = primary.getDB("test"); + + // + // Explicitly set the replica set to feature compatibility version 3.6. + // + assert.commandWorked(primary.adminCommand({setFeatureCompatibilityVersion: "3.6"})); + + // + // Create a collection with a validator using 3.6 query features. + // + assert.commandWorked(testDB.createCollection("coll", {validator: validator})); + + // + // Downgrade the replica set to feature compatibility version 3.4. + // + assert.commandWorked(primary.adminCommand({setFeatureCompatibilityVersion: "3.4"})); + + // + // Add a new member to the replica set. + // + let secondaryDBPath = MongoRunner.dataPath + testName + "_secondary"; + resetDbpath(secondaryDBPath); + let secondary = replTest.add({dbpath: secondaryDBPath}); + replTest.reInitiate(secondary); + reconnect(primary); + reconnect(secondary); + + // + // Once the new member completes its initial sync, stop it, remove it from the replica + // set, and start it back up as an individual instance. + // + replTest.waitForState(secondary, [ReplSetTest.State.PRIMARY, ReplSetTest.State.SECONDARY]); + + replTest.stopSet(undefined /* send default signal */, + true /* don't clear data directory */); + + secondary = MongoRunner.runMongod({dbpath: secondaryDBPath, noCleanData: true}); + assert.neq(null, secondary, "mongod was unable to start up"); + + // + // Verify that the validator synced to the new member by attempting to insert a document + // that does not validate and checking that the insert fails. + // + let secondaryDB = secondary.getDB("test"); + assert.writeError(secondaryDB.coll.insert(nonMatchingDocument), + ErrorCodes.DocumentValidationFailure); + + // + // Verify that, even though the existing validator still works, it is not possible to + // create a new validator using 3.6 query features because of feature compatibility version + // 3.4. + // + assert.commandFailedWithCode( + secondaryDB.runCommand({collMod: "coll", validator: validator}), + ErrorCodes.QueryFeatureNotAllowed); + + MongoRunner.stopMongod(secondary); + } + + testValidator({$jsonSchema: {properties: {foo: {type: "string"}}}}, {foo: 1.0}); + testValidator({$expr: {$eq: ["$a", "good"]}}, {a: "bad"}); +}());
\ No newline at end of file diff --git a/jstests/replsets/json_schema_initial_sync_with_feature_compatibility.js b/jstests/replsets/json_schema_initial_sync_with_feature_compatibility.js deleted file mode 100644 index 5550be61d79..00000000000 --- a/jstests/replsets/json_schema_initial_sync_with_feature_compatibility.js +++ /dev/null @@ -1,91 +0,0 @@ -/** - * Test that a new replica set member can successfully sync a collection with a $jsonSchema - * validator, even when the replica set was downgraded to feature compatibility version 3.4. - * - * We restart the secondary as part of this test with the expecation that it still has the same - * data after the restart. - * @tags: [requires_persistence] - */ - -load("jstests/replsets/rslib.js"); - -(function() { - "use strict"; - var testName = "json_schema_initial_sync_with_feature_compatibility"; - - // - // Create a single-node replica set. - // - var replTest = new ReplSetTest({name: testName, nodes: 1}); - - var conns = replTest.startSet(); - replTest.initiate(); - - var primary = replTest.getPrimary(); - var adminDB = primary.getDB("admin"); - var testDB = primary.getDB("test"); - - // - // Explicitly set the replica set to feature compatibility version 3.6. - // - var res; - assert.commandWorked(adminDB.runCommand({setFeatureCompatibilityVersion: "3.6"})); - res = adminDB.runCommand({getParameter: 1, featureCompatibilityVersion: 1}); - assert.commandWorked(res); - assert.eq("3.6", res.featureCompatibilityVersion); - - // - // Create and populate a collection with a $jsonSchema validator. - // - assert.commandWorked(testDB.createCollection( - "coll", {validator: {$jsonSchema: {properties: {str: {type: "string"}}}}})); - - var bulk = testDB.coll.initializeUnorderedBulkOp(); - for (var i = 0; i < 100; i++) { - bulk.insert({date: new Date(), x: i, str: "all the talk on the market"}); - } - assert.writeOK(bulk.execute()); - - // - // Downgrade the replica set to feature compatibility version 3.4. - // - var res; - assert.commandWorked(adminDB.runCommand({setFeatureCompatibilityVersion: "3.4"})); - res = adminDB.runCommand({getParameter: 1, featureCompatibilityVersion: 1}); - assert.commandWorked(res); - assert.eq("3.4", res.featureCompatibilityVersion); - - // - // Add a new member to the replica set. - // - var secondaryDBPath = MongoRunner.dataPath + testName + "_secondary"; - resetDbpath(secondaryDBPath); - var secondary = replTest.add({dbpath: secondaryDBPath}); - replTest.reInitiate(secondary); - reconnect(primary); - reconnect(secondary); - - // - // Once the new member completes its initial sync, stop it, remove it from the replica set, and - // start it back up as an individual instance. - // - replTest.waitForState(secondary, [ReplSetTest.State.PRIMARY, ReplSetTest.State.SECONDARY]); - - replTest.stopSet(undefined /* send default signal */, true /* don't clear data directory */); - - secondary = MongoRunner.runMongod({dbpath: secondaryDBPath, noCleanData: true}); - - // - // Verify that the $jsonSchema validator synced to the new member by attempting to insert a - // document that does not validate and checking that the insert fails. - // - var secondaryDB = secondary.getDB("test"); - assert.writeError(secondaryDB.coll.insert({str: 1.0}), ErrorCodes.DocumentValidationFailure); - - // - // Verify that, even though the existing $jsonSchema validator still works, it is not possible - // to create a new $jsonSchema validator because of feature compatibility 3.4. - // - assert.commandFailed(secondaryDB.runCommand({collMod: "coll", validator: {$jsonSchema: {}}}), - ErrorCodes.InvalidOptions); -}());
\ No newline at end of file diff --git a/src/mongo/base/error_codes.err b/src/mongo/base/error_codes.err index 24825fb09ba..dec401d6d28 100644 --- a/src/mongo/base/error_codes.err +++ b/src/mongo/base/error_codes.err @@ -221,7 +221,7 @@ error_code("FTDCPathAlreadySet", 220) error_code("IndexModified", 221) error_code("CloseChangeStream", 222) error_code("IllegalOpMsgFlag", 223) -error_code("JSONSchemaNotAllowed", 224) +error_code("QueryFeatureNotAllowed", 224) error_code("TransactionTooOld", 225) error_code("AtomicityFailure", 226) error_code("CannotImplicitlyCreateCollection", 227); diff --git a/src/mongo/db/catalog/coll_mod.cpp b/src/mongo/db/catalog/coll_mod.cpp index 1a24d9a8caa..2d7e6e60538 100644 --- a/src/mongo/db/catalog/coll_mod.cpp +++ b/src/mongo/db/catalog/coll_mod.cpp @@ -175,19 +175,20 @@ StatusWith<CollModRequest> parseCollModRequest(OperationContext* opCtx, if (!serverGlobalParams.featureCompatibility.validateFeaturesAsMaster.load() || serverGlobalParams.featureCompatibility.version.load() != ServerGlobalParams::FeatureCompatibility::Version::k34) { - // Allow $jsonSchema only if the feature compatibility version is newer than 3.4. - // Note that we don't enforce this restriction on the secondary or on backup - // instances, as indicated by !validateFeaturesAsMaster. + // Allow $jsonSchema and $expr only if the feature compatibility version is newer + // than 3.4. Note that we don't enforce this restriction on secondary instances, as + // indicated by !validateFeaturesAsMaster. allowedFeatures |= MatchExpressionParser::kJSONSchema; + allowedFeatures |= MatchExpressionParser::kExpr; } auto statusW = coll->parseValidator(opCtx, e.Obj(), allowedFeatures); if (!statusW.isOK()) { - if (statusW.getStatus().code() == ErrorCodes::JSONSchemaNotAllowed) { - // The default error message for disallowed $jsonSchema is not descriptive - // enough, so we rewrite it here. - return {ErrorCodes::JSONSchemaNotAllowed, + if (statusW.getStatus().code() == ErrorCodes::QueryFeatureNotAllowed) { + // The default error message for disallowed $jsonSchema and $expr is not + // descriptive enough, so we rewrite it here. + return {ErrorCodes::QueryFeatureNotAllowed, str::stream() << "The featureCompatibilityVersion must be 3.6 to add a " - "$jsonSchema validator to a collection. See " + "collection validator using 3.6 query features. See " << feature_compatibility_version::kDochubLink << "."}; } else { diff --git a/src/mongo/db/catalog/collection_impl.cpp b/src/mongo/db/catalog/collection_impl.cpp index f41d9d2a467..2262613bee8 100644 --- a/src/mongo/db/catalog/collection_impl.cpp +++ b/src/mongo/db/catalog/collection_impl.cpp @@ -159,11 +159,8 @@ CollectionImpl::CollectionImpl(Collection* _this_init, _indexCatalog(_this_init, this->getCatalogEntry()->getMaxAllowedIndexes()), _collator(parseCollation(opCtx, _ns, _details->getCollectionOptions(opCtx).collation)), _validatorDoc(_details->getCollectionOptions(opCtx).validator.getOwned()), - _validator( - uassertStatusOK(parseValidator(opCtx, - _validatorDoc, - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr))), + _validator(uassertStatusOK( + parseValidator(opCtx, _validatorDoc, MatchExpressionParser::kAllowAllSpecialFeatures))), _validationAction(uassertStatusOK( parseValidationAction(_details->getCollectionOptions(opCtx).validationAction))), _validationLevel(uassertStatusOK( @@ -898,10 +895,8 @@ Status CollectionImpl::setValidator(OperationContext* opCtx, BSONObj validatorDo // Note that, by the time we reach this, we should have already done a pre-parse that checks for // banned features, so we don't need to include that check again. - auto statusWithMatcher = parseValidator(opCtx, - validatorDoc, - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + auto statusWithMatcher = + parseValidator(opCtx, validatorDoc, MatchExpressionParser::kAllowAllSpecialFeatures); if (!statusWithMatcher.isOK()) return statusWithMatcher.getStatus(); diff --git a/src/mongo/db/catalog/database_impl.cpp b/src/mongo/db/catalog/database_impl.cpp index f4fe399674c..096b7287e2d 100644 --- a/src/mongo/db/catalog/database_impl.cpp +++ b/src/mongo/db/catalog/database_impl.cpp @@ -1021,10 +1021,11 @@ auto mongo::userCreateNSImpl(OperationContext* opCtx, if (!serverGlobalParams.featureCompatibility.validateFeaturesAsMaster.load() || serverGlobalParams.featureCompatibility.version.load() != ServerGlobalParams::FeatureCompatibility::Version::k34) { - // $jsonSchema is only permitted when the feature compatibility version is newer - // than 3.4. Note that we don't enforce this feature compatibility check when we are on - // the secondary or on a backup instance, as indicated by !validateFeaturesAsMaster. + // $jsonSchema and $expr are only permitted when the feature compatibility version is + // newer than 3.4. Note that we don't enforce this feature compatibility check when we + // are on the secondary, as indicated by !validateFeaturesAsMaster. allowedFeatures |= MatchExpressionParser::kJSONSchema; + allowedFeatures |= MatchExpressionParser::kExpr; } boost::intrusive_ptr<ExpressionContext> expCtx( new ExpressionContext(opCtx, collator.get())); @@ -1036,12 +1037,12 @@ auto mongo::userCreateNSImpl(OperationContext* opCtx, // We check the status of the parse to see if there are any banned features, but we don't // actually need the result for now. if (!statusWithMatcher.isOK()) { - if (statusWithMatcher.getStatus().code() == ErrorCodes::JSONSchemaNotAllowed) { - // The default error message for disallowed $jsonSchema is not descriptive enough, - // so we rewrite it here. - return {ErrorCodes::JSONSchemaNotAllowed, + if (statusWithMatcher.getStatus().code() == ErrorCodes::QueryFeatureNotAllowed) { + // The default error message for disallowed $jsonSchema and $expr is not descriptive + // enough, so we rewrite it here. + return {ErrorCodes::QueryFeatureNotAllowed, str::stream() << "The featureCompatibilityVersion must be 3.6 to create a " - "collection with a $jsonSchema validator. See " + "collection validator using 3.6 query featuress. See " << feature_compatibility_version::kDochubLink << "."}; } else { diff --git a/src/mongo/db/catalog/index_catalog_impl.cpp b/src/mongo/db/catalog/index_catalog_impl.cpp index 7636cd6af6a..578291a06e9 100644 --- a/src/mongo/db/catalog/index_catalog_impl.cpp +++ b/src/mongo/db/catalog/index_catalog_impl.cpp @@ -668,7 +668,10 @@ Status IndexCatalogImpl::_isSpecOk(OperationContext* opCtx, const BSONObj& spec) boost::intrusive_ptr<ExpressionContext> expCtx( new ExpressionContext(opCtx, collator.get())); StatusWithMatchExpression statusWithMatcher = - MatchExpressionParser::parse(filterElement.Obj(), std::move(expCtx)); + MatchExpressionParser::parse(filterElement.Obj(), + std::move(expCtx), + ExtensionsCallbackNoop(), + MatchExpressionParser::kBanAllSpecialFeatures); if (!statusWithMatcher.isOK()) { return statusWithMatcher.getStatus(); } diff --git a/src/mongo/db/commands/find_cmd.cpp b/src/mongo/db/commands/find_cmd.cpp index d3d9aafc096..3ef407c1d3f 100644 --- a/src/mongo/db/commands/find_cmd.cpp +++ b/src/mongo/db/commands/find_cmd.cpp @@ -163,8 +163,7 @@ public: std::move(qrStatus.getValue()), expCtx, extensionsCallback, - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + MatchExpressionParser::kAllowAllSpecialFeatures); if (!statusWithCQ.isOK()) { return statusWithCQ.getStatus(); } @@ -281,8 +280,7 @@ public: std::move(qr), expCtx, extensionsCallback, - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + MatchExpressionParser::kAllowAllSpecialFeatures); if (!statusWithCQ.isOK()) { return appendCommandStatus(result, statusWithCQ.getStatus()); } diff --git a/src/mongo/db/commands/geo_near_cmd.cpp b/src/mongo/db/commands/geo_near_cmd.cpp index 02a57e7d0e8..0df75717f42 100644 --- a/src/mongo/db/commands/geo_near_cmd.cpp +++ b/src/mongo/db/commands/geo_near_cmd.cpp @@ -221,8 +221,7 @@ public: std::move(qr), expCtx, extensionsCallback, - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + MatchExpressionParser::kAllowAllSpecialFeatures); if (!statusWithCQ.isOK()) { errmsg = "Can't parse filter / create query"; return false; diff --git a/src/mongo/db/commands/index_filter_commands.cpp b/src/mongo/db/commands/index_filter_commands.cpp index 1992f122edc..9945beb82f5 100644 --- a/src/mongo/db/commands/index_filter_commands.cpp +++ b/src/mongo/db/commands/index_filter_commands.cpp @@ -316,8 +316,7 @@ Status ClearFilters::clear(OperationContext* opCtx, std::move(qr), expCtx, extensionsCallback, - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + MatchExpressionParser::kAllowAllSpecialFeatures); invariantOK(statusWithCQ.getStatus()); std::unique_ptr<CanonicalQuery> cq = std::move(statusWithCQ.getValue()); diff --git a/src/mongo/db/commands/mr.cpp b/src/mongo/db/commands/mr.cpp index b9ecf5b4006..27f907f4ab8 100644 --- a/src/mongo/db/commands/mr.cpp +++ b/src/mongo/db/commands/mr.cpp @@ -1122,8 +1122,7 @@ void State::finalReduce(OperationContext* opCtx, CurOp* curOp, ProgressMeterHold std::move(qr), expCtx, extensionsCallback, - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + MatchExpressionParser::kAllowAllSpecialFeatures); verify(statusWithCQ.isOK()); std::unique_ptr<CanonicalQuery> cq = std::move(statusWithCQ.getValue()); @@ -1489,13 +1488,12 @@ public: const ExtensionsCallbackReal extensionsCallback(opCtx, &config.nss); const boost::intrusive_ptr<ExpressionContext> expCtx; - auto statusWithCQ = CanonicalQuery::canonicalize( - opCtx, - std::move(qr), - expCtx, - extensionsCallback, - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + auto statusWithCQ = + CanonicalQuery::canonicalize(opCtx, + std::move(qr), + expCtx, + extensionsCallback, + MatchExpressionParser::kAllowAllSpecialFeatures); if (!statusWithCQ.isOK()) { uasserted(17238, "Can't canonicalize query " + config.filter.toString()); return 0; diff --git a/src/mongo/db/commands/plan_cache_commands.cpp b/src/mongo/db/commands/plan_cache_commands.cpp index 031d59e408d..3704105d17f 100644 --- a/src/mongo/db/commands/plan_cache_commands.cpp +++ b/src/mongo/db/commands/plan_cache_commands.cpp @@ -213,8 +213,7 @@ StatusWith<unique_ptr<CanonicalQuery>> PlanCacheCommand::canonicalize(OperationC std::move(qr), expCtx, extensionsCallback, - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + MatchExpressionParser::kAllowAllSpecialFeatures); if (!statusWithCQ.isOK()) { return statusWithCQ.getStatus(); } diff --git a/src/mongo/db/dbhelpers.cpp b/src/mongo/db/dbhelpers.cpp index bb27f53b049..f3ed7df4f97 100644 --- a/src/mongo/db/dbhelpers.cpp +++ b/src/mongo/db/dbhelpers.cpp @@ -119,8 +119,7 @@ RecordId Helpers::findOne(OperationContext* opCtx, std::move(qr), expCtx, extensionsCallback, - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + MatchExpressionParser::kAllowAllSpecialFeatures); massert(17244, "Could not canonicalize " + query.toString(), statusWithCQ.isOK()); unique_ptr<CanonicalQuery> cq = std::move(statusWithCQ.getValue()); diff --git a/src/mongo/db/exec/stagedebug_cmd.cpp b/src/mongo/db/exec/stagedebug_cmd.cpp index f3d9c92cafd..6e65094764c 100644 --- a/src/mongo/db/exec/stagedebug_cmd.cpp +++ b/src/mongo/db/exec/stagedebug_cmd.cpp @@ -250,12 +250,11 @@ public: const CollatorInterface* collator = nullptr; const boost::intrusive_ptr<ExpressionContext> expCtx( new ExpressionContext(opCtx, collator)); - StatusWithMatchExpression statusWithMatcher = MatchExpressionParser::parse( - argObj, - expCtx, - ExtensionsCallbackReal(opCtx, &collection->ns()), - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + auto statusWithMatcher = + MatchExpressionParser::parse(argObj, + expCtx, + ExtensionsCallbackReal(opCtx, &collection->ns()), + MatchExpressionParser::kAllowAllSpecialFeatures); if (!statusWithMatcher.isOK()) { return NULL; } diff --git a/src/mongo/db/matcher/expression_expr.cpp b/src/mongo/db/matcher/expression_expr.cpp index 8963dd003d8..77349430e46 100644 --- a/src/mongo/db/matcher/expression_expr.cpp +++ b/src/mongo/db/matcher/expression_expr.cpp @@ -64,6 +64,10 @@ bool ExprMatchExpression::equivalent(const MatchExpression* other) const { realOther->_expression->serialize(false)); } +void ExprMatchExpression::_doSetCollator(const CollatorInterface* collator) { + _expCtx->setCollator(collator); +} + std::unique_ptr<MatchExpression> ExprMatchExpression::shallowClone() const { // TODO SERVER-31003: Replace Expression clone via serialization with Expression::clone(). diff --git a/src/mongo/db/matcher/expression_expr.h b/src/mongo/db/matcher/expression_expr.h index e97cce982dc..1a09e2a9334 100644 --- a/src/mongo/db/matcher/expression_expr.h +++ b/src/mongo/db/matcher/expression_expr.h @@ -85,6 +85,8 @@ public: private: ExpressionOptimizerFunc getOptimizer() const final; + void _doSetCollator(const CollatorInterface* collator) final; + void _doAddDependencies(DepsTracker* deps) const final { if (_expression) { _expression->addDependencies(deps); diff --git a/src/mongo/db/matcher/expression_expr_test.cpp b/src/mongo/db/matcher/expression_expr_test.cpp index 3ad0ac4340e..e89ef9a5c39 100644 --- a/src/mongo/db/matcher/expression_expr_test.cpp +++ b/src/mongo/db/matcher/expression_expr_test.cpp @@ -32,12 +32,15 @@ #include "mongo/db/matcher/expression_expr.h" #include "mongo/db/matcher/matcher.h" #include "mongo/db/pipeline/expression_context_for_test.h" +#include "mongo/db/query/collation/collator_interface_mock.h" #include "mongo/unittest/unittest.h" namespace mongo { namespace { +using unittest::assertGet; + TEST(ExprMatchExpression, ComparisonToConstantMatchesCorrectly) { boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest()); auto match = BSON("a" << 5); @@ -148,5 +151,25 @@ TEST(ExprMatchExpression, ShallowClonedExpressionIsEquivalentToOriginal) { ASSERT_TRUE(pipelineExpr.equivalent(shallowClone.get())); } +TEST(ExprMatchExpression, SetCollatorChangesCollationUsedForComparisons) { + boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest()); + auto match = BSON("a" + << "abc"); + auto notMatch = BSON("a" + << "ABC"); + + auto expression = BSON("$expr" << BSON("$eq" << BSON_ARRAY("$a" + << "abc"))); + auto matchExpression = assertGet(MatchExpressionParser::parse(expression, expCtx)); + ASSERT_TRUE(matchExpression->matchesBSON(match)); + ASSERT_FALSE(matchExpression->matchesBSON(notMatch)); + + CollatorInterfaceMock collator(CollatorInterfaceMock::MockType::kAlwaysEqual); + matchExpression->setCollator(&collator); + + ASSERT_TRUE(matchExpression->matchesBSON(match)); + ASSERT_TRUE(matchExpression->matchesBSON(notMatch)); +} + } // namespace } // namespace mongo diff --git a/src/mongo/db/matcher/expression_parser.cpp b/src/mongo/db/matcher/expression_parser.cpp index de8d2717a96..ddb6491739b 100644 --- a/src/mongo/db/matcher/expression_parser.cpp +++ b/src/mongo/db/matcher/expression_parser.cpp @@ -598,7 +598,7 @@ StatusWithMatchExpression MatchExpressionParser::_parse( root->add(maxPropsExpr.getValue().release()); } else if (mongoutils::str::equals("jsonSchema", rest)) { if ((allowedFeatures & AllowedFeatures::kJSONSchema) == 0u) { - return Status(ErrorCodes::JSONSchemaNotAllowed, + return Status(ErrorCodes::QueryFeatureNotAllowed, "$jsonSchema is not allowed in this context"); } @@ -1692,7 +1692,7 @@ StatusWithMatchExpression MatchExpressionParser::_parseExpr( AllowedFeatureSet allowedFeatures, const boost::intrusive_ptr<ExpressionContext>& expCtx) { if ((allowedFeatures & AllowedFeatures::kExpr) == 0u) { - return {Status(ErrorCodes::BadValue, "$expr is not allowed in this context")}; + return {Status(ErrorCodes::QueryFeatureNotAllowed, "$expr is not allowed in this context")}; } invariant(expCtx); diff --git a/src/mongo/db/matcher/expression_parser.h b/src/mongo/db/matcher/expression_parser.h index 44ffd4ae9a0..1adb7640ed1 100644 --- a/src/mongo/db/matcher/expression_parser.h +++ b/src/mongo/db/matcher/expression_parser.h @@ -99,7 +99,8 @@ public: static constexpr AllowedFeatureSet kBanAllSpecialFeatures = 0; static constexpr AllowedFeatureSet kAllowAllSpecialFeatures = std::numeric_limits<unsigned long long>::max(); - static constexpr AllowedFeatureSet kDefaultSpecialFeatures = AllowedFeatures::kJSONSchema; + static constexpr AllowedFeatureSet kDefaultSpecialFeatures = + AllowedFeatures::kExpr | AllowedFeatures::kJSONSchema; /** * Constant double representation of 2^63. diff --git a/src/mongo/db/matcher/expression_parser_test.cpp b/src/mongo/db/matcher/expression_parser_test.cpp index ce520064261..3c8796833b6 100644 --- a/src/mongo/db/matcher/expression_parser_test.cpp +++ b/src/mongo/db/matcher/expression_parser_test.cpp @@ -367,33 +367,27 @@ TEST(MatchExpressionParserTest, NearParsesSuccessfullyWhenAllowed) { TEST(MatchExpressionParserTest, ExprFailsToParseWhenDisallowed) { auto query = fromjson("{$expr: {$eq: ['$a', 5]}}"); boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest()); - ASSERT_NOT_OK(MatchExpressionParser::parse(query, expCtx).getStatus()); + ASSERT_NOT_OK( + MatchExpressionParser::parse( + query, expCtx, ExtensionsCallbackNoop(), MatchExpressionParser::kBanAllSpecialFeatures) + .getStatus()); } TEST(MatchExpressionParserTest, ExprParsesSuccessfullyWhenAllowed) { auto query = fromjson("{$expr: {$eq: ['$a', 5]}}"); boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest()); - ASSERT_OK( - MatchExpressionParser::parse( - query, expCtx, ExtensionsCallbackNoop(), MatchExpressionParser::AllowedFeatures::kExpr) - .getStatus()); + ASSERT_OK(MatchExpressionParser::parse(query, expCtx).getStatus()); } TEST(MatchExpressionParserTest, ExprParsesSuccessfullyWithAdditionalTopLevelPredicates) { auto query = fromjson("{x: 1, $expr: {$eq: ['$a', 5]}, y: 1}"); boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest()); - ASSERT_OK( - MatchExpressionParser::parse( - query, expCtx, ExtensionsCallbackNoop(), MatchExpressionParser::AllowedFeatures::kExpr) - .getStatus()); + ASSERT_OK(MatchExpressionParser::parse(query, expCtx).getStatus()); } TEST(MatchExpressionParserTest, ExprFailsToParseWithinTopLevelOr) { auto query = fromjson("{$or: [{x: 1}, {$expr: {$eq: ['$a', 5]}}]}"); boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest()); - ASSERT_NOT_OK( - MatchExpressionParser::parse( - query, expCtx, ExtensionsCallbackNoop(), MatchExpressionParser::AllowedFeatures::kExpr) - .getStatus()); + ASSERT_NOT_OK(MatchExpressionParser::parse(query, expCtx).getStatus()); } } diff --git a/src/mongo/db/matcher/expression_with_placeholder.cpp b/src/mongo/db/matcher/expression_with_placeholder.cpp index cd68587b057..bb878e05cfa 100644 --- a/src/mongo/db/matcher/expression_with_placeholder.cpp +++ b/src/mongo/db/matcher/expression_with_placeholder.cpp @@ -90,7 +90,8 @@ const std::regex ExpressionWithPlaceholder::placeholderRegex("^[a-z][a-zA-Z0-9]* // static StatusWith<std::unique_ptr<ExpressionWithPlaceholder>> ExpressionWithPlaceholder::parse( BSONObj rawFilter, const boost::intrusive_ptr<ExpressionContext>& expCtx) { - StatusWithMatchExpression statusWithFilter = MatchExpressionParser::parse(rawFilter, expCtx); + StatusWithMatchExpression statusWithFilter = MatchExpressionParser::parse( + rawFilter, expCtx, ExtensionsCallbackNoop(), MatchExpressionParser::kBanAllSpecialFeatures); if (!statusWithFilter.isOK()) { return statusWithFilter.getStatus(); diff --git a/src/mongo/db/matcher/expression_with_placeholder_test.cpp b/src/mongo/db/matcher/expression_with_placeholder_test.cpp index 7bd19b12d00..27a2a1d549c 100644 --- a/src/mongo/db/matcher/expression_with_placeholder_test.cpp +++ b/src/mongo/db/matcher/expression_with_placeholder_test.cpp @@ -245,6 +245,15 @@ TEST(ExpressionWithPlaceholderTest, ExprExpressionFailsToParse) { auto rawFilter = fromjson("{$expr: {$eq: ['$i', 5]}}"); auto status = ExpressionWithPlaceholder::parse(rawFilter, expCtx); ASSERT_NOT_OK(status.getStatus()); + ASSERT_EQ(status.getStatus().code(), ErrorCodes::QueryFeatureNotAllowed); +} + +TEST(ExpressionWithPlaceholderTest, JSONSchemaExpressionFailsToParse) { + boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest()); + auto rawFilter = fromjson("{$jsonSchema: {}}"); + auto status = ExpressionWithPlaceholder::parse(rawFilter, expCtx); + ASSERT_NOT_OK(status.getStatus()); + ASSERT_EQ(status.getStatus().code(), ErrorCodes::QueryFeatureNotAllowed); } TEST(ExpressionWithPlaceholderTest, EquivalentIfPlaceholderAndExpressionMatch) { diff --git a/src/mongo/db/ops/modifier_pull.cpp b/src/mongo/db/ops/modifier_pull.cpp index a030e4e5598..2d67fdf9505 100644 --- a/src/mongo/db/ops/modifier_pull.cpp +++ b/src/mongo/db/ops/modifier_pull.cpp @@ -122,8 +122,12 @@ Status ModifierPull::init(const BSONElement& modExpr, const Options& opts, bool* } // Build the matcher around the object we built above. Currently, we do not allow $pull - // operations to contain $text/$where/$geoNear/$near/$nearSphere/$expr clauses. - StatusWithMatchExpression parseResult = MatchExpressionParser::parse(_exprObj, opts.expCtx); + // operations to contain $text/$where/$geoNear/$near/$nearSphere/$expr/$jsonSchema clauses. + StatusWithMatchExpression parseResult = + MatchExpressionParser::parse(_exprObj, + opts.expCtx, + ExtensionsCallbackNoop(), + MatchExpressionParser::kBanAllSpecialFeatures); if (!parseResult.isOK()) { return parseResult.getStatus(); } diff --git a/src/mongo/db/ops/modifier_pull_test.cpp b/src/mongo/db/ops/modifier_pull_test.cpp index ee5a2025310..e21e1648e0c 100644 --- a/src/mongo/db/ops/modifier_pull_test.cpp +++ b/src/mongo/db/ops/modifier_pull_test.cpp @@ -130,7 +130,7 @@ TEST(SimpleMod, InitWithExprElemFails) { ModifierPull node; auto status = node.init(update["$pull"]["a"], ModifierInterface::Options::normal(expCtx)); ASSERT_NOT_OK(status); - ASSERT_EQUALS(ErrorCodes::BadValue, status); + ASSERT_EQUALS(ErrorCodes::QueryFeatureNotAllowed, status); } TEST(SimpleMod, InitWithExprObjectFails) { @@ -139,7 +139,16 @@ TEST(SimpleMod, InitWithExprObjectFails) { ModifierPull node; auto status = node.init(update["$pull"]["a"], ModifierInterface::Options::normal(expCtx)); ASSERT_NOT_OK(status); - ASSERT_EQUALS(ErrorCodes::BadValue, status); + ASSERT_EQUALS(ErrorCodes::QueryFeatureNotAllowed, status); +} + +TEST(SimpleMod, InitWithJSONSchemaFails) { + auto update = fromjson("{$pull: {a: {$jsonSchema: {}}}}"); + boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest()); + ModifierPull node; + auto status = node.init(update["$pull"]["a"], ModifierInterface::Options::normal(expCtx)); + ASSERT_NOT_OK(status); + ASSERT_EQUALS(ErrorCodes::QueryFeatureNotAllowed, status); } TEST(SimpleMod, PrepareOKTargetNotFound) { diff --git a/src/mongo/db/ops/parsed_delete.cpp b/src/mongo/db/ops/parsed_delete.cpp index 8e5d146562e..839381e358f 100644 --- a/src/mongo/db/ops/parsed_delete.cpp +++ b/src/mongo/db/ops/parsed_delete.cpp @@ -96,8 +96,7 @@ Status ParsedDelete::parseQueryToCQ() { std::move(qr), expCtx, extensionsCallback, - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + MatchExpressionParser::kAllowAllSpecialFeatures); if (statusWithCQ.isOK()) { _canonicalQuery = std::move(statusWithCQ.getValue()); diff --git a/src/mongo/db/ops/parsed_update.cpp b/src/mongo/db/ops/parsed_update.cpp index c3da51e352e..108475f84be 100644 --- a/src/mongo/db/ops/parsed_update.cpp +++ b/src/mongo/db/ops/parsed_update.cpp @@ -115,18 +115,28 @@ Status ParsedUpdate::parseQueryToCQ() { qr->setLimit(1); } + // $expr is not allowed in the query for an upsert, since it is not clear what the equality + // extraction behavior for $expr should be. + MatchExpressionParser::AllowedFeatureSet allowedMatcherFeatures = + MatchExpressionParser::kAllowAllSpecialFeatures; + if (_request->isUpsert()) { + allowedMatcherFeatures &= ~MatchExpressionParser::AllowedFeatures::kExpr; + } + boost::intrusive_ptr<ExpressionContext> expCtx; - auto statusWithCQ = - CanonicalQuery::canonicalize(_opCtx, - std::move(qr), - std::move(expCtx), - extensionsCallback, - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + auto statusWithCQ = CanonicalQuery::canonicalize( + _opCtx, std::move(qr), std::move(expCtx), extensionsCallback, allowedMatcherFeatures); if (statusWithCQ.isOK()) { _canonicalQuery = std::move(statusWithCQ.getValue()); } + if (statusWithCQ.getStatus().code() == ErrorCodes::QueryFeatureNotAllowed) { + // The default error message for disallowed $expr is not descriptive enough, so we rewrite + // it here. + return {ErrorCodes::QueryFeatureNotAllowed, + "$expr is not allowed in the query predicate for an upsert"}; + } + return statusWithCQ.getStatus(); } diff --git a/src/mongo/db/pipeline/document_source_match.cpp b/src/mongo/db/pipeline/document_source_match.cpp index bd212cf1925..f65aa8db073 100644 --- a/src/mongo/db/pipeline/document_source_match.cpp +++ b/src/mongo/db/pipeline/document_source_match.cpp @@ -368,12 +368,8 @@ bool DocumentSourceMatch::isTextQuery(const BSONObj& query) { void DocumentSourceMatch::joinMatchWith(intrusive_ptr<DocumentSourceMatch> other) { _predicate = BSON("$and" << BSON_ARRAY(_predicate << other->getQuery())); - StatusWithMatchExpression status = uassertStatusOK( - MatchExpressionParser::parse(_predicate, - pExpCtx, - ExtensionsCallbackNoop(), - MatchExpressionParser::AllowedFeatures::kText | - MatchExpressionParser::AllowedFeatures::kExpr)); + StatusWithMatchExpression status = uassertStatusOK(MatchExpressionParser::parse( + _predicate, pExpCtx, ExtensionsCallbackNoop(), Pipeline::kAllowedMatcherFeatures)); _expression = std::move(status.getValue()); _dependencies = DepsTracker(_dependencies.getMetadataAvailable()); getDependencies(&_dependencies); diff --git a/src/mongo/db/query/find.cpp b/src/mongo/db/query/find.cpp index e11706e99b0..26a249f7928 100644 --- a/src/mongo/db/query/find.cpp +++ b/src/mongo/db/query/find.cpp @@ -533,8 +533,7 @@ std::string runQuery(OperationContext* opCtx, q, expCtx, ExtensionsCallbackReal(opCtx, &nss), - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + MatchExpressionParser::kAllowAllSpecialFeatures); if (!statusWithCQ.isOK()) { uasserted(17287, str::stream() << "Can't canonicalize query: " diff --git a/src/mongo/db/query/get_executor.cpp b/src/mongo/db/query/get_executor.cpp index cf241557d19..e818b7429c4 100644 --- a/src/mongo/db/query/get_executor.cpp +++ b/src/mongo/db/query/get_executor.cpp @@ -1010,8 +1010,7 @@ StatusWith<unique_ptr<PlanExecutor, PlanExecutor::Deleter>> getExecutorGroup( std::move(qr), expCtx, extensionsCallback, - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + MatchExpressionParser::kAllowAllSpecialFeatures); if (!statusWithCQ.isOK()) { return statusWithCQ.getStatus(); } @@ -1249,8 +1248,7 @@ StatusWith<unique_ptr<PlanExecutor, PlanExecutor::Deleter>> getExecutorCount( collection ? static_cast<const ExtensionsCallback&>( ExtensionsCallbackReal(opCtx, &collection->ns())) : static_cast<const ExtensionsCallback&>(ExtensionsCallbackNoop()), - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + MatchExpressionParser::kAllowAllSpecialFeatures); if (!statusWithCQ.isOK()) { return statusWithCQ.getStatus(); @@ -1507,8 +1505,7 @@ StatusWith<unique_ptr<PlanExecutor, PlanExecutor::Deleter>> getExecutorDistinct( std::move(qr), expCtx, extensionsCallback, - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + MatchExpressionParser::kAllowAllSpecialFeatures); if (!statusWithCQ.isOK()) { return statusWithCQ.getStatus(); } diff --git a/src/mongo/db/query/parsed_distinct.cpp b/src/mongo/db/query/parsed_distinct.cpp index da071756726..915636dd98c 100644 --- a/src/mongo/db/query/parsed_distinct.cpp +++ b/src/mongo/db/query/parsed_distinct.cpp @@ -206,8 +206,7 @@ StatusWith<ParsedDistinct> ParsedDistinct::parse(OperationContext* opCtx, std::move(qr), expCtx, extensionsCallback, - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + MatchExpressionParser::kAllowAllSpecialFeatures); if (!cq.isOK()) { return cq.getStatus(); } diff --git a/src/mongo/db/query/parsed_projection.cpp b/src/mongo/db/query/parsed_projection.cpp index 069ff5a235e..53e5f5a523e 100644 --- a/src/mongo/db/query/parsed_projection.cpp +++ b/src/mongo/db/query/parsed_projection.cpp @@ -127,13 +127,17 @@ Status ParsedProjection::make(OperationContext* opCtx, // is ok because the parsed MatchExpression is not used after being created. We are // only parsing here in order to ensure that the elemMatch projection is valid. // - // Match expression extensions such as $text, $where, $geoNear, $near, $nearSphere, - // and $expr are not allowed in $elemMatch projections. + // Match expression extensions such as $text, $where, $geoNear, $near, and + // $nearSphere are not allowed in $elemMatch projections. $expr and $jsonSchema are + // not allowed because the matcher is not applied to the root of the document. const CollatorInterface* collator = nullptr; boost::intrusive_ptr<ExpressionContext> expCtx( new ExpressionContext(opCtx, collator)); StatusWithMatchExpression statusWithMatcher = - MatchExpressionParser::parse(elemMatchObj, std::move(expCtx)); + MatchExpressionParser::parse(elemMatchObj, + std::move(expCtx), + ExtensionsCallbackNoop(), + MatchExpressionParser::kBanAllSpecialFeatures); if (!statusWithMatcher.isOK()) { return statusWithMatcher.getStatus(); } diff --git a/src/mongo/db/sessions_collection_sharded.cpp b/src/mongo/db/sessions_collection_sharded.cpp index 38738207196..bbcf195d8dd 100644 --- a/src/mongo/db/sessions_collection_sharded.cpp +++ b/src/mongo/db/sessions_collection_sharded.cpp @@ -113,8 +113,7 @@ StatusWith<LogicalSessionIdSet> SessionsCollectionSharded::findRemovedSessions( std::move(qr.getValue()), expCtx, ExtensionsCallbackNoop(), - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + MatchExpressionParser::kBanAllSpecialFeatures); if (!cq.isOK()) { return cq.getStatus(); } diff --git a/src/mongo/db/update/pull_node.cpp b/src/mongo/db/update/pull_node.cpp index b712e2ece4e..2e29b3a9527 100644 --- a/src/mongo/db/update/pull_node.cpp +++ b/src/mongo/db/update/pull_node.cpp @@ -42,7 +42,10 @@ namespace mongo { class PullNode::ObjectMatcher final : public PullNode::ElementMatcher { public: ObjectMatcher(BSONObj matchCondition, const boost::intrusive_ptr<ExpressionContext>& expCtx) - : _matchExpr(matchCondition, expCtx) {} + : _matchExpr(matchCondition, + expCtx, + stdx::make_unique<ExtensionsCallbackNoop>(), + MatchExpressionParser::kBanAllSpecialFeatures) {} std::unique_ptr<ElementMatcher> clone() const final { return stdx::make_unique<ObjectMatcher>(*this); @@ -75,7 +78,10 @@ class PullNode::WrappedObjectMatcher final : public PullNode::ElementMatcher { public: WrappedObjectMatcher(BSONElement matchCondition, const boost::intrusive_ptr<ExpressionContext>& expCtx) - : _matchExpr(matchCondition.wrap(""), expCtx) {} + : _matchExpr(matchCondition.wrap(""), + expCtx, + stdx::make_unique<ExtensionsCallbackNoop>(), + MatchExpressionParser::kBanAllSpecialFeatures) {} std::unique_ptr<ElementMatcher> clone() const final { return stdx::make_unique<WrappedObjectMatcher>(*this); diff --git a/src/mongo/db/update/pull_node_test.cpp b/src/mongo/db/update/pull_node_test.cpp index 744abd739f4..7cfb15542cb 100644 --- a/src/mongo/db/update/pull_node_test.cpp +++ b/src/mongo/db/update/pull_node_test.cpp @@ -108,7 +108,7 @@ TEST(PullNodeTest, InitWithExprElemFails) { PullNode node; auto status = node.init(update["$pull"]["a"], expCtx); ASSERT_NOT_OK(status); - ASSERT_EQUALS(ErrorCodes::BadValue, status); + ASSERT_EQUALS(ErrorCodes::QueryFeatureNotAllowed, status); } TEST(PullNodeTest, InitWithExprObjectFails) { @@ -117,7 +117,16 @@ TEST(PullNodeTest, InitWithExprObjectFails) { PullNode node; auto status = node.init(update["$pull"]["a"], expCtx); ASSERT_NOT_OK(status); - ASSERT_EQUALS(ErrorCodes::BadValue, status); + ASSERT_EQUALS(ErrorCodes::QueryFeatureNotAllowed, status); +} + +TEST(PullNodeTest, InitWithJSONSchemaFails) { + auto update = fromjson("{$pull: {a: {$jsonSchema: {}}}}"); + boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest()); + PullNode node; + auto status = node.init(update["$pull"]["a"], expCtx); + ASSERT_NOT_OK(status); + ASSERT_EQUALS(ErrorCodes::QueryFeatureNotAllowed, status); } TEST_F(PullNodeTest, TargetNotFound) { diff --git a/src/mongo/db/update/update_driver.cpp b/src/mongo/db/update/update_driver.cpp index 7eb4f4ffd7c..5380de021ae 100644 --- a/src/mongo/db/update/update_driver.cpp +++ b/src/mongo/db/update/update_driver.cpp @@ -288,6 +288,8 @@ Status UpdateDriver::populateDocumentWithQueryFields(OperationContext* opCtx, auto qr = stdx::make_unique<QueryRequest>(NamespaceString("")); qr->setFilter(query); const boost::intrusive_ptr<ExpressionContext> expCtx; + // $expr is not allowed in the query for an upsert, since it is not clear what the equality + // extraction behavior for $expr should be. auto statusWithCQ = CanonicalQuery::canonicalize(opCtx, std::move(qr), diff --git a/src/mongo/s/chunk_manager.cpp b/src/mongo/s/chunk_manager.cpp index 9b64c68bdfd..a7bc21d39df 100644 --- a/src/mongo/s/chunk_manager.cpp +++ b/src/mongo/s/chunk_manager.cpp @@ -113,16 +113,13 @@ void ChunkManager::getShardIdsForQuery(OperationContext* opCtx, qr->setCollation(_defaultCollator->getSpec().toBSON()); } - // TODO SERVER-30731: Allow AllowedFeatures::kExpr here so that $expr can be used in queries - // against sharded collections. const boost::intrusive_ptr<ExpressionContext> expCtx; auto cq = uassertStatusOK( CanonicalQuery::canonicalize(opCtx, std::move(qr), expCtx, ExtensionsCallbackNoop(), - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr)); + MatchExpressionParser::kAllowAllSpecialFeatures)); // Query validation if (QueryPlannerCommon::hasNode(cq->root(), MatchExpression::GEO_NEAR)) { diff --git a/src/mongo/s/commands/chunk_manager_targeter.cpp b/src/mongo/s/commands/chunk_manager_targeter.cpp index 7b58421ebd3..7efc99785ec 100644 --- a/src/mongo/s/commands/chunk_manager_targeter.cpp +++ b/src/mongo/s/commands/chunk_manager_targeter.cpp @@ -331,7 +331,7 @@ Status ChunkManagerTargeter::targetUpdate( // into the query doc, and to correctly support upsert we must target a single shard. // // The rule is simple - If the update is replacement style (no '$set'), we target using the - // update. If the update is replacement style, we target using the query. + // update. If the update is not replacement style, we target using the query. // // If we have the exact shard key in either the query or replacement doc, we target using // that extracted key. @@ -413,13 +413,21 @@ Status ChunkManagerTargeter::targetUpdate( if (!collation.isEmpty()) { qr->setCollation(collation); } + // $expr is not allowed in the query for an upsert, since it is not clear what the equality + // extraction behavior for $expr should be. + auto allowedMatcherFeatures = MatchExpressionParser::kAllowAllSpecialFeatures; + if (updateDoc.getUpsert()) { + allowedMatcherFeatures &= ~MatchExpressionParser::AllowedFeatures::kExpr; + } const boost::intrusive_ptr<ExpressionContext> expCtx; - auto cq = CanonicalQuery::canonicalize(opCtx, - std::move(qr), - expCtx, - ExtensionsCallbackNoop(), - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + auto cq = CanonicalQuery::canonicalize( + opCtx, std::move(qr), expCtx, ExtensionsCallbackNoop(), allowedMatcherFeatures); + if (!cq.isOK() && cq.getStatus().code() == ErrorCodes::QueryFeatureNotAllowed) { + // The default error message for disallowed $expr is not descriptive enough, so we rewrite + // it here. + return {ErrorCodes::QueryFeatureNotAllowed, + "$expr is not allowed in the query predicate for an upsert"}; + } if (!cq.isOK()) { return Status(cq.getStatus().code(), str::stream() << "Could not parse update query " << updateDoc.getQ() @@ -497,8 +505,7 @@ Status ChunkManagerTargeter::targetDelete( std::move(qr), expCtx, ExtensionsCallbackNoop(), - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + MatchExpressionParser::kAllowAllSpecialFeatures); if (!cq.isOK()) { return Status(cq.getStatus().code(), str::stream() << "Could not parse delete query " << deleteDoc.getQ() diff --git a/src/mongo/s/commands/cluster_find_cmd.cpp b/src/mongo/s/commands/cluster_find_cmd.cpp index 8dbef24be73..51d796f21fb 100644 --- a/src/mongo/s/commands/cluster_find_cmd.cpp +++ b/src/mongo/s/commands/cluster_find_cmd.cpp @@ -169,8 +169,7 @@ public: std::move(qr.getValue()), expCtx, ExtensionsCallbackNoop(), - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + MatchExpressionParser::kAllowAllSpecialFeatures); if (!cq.isOK()) { return appendCommandStatus(result, cq.getStatus()); } diff --git a/src/mongo/s/commands/strategy.cpp b/src/mongo/s/commands/strategy.cpp index cd70651b62f..6f97edeae2c 100644 --- a/src/mongo/s/commands/strategy.cpp +++ b/src/mongo/s/commands/strategy.cpp @@ -42,6 +42,7 @@ #include "mongo/db/auth/authorization_session.h" #include "mongo/db/commands.h" #include "mongo/db/initialize_operation_session_info.h" +#include "mongo/db/lasterror.h" #include "mongo/db/logical_clock.h" #include "mongo/db/logical_session_id_helpers.h" #include "mongo/db/logical_time_validator.h" @@ -209,33 +210,21 @@ void execCommandClient(OperationContext* opCtx, return; } - try { - bool ok = false; - if (!supportsWriteConcern) { - ok = c->publicRun(opCtx, request, result); - } else { - // Change the write concern while running the command. - const auto oldWC = opCtx->getWriteConcern(); - ON_BLOCK_EXIT([&] { opCtx->setWriteConcern(oldWC); }); - opCtx->setWriteConcern(wcResult.getValue()); - - ok = c->publicRun(opCtx, request, result); - } - if (!ok) { - c->incrementCommandsFailed(); - } - Command::appendCommandStatus(result, ok); - } catch (const DBException& e) { - result.resetToEmpty(); - const int code = e.code(); - - if (code == ErrorCodes::StaleConfig) { - throw; - } + bool ok = false; + if (!supportsWriteConcern) { + ok = c->publicRun(opCtx, request, result); + } else { + // Change the write concern while running the command. + const auto oldWC = opCtx->getWriteConcern(); + ON_BLOCK_EXIT([&] { opCtx->setWriteConcern(oldWC); }); + opCtx->setWriteConcern(wcResult.getValue()); + ok = c->publicRun(opCtx, request, result); + } + if (!ok) { c->incrementCommandsFailed(); - Command::appendCommandStatus(result, e.toStatus()); } + Command::appendCommandStatus(result, ok); } void runCommand(OperationContext* opCtx, const OpMsgRequest& request, BSONObjBuilder&& builder) { @@ -295,7 +284,9 @@ void runCommand(OperationContext* opCtx, const OpMsgRequest& request, BSONObjBui continue; } catch (const DBException& e) { builder.resetToEmpty(); + command->incrementCommandsFailed(); Command::appendCommandStatus(builder, e.toStatus()); + LastError::get(opCtx->getClient()).setLastError(e.code(), e.reason()); return; } @@ -341,8 +332,7 @@ DbResponse Strategy::queryOp(OperationContext* opCtx, const NamespaceString& nss q, expCtx, ExtensionsCallbackNoop(), - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr)); + MatchExpressionParser::kAllowAllSpecialFeatures)); // If the $explain flag was set, we must run the operation on the shards as an explain command // rather than a find command. diff --git a/src/mongo/s/shard_key_pattern.cpp b/src/mongo/s/shard_key_pattern.cpp index baefac56f4f..549e753ea65 100644 --- a/src/mongo/s/shard_key_pattern.cpp +++ b/src/mongo/s/shard_key_pattern.cpp @@ -289,8 +289,7 @@ StatusWith<BSONObj> ShardKeyPattern::extractShardKeyFromQuery(OperationContext* std::move(qr), expCtx, ExtensionsCallbackNoop(), - MatchExpressionParser::kAllowAllSpecialFeatures & - ~MatchExpressionParser::AllowedFeatures::kExpr); + MatchExpressionParser::kAllowAllSpecialFeatures); if (!statusWithCQ.isOK()) { return StatusWith<BSONObj>(statusWithCQ.getStatus()); } |