SERVER-54603 Improve AggregationRequest validation unit tests

7 files changed, 315 insertions, 161 deletions

diff --git a/src/mongo/db/pipeline/aggregation_request_test.cpp b/src/mongo/db/pipeline/aggregation_request_test.cpp
index cd776dc2d40..0ad194bfc9f 100644
--- a/src/mongo/db/pipeline/aggregation_request_test.cpp
+++ b/src/mongo/db/pipeline/aggregation_request_test.cpp
@@ -310,207 +310,330 @@ TEST(AggregationRequestTest, ShouldNotSerializeBatchSizeWhenExplainSet) {
 // Error cases.
 //
 
+/**
+ * Combines 'validRequest' and 'invalidFields' into a single BSONObj. Note that if the two share
+ * a common field, the field from 'invalidFields' will be kept.
+ */
+BSONObj constructInvalidRequest(const BSONObj& validRequest, const BSONObj& invalidFields) {
+    BSONObjBuilder invalidRequestBuilder;
+
+    // An aggregate command expects the first field in the request to be 'aggregate'. As such, we
+    // pull out the aggregate field from whichever BSONObj supplied it and append it before any
+    // other fields.
+    auto validAggregateField = validRequest.getField(AggregateCommand::kCommandName);
+    auto invalidAggregateField = invalidFields.getField(AggregateCommand::kCommandName);
+    if (!invalidAggregateField.eoo()) {
+        invalidRequestBuilder.append(invalidAggregateField);
+    } else {
+        invariant(!validAggregateField.eoo());
+        invalidRequestBuilder.append(validAggregateField);
+    }
+
+    // Construct a command object containing a union of the two objects.
+    for (auto&& elem : invalidFields) {
+        auto fieldName = elem.fieldName();
+        if (!invalidRequestBuilder.hasField(fieldName)) {
+            invalidRequestBuilder.append(elem);
+        }
+    }
+
+
+    for (auto&& elem : validRequest) {
+        auto fieldName = elem.fieldName();
+        if (!invalidRequestBuilder.hasField(fieldName)) {
+            invalidRequestBuilder.append(elem);
+        }
+    }
+    return invalidRequestBuilder.obj();
+}
+
+/**
+ * Helper which verifies that 'validRequest' parses correctly, but fails to parse once
+ * 'invalidFields' are added to it.
+ */
+void aggregationRequestParseFailureHelper(
+    const NamespaceString& nss,
+    const BSONObj& validRequest,
+    const BSONObj& invalidFields,
+    ErrorCodes::Error expectedCode,
+    boost::optional<ExplainOptions::Verbosity> explainVerbosity = boost::none) {
+    // Verify that 'validRequest' parses correctly.
+    ASSERT_OK(aggregation_request_helper::parseFromBSONForTests(nss, validRequest, explainVerbosity)
+                  .getStatus());
+
+    auto invalidRequest = constructInvalidRequest(validRequest, invalidFields);
+
+    // Verify that the constructed invalid request fails to parse.
+    auto status =
+        aggregation_request_helper::parseFromBSONForTests(nss, invalidRequest, explainVerbosity)
+            .getStatus();
+    ASSERT_NOT_OK(status);
+    ASSERT_EQ(status.code(), expectedCode);
+}
+
+/**
+ * Verifies that 'validRequest' parses correctly, but throws 'expectedCode' once 'invalidFields'
+ * are added to it.
+ */
+void parseNSHelper(const std::string& dbName,
+                   const BSONObj& validRequest,
+                   const BSONObj& invalidFields,
+                   ErrorCodes::Error expectedCode) {
+    // Verify that 'validRequest' parses correctly.
+    auto shouldNotThrow = aggregation_request_helper::parseNs(dbName, validRequest);
+
+    auto invalidRequest = constructInvalidRequest(validRequest, invalidFields);
+
+    // Verify that the constructed invalid request fails to parse with 'expectedCode'.
+    ASSERT_THROWS_CODE(
+        aggregation_request_helper::parseNs("a", invalidRequest), AssertionException, expectedCode);
+}
+
 TEST(AggregationRequestTest, ShouldRejectNonArrayPipeline) {
     NamespaceString nss("a.collection");
-    const BSONObj inputBson =
-        fromjson("{aggregate: 'collection', pipeline: {}, cursor: {}, $db: 'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
+    const BSONObj validRequest =
+        fromjson("{aggregate: 'collection', pipeline: [], cursor: {}, $db: 'a'}");
+    const BSONObj nonArrayPipeline = fromjson("{pipeline: {}}");
+    aggregationRequestParseFailureHelper(
+        nss, validRequest, nonArrayPipeline, ErrorCodes::TypeMismatch);
 }
 
 TEST(AggregationRequestTest, ShouldRejectPipelineArrayIfAnElementIsNotAnObject) {
     NamespaceString nss("a.collection");
-    BSONObj inputBson = fromjson("{aggregate: 'collection', pipeline: [4], cursor: {}, $db: 'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
+    const BSONObj validRequest = fromjson(
+        "{aggregate: 'collection', pipeline: [{$match: {a: 'abc'}}], cursor: {}, $db: 'a'}");
+    BSONObj nonObjectPipelineElem = fromjson("{pipeline: [4]}");
+    aggregationRequestParseFailureHelper(
+        nss, validRequest, nonObjectPipelineElem, ErrorCodes::TypeMismatch);
 
-    inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [{$match: {a: 'abc'}}, 4], cursor: {}, $db: 'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
+    nonObjectPipelineElem = fromjson("{pipeline: [{$match: {a: 'abc'}}, 4]}");
+    aggregationRequestParseFailureHelper(
+        nss, validRequest, nonObjectPipelineElem, ErrorCodes::TypeMismatch);
 }
 
 TEST(AggregationRequestTest, ShouldRejectNonObjectCollation) {
     NamespaceString nss("a.collection");
-    const BSONObj inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [{$match: {a: 'abc'}}], cursor: {}, collation: 1, "
+    const BSONObj validRequest = fromjson(
+        "{aggregate: 'collection', "
+        "pipeline: [{$match: {a: 'abc'}}],"
+        "cursor: {}, "
+        "collation: {locale: 'simple'}, "
         "$db: 'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(NamespaceString("a.collection"),
-                                                                    inputBson)
-                      .getStatus());
+    const BSONObj nonObjectCollation = fromjson("{collation: 1}");
+    aggregationRequestParseFailureHelper(
+        nss, validRequest, nonObjectCollation, ErrorCodes::TypeMismatch);
 }
 
 TEST(AggregationRequestTest, ShouldRejectNonStringNonObjectHint) {
     NamespaceString nss("a.collection");
-    const BSONObj inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [{$match: {a: 'abc'}}], cursor: {}, hint: 1, $db: "
-        "'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(NamespaceString("a.collection"),
-                                                                    inputBson)
-                      .getStatus());
-}
-
-TEST(AggregationRequestTest, ShouldRejectHintAsArray) {
-    NamespaceString nss("a.collection");
-    const BSONObj inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [{$match: {a: 'abc'}}], cursor: {}, hint: [], $db: "
-        "'a'}]}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(NamespaceString("a.collection"),
-                                                                    inputBson)
-                      .getStatus());
+    const BSONObj validRequest = fromjson(
+        "{aggregate: 'collection', "
+        "pipeline: [{$match: {a: 'abc'}}],"
+        "cursor: {},"
+        "hint: {_id: 1},"
+        "$db: 'a'}");
+    const BSONObj nonObjectHint = fromjson("{hint: 1}");
+    aggregationRequestParseFailureHelper(
+        nss, validRequest, nonObjectHint, ErrorCodes::FailedToParse);
 }
 
 TEST(AggregationRequestTest, ShouldRejectExplainIfNumber) {
     NamespaceString nss("a.collection");
-    const BSONObj inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [{$match: {a: 'abc'}}], cursor: {}, explain: 1, $db: "
-        "'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
+    const BSONObj validRequest = fromjson(
+        "{aggregate: 'collection',"
+        "pipeline: [{$match: {a: 'abc'}}],"
+        "cursor: {},"
+        "explain: true, "
+        "$db: 'a'}");
+    const BSONObj numericExplain = fromjson("{explain: 1}");
+    aggregationRequestParseFailureHelper(
+        nss, validRequest, numericExplain, ErrorCodes::TypeMismatch);
 }
 
 TEST(AggregationRequestTest, ShouldRejectExplainIfObject) {
     NamespaceString nss("a.collection");
-    const BSONObj inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [{$match: {a: 'abc'}}], cursor: {}, explain: {}, $db: "
-        "'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
+    const BSONObj validRequest = fromjson(
+        "{aggregate: 'collection',"
+        "pipeline: [{$match: {a: 'abc'}}],"
+        "cursor: {},"
+        "explain: true, "
+        "$db: 'a'}");
+    const BSONObj objectExplain = fromjson("{explain: {}}");
+    aggregationRequestParseFailureHelper(
+        nss, validRequest, objectExplain, ErrorCodes::TypeMismatch);
 }
 
 TEST(AggregationRequestTest, ShouldRejectNonBoolFromMongos) {
     NamespaceString nss("a.collection");
-    const BSONObj inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [{$match: {a: 'abc'}}], cursor: {}, fromMongos: 1, "
+    const BSONObj validRequest = fromjson(
+        "{aggregate: 'collection',"
+        "pipeline: [{$match: {a: 'abc'}}],"
+        "cursor: {}, "
+        "fromMongos: true, "
         "$db: 'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
+    const BSONObj nonBoolFromMongos = fromjson("{fromMongos: 1}");
+    aggregationRequestParseFailureHelper(
+        nss, validRequest, nonBoolFromMongos, ErrorCodes::TypeMismatch);
 }
 
 TEST(AggregationRequestTest, ShouldRejectNonBoolNeedsMerge) {
     NamespaceString nss("a.collection");
-    const BSONObj inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [{$match: {a: 'abc'}}], cursor: {}, needsMerge: 1, "
-        "fromMongos: true, $db: 'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
+    const BSONObj validRequest = fromjson(
+        "{aggregate: 'collection',"
+        "pipeline: [{$match: {a: 'abc'}}], "
+        "cursor: {},"
+        "needsMerge: true, "
+        "fromMongos: true,"
+        "$db: 'a'}");
+    const BSONObj nonBoolNeedsMerge = fromjson("{needsMerge: 1}");
+    aggregationRequestParseFailureHelper(
+        nss, validRequest, nonBoolNeedsMerge, ErrorCodes::TypeMismatch);
 }
 
 TEST(AggregationRequestTest, ShouldRejectNeedsMergeIfFromMongosNotPresent) {
     NamespaceString nss("a.collection");
-    const BSONObj inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [{$match: {a: 'abc'}}], cursor: {}, needsMerge: true, "
+    const BSONObj validRequest = fromjson(
+        "{aggregate: 'collection',"
+        "pipeline: [{$match: {a: 'abc'}}],"
+        "cursor: {}, "
         "$db: 'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
+    const BSONObj needsMergeNoFromMongos = fromjson("{needsMerge: true}");
+    aggregationRequestParseFailureHelper(
+        nss, validRequest, needsMergeNoFromMongos, ErrorCodes::FailedToParse);
 }
 
-TEST(AggregationRequestTest, ShouldRejectNonBoolNeedsMerge34) {
+TEST(AggregationRequestTest, ShouldRejectNonBoolAllowDiskUse) {
     NamespaceString nss("a.collection");
-    const BSONObj inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [{$match: {a: 'abc'}}], cursor: {}, fromRouter: 1, "
+    const BSONObj validRequest = fromjson(
+        "{aggregate: 'collection',"
+        "pipeline: [{$match: {a: 'abc'}}],"
+        "cursor: {},"
+        "allowDiskUse: true, "
         "$db: 'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
+    const BSONObj nonBoolAllowDiskUse = fromjson("{allowDiskUse: 1}");
+    aggregationRequestParseFailureHelper(
+        nss, validRequest, nonBoolAllowDiskUse, ErrorCodes::TypeMismatch);
 }
 
-TEST(AggregationRequestTest, ShouldRejectNeedsMergeIfNeedsMerge34AlsoPresent) {
+TEST(AggregationRequestTest, ShouldRejectNonBoolIsMapReduceCommand) {
     NamespaceString nss("a.collection");
-    const BSONObj inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [{$match: {a: 'abc'}}], cursor: {}, needsMerge: true, "
-        "fromMongos: true, "
-        "fromRouter: true, $db: 'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
+    const BSONObj validRequest = fromjson(
+        "{aggregate: 'collection',"
+        "pipeline: [{$match: {a: 'abc'}}],"
+        "cursor: {}, "
+        "isMapReduceCommand: true,"
+        "$db: 'a'}");
+    const BSONObj nonBoolIsMapReduceCommand = fromjson("{isMapReduceCommand: 1}");
+    aggregationRequestParseFailureHelper(
+        nss, validRequest, nonBoolIsMapReduceCommand, ErrorCodes::TypeMismatch);
 }
 
-TEST(AggregationRequestTest, ShouldRejectFromMongosIfNeedsMerge34AlsoPresent) {
+TEST(AggregationRequestTest, ShouldRejectNoCursorNoExplain) {
     NamespaceString nss("a.collection");
-    const BSONObj inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [{$match: {a: 'abc'}}], cursor: {}, fromMongos: true, "
-        "fromRouter: true, $db: 'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
-}
 
-TEST(AggregationRequestTest, ShouldRejectNonBoolAllowDiskUse) {
-    NamespaceString nss("a.collection");
-    const BSONObj inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [{$match: {a: 'abc'}}], cursor: {}, allowDiskUse: 1, "
+    // An aggregate with neither cursor nor explain should fail to parse.
+    const BSONObj invalidRequest = fromjson(
+        "{aggregate: 'collection',"
+        "pipeline: [{$match: {a: 'abc'}}],"
         "$db: 'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
-}
+    auto status =
+        aggregation_request_helper::parseFromBSONForTests(nss, invalidRequest).getStatus();
+    ASSERT_NOT_OK(status);
+    ASSERT_EQ(status.code(), ErrorCodes::FailedToParse);
 
-TEST(AggregationRequestTest, ShouldRejectNonBoolIsMapReduceCommand) {
-    NamespaceString nss("a.collection");
-    const BSONObj inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [{$match: {a: 'abc'}}], cursor: {}, "
-        "isMapReduceCommand: 1, $db: 'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
-}
+    // Adding explain should cause the aggregate to parse successfully.
+    BSONObjBuilder explainRequest(invalidRequest);
+    explainRequest.append("explain", true);
+    ASSERT_OK(
+        aggregation_request_helper::parseFromBSONForTests(nss, explainRequest.done()).getStatus());
 
-TEST(AggregationRequestTest, ShouldRejectNoCursorNoExplain) {
-    NamespaceString nss("a.collection");
-    const BSONObj inputBson =
-        fromjson("{aggregate: 'collection', pipeline: [{$match: {a: 'abc'}}], $db: 'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
+    // Adding cursor should cause the aggregate to parse successfully.
+    BSONObjBuilder cursorRequest(invalidRequest);
+    cursorRequest.append("cursor", BSONObj());
+    ASSERT_OK(
+        aggregation_request_helper::parseFromBSONForTests(nss, cursorRequest.done()).getStatus());
 }
 
 TEST(AggregationRequestTest, ShouldRejectExplainTrueWithSeparateExplainArg) {
     NamespaceString nss("a.collection");
-    const BSONObj inputBson =
-        fromjson("{aggregate: 'collection', pipeline: [], explain: true, $db: 'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(
-                      nss, inputBson, ExplainOptions::Verbosity::kExecStats)
-                      .getStatus());
+    const BSONObj validRequest = fromjson(
+        "{aggregate: 'collection',"
+        "pipeline: [],"
+        "cursor: {},"
+        "$db: 'a'}");
+    const BSONObj explainTrue = fromjson("{explain: true}");
+    aggregationRequestParseFailureHelper(nss,
+                                         validRequest,
+                                         explainTrue,
+                                         ErrorCodes::FailedToParse,
+                                         ExplainOptions::Verbosity::kExecStats);
 }
 
 TEST(AggregationRequestTest, ShouldRejectExplainFalseWithSeparateExplainArg) {
     NamespaceString nss("a.collection");
-    const BSONObj inputBson =
-        fromjson("{aggregate: 'collection', pipeline: [], explain: false, $db: 'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(
-                      nss, inputBson, ExplainOptions::Verbosity::kExecStats)
-                      .getStatus());
-}
-
-TEST(AggregationRequestTest, ShouldRejectExplainExecStatsVerbosityWithReadConcernMajority) {
-    NamespaceString nss("a.collection");
-    const BSONObj inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [], readConcern: {level: 'majority'}, $db: 'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(
-                      nss, inputBson, ExplainOptions::Verbosity::kExecStats)
-                      .getStatus());
+    const BSONObj validRequest = fromjson(
+        "{aggregate: 'collection',"
+        "pipeline: [],"
+        "cursor: {},"
+        "$db: 'a'}");
+    const BSONObj explainFalse = fromjson("{explain: false}");
+    aggregationRequestParseFailureHelper(nss,
+                                         validRequest,
+                                         explainFalse,
+                                         ErrorCodes::FailedToParse,
+                                         ExplainOptions::Verbosity::kExecStats);
 }
 
 TEST(AggregationRequestTest, ShouldRejectExplainWithWriteConcernMajority) {
     NamespaceString nss("a.collection");
-    const BSONObj inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [], explain: true, writeConcern: {w: 'majority'}, "
-        "$db: 'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
+    const BSONObj validRequest =
+        fromjson("{aggregate: 'collection', pipeline: [], explain: true, $db: 'a'}");
+    const BSONObj wcMajority = fromjson("{writeConcern: {w: 'majority'}}");
+    aggregationRequestParseFailureHelper(nss, validRequest, wcMajority, ErrorCodes::FailedToParse);
 }
 
 TEST(AggregationRequestTest, ShouldRejectExplainExecStatsVerbosityWithWriteConcernMajority) {
     NamespaceString nss("a.collection");
-    const BSONObj inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [], writeConcern: {w: 'majority'}, $db: 'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(
-                      nss, inputBson, ExplainOptions::Verbosity::kExecStats)
-                      .getStatus());
+    const BSONObj validRequest =
+        fromjson("{aggregate: 'collection', pipeline: [], cursor: {}, $db: 'a'}");
+    const BSONObj wcMajority = fromjson("{writeConcern: {w: 'majority'}}");
+    aggregationRequestParseFailureHelper(nss,
+                                         validRequest,
+                                         wcMajority,
+                                         ErrorCodes::FailedToParse,
+                                         ExplainOptions::Verbosity::kExecStats);
 }
 
 TEST(AggregationRequestTest, ShouldRejectRequestReshardingResumeTokenIfNonBooleanType) {
-    NamespaceString nss("a.collection");
-    const BSONObj inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [], $_requestReshardingResumeToken: 'yes', $db: 'a', "
+    NamespaceString oplogNss("local.oplog.rs");
+    const BSONObj validRequest = fromjson(
+        "{aggregate: 'collection',"
+        "pipeline: [],"
+        "$_requestReshardingResumeToken: true,"
+        "$db: 'a', "
         "cursor: {}}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
+    const BSONObj nonBoolReshardingResumeToken =
+        fromjson("{$_requestReshardingResumeToken: 'yes'}");
+    aggregationRequestParseFailureHelper(
+        oplogNss, validRequest, nonBoolReshardingResumeToken, ErrorCodes::TypeMismatch);
 }
 
 TEST(AggregationRequestTest, ShouldRejectRequestReshardingResumeTokenIfNonOplogNss) {
-    NamespaceString nss("a.collection");
-    const BSONObj inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [], $_requestReshardingResumeToken: true, $db: 'a', "
+    NamespaceString oplogNss("local.oplog.rs");
+    const BSONObj validRequest = fromjson(
+        "{aggregate: 'collection',"
+        "pipeline: [],"
+        "$_requestReshardingResumeToken: true,"
+        "$db: 'a', "
         "cursor: {}}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
-}
+    ASSERT_OK(
+        aggregation_request_helper::parseFromBSONForTests(oplogNss, validRequest).getStatus());
 
-TEST(AggregationRequestTest, CannotParseNeedsMerge34) {
-    NamespaceString nss("a.collection");
-    const BSONObj inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [{$match: {a: 'abc'}}], cursor: {}, fromRouter: true, "
-        "$db: 'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
+    NamespaceString nonOplogNss("a.collection");
+    auto status =
+        aggregation_request_helper::parseFromBSONForTests(nonOplogNss, validRequest).getStatus();
+    ASSERT_NOT_OK(status);
+    ASSERT_EQ(status, ErrorCodes::FailedToParse);
 }
 
 TEST(AggregationRequestTest, ParseNSShouldReturnAggregateOneNSIfAggregateFieldIsOne) {
@@ -525,31 +648,27 @@ TEST(AggregationRequestTest, ParseNSShouldReturnAggregateOneNSIfAggregateFieldIs
 }
 
 TEST(AggregationRequestTest, ParseNSShouldRejectNumericNSIfAggregateFieldIsNotOne) {
-    const BSONObj inputBSON = fromjson("{aggregate: 2, pipeline: [], $db: 'a'}");
-    ASSERT_THROWS_CODE(aggregation_request_helper::parseNs("a", inputBSON),
-                       AssertionException,
-                       ErrorCodes::FailedToParse);
+    const BSONObj validRequest = fromjson("{aggregate: 1, pipeline: [], $db: 'a'}");
+    BSONObj nonOneAggregate = fromjson("{aggregate: 2}");
+    parseNSHelper("a", validRequest, nonOneAggregate, ErrorCodes::FailedToParse);
 }
 
 TEST(AggregationRequestTest, ParseNSShouldRejectNonStringNonNumericNS) {
-    const BSONObj inputBSON = fromjson("{aggregate: {}, pipeline: [], $db: 'a'}");
-    ASSERT_THROWS_CODE(aggregation_request_helper::parseNs("a", inputBSON),
-                       AssertionException,
-                       ErrorCodes::TypeMismatch);
+    const BSONObj validRequest = fromjson("{aggregate: 1, pipeline: [], $db: 'a'}");
+    BSONObj nonStringNonNumericNS = fromjson("{aggregate: {}}");
+    parseNSHelper("a", validRequest, nonStringNonNumericNS, ErrorCodes::TypeMismatch);
 }
 
 TEST(AggregationRequestTest, ParseNSShouldRejectAggregateOneStringAsCollectionName) {
-    const BSONObj inputBSON = fromjson("{aggregate: '$cmd.aggregate', pipeline: [], $db: 'a'}");
-    ASSERT_THROWS_CODE(aggregation_request_helper::parseNs("a", inputBSON),
-                       AssertionException,
-                       ErrorCodes::InvalidNamespace);
+    const BSONObj validRequest = fromjson("{aggregate: 1, pipeline: [], $db: 'a'}");
+    BSONObj oneStringAsCollectionName = fromjson("{aggregate: '$cmd.aggregate'}");
+    parseNSHelper("a", validRequest, oneStringAsCollectionName, ErrorCodes::InvalidNamespace);
 }
 
 TEST(AggregationRequestTest, ParseNSShouldRejectInvalidCollectionName) {
-    const BSONObj inputBSON = fromjson("{aggregate: '', pipeline: [], $db: 'a'}");
-    ASSERT_THROWS_CODE(aggregation_request_helper::parseNs("a", inputBSON),
-                       AssertionException,
-                       ErrorCodes::InvalidNamespace);
+    const BSONObj validRequest = fromjson("{aggregate: 1, pipeline: [], $db: 'a'}");
+    BSONObj invalidCollectionName = fromjson("{aggregate: ''}");
+    parseNSHelper("a", validRequest, invalidCollectionName, ErrorCodes::InvalidNamespace);
 }
 
 TEST(AggregationRequestTest, ParseFromBSONOverloadsShouldProduceIdenticalRequests) {
@@ -569,40 +688,73 @@ TEST(AggregationRequestTest, ParseFromBSONOverloadsShouldProduceIdenticalRequest
 
 TEST(AggregationRequestTest, ShouldRejectExchangeNotObject) {
     NamespaceString nss("a.collection");
-    const BSONObj inputBson =
-        fromjson("{aggregate: 'collection', pipeline: [], exchange: '42', $db: 'a', cursor: {}}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
+    const BSONObj validRequest = fromjson(
+        "{aggregate: 'collection', "
+        "pipeline: [],"
+        "cursor: {},"
+        "exchange: {policy: 'roundrobin', consumers: 2},"
+        "$db: 'a'}");
+    const BSONObj nonObjectExchange = fromjson("{exchange: '42'}");
+    aggregationRequestParseFailureHelper(
+        nss, validRequest, nonObjectExchange, ErrorCodes::TypeMismatch);
 }
 
 TEST(AggregationRequestTest, ShouldRejectExchangeInvalidSpec) {
     NamespaceString nss("a.collection");
-    const BSONObj inputBson =
-        fromjson("{aggregate: 'collection', pipeline: [], exchange: {}, $db: 'a', cursor: {}}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
+    const BSONObj validRequest = fromjson(
+        "{aggregate: 'collection',"
+        "pipeline: [],"
+        "cursor: {},"
+        "exchange: {policy: 'roundrobin', consumers: 2}, "
+        "$db: 'a'}");
+    const BSONObj invalidExchangeSpec = fromjson("{exchange: {}}");
+    auto missingFieldErrorCode = ErrorCodes::duplicateCodeForTest(40414);
+    aggregationRequestParseFailureHelper(
+        nss, validRequest, invalidExchangeSpec, missingFieldErrorCode);
 }
 
 TEST(AggregationRequestTest, ShouldRejectInvalidWriteConcern) {
     NamespaceString nss("a.collection");
-    const BSONObj inputBson = fromjson(
-        "{aggregate: 'collection', pipeline: [{$match: {a: 'abc'}}], cursor: {}, writeConcern: "
-        "'invalid', $db: 'a'}");
-    ASSERT_NOT_OK(aggregation_request_helper::parseFromBSONForTests(nss, inputBson).getStatus());
+    const BSONObj validRequest = fromjson(
+        "{aggregate: 'collection',"
+        "pipeline: [{$match: {a: 'abc'}}],"
+        "cursor: {},"
+        "writeConcern: {w: 'majority'}, "
+        "$db: 'a'}");
+    const BSONObj invalidWC = fromjson("{writeConcern: 'invalid'}");
+    aggregationRequestParseFailureHelper(nss, validRequest, invalidWC, ErrorCodes::TypeMismatch);
 }
 
 TEST(AggregationRequestTest, ShouldRejectInvalidCollectionUUID) {
     NamespaceString nss("a.collection");
-    const BSONObj inputBSON = fromjson(
-        "{aggregate: 'collection', pipeline: [{$match: {}}], collectionUUID: 2, $db: 'a', cursor: "
-        "{}}");
-    ASSERT_EQUALS(
-        aggregation_request_helper::parseFromBSONForTests(nss, inputBSON).getStatus().code(),
-        ErrorCodes::TypeMismatch);
+    auto uuid = UUID::gen();
+    BSONObjBuilder validRequestBuilder(
+        fromjson("{aggregate: 'collection', cursor: {}, pipeline: [{$match: {}}], $db: 'a'}"));
+    uuid.appendToBuilder(&validRequestBuilder, AggregateCommand::kCollectionUUIDFieldName);
+    const BSONObj validRequest = validRequestBuilder.done();
+    const BSONObj invalidCollectionUUID = fromjson("{collectionUUID: 2}");
+    aggregationRequestParseFailureHelper(
+        nss, validRequest, invalidCollectionUUID, ErrorCodes::TypeMismatch);
 }
 
 //
 // Ignore fields parsed elsewhere.
 //
 
+TEST(AggregationRequestTest, ShouldRejectUnknownField) {
+    NamespaceString nss("a.collection");
+    const BSONObj validRequest = fromjson(
+        "{aggregate: 'collection', "
+        "pipeline: [],"
+        "cursor: {},"
+        "$db: 'a'}");
+    const BSONObj nonObjectExchange =
+        fromjson("{thisIsNotARealField: 'this is not a real option'}");
+    auto unknownFieldErrorCode = ErrorCodes::duplicateCodeForTest(40415);
+    aggregationRequestParseFailureHelper(
+        nss, validRequest, nonObjectExchange, unknownFieldErrorCode);
+}
+
 TEST(AggregationRequestTest, ShouldIgnoreQueryOptions) {
     NamespaceString nss("a.collection");
     const BSONObj inputBson = fromjson(
diff --git a/src/mongo/db/query/hint_parser_test.cpp b/src/mongo/db/query/hint_parser_test.cpp
index 8441eb913f1..2760a2226e8 100644
--- a/src/mongo/db/query/hint_parser_test.cpp
+++ b/src/mongo/db/query/hint_parser_test.cpp
@@ -58,6 +58,13 @@ TEST(CommandParsers, BadHintType) {
         parseHint(hint.firstElement()), AssertionException, ErrorCodes::FailedToParse);
 }
 
+TEST(AggregationRequestTest, ShouldRejectHintAsArray) {
+    BSONObj arrayHint = BSON("hint" << BSON_ARRAY("invalid"
+                                                  << "hint"));
+    ASSERT_THROWS_CODE(
+        parseHint(arrayHint.firstElement()), AssertionException, ErrorCodes::FailedToParse);
+}
+
 TEST(CommandParsers, SerializeNonEmptyHint) {
     auto hint = BSON("x" << 1);
     BSONObjBuilder bob;
diff --git a/src/mongo/db/s/sharding_mongod_test_fixture.cpp b/src/mongo/db/s/sharding_mongod_test_fixture.cpp
index 11529d01e58..e65baf54fb6 100644
--- a/src/mongo/db/s/sharding_mongod_test_fixture.cpp
+++ b/src/mongo/db/s/sharding_mongod_test_fixture.cpp
@@ -43,7 +43,6 @@
 #include "mongo/db/namespace_string.h"
 #include "mongo/db/op_observer_registry.h"
 #include "mongo/db/query/cursor_response.h"
-#include "mongo/db/query/query_request_helper.h"
 #include "mongo/db/repl/drop_pending_collection_reaper.h"
 #include "mongo/db/repl/oplog.h"
 #include "mongo/db/repl/read_concern_args.h"
diff --git a/src/mongo/db/transaction_history_iterator.cpp b/src/mongo/db/transaction_history_iterator.cpp
index 70f38a7ed01..02ade00bf10 100644
--- a/src/mongo/db/transaction_history_iterator.cpp
+++ b/src/mongo/db/transaction_history_iterator.cpp
@@ -35,7 +35,6 @@
 #include "mongo/db/namespace_string.h"
 #include "mongo/db/operation_context.h"
 #include "mongo/db/query/get_executor.h"
-#include "mongo/db/query/query_request_helper.h"
 #include "mongo/db/repl/local_oplog_info.h"
 #include "mongo/db/repl/oplog_entry.h"
 #include "mongo/db/transaction_history_iterator.h"
diff --git a/src/mongo/s/query/async_results_merger_test.cpp b/src/mongo/s/query/async_results_merger_test.cpp
index a4bca4cafeb..2077be2f4b1 100644
--- a/src/mongo/s/query/async_results_merger_test.cpp
+++ b/src/mongo/s/query/async_results_merger_test.cpp
@@ -38,7 +38,6 @@
 #include "mongo/db/pipeline/resume_token.h"
 #include "mongo/db/query/cursor_response.h"
 #include "mongo/db/query/getmore_command_gen.h"
-#include "mongo/db/query/query_request_helper.h"
 #include "mongo/executor/task_executor.h"
 #include "mongo/s/catalog/type_shard.h"
 #include "mongo/s/client/shard_registry.h"
diff --git a/src/mongo/s/query/cluster_aggregate.h b/src/mongo/s/query/cluster_aggregate.h
index b4239e47891..5a071c80d32 100644
--- a/src/mongo/s/query/cluster_aggregate.h
+++ b/src/mongo/s/query/cluster_aggregate.h
@@ -33,7 +33,6 @@
 #include "mongo/bson/bsonobj.h"
 #include "mongo/db/namespace_string.h"
 #include "mongo/db/pipeline/aggregate_command_gen.h"
-#include "mongo/db/pipeline/aggregation_request_helper.h"
 #include "mongo/db/pipeline/document_source.h"
 #include "mongo/db/pipeline/lite_parsed_pipeline.h"
 #include "mongo/s/query/cluster_client_cursor_params.h"
diff --git a/src/mongo/s/request_types/set_shard_version_request.cpp b/src/mongo/s/request_types/set_shard_version_request.cpp
index 808f480a725..2dfdb6a0574 100644
--- a/src/mongo/s/request_types/set_shard_version_request.cpp
+++ b/src/mongo/s/request_types/set_shard_version_request.cpp
@@ -35,7 +35,6 @@
 #include "mongo/bson/bsonobj.h"
 #include "mongo/bson/bsonobjbuilder.h"
 #include "mongo/bson/util/bson_extract.h"
-#include "mongo/db/query/query_request_helper.h"
 #include "mongo/util/assert_util.h"
 #include "mongo/util/str.h"