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

This reverts commit 6feae12fe29a4c921bdbf03dd8b1ae6d5dd27f92.

13 files changed, 119 insertions, 86 deletions

diff --git a/src/mongo/db/auth/authorization_session_impl.cpp b/src/mongo/db/auth/authorization_session_impl.cpp
index 0f131648506..9b54154225e 100644
--- a/src/mongo/db/auth/authorization_session_impl.cpp
+++ b/src/mongo/db/auth/authorization_session_impl.cpp
@@ -90,14 +90,17 @@ Status checkAuthForCreateOrModifyView(AuthorizationSession* authzSession,
         return Status::OK();
     }
 
-    auto request = aggregation_request_helper::parseFromBSON(
+    auto status = 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, request, isMongos);
+    auto statusWithPrivs =
+        authzSession->getPrivilegesForAggregate(viewOnNs, status.getValue(), 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 8ca97e3ae2a..1164eb1e07c 100644
--- a/src/mongo/db/commands/count_cmd.cpp
+++ b/src/mongo/db/commands/count_cmd.cpp
@@ -168,12 +168,15 @@ 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.getNamespace(),
-                                viewAggRequest,
+                                viewAggRequest.getValue().getNamespace(),
+                                viewAggRequest.getValue(),
                                 viewAggregation.getValue(),
                                 PrivilegeVector(),
                                 result);
