SERVER-53127 Let AggregationRequestHelper::parseFromBSON() return an AggregateCommand

13 files changed, 86 insertions, 119 deletions

diff --git a/src/mongo/db/auth/authorization_session_impl.cpp b/src/mongo/db/auth/authorization_session_impl.cpp
index 9b54154225e..0f131648506 100644
--- a/src/mongo/db/auth/authorization_session_impl.cpp
+++ b/src/mongo/db/auth/authorization_session_impl.cpp
@@ -90,17 +90,14 @@ Status checkAuthForCreateOrModifyView(AuthorizationSession* authzSession,
         return Status::OK();
     }
 
-    auto status = aggregation_request_helper::parseFromBSON(
+    auto request = aggregation_request_helper::parseFromBSON(
         viewNs,
         BSON("aggregate" << viewOnNs.coll() << "pipeline" << viewPipeline << "cursor" << BSONObj()
                          << "$db" << viewOnNs.db()),
         boost::none,
         false);
-    if (!status.isOK())
-        return status.getStatus();
 
-    auto statusWithPrivs =
-        authzSession->getPrivilegesForAggregate(viewOnNs, status.getValue(), isMongos);
+    auto statusWithPrivs = authzSession->getPrivilegesForAggregate(viewOnNs, request, isMongos);
     PrivilegeVector privileges = uassertStatusOK(statusWithPrivs);
     if (!authzSession->isAuthorizedForPrivileges(privileges)) {
         return Status(ErrorCodes::Unauthorized, "unauthorized");
diff --git a/src/mongo/db/commands/count_cmd.cpp b/src/mongo/db/commands/count_cmd.cpp
index 1164eb1e07c..8ca97e3ae2a 100644
--- a/src/mongo/db/commands/count_cmd.cpp
+++ b/src/mongo/db/commands/count_cmd.cpp
@@ -168,15 +168,12 @@ public:
                 viewAggCmd,
                 verbosity,
                 APIParameters::get(opCtx).getAPIStrict().value_or(false));
-            if (!viewAggRequest.isOK()) {
-                return viewAggRequest.getStatus();
-            }
 
             // An empty PrivilegeVector is acceptable because these privileges are only checked on
             // getMore and explain will not open a cursor.
             return runAggregate(opCtx,
-                                viewAggRequest.getValue().getNamespace(),
-                                viewAggRequest.getValue(),
+                                viewAggRequest.getNamespace(),
+                                viewAggRequest,
                                 viewAggregation.getValue(),
                                 PrivilegeVector(),
                                 result);
diff --git a/src/mongo/db/commands/distinct.cpp b/src/mongo/db/commands/distinct.cpp
index 53c8497beee..738db9ffb2c 100644
--- a/src/mongo/db/commands/distinct.cpp
+++ b/src/mongo/db/commands/distinct.cpp
@@ -166,18 +166,11 @@ public:
                 viewAggCmd,
                 verbosity,
                 APIParameters::get(opCtx).getAPIStrict().value_or(false));
-            if (!viewAggRequest.isOK()) {
-                return viewAggRequest.getStatus();
-            }
 
             // An empty PrivilegeVector is acceptable because these privileges are only checked on
             // getMore and explain will not open a cursor.
-            return runAggregate(opCtx,
-                                nss,
-                                viewAggRequest.getValue(),
-                                viewAggregation.getValue(),
-                                PrivilegeVector(),
-                                result);
+            return runAggregate(
+                opCtx, nss, viewAggRequest, viewAggregation.getValue(), PrivilegeVector(), result);
         }
 
         const auto& collection = ctx->getCollection();
diff --git a/src/mongo/db/commands/find_cmd.cpp b/src/mongo/db/commands/find_cmd.cpp
index 32c6cdcb0b8..a26fbf4f343 100644
--- a/src/mongo/db/commands/find_cmd.cpp
+++ b/src/mongo/db/commands/find_cmd.cpp
@@ -274,11 +274,11 @@ public:
                 auto viewAggCmd = OpMsgRequest::fromDBAndBody(_dbName, viewAggregationCommand).body;
                 // Create the agg request equivalent of the find operation, with the explain
                 // verbosity included.
