SERVER-34628 Prep for removing appendCommandStatus

* Added appendCommandStatusNoThrow matching the current aCS behavior
* Make appendCommandStatus call uassertStatusOK then aCS on success
* Make the few places that need to not throw call aCSNT

A following commit will completely remove appendCommandStatus. It is split out
because that commit is fairly huge.

src/mongo/db/auth/sasl_commands.cpp

@@ -334,7 +334,7 @@ bool CmdSaslContinue::run(OperationContext* opCtx,
     }
 
     Status status = doSaslContinue(opCtx, session, cmdObj, &result);
-    CommandHelpers::appendCommandStatus(result, status);
+    CommandHelpers::appendCommandStatusNoThrow(result, status);
 
     if (mechanism.isDone()) {
         audit::logAuthentication(

src/mongo/db/commands.cpp

@@ -91,7 +91,7 @@ BSONObj CommandHelpers::runCommandDirectly(OperationContext* opCtx, const OpMsgR
     } catch (const DBException& ex) {
         auto body = crb.getBodyBuilder();
         body.resetToEmpty();
-        appendCommandStatus(body, ex.toStatus());
+        appendCommandStatusNoThrow(body, ex.toStatus());
     }
     return BSONObj(bb.release());
 }
@@ -186,7 +186,13 @@ Command* CommandHelpers::findCommand(StringData name) {
 }
 
 bool CommandHelpers::appendCommandStatus(BSONObjBuilder& result, const Status& status) {
-    appendCommandStatus(result, status.isOK(), status.reason());
+    uassertStatusOK(status);
+    appendSimpleCommandStatus(result, true);
+    return true;
+}
+
+bool CommandHelpers::appendCommandStatusNoThrow(BSONObjBuilder& result, const Status& status) {
+    appendSimpleCommandStatus(result, status.isOK(), status.reason());
     BSONObj tmp = result.asTempObj();
     if (!status.isOK() && !tmp.hasField("code")) {
         result.append("code", status.code());
@@ -198,9 +204,9 @@ bool CommandHelpers::appendCommandStatus(BSONObjBuilder& result, const Status& s
     return status.isOK();
 }
 
-void CommandHelpers::appendCommandStatus(BSONObjBuilder& result,
-                                         bool ok,
-                                         const std::string& errmsg) {
+void CommandHelpers::appendSimpleCommandStatus(BSONObjBuilder& result,
+                                               bool ok,
+                                               const std::string& errmsg) {
     BSONObj tmp = result.asTempObj();
     bool have_ok = tmp.hasField("ok");
     bool need_errmsg = !ok && !tmp.hasField("errmsg");
@@ -219,7 +225,7 @@ bool CommandHelpers::extractOrAppendOk(BSONObjBuilder& reply) {
         return okField.trueValue();
     }
     // Missing "ok" field is an implied success.
-    CommandHelpers::appendCommandStatus(reply, true);
+    CommandHelpers::appendSimpleCommandStatus(reply, true);
     return true;
 }
 
@@ -350,7 +356,7 @@ void CommandReplyBuilder::fillFrom(const Status& status) {
         reset();
     }
     auto bob = getBodyBuilder();
-    CommandHelpers::appendCommandStatus(bob, status);
+    CommandHelpers::appendCommandStatusNoThrow(bob, status);
 }
 
 //////////////////////////////////////////////////////////////
@@ -384,7 +390,7 @@ class BasicCommand::Invocation final : public CommandInvocation {
         try {
             BSONObjBuilder bob = result->getBodyBuilder();
             bool ok = _command->run(opCtx, _dbName, _request->body, bob);
-            CommandHelpers::appendCommandStatus(bob, ok);
+            CommandHelpers::appendSimpleCommandStatus(bob, ok);
         } catch (const ExceptionFor<ErrorCodes::Unauthorized>& e) {
             CommandHelpers::logAuthViolation(opCtx, this, *_request, e.code());
             throw;
@@ -521,7 +527,7 @@ bool ErrmsgCommandDeprecated::run(OperationContext* opCtx,
     std::string errmsg;
     auto ok = errmsgRun(opCtx, db, cmdObj, errmsg, result);
     if (!errmsg.empty()) {
-        CommandHelpers::appendCommandStatus(result, ok, errmsg);
+        CommandHelpers::appendSimpleCommandStatus(result, ok, errmsg);
     }
     return ok;
 }

src/mongo/db/commands.h

@@ -90,12 +90,25 @@ struct CommandHelpers {
 
     static Command* findCommand(StringData name);
 
-    // Helper for setting errmsg and ok field in command result object.
-    static void appendCommandStatus(BSONObjBuilder& result,
-                                    bool ok,
-                                    const std::string& errmsg = {});
+    /**
+     * Helper for setting errmsg and ok field in command result object.
+     *
+     * This should generally only be called from the command dispatch code or to finish off the
+     * result of serializing a reply BSONObj in the case when it isn't going directly into a real
+     * command reply to be returned to the user.
+     */
+    static void appendSimpleCommandStatus(BSONObjBuilder& result,
+                                          bool ok,
+                                          const std::string& errmsg = {});
+
+    /**
+     * Adds the status fields to command replies.
+     *
+     * Calling this inside of commands to produce their reply is now deprecated. Just throw instead.
+     */
+    static bool appendCommandStatusNoThrow(BSONObjBuilder& result, const Status& status);
 
-    // @return s.isOK()
+    // About to be deleted
     static bool appendCommandStatus(BSONObjBuilder& result, const Status& status);
 
     /**

src/mongo/db/commands/apply_ops_cmd.cpp

@@ -267,7 +267,7 @@ class ApplyOpsCmd : public BasicCommand {
                                    << repl::ApplyOps::kOplogApplicationModeFieldName));
         }
 
-        auto applyOpsStatus = CommandHelpers::appendCommandStatus(
+        auto applyOpsStatus = CommandHelpers::appendCommandStatusNoThrow(
             result, repl::applyOps(opCtx, dbname, cmdObj, oplogApplicationMode, &result));
 
         return applyOpsStatus;

src/mongo/db/commands/create_indexes.cpp

@@ -369,19 +369,15 @@ class CmdCreateIndex : public ErrmsgCommandDeprecated {
                     // that day, to avoid data corruption due to lack of index cleanup.
                     opCtx->recoveryUnit()->abandonSnapshot();
                     dbLock.relockWithMode(MODE_X);
-                    if (!repl::ReplicationCoordinator::get(opCtx)->canAcceptWritesFor(opCtx, ns)) {
-                        return CommandHelpers::appendCommandStatus(
-                            result,
-                            Status(ErrorCodes::NotMaster,
-                                   str::stream()
-                                       << "Not primary while creating background indexes in "
-                                       << ns.ns()
-                                       << ": cleaning up index build failure due to "
-                                       << e.toString()));
-                    }
                 } catch (...) {
                     std::terminate();
                 }
+                uassert(ErrorCodes::NotMaster,
+                        str::stream() << "Not primary while creating background indexes in "
+                                      << ns.ns()
+                                      << ": cleaning up index build failure due to "
+                                      << e.toString(),
+                        repl::ReplicationCoordinator::get(opCtx)->canAcceptWritesFor(opCtx, ns));
             }
             throw;
         }

src/mongo/db/commands/do_txn_cmd.cpp

@@ -159,8 +159,8 @@ class DoTxnCmd : public BasicCommand {
         // was acknowledged. To fix this, we should wait for replication of the node’s last applied
         // OpTime if the last write operation was a no-op write.
 
-        auto doTxnStatus =
-            CommandHelpers::appendCommandStatus(result, doTxn(opCtx, dbname, cmdObj, &result));
+        auto doTxnStatus = CommandHelpers::appendCommandStatusNoThrow(
+            result, doTxn(opCtx, dbname, cmdObj, &result));
 
         return doTxnStatus;
     }

src/mongo/db/commands/get_last_error.cpp

@@ -177,7 +177,7 @@ class CmdGetLastError : public ErrmsgCommandDeprecated {
             Status status = bsonExtractOpTimeField(cmdObj, "wOpTime", &lastOpTime);
             if (!status.isOK()) {
                 result.append("badGLE", cmdObj);
-                return CommandHelpers::appendCommandStatus(result, status);
+                return CommandHelpers::appendCommandStatusNoThrow(result, status);
             }
         } else {
             return CommandHelpers::appendCommandStatus(
@@ -195,7 +195,7 @@ class CmdGetLastError : public ErrmsgCommandDeprecated {
             FieldParser::extract(cmdObj, wElectionIdField, &electionId, &errmsg);
         if (!extracted) {
             result.append("badGLE", cmdObj);
-            CommandHelpers::appendCommandStatus(result, false, errmsg);
+            CommandHelpers::appendSimpleCommandStatus(result, false, errmsg);
             return false;
         }
 
@@ -242,7 +242,7 @@ class CmdGetLastError : public ErrmsgCommandDeprecated {
 
         if (!status.isOK()) {
             result.append("badGLE", writeConcernDoc);
-            return CommandHelpers::appendCommandStatus(result, status);
+            return CommandHelpers::appendCommandStatusNoThrow(result, status);
         }
 
         // Don't wait for replication if there was an error reported - this matches 2.4 behavior
@@ -298,7 +298,7 @@ class CmdGetLastError : public ErrmsgCommandDeprecated {
             return true;
         }
 
-        return CommandHelpers::appendCommandStatus(result, status);
+        return CommandHelpers::appendCommandStatusNoThrow(result, status);
     }
 
 } cmdGetLastError;

src/mongo/db/commands/user_management_commands.cpp

@@ -1463,7 +1463,7 @@ class CmdUsersInfo : public BasicCommand {
                 return CommandHelpers::appendCommandStatus(result, status);
             }
 
-            CommandHelpers::appendCommandStatus(responseBuilder, Status::OK());
+            CommandHelpers::appendSimpleCommandStatus(responseBuilder, true);
             auto swResponse = CursorResponse::parseFromBSON(responseBuilder.obj());
             if (!swResponse.isOK()) {
                 return CommandHelpers::appendCommandStatus(result, swResponse.getStatus());

src/mongo/db/commands/validate.cpp

@@ -176,7 +176,7 @@ class ValidateCmd : public BasicCommand {
                     opCtx->waitForConditionOrInterrupt(_validationNotifier, lock);
                 }
             } catch (AssertionException& e) {
-                CommandHelpers::appendCommandStatus(
+                CommandHelpers::appendCommandStatusNoThrow(
                     result,
                     {ErrorCodes::CommandFailed,
                      str::stream() << "Exception during validation: " << e.toString()});
@@ -196,7 +196,7 @@ class ValidateCmd : public BasicCommand {
         Status status =
             collection->validate(opCtx, level, background, std::move(collLk), &results, &result);
         if (!status.isOK()) {
-            return CommandHelpers::appendCommandStatus(result, status);
+            return CommandHelpers::appendCommandStatusNoThrow(result, status);
         }
 
         CollectionCatalogEntry* catalogEntry = collection->getCatalogEntry();

src/mongo/db/commands_test.cpp

@@ -40,7 +40,7 @@ namespace {
 
 TEST(Commands, appendCommandStatusOK) {
     BSONObjBuilder actualResult;
-    CommandHelpers::appendCommandStatus(actualResult, Status::OK());
+    CommandHelpers::appendCommandStatusNoThrow(actualResult, Status::OK());
 
     BSONObjBuilder expectedResult;
     expectedResult.append("ok", 1.0);
@@ -51,7 +51,7 @@ TEST(Commands, appendCommandStatusOK) {
 TEST(Commands, appendCommandStatusError) {
     BSONObjBuilder actualResult;
     const Status status(ErrorCodes::InvalidLength, "Response payload too long");
-    CommandHelpers::appendCommandStatus(actualResult, status);
+    CommandHelpers::appendCommandStatusNoThrow(actualResult, status);
 
     BSONObjBuilder expectedResult;
     expectedResult.append("ok", 0.0);
@@ -68,7 +68,7 @@ TEST(Commands, appendCommandStatusNoOverwrite) {
     actualResult.append("c", "d");
     actualResult.append("ok", "not ok");
     const Status status(ErrorCodes::InvalidLength, "Response payload too long");
-    CommandHelpers::appendCommandStatus(actualResult, status);
+    CommandHelpers::appendCommandStatusNoThrow(actualResult, status);
 
     BSONObjBuilder expectedResult;
     expectedResult.append("a", "b");
@@ -84,7 +84,7 @@ TEST(Commands, appendCommandStatusNoOverwrite) {
 TEST(Commands, appendCommandStatusErrorExtraInfo) {
     BSONObjBuilder actualResult;
     const Status status(ErrorExtraInfoExample(123), "not again!");
-    CommandHelpers::appendCommandStatus(actualResult, status);
+    CommandHelpers::appendCommandStatusNoThrow(actualResult, status);
 
     BSONObjBuilder expectedResult;
     expectedResult.append("ok", 0.0);
@@ -362,7 +362,7 @@ struct IncrementTestCommon {
                     CommandHelpers::extractOrAppendOk(bob);
                 } catch (const DBException& e) {
                     auto bob = crb.getBodyBuilder();
-                    CommandHelpers::appendCommandStatus(bob, e.toStatus());
+                    CommandHelpers::appendCommandStatusNoThrow(bob, e.toStatus());
                 }
                 return BSONObj(bb.release());
             }();

src/mongo/db/repl/repl_set_commands.cpp

@@ -619,7 +619,8 @@ class CmdReplSetUpdatePosition : public ReplSetCommand {
             if (status == ErrorCodes::InvalidReplicaSetConfig) {
                 result.append("configVersion", configVersion);
             }
-            return CommandHelpers::appendCommandStatus(result, status);
+            // TODO convert to uassertStatusOK once SERVER-34806 is done.
+            return CommandHelpers::appendCommandStatusNoThrow(result, status);
         } else {
             // Parsing error from UpdatePositionArgs.
             return CommandHelpers::appendCommandStatus(result, status);

src/mongo/db/repl/repl_set_request_votes.cpp

@@ -66,7 +66,7 @@ class CmdReplSetRequestVotes : public ReplSetCommand {
         status = ReplicationCoordinator::get(opCtx)->processReplSetRequestVotes(
             opCtx, parsedArgs, &response);
         response.addToBSON(&result);
-        return CommandHelpers::appendCommandStatus(result, status);
+        return CommandHelpers::appendCommandStatusNoThrow(result, status);
     }
 } cmdReplSetRequestVotes;
 

src/mongo/db/s/balancer/migration_manager_test.cpp

@@ -278,7 +278,7 @@ void MigrationManagerTest::expectMoveChunkCommand(const ChunkType& chunk,
                                                   const ShardId& toShardId,
                                                   const Status& returnStatus) {
     BSONObjBuilder resultBuilder;
-    CommandHelpers::appendCommandStatus(resultBuilder, returnStatus);
+    CommandHelpers::appendCommandStatusNoThrow(resultBuilder, returnStatus);
     expectMoveChunkCommand(chunk, toShardId, resultBuilder.obj());
 }
 

src/mongo/db/s/config/sharding_catalog_manager_create_collection_test.cpp

@@ -78,7 +78,7 @@ class CreateCollectionTest : public ConfigServerTestFixture {
             ASSERT_EQUALS(expectedNs.toString(), nss.toString());
 
             BSONObjBuilder responseBuilder;
-            CommandHelpers::appendCommandStatus(responseBuilder, response);
+            CommandHelpers::appendCommandStatusNoThrow(responseBuilder, response);
             return responseBuilder.obj();
         });
     }
@@ -98,8 +98,8 @@ class CreateCollectionTest : public ConfigServerTestFixture {
             BSONObjBuilder responseBuilder;
 
             if (!collectionOptionsReponse.isOK()) {
-                CommandHelpers::appendCommandStatus(responseBuilder,
-                                                    collectionOptionsReponse.getStatus());
+                CommandHelpers::appendCommandStatusNoThrow(responseBuilder,
+                                                           collectionOptionsReponse.getStatus());
             } else {
                 BSONObjBuilder listCollResponse(responseBuilder.subobjStart("cursor"));
                 BSONArrayBuilder collArrayBuilder(listCollResponse.subarrayStart("firstBatch"));

src/mongo/db/s/config/sharding_catalog_manager_shard_collection_test.cpp

@@ -102,7 +102,7 @@ class ShardCollectionTest : public ConfigServerTestFixture {
             }
 
             BSONObjBuilder responseBuilder;
-            CommandHelpers::appendCommandStatus(responseBuilder, response.getStatus());
+            CommandHelpers::appendCommandStatusNoThrow(responseBuilder, response.getStatus());
             return responseBuilder.obj();
         });
     }

src/mongo/db/s/implicit_create_collection_test.cpp

@@ -59,7 +59,7 @@ class ImplicitCreateTest : public ShardServerTestFixture {
             ASSERT_EQ(expectedNss.ns(), request.cmdObj.firstElement().String());
 
             BSONObjBuilder responseBuilder;
-            CommandHelpers::appendCommandStatus(responseBuilder, response);
+            CommandHelpers::appendCommandStatusNoThrow(responseBuilder, response);
             return responseBuilder.obj();
         });
     }

src/mongo/db/service_entry_point_mongod.cpp

@@ -94,7 +94,7 @@ class ServiceEntryPointMongod::Hooks final : public ServiceEntryPointCommon::Hoo
         if (!waitForWCStatus.isOK() && invocation->definition()->isUserManagementCommand()) {
             BSONObj temp = commandResponseBuilder.asTempObj().copy();
             commandResponseBuilder.resetToEmpty();
-            CommandHelpers::appendCommandStatus(commandResponseBuilder, waitForWCStatus);
+            CommandHelpers::appendCommandStatusNoThrow(commandResponseBuilder, waitForWCStatus);
             commandResponseBuilder.appendElementsUnique(temp);
         }
     }

src/mongo/executor/network_test_env.cpp

@@ -51,7 +51,7 @@ void NetworkTestEnv::onCommand(OnCommandFunction func) {
 
     if (resultStatus.isOK()) {
         BSONObjBuilder result(std::move(resultStatus.getValue()));
-        CommandHelpers::appendCommandStatus(result, resultStatus.getStatus());
+        CommandHelpers::appendCommandStatusNoThrow(result, resultStatus.getStatus());
         const RemoteCommandResponse response(result.obj(), BSONObj(), Milliseconds(1));
 
         _mockNetwork->scheduleResponse(noi, _mockNetwork->now(), response);
@@ -74,7 +74,7 @@ void NetworkTestEnv::onCommandWithMetadata(OnCommandWithMetadataFunction func) {
 
     if (cmdResponseStatus.isOK()) {
         BSONObjBuilder result(std::move(cmdResponseStatus.data));
-        CommandHelpers::appendCommandStatus(result, cmdResponseStatus.status);
+        CommandHelpers::appendCommandStatusNoThrow(result, cmdResponseStatus.status);
         const RemoteCommandResponse response(
             result.obj(), cmdResponseStatus.metadata, Milliseconds(1));