diff --git a/src/mongo/db/commands/distinct.cpp b/src/mongo/db/commands/distinct.cpp
index 738db9ffb2c..53c8497beee 100644
--- a/src/mongo/db/commands/distinct.cpp
+++ b/src/mongo/db/commands/distinct.cpp
@@ -166,11 +166,18 @@ 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, viewAggregation.getValue(), PrivilegeVector(), result);
+            return runAggregate(opCtx,
+                                nss,
+                                viewAggRequest.getValue(),
+                                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 a26fbf4f343..32c6cdcb0b8 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 = aggregation_request_helper::parseFromBSON(
+                auto aggRequest = uassertStatusOK(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 39e25705af1..341c42d1aca 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 = aggregation_request_helper::parseFromBSON(
+        const auto aggregationRequest = uassertStatusOK(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 bdb6dc65b20..2caa2c6c6fe 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 =
-                aggregation_request_helper::parseFromBSON(nss, cmdObj, boost::none, apiStrict);
+            auto aggRequest = uassertStatusOK(
+                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 6d575759482..0dfba6d8133 100644
--- a/src/mongo/db/pipeline/aggregation_request_helper.cpp
+++ b/src/mongo/db/pipeline/aggregation_request_helper.cpp
@@ -46,15 +46,11 @@
 namespace mongo {
 namespace aggregation_request_helper {
 
-/**
- * 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) {
+StatusWith<AggregateCommand> parseFromBSON(
+    const std::string& dbName,
+    const BSONObj& cmdObj,
+    boost::optional<ExplainOptions::Verbosity> explainVerbosity,
+    bool apiStrict) {
     return parseFromBSON(parseNs(dbName, cmdObj), cmdObj, explainVerbosity, apiStrict);
 }
 
@@ -63,11 +59,7 @@ StatusWith<AggregateCommand> parseFromBSONForTests(
     const BSONObj& cmdObj,
     boost::optional<ExplainOptions::Verbosity> explainVerbosity,
     bool apiStrict) {
-    try {
-        return parseFromBSON(nss, cmdObj, explainVerbosity, apiStrict);
-    } catch (const AssertionException&) {
-        return exceptionToStatus();
-    }
+    return parseFromBSON(nss, cmdObj, explainVerbosity, apiStrict);
 }
 
 StatusWith<AggregateCommand> parseFromBSONForTests(
@@ -75,17 +67,14 @@ StatusWith<AggregateCommand> parseFromBSONForTests(
     const BSONObj& cmdObj,
     boost::optional<ExplainOptions::Verbosity> explainVerbosity,
     bool apiStrict) {
-    try {
-        return parseFromBSON(dbName, cmdObj, explainVerbosity, apiStrict);
-    } catch (const AssertionException&) {
-        return exceptionToStatus();
-    }
+    return parseFromBSON(dbName, cmdObj, explainVerbosity, apiStrict);
 }
 
-AggregateCommand parseFromBSON(NamespaceString nss,
-                               const BSONObj& cmdObj,
-                               boost::optional<ExplainOptions::Verbosity> explainVerbosity,
-                               bool apiStrict) {
+StatusWith<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;
@@ -98,18 +87,28 @@ AggregateCommand parseFromBSON(NamespaceString nss,
     }
 
     AggregateCommand request(nss);
-    request = AggregateCommand::parse(IDLParserErrorContext("aggregate", apiStrict),
-                                      cmdObjChanged ? cmdObjBob.obj() : cmdObj);
+    try {
+        request = AggregateCommand::parse(IDLParserErrorContext("aggregate", apiStrict),
+                                          cmdObjChanged ? cmdObjBob.obj() : cmdObj);
+    } catch (const AssertionException&) {
+        return exceptionToStatus();
+    }
 
     if (explainVerbosity) {
-        uassert(ErrorCodes::FailedToParse,
+        if (cmdObj.hasField(AggregateCommand::kExplainFieldName)) {
+            return {
+                ErrorCodes::FailedToParse,
                 str::stream() << "The '" << AggregateCommand::kExplainFieldName
-                              << "' option is illegal when a explain verbosity is also provided",
-                !cmdObj.hasField(AggregateCommand::kExplainFieldName));
+                              << "' option is illegal when a explain verbosity is also provided"};
+        }
+
         request.setExplain(explainVerbosity);
     }
 
-    validate(cmdObj, explainVerbosity);
+    auto status = validate(cmdObj, explainVerbosity);
+    if (!status.isOK()) {
+        return status;
+    }
 
     return request;
 }
@@ -148,7 +147,8 @@ Document serializeToCommandDoc(const AggregateCommand& request) {
     return Document(request.toBSON(BSONObj()).getOwned());
 }
 
-void validate(const BSONObj& cmdObj, boost::optional<ExplainOptions::Verbosity> explainVerbosity) {
+Status 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,25 +159,32 @@ void validate(const BSONObj& cmdObj, boost::optional<ExplainOptions::Verbosity>
 
     // 'hasExplainElem' implies an aggregate command-level explain option, which does not require
     // a cursor argument.
-    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));
+    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();
 }
 }  // 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 9c021abdd4b..203e7d41d25 100644
--- a/src/mongo/db/pipeline/aggregation_request_helper.h
+++ b/src/mongo/db/pipeline/aggregation_request_helper.h
@@ -58,18 +58,19 @@ static constexpr StringData kBatchSizeField = "batchSize"_sd;
 static constexpr long long kDefaultBatchSize = 101;
 
 /**
- * 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.
+ * 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.
  *
  * 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.
  */
-AggregateCommand parseFromBSON(NamespaceString nss,
-                               const BSONObj& cmdObj,
-                               boost::optional<ExplainOptions::Verbosity> explainVerbosity,
-                               bool apiStrict);
+StatusWith<AggregateCommand> parseFromBSON(
+    NamespaceString nss,
+    const BSONObj& cmdObj,
+    boost::optional<ExplainOptions::Verbosity> explainVerbosity,
+    bool apiStrict);
 
 StatusWith<AggregateCommand> parseFromBSONForTests(
     NamespaceString nss,
@@ -81,10 +82,11 @@ StatusWith<AggregateCommand> parseFromBSONForTests(
  * Convenience overload which constructs the request's NamespaceString from the given database
  * name and command object.
  */
-AggregateCommand parseFromBSON(const std::string& dbName,
-                               const BSONObj& cmdObj,
-                               boost::optional<ExplainOptions::Verbosity> explainVerbosity,
-                               bool apiStrict);
+StatusWith<AggregateCommand> parseFromBSON(
+    const std::string& dbName,
+    const BSONObj& cmdObj,
+    boost::optional<ExplainOptions::Verbosity> explainVerbosity,
+    bool apiStrict);
 
 StatusWith<AggregateCommand> parseFromBSONForTests(
     const std::string& dbName,
@@ -111,6 +113,11 @@ 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 cb5c1b94fde..986ab2cc65b 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 = aggregation_request_helper::parseFromBSON(
-                        nss, aggRequestBSON, boost::none, false);
+                    auto request = uassertStatusOK(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 cf8f22a44fd..22c69d70e43 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 = aggregation_request_helper::parseFromBSON(
+            auto aggRequestOnView = uassertStatusOK(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,12 +248,15 @@ 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,
+                                                      aggRequestOnView.getValue(),
                                                       *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 7e695d526e2..7d1e5bc7c0b 100644
--- a/src/mongo/s/commands/cluster_distinct_cmd.cpp
+++ b/src/mongo/s/commands/cluster_distinct_cmd.cpp
@@ -138,13 +138,15 @@ 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,
+                                                      aggRequestOnView.getValue(),
                                                       *ex.extraInfo<ResolvedView>(),
                                                       nss,
                                                       PrivilegeVector(),
@@ -214,8 +216,9 @@ public:
                 viewAggCmd,
                 boost::none,
                 APIParameters::get(opCtx).getAPIStrict().value_or(false));
+            uassertStatusOK(aggRequestOnView.getStatus());
 
-            auto resolvedAggRequest = ex->asExpandedViewAggregation(aggRequestOnView);
+            auto resolvedAggRequest = ex->asExpandedViewAggregation(aggRequestOnView.getValue());
             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 1199afe30ad..813bee06bad 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 = aggregation_request_helper::parseFromBSON(
+                auto aggRequestOnView = uassertStatusOK(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 = aggregation_request_helper::parseFromBSON(
+                auto aggRequestOnView = uassertStatusOK(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 d8adbc4ed5b..8568a46d9aa 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 = aggregation_request_helper::parseFromBSON(
+        const auto aggregationRequest = uassertStatusOK(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())