SERVER-45253 Throw errors on invalid options for sharded cluster count command

3 files changed, 95 insertions, 159 deletions

diff --git a/buildscripts/resmokeconfig/suites/sharding_last_stable_mongos_and_mixed_shards.yml b/buildscripts/resmokeconfig/suites/sharding_last_stable_mongos_and_mixed_shards.yml
index a514dc172d5..b1b648d8f36 100644
--- a/buildscripts/resmokeconfig/suites/sharding_last_stable_mongos_and_mixed_shards.yml
+++ b/buildscripts/resmokeconfig/suites/sharding_last_stable_mongos_and_mixed_shards.yml
@@ -22,6 +22,7 @@ selector:
   - jstests/sharding/out_fails_to_replace_sharded_collection.js
   - jstests/sharding/merge_from_stale_mongos.js
   - jstests/sharding/migration_coordinator_basic.js
+  - jstests/sharding/count1.js
   # Enable when SERVER-44733 is backported
   - jstests/sharding/change_streams_update_lookup_shard_metadata_missing.js
   # Enable when SERVER-43310 is backported
diff --git a/jstests/sharding/count1.js b/jstests/sharding/count1.js
index b712191e6ed..ce9ce017692 100644
--- a/jstests/sharding/count1.js
+++ b/jstests/sharding/count1.js
@@ -1,42 +1,26 @@
 (function() {
 'use strict';
 
-var s = new ShardingTest({shards: 2});
-var db = s.getDB("test");
+let s = new ShardingTest({shards: 2});
+let db = s.getDB("test");
 
 // ************** Test Set #1 *************
-// Basic counts on "bar" collections, not yet sharded
+// Basic counts on "bar" collections, not yet sharded.
+assert.commandWorked(db.bar.insert([{n: 1}, {n: 2}, {n: 3}]));
 
-db.bar.save({n: 1});
-db.bar.save({n: 2});
-db.bar.save({n: 3});
-
-assert.eq(3, db.bar.find().count(), "bar 1");
-assert.eq(1, db.bar.find({n: 1}).count(), "bar 2");
+assert.eq(3, db.bar.find().count());
+assert.eq(1, db.bar.find({n: 1}).count());
 
 //************** Test Set #2 *************
 // Basic counts on sharded  "foo" collection.
-// 1. Create foo collection, insert 6 docs
-// 2. Divide into three chunks
-// 3. Test counts before chunk migrations
-// 4. Manually move chunks.  Now each shard should have 3 docs.
-// 5. i. Test basic counts on foo
-//    ii. Test counts with limit
-//    iii. Test counts with skip
-//    iv. Test counts with skip + limit
-//    v. Test counts with skip + limit + sorting
-// 6. Insert 10 more docs. Further limit/skip testing with a find query
-// 7. test invalid queries/values
-
-// part 1
+
+// 1. Create foo collection, insert 6 docs.
 s.adminCommand({enablesharding: "test"});
 s.ensurePrimaryShard('test', s.shard1.shardName);
 s.adminCommand({shardcollection: "test.foo", key: {name: 1}});
 
-var primary = s.getPrimaryShard("test").getDB("test");
-var secondary = s.getOther(primary).getDB("test");
-
-assert.eq(1, s.config.chunks.count({"ns": "test.foo"}), "sanity check A");
+let primary = s.getPrimaryShard("test").getDB("test");
+let secondary = s.getOther(primary).getDB("test");
 
 assert.commandWorked(db.foo.insert({_id: 1, name: "eliot"}));
 assert.commandWorked(db.foo.insert({_id: 2, name: "sara"}));
@@ -45,22 +29,18 @@ assert.commandWorked(db.foo.insert({_id: 4, name: "joe"}));
 assert.commandWorked(db.foo.insert({_id: 5, name: "mark"}));
 assert.commandWorked(db.foo.insert({_id: 6, name: "allan"}));
 
-assert.eq(6, db.foo.find().count(), "basic count");
+assert.eq(6, db.foo.find().count());
 
-// part 2
+// 2. Divide into three chunks.
 assert.commandWorked(s.s0.adminCommand({split: "test.foo", middle: {name: "allan"}}));
 assert.commandWorked(s.s0.adminCommand({split: "test.foo", middle: {name: "sara"}}));
 assert.commandWorked(s.s0.adminCommand({split: "test.foo", middle: {name: "eliot"}}));
 
-// MINKEY->allan,bob->eliot,joe,mark->sara,MAXKEY
+// 3. Test counts before chunk migrations.
+assert.eq(6, db.foo.find().count());
+assert.eq(6, db.foo.find().sort({name: 1}).count());
 
-s.printChunks();
-
-// part 3
-assert.eq(6, db.foo.find().count(), "basic count after split ");
-assert.eq(6, db.foo.find().sort({name: 1}).count(), "basic count after split sorted ");
-
-// part 4
+// 4. Manually move chunks.  Now each shard should have 3 docs.
 assert.commandWorked(s.s0.adminCommand({
     moveChunk: "test.foo",
     find: {name: "eliot"},
@@ -68,28 +48,27 @@ assert.commandWorked(s.s0.adminCommand({
     _waitForDelete: true
 }));
 
-assert.eq(3, primary.foo.find().toArray().length, "primary count");
-assert.eq(3, secondary.foo.find().toArray().length, "secondary count");
-assert.eq(3, primary.foo.find().sort({name: 1}).toArray().length, "primary count sorted");
-assert.eq(3, secondary.foo.find().sort({name: 1}).toArray().length, "secondary count sorted");
+assert.eq(3, primary.foo.find().toArray().length);
+assert.eq(3, secondary.foo.find().toArray().length);
+assert.eq(3, primary.foo.find().sort({name: 1}).toArray().length);
+assert.eq(3, secondary.foo.find().sort({name: 1}).toArray().length);
 
-// part 5
-// Some redundant tests, but better safe than sorry. These are fast tests, anyway.
+// 5. Some redundant tests, but better safe than sorry. These are fast tests, anyway.
 
-// i.
-assert.eq(6, db.foo.find().count(), "total count after move");
-assert.eq(6, db.foo.find().toArray().length, "total count after move");
-assert.eq(6, db.foo.find().sort({name: 1}).toArray().length, "total count() sorted");
-assert.eq(6, db.foo.find().sort({name: 1}).count(), "total count with count() after move");
+// i. Test basic counts on foo.
+assert.eq(6, db.foo.find().count());
+assert.eq(6, db.foo.find().toArray().length);
+assert.eq(6, db.foo.find().sort({name: 1}).toArray().length);
+assert.eq(6, db.foo.find().sort({name: 1}).count());
 
-// ii.
+// ii. Test counts with limit.
 assert.eq(2, db.foo.find().limit(2).count(true));
 assert.eq(2, db.foo.find().limit(-2).count(true));
 assert.eq(6, db.foo.find().limit(100).count(true));
 assert.eq(6, db.foo.find().limit(-100).count(true));
 assert.eq(6, db.foo.find().limit(0).count(true));
 
-// iii.
+// iii. Test counts with skip.
 assert.eq(6, db.foo.find().skip(0).count(true));
 assert.eq(5, db.foo.find().skip(1).count(true));
 assert.eq(4, db.foo.find().skip(2).count(true));
@@ -99,7 +78,7 @@ assert.eq(1, db.foo.find().skip(5).count(true));
 assert.eq(0, db.foo.find().skip(6).count(true));
 assert.eq(0, db.foo.find().skip(7).count(true));
 
-// iv.
+// iv. Test counts with skip + limit.
 assert.eq(2, db.foo.find().limit(2).skip(1).count(true));
 assert.eq(2, db.foo.find().limit(-2).skip(1).count(true));
 assert.eq(5, db.foo.find().limit(100).skip(1).count(true));
@@ -112,69 +91,72 @@ assert.eq(0, db.foo.find().limit(100).skip(10).count(true));
 assert.eq(0, db.foo.find().limit(-100).skip(10).count(true));
 assert.eq(0, db.foo.find().limit(0).skip(10).count(true));
 
-assert.eq(2, db.foo.find().limit(2).itcount(), "LS1");
-assert.eq(2, db.foo.find().skip(2).limit(2).itcount(), "LS2");
-assert.eq(1, db.foo.find().skip(5).limit(2).itcount(), "LS3");
-assert.eq(6, db.foo.find().limit(2).count(), "LSC1");
-assert.eq(2, db.foo.find().limit(2).size(), "LSC2");
-assert.eq(2, db.foo.find().skip(2).limit(2).size(), "LSC3");
-assert.eq(1, db.foo.find().skip(5).limit(2).size(), "LSC4");
-assert.eq(4, db.foo.find().skip(1).limit(4).size(), "LSC5");
-assert.eq(5, db.foo.find().skip(1).limit(6).size(), "LSC6");
+assert.eq(2, db.foo.find().limit(2).itcount());
+assert.eq(2, db.foo.find().skip(2).limit(2).itcount());
+assert.eq(1, db.foo.find().skip(5).limit(2).itcount());
+assert.eq(6, db.foo.find().limit(2).count());
+assert.eq(2, db.foo.find().limit(2).size());
+assert.eq(2, db.foo.find().skip(2).limit(2).size());
+assert.eq(1, db.foo.find().skip(5).limit(2).size());
+assert.eq(4, db.foo.find().skip(1).limit(4).size());
+assert.eq(5, db.foo.find().skip(1).limit(6).size());
 
 // SERVER-3567 older negative limit tests
-assert.eq(2, db.foo.find().limit(2).itcount(), "N1");
-assert.eq(2, db.foo.find().limit(-2).itcount(), "N2");
-assert.eq(2, db.foo.find().skip(4).limit(2).itcount(), "N3");
-assert.eq(2, db.foo.find().skip(4).limit(-2).itcount(), "N4");
+assert.eq(2, db.foo.find().limit(2).itcount());
+assert.eq(2, db.foo.find().limit(-2).itcount());
+assert.eq(2, db.foo.find().skip(4).limit(2).itcount());
+assert.eq(2, db.foo.find().skip(4).limit(-2).itcount());
 
-// v.
+// v. Test counts with skip + limit + sorting.
 function nameString(c) {
-    var s = "";
+    let s = "";
     while (c.hasNext()) {
-        var o = c.next();
+        let o = c.next();
         if (s.length > 0)
             s += ",";
         s += o.name;
     }
     return s;
 }
-assert.eq("allan,bob,eliot,joe,mark,sara", nameString(db.foo.find().sort({name: 1})), "sort 1");
-assert.eq("sara,mark,joe,eliot,bob,allan", nameString(db.foo.find().sort({name: -1})), "sort 2");
+assert.eq("allan,bob,eliot,joe,mark,sara", nameString(db.foo.find().sort({name: 1})));
+assert.eq("sara,mark,joe,eliot,bob,allan", nameString(db.foo.find().sort({name: -1})));
 
-assert.eq("allan,bob", nameString(db.foo.find().sort({name: 1}).limit(2)), "LSD1");
-assert.eq("bob,eliot", nameString(db.foo.find().sort({name: 1}).skip(1).limit(2)), "LSD2");
-assert.eq("joe,mark", nameString(db.foo.find().sort({name: 1}).skip(3).limit(2)), "LSD3");
+assert.eq("allan,bob", nameString(db.foo.find().sort({name: 1}).limit(2)));
+assert.eq("bob,eliot", nameString(db.foo.find().sort({name: 1}).skip(1).limit(2)));
+assert.eq("joe,mark", nameString(db.foo.find().sort({name: 1}).skip(3).limit(2)));
 
-assert.eq("eliot,sara", nameString(db.foo.find().sort({_id: 1}).limit(2)), "LSE1");
-assert.eq("sara,bob", nameString(db.foo.find().sort({_id: 1}).skip(1).limit(2)), "LSE2");
-assert.eq("joe,mark", nameString(db.foo.find().sort({_id: 1}).skip(3).limit(2)), "LSE3");
+assert.eq("eliot,sara", nameString(db.foo.find().sort({_id: 1}).limit(2)));
+assert.eq("sara,bob", nameString(db.foo.find().sort({_id: 1}).skip(1).limit(2)));
+assert.eq("joe,mark", nameString(db.foo.find().sort({_id: 1}).skip(3).limit(2)));
 
-// part 6
-for (var i = 0; i < 10; i++) {
+// 6. Insert 10 more docs. Further limit/skip testing with a find query.
+for (let i = 0; i < 10; i++) {
     assert.commandWorked(db.foo.insert({_id: 7 + i, name: "zzz" + i}));
 }
 
-assert.eq(10, db.foo.find({name: {$gt: "z"}}).itcount(), "LSF1");
-assert.eq(10, db.foo.find({name: {$gt: "z"}}).sort({_id: 1}).itcount(), "LSF2");
-assert.eq(5, db.foo.find({name: {$gt: "z"}}).sort({_id: 1}).skip(5).itcount(), "LSF3");
-assert.eq(3, db.foo.find({name: {$gt: "z"}}).sort({_id: 1}).skip(5).limit(3).itcount(), "LSF4");
-
-// part 7
-// Make sure count command returns error for invalid queries
-var badCmdResult = db.runCommand({count: 'foo', query: {$c: {$abc: 3}}});
-assert(!badCmdResult.ok, "invalid query syntax didn't return error");
-assert(badCmdResult.errmsg.length > 0, "no error msg for invalid query");
-
-// Negative skip values should return error
-var negSkipResult = db.runCommand({count: 'foo', skip: -2});
-assert(!negSkipResult.ok, "negative skip value shouldn't work");
-assert(negSkipResult.errmsg.length > 0, "no error msg for negative skip");
-
-// Negative skip values with positive limit should return error
-var negSkipLimitResult = db.runCommand({count: 'foo', skip: -2, limit: 1});
-assert(!negSkipLimitResult.ok, "negative skip value with limit shouldn't work");
-assert(negSkipLimitResult.errmsg.length > 0, "no error msg for negative skip");
+assert.eq(10, db.foo.find({name: {$gt: "z"}}).itcount());
+assert.eq(10, db.foo.find({name: {$gt: "z"}}).sort({_id: 1}).itcount());
+assert.eq(5, db.foo.find({name: {$gt: "z"}}).sort({_id: 1}).skip(5).itcount());
+assert.eq(3, db.foo.find({name: {$gt: "z"}}).sort({_id: 1}).skip(5).limit(3).itcount());
+
+// 7. Test invalid queries/values.
+
+// i. Make sure count command returns error for invalid queries.
+assert.commandFailedWithCode(db.runCommand({count: 'foo', query: {$c: {$abc: 3}}}),
+                             ErrorCodes.BadValue);
+
+// ii. Negative skip values should return error.
+assert.commandFailedWithCode(db.runCommand({count: 'foo', skip: -2}), ErrorCodes.FailedToParse);
+
+// iii. Negative skip values with positive limit should return error.
+assert.commandFailedWithCode(db.runCommand({count: 'foo', skip: -2, limit: 1}),
+                             ErrorCodes.FailedToParse);
+
+// iv. Unknown options should return error.
+assert.commandFailedWithCode(db.runCommand({count: 'foo', random: true}), 40415);
+
+// v. Unknown options should return error for explain.
+assert.commandFailedWithCode(db.runCommand({explain: {count: 'foo', random: true}}), 40415);
 
 s.stop();
 })();
diff --git a/src/mongo/s/commands/cluster_count_cmd.cpp b/src/mongo/s/commands/cluster_count_cmd.cpp
index 1496a42c801..7bf8f82751b 100644
--- a/src/mongo/s/commands/cluster_count_cmd.cpp
+++ b/src/mongo/s/commands/cluster_count_cmd.cpp
@@ -89,86 +89,39 @@ public:
                 str::stream() << "Invalid namespace specified '" << nss.ns() << "'",
                 nss.isValid());
 
-        long long skip = 0;
-
-        if (cmdObj["skip"].isNumber()) {
-            skip = cmdObj["skip"].numberLong();
-            if (skip < 0) {
-                errmsg = "skip value is negative in count query";
-                return false;
-            }
-        } else if (cmdObj["skip"].ok()) {
-            errmsg = "skip value is not a valid number";
-            return false;
-        }
-
-        BSONObjBuilder countCmdBuilder;
-        countCmdBuilder.append("count", nss.coll());
-
-        BSONObj filter;
-        if (cmdObj["query"].isABSONObj()) {
-            countCmdBuilder.append("query", cmdObj["query"].Obj());
-            filter = cmdObj["query"].Obj();
-        }
-
-        BSONObj collation;
-        BSONElement collationElement;
-        auto status =
-            bsonExtractTypedField(cmdObj, "collation", BSONType::Object, &collationElement);
-        if (status.isOK()) {
-            collation = collationElement.Obj();
-        } else if (status != ErrorCodes::NoSuchKey) {
-            uassertStatusOK(status);
-        }
-
-        if (cmdObj["limit"].isNumber()) {
-            long long limit = cmdObj["limit"].numberLong();
+        std::vector<AsyncRequestsSender::Response> shardResponses;
+        try {
+            auto countRequest = CountCommand::parse(IDLParserErrorContext("count"), cmdObj);
 
             // We only need to factor in the skip value when sending to the shards if we
             // have a value for limit, otherwise, we apply it only once we have collected all
             // counts.
-            if (limit != 0 && cmdObj["skip"].isNumber()) {
-                if (limit > 0)
-                    limit += skip;
-                else
-                    limit -= skip;
-            }
-
-            countCmdBuilder.append("limit", limit);
-        }
-
-        const std::initializer_list<StringData> passthroughFields = {
-            "$queryOptions",
-            "collation",
-            "hint",
-            "readConcern",
-            QueryRequest::cmdOptionMaxTimeMS,
-        };
-        for (auto name : passthroughFields) {
-            if (auto field = cmdObj[name]) {
-                countCmdBuilder.append(field);
+            if (countRequest.getLimit() && countRequest.getSkip()) {
+                const auto limit = countRequest.getLimit().get();
+                if (limit != 0) {
+                    countRequest.setLimit(limit + countRequest.getSkip().get());
+                }
             }
-        }
-
-        auto countCmdObj = countCmdBuilder.done();
-
-        std::vector<AsyncRequestsSender::Response> shardResponses;
-        try {
+            countRequest.setSkip(boost::none);
             const auto routingInfo = uassertStatusOK(
                 Grid::get(opCtx)->catalogCache()->getCollectionRoutingInfo(opCtx, nss));
+            const auto collation = countRequest.getCollation().get_value_or(BSONObj());
             shardResponses = scatterGatherVersionedTargetByRoutingTable(
                 opCtx,
                 nss.db(),
                 nss,
                 routingInfo,
-                applyReadWriteConcern(opCtx, this, countCmdObj),
+                applyReadWriteConcern(
+                    opCtx,
+                    this,
+                    countRequest.toBSON(
+                        CommandHelpers::filterCommandRequestForPassthrough(cmdObj))),
                 ReadPreferenceSetting::get(opCtx),
                 Shard::RetryPolicy::kIdempotent,
-                filter,
+                countRequest.getQuery(),
                 collation);
         } catch (const ExceptionFor<ErrorCodes::CommandOnShardedViewNotSupportedOnMongod>& ex) {
             // Rewrite the count command as an aggregation.
-
             auto countRequest = CountCommand::parse(IDLParserErrorContext("count"), cmdObj);
             auto aggCmdOnView =
                 uassertStatusOK(countCommandAsAggregationCommand(countRequest, nss));