From d77297f4a454073505741ae586c885e087b30165 Mon Sep 17 00:00:00 2001 From: Benety Goh Date: Sat, 6 Feb 2021 07:57:41 -0500 Subject: Revert "SERVER-53127 Let AggregationRequestHelper::parseFromBSON() return an AggregateCommand" This reverts commit 6feae12fe29a4c921bdbf03dd8b1ae6d5dd27f92. --- src/mongo/db/auth/authorization_session_impl.cpp | 7 +- src/mongo/db/commands/count_cmd.cpp | 7 +- src/mongo/db/commands/distinct.cpp | 11 ++- src/mongo/db/commands/find_cmd.cpp | 4 +- src/mongo/db/commands/pipeline_command.cpp | 4 +- src/mongo/db/error_labels.cpp | 4 +- .../db/pipeline/aggregation_request_helper.cpp | 105 +++++++++++---------- src/mongo/db/pipeline/aggregation_request_helper.h | 29 +++--- .../periodic_sharded_index_consistency_checker.cpp | 4 +- src/mongo/s/commands/cluster_count_cmd.cpp | 9 +- src/mongo/s/commands/cluster_distinct_cmd.cpp | 9 +- src/mongo/s/commands/cluster_find_cmd.cpp | 8 +- src/mongo/s/commands/cluster_pipeline_cmd.cpp | 4 +- 13 files changed, 119 insertions(+), 86 deletions(-) (limited to 'src') 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 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 { 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 explainVerbosity); - -AggregateCommand parseFromBSON(const std::string& dbName, - const BSONObj& cmdObj, - boost::optional explainVerbosity, - bool apiStrict) { +StatusWith parseFromBSON( + const std::string& dbName, + const BSONObj& cmdObj, + boost::optional explainVerbosity, + bool apiStrict) { return parseFromBSON(parseNs(dbName, cmdObj), cmdObj, explainVerbosity, apiStrict); } @@ -63,11 +59,7 @@ StatusWith parseFromBSONForTests( const BSONObj& cmdObj, boost::optional explainVerbosity, bool apiStrict) { - try { - return parseFromBSON(nss, cmdObj, explainVerbosity, apiStrict); - } catch (const AssertionException&) { - return exceptionToStatus(); - } + return parseFromBSON(nss, cmdObj, explainVerbosity, apiStrict); } StatusWith parseFromBSONForTests( @@ -75,17 +67,14 @@ StatusWith parseFromBSONForTests( const BSONObj& cmdObj, boost::optional 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 explainVerbosity, - bool apiStrict) { +StatusWith parseFromBSON( + NamespaceString nss, + const BSONObj& cmdObj, + boost::optional 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 explainVerbosity) { +Status validate(const BSONObj& cmdObj, + boost::optional 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 // '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 explainVerbosity, - bool apiStrict); +StatusWith parseFromBSON( + NamespaceString nss, + const BSONObj& cmdObj, + boost::optional explainVerbosity, + bool apiStrict); StatusWith parseFromBSONForTests( NamespaceString nss, @@ -81,10 +82,11 @@ StatusWith 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 explainVerbosity, - bool apiStrict); +StatusWith parseFromBSON( + const std::string& dbName, + const BSONObj& cmdObj, + boost::optional explainVerbosity, + bool apiStrict); StatusWith 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 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(), 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(), 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 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()) -- cgit v1.2.1