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

2 files changed, 60 insertions, 74 deletions

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
 
 /**