-                auto aggRequest = uassertStatusOK(aggregation_request_helper::parseFromBSON(
+                auto aggRequest = aggregation_request_helper::parseFromBSON(
                     nss,
                     viewAggCmd,
                     verbosity,
-                    APIParameters::get(opCtx).getAPIStrict().value_or(false)));
+                    APIParameters::get(opCtx).getAPIStrict().value_or(false));
 
                 try {
                     // An empty PrivilegeVector is acceptable because these privileges are only
diff --git a/src/mongo/db/commands/pipeline_command.cpp b/src/mongo/db/commands/pipeline_command.cpp
index 341c42d1aca..39e25705af1 100644
--- a/src/mongo/db/commands/pipeline_command.cpp
+++ b/src/mongo/db/commands/pipeline_command.cpp
@@ -72,11 +72,11 @@ public:
         OperationContext* opCtx,
         const OpMsgRequest& opMsgRequest,
         boost::optional<ExplainOptions::Verbosity> explainVerbosity) override {
-        const auto aggregationRequest = uassertStatusOK(aggregation_request_helper::parseFromBSON(
+        const auto aggregationRequest = aggregation_request_helper::parseFromBSON(
             opMsgRequest.getDatabase().toString(),
             opMsgRequest.body,
             explainVerbosity,
-            APIParameters::get(opCtx).getAPIStrict().value_or(false)));
+            APIParameters::get(opCtx).getAPIStrict().value_or(false));
 
         auto privileges =
             uassertStatusOK(AuthorizationSession::get(opCtx->getClient())
diff --git a/src/mongo/db/error_labels.cpp b/src/mongo/db/error_labels.cpp
index 2caa2c6c6fe..bdb6dc65b20 100644
--- a/src/mongo/db/error_labels.cpp
+++ b/src/mongo/db/error_labels.cpp
@@ -102,8 +102,8 @@ bool ErrorLabelBuilder::isResumableChangeStreamError() const {
     // Do enough parsing to confirm that this is a well-formed pipeline with a $changeStream.
     const auto swLitePipe = [&nss, &cmdObj, apiStrict]() -> StatusWith<LiteParsedPipeline> {
         try {
-            auto aggRequest = uassertStatusOK(
-                aggregation_request_helper::parseFromBSON(nss, cmdObj, boost::none, apiStrict));
+            auto aggRequest =
+                aggregation_request_helper::parseFromBSON(nss, cmdObj, boost::none, apiStrict);
             return LiteParsedPipeline(aggRequest);
         } catch (const DBException& ex) {
             return ex.toStatus();
diff --git a/src/mongo/db/pipeline/aggregation_request_helper.cpp b/src/mongo/db/pipeline/aggregation_request_helper.cpp
index 0dfba6d8133..6d575759482 100644
--- a/src/mongo/db/pipeline/aggregation_request_helper.cpp
+++ b/src/mongo/db/pipeline/aggregation_request_helper.cpp
@@ -46,11 +46,15 @@
 namespace mongo {
 namespace aggregation_request_helper {
 
-StatusWith<AggregateCommand> parseFromBSON(
-    const std::string& dbName,
-    const BSONObj& cmdObj,
-    boost::optional<ExplainOptions::Verbosity> explainVerbosity,
-    bool apiStrict) {
+/**
+ * Validate the aggregate command object.
+ */
+void validate(const BSONObj& cmdObj, boost::optional<ExplainOptions::Verbosity> explainVerbosity);
+
+AggregateCommand parseFromBSON(const std::string& dbName,
+                               const BSONObj& cmdObj,
+                               boost::optional<ExplainOptions::Verbosity> explainVerbosity,
+                               bool apiStrict) {
     return parseFromBSON(parseNs(dbName, cmdObj), cmdObj, explainVerbosity, apiStrict);
 }
 
@@ -59,7 +63,11 @@ StatusWith<AggregateCommand> parseFromBSONForTests(
     const BSONObj& cmdObj,
     boost::optional<ExplainOptions::Verbosity> explainVerbosity,
     bool apiStrict) {
-    return parseFromBSON(nss, cmdObj, explainVerbosity, apiStrict);
+    try {
+        return parseFromBSON(nss, cmdObj, explainVerbosity, apiStrict);
+    } catch (const AssertionException&) {
+        return exceptionToStatus();
+    }
 }
 
 StatusWith<AggregateCommand> parseFromBSONForTests(
@@ -67,14 +75,17 @@ StatusWith<AggregateCommand> parseFromBSONForTests(
     const BSONObj& cmdObj,
     boost::optional<ExplainOptions::Verbosity> explainVerbosity,
     bool apiStrict) {
-    return parseFromBSON(dbName, cmdObj, explainVerbosity, apiStrict);
+    try {
+        return parseFromBSON(dbName, cmdObj, explainVerbosity, apiStrict);
+    } catch (const AssertionException&) {
+        return exceptionToStatus();
+    }
 }
 
-StatusWith<AggregateCommand> parseFromBSON(
-    NamespaceString nss,
-    const BSONObj& cmdObj,
-    boost::optional<ExplainOptions::Verbosity> explainVerbosity,
-    bool apiStrict) {
+AggregateCommand parseFromBSON(NamespaceString nss,
+                               const BSONObj& cmdObj,
+                               boost::optional<ExplainOptions::Verbosity> explainVerbosity,
+                               bool apiStrict) {
 
     // if the command object lacks field 'aggregate' or '$db', we will use the namespace in 'nss'.
     bool cmdObjChanged = false;
@@ -87,28 +98,18 @@ StatusWith<AggregateCommand> parseFromBSON(
     }
 
     AggregateCommand request(nss);
-    try {
-        request = AggregateCommand::parse(IDLParserErrorContext("aggregate", apiStrict),
-                                          cmdObjChanged ? cmdObjBob.obj() : cmdObj);
-    } catch (const AssertionException&) {
-        return exceptionToStatus();
-    }
+    request = AggregateCommand::parse(IDLParserErrorContext("aggregate", apiStrict),
+                                      cmdObjChanged ? cmdObjBob.obj() : cmdObj);
 
     if (explainVerbosity) {
-        if (cmdObj.hasField(AggregateCommand::kExplainFieldName)) {
-            return {
-                ErrorCodes::FailedToParse,
+        uassert(ErrorCodes::FailedToParse,
                 str::stream() << "The '" << AggregateCommand::kExplainFieldName
-                              << "' option is illegal when a explain verbosity is also provided"};
-        }
-
+                              << "' option is illegal when a explain verbosity is also provided",
+                !cmdObj.hasField(AggregateCommand::kExplainFieldName));
         request.setExplain(explainVerbosity);
     }
 
-    auto status = validate(cmdObj, explainVerbosity);
-    if (!status.isOK()) {
-        return status;
-    }
+    validate(cmdObj, explainVerbosity);
 
     return request;
 }
@@ -147,8 +148,7 @@ Document serializeToCommandDoc(const AggregateCommand& request) {
     return Document(request.toBSON(BSONObj()).getOwned());
 }
 
-Status validate(const BSONObj& cmdObj,
-                boost::optional<ExplainOptions::Verbosity> explainVerbosity) {
+void validate(const BSONObj& cmdObj, boost::optional<ExplainOptions::Verbosity> explainVerbosity) {
     bool hasAllowDiskUseElem = cmdObj.hasField(AggregateCommand::kAllowDiskUseFieldName);
     bool hasCursorElem = cmdObj.hasField(AggregateCommand::kCursorFieldName);
     bool hasExplainElem = cmdObj.hasField(AggregateCommand::kExplainFieldName);
@@ -159,32 +159,25 @@ Status validate(const BSONObj& cmdObj,
 
     // 'hasExplainElem' implies an aggregate command-level explain option, which does not require
     // a cursor argument.
-    if (!hasCursorElem && !hasExplainElem) {
-        return {ErrorCodes::FailedToParse,
-                str::stream()
-                    << "The '" << AggregateCommand::kCursorFieldName
-                    << "' option is required, except for aggregate with the explain argument"};
-    }
-
-    if (hasExplain && cmdObj[WriteConcernOptions::kWriteConcernField]) {
-        return {ErrorCodes::FailedToParse,
-                str::stream() << "Aggregation explain does not support the'"
-                              << WriteConcernOptions::kWriteConcernField << "' option"};
-    }
-
-    if (hasNeedsMergeElem && !hasFromMongosElem) {
-        return {ErrorCodes::FailedToParse,
-                str::stream() << "Cannot specify '" << AggregateCommand::kNeedsMergeFieldName
-                              << "' without '" << AggregateCommand::kFromMongosFieldName << "'"};
-    }
-
-    if (hasAllowDiskUseElem && storageGlobalParams.readOnly) {
-        return {ErrorCodes::IllegalOperation,
-                str::stream() << "The '" << AggregateCommand::kAllowDiskUseFieldName
-                              << "' option is not permitted in read-only mode."};
-    }
-
-    return Status::OK();
+    uassert(ErrorCodes::FailedToParse,
+            str::stream() << "The '" << AggregateCommand::kCursorFieldName
+                          << "' option is required, except for aggregate with the explain argument",
+            hasCursorElem || hasExplainElem);
+
+    uassert(ErrorCodes::FailedToParse,
+            str::stream() << "Aggregation explain does not support the'"
+                          << WriteConcernOptions::kWriteConcernField << "' option",
+            !hasExplain || !cmdObj[WriteConcernOptions::kWriteConcernField]);
+
+    uassert(ErrorCodes::FailedToParse,
+            str::stream() << "Cannot specify '" << AggregateCommand::kNeedsMergeFieldName
+                          << "' without '" << AggregateCommand::kFromMongosFieldName << "'",
+            (!hasNeedsMergeElem || hasFromMongosElem));
+
+    uassert(ErrorCodes::IllegalOperation,
+            str::stream() << "The '" << AggregateCommand::kAllowDiskUseFieldName
+                          << "' option is not permitted in read-only mode.",
+            (!hasAllowDiskUseElem || !storageGlobalParams.readOnly));
 }
 }  // namespace aggregation_request_helper
 
diff --git a/src/mongo/db/pipeline/aggregation_request_helper.h b/src/mongo/db/pipeline/aggregation_request_helper.h
index 203e7d41d25..9c021abdd4b 100644
--- a/src/mongo/db/pipeline/aggregation_request_helper.h
+++ b/src/mongo/db/pipeline/aggregation_request_helper.h
@@ -58,19 +58,18 @@ static constexpr StringData kBatchSizeField = "batchSize"_sd;
 static constexpr long long kDefaultBatchSize = 101;
 
 /**
- * Create a new instance of AggregateCommand by parsing the raw command object. Returns a
- * non-OK status if a required field was missing, if there was an unrecognized field name or if
- * there was a bad value for one of the fields.
+ * Create a new instance of AggregateCommand by parsing the raw command object. Throws an exception
+ * if a required field was missing, if there was an unrecognized field name, or if there was a bad
+ * value for one of the fields.
  *
  * If we are parsing a request for an explained aggregation with an explain verbosity provided,
  * then 'explainVerbosity' contains this information. In this case, 'cmdObj' may not itself
  * contain the explain specifier. Otherwise, 'explainVerbosity' should be boost::none.
  */
-StatusWith<AggregateCommand> parseFromBSON(
-    NamespaceString nss,
-    const BSONObj& cmdObj,
-    boost::optional<ExplainOptions::Verbosity> explainVerbosity,
-    bool apiStrict);
+AggregateCommand parseFromBSON(NamespaceString nss,
+                               const BSONObj& cmdObj,
+                               boost::optional<ExplainOptions::Verbosity> explainVerbosity,
+                               bool apiStrict);
 
 StatusWith<AggregateCommand> parseFromBSONForTests(
     NamespaceString nss,
@@ -82,11 +81,10 @@ StatusWith<AggregateCommand> parseFromBSONForTests(
  * Convenience overload which constructs the request's NamespaceString from the given database
  * name and command object.
  */
-StatusWith<AggregateCommand> parseFromBSON(
-    const std::string& dbName,
-    const BSONObj& cmdObj,
-    boost::optional<ExplainOptions::Verbosity> explainVerbosity,
-    bool apiStrict);
+AggregateCommand parseFromBSON(const std::string& dbName,
+                               const BSONObj& cmdObj,
+                               boost::optional<ExplainOptions::Verbosity> explainVerbosity,
+                               bool apiStrict);
 
 StatusWith<AggregateCommand> parseFromBSONForTests(
     const std::string& dbName,
@@ -113,11 +111,6 @@ NamespaceString parseNs(const std::string& dbname, const BSONObj& cmdObj);
 Document serializeToCommandDoc(const AggregateCommand& request);
 
 BSONObj serializeToCommandObj(const AggregateCommand& request);
-
-/**
- * Validate the aggregate command object.
- */
-Status validate(const BSONObj& cmdObj, boost::optional<ExplainOptions::Verbosity> explainVerbosity);
 }  // namespace aggregation_request_helper
 
 /**
diff --git a/src/mongo/db/s/periodic_sharded_index_consistency_checker.cpp b/src/mongo/db/s/periodic_sharded_index_consistency_checker.cpp
index 986ab2cc65b..cb5c1b94fde 100644
--- a/src/mongo/db/s/periodic_sharded_index_consistency_checker.cpp
+++ b/src/mongo/db/s/periodic_sharded_index_consistency_checker.cpp
@@ -141,8 +141,8 @@ void PeriodicShardedIndexConsistencyChecker::_launchShardedIndexConsistencyCheck
                         continue;
                     }
 
-                    auto request = uassertStatusOK(aggregation_request_helper::parseFromBSON(
-                        nss, aggRequestBSON, boost::none, false));
+                    auto request = aggregation_request_helper::parseFromBSON(
+                        nss, aggRequestBSON, boost::none, false);
 
                     auto catalogCache = Grid::get(opCtx)->catalogCache();
                     shardVersionRetry(
diff --git a/src/mongo/s/commands/cluster_count_cmd.cpp b/src/mongo/s/commands/cluster_count_cmd.cpp
index 22c69d70e43..cf8f22a44fd 100644
--- a/src/mongo/s/commands/cluster_count_cmd.cpp
+++ b/src/mongo/s/commands/cluster_count_cmd.cpp
@@ -129,11 +129,11 @@ public:
             auto aggCmdOnView =
                 uassertStatusOK(countCommandAsAggregationCommand(countRequest, nss));
             auto aggCmdOnViewObj = OpMsgRequest::fromDBAndBody(nss.db(), aggCmdOnView).body;
-            auto aggRequestOnView = uassertStatusOK(aggregation_request_helper::parseFromBSON(
+            auto aggRequestOnView = aggregation_request_helper::parseFromBSON(
                 nss,
                 aggCmdOnViewObj,
                 boost::none,
-                APIParameters::get(opCtx).getAPIStrict().value_or(false)));
+                APIParameters::get(opCtx).getAPIStrict().value_or(false));
 
             auto resolvedAggRequest = ex->asExpandedViewAggregation(aggRequestOnView);
             auto resolvedAggCmd =
@@ -248,15 +248,12 @@ public:
                 aggCmdOnViewObj,
                 verbosity,
                 APIParameters::get(opCtx).getAPIStrict().value_or(false));
-            if (!aggRequestOnView.isOK()) {
-                return aggRequestOnView.getStatus();
-            }
 
             auto bodyBuilder = result->getBodyBuilder();
             // An empty PrivilegeVector is acceptable because these privileges are only checked on
             // getMore and explain will not open a cursor.
             return ClusterAggregate::retryOnViewError(opCtx,
-                                                      aggRequestOnView.getValue(),
+                                                      aggRequestOnView,
                                                       *ex.extraInfo<ResolvedView>(),
                                                       nss,
                                                       PrivilegeVector(),
diff --git a/src/mongo/s/commands/cluster_distinct_cmd.cpp b/src/mongo/s/commands/cluster_distinct_cmd.cpp
index 7d1e5bc7c0b..7e695d526e2 100644
--- a/src/mongo/s/commands/cluster_distinct_cmd.cpp
+++ b/src/mongo/s/commands/cluster_distinct_cmd.cpp
@@ -138,15 +138,13 @@ public:
                 viewAggCmd,
                 verbosity,
                 APIParameters::get(opCtx).getAPIStrict().value_or(false));
-            if (!aggRequestOnView.isOK()) {
-                return aggRequestOnView.getStatus();
-            }
+
 
             auto bodyBuilder = result->getBodyBuilder();
             // An empty PrivilegeVector is acceptable because these privileges are only checked on
             // getMore and explain will not open a cursor.
             return ClusterAggregate::retryOnViewError(opCtx,
-                                                      aggRequestOnView.getValue(),
+                                                      aggRequestOnView,
                                                       *ex.extraInfo<ResolvedView>(),
                                                       nss,
                                                       PrivilegeVector(),
@@ -216,9 +214,8 @@ public:
                 viewAggCmd,
                 boost::none,
                 APIParameters::get(opCtx).getAPIStrict().value_or(false));
-            uassertStatusOK(aggRequestOnView.getStatus());
 
-            auto resolvedAggRequest = ex->asExpandedViewAggregation(aggRequestOnView.getValue());
+            auto resolvedAggRequest = ex->asExpandedViewAggregation(aggRequestOnView);
             auto resolvedAggCmd =
                 aggregation_request_helper::serializeToCommandObj(resolvedAggRequest);
 
diff --git a/src/mongo/s/commands/cluster_find_cmd.cpp b/src/mongo/s/commands/cluster_find_cmd.cpp
index 813bee06bad..1199afe30ad 100644
--- a/src/mongo/s/commands/cluster_find_cmd.cpp
+++ b/src/mongo/s/commands/cluster_find_cmd.cpp
@@ -198,11 +198,11 @@ public:
                 auto viewAggregationCommand =
                     OpMsgRequest::fromDBAndBody(_dbName, aggCmdOnView).body;
 
-                auto aggRequestOnView = uassertStatusOK(aggregation_request_helper::parseFromBSON(
+                auto aggRequestOnView = aggregation_request_helper::parseFromBSON(
                     ns(),
                     viewAggregationCommand,
                     verbosity,
-                    APIParameters::get(opCtx).getAPIStrict().value_or(false)));
+                    APIParameters::get(opCtx).getAPIStrict().value_or(false));
 
                 // An empty PrivilegeVector is acceptable because these privileges are only checked
                 // on getMore and explain will not open a cursor.
@@ -264,11 +264,11 @@ public:
                 auto viewAggregationCommand =
                     OpMsgRequest::fromDBAndBody(_dbName, aggCmdOnView).body;
 
-                auto aggRequestOnView = uassertStatusOK(aggregation_request_helper::parseFromBSON(
+                auto aggRequestOnView = aggregation_request_helper::parseFromBSON(
                     ns(),
                     viewAggregationCommand,
                     boost::none,
-                    APIParameters::get(opCtx).getAPIStrict().value_or(false)));
+                    APIParameters::get(opCtx).getAPIStrict().value_or(false));
 
                 auto bodyBuilder = result->getBodyBuilder();
                 uassertStatusOK(ClusterAggregate::retryOnViewError(
diff --git a/src/mongo/s/commands/cluster_pipeline_cmd.cpp b/src/mongo/s/commands/cluster_pipeline_cmd.cpp
index 8568a46d9aa..d8adbc4ed5b 100644
--- a/src/mongo/s/commands/cluster_pipeline_cmd.cpp
+++ b/src/mongo/s/commands/cluster_pipeline_cmd.cpp
@@ -73,11 +73,11 @@ public:
         OperationContext* opCtx,
         const OpMsgRequest& opMsgRequest,
         boost::optional<ExplainOptions::Verbosity> explainVerbosity) override {
-        const auto aggregationRequest = uassertStatusOK(aggregation_request_helper::parseFromBSON(
+        const auto aggregationRequest = aggregation_request_helper::parseFromBSON(
             opMsgRequest.getDatabase().toString(),
             opMsgRequest.body,
             explainVerbosity,
-            APIParameters::get(opCtx).getAPIStrict().value_or(false)));
+            APIParameters::get(opCtx).getAPIStrict().value_or(false));
 
         auto privileges =
             uassertStatusOK(AuthorizationSession::get(opCtx->getClient())