SERVER-51374 Create IDL definition for ErrorReply

7 files changed, 128 insertions, 3 deletions

diff --git a/src/mongo/db/commands.cpp b/src/mongo/db/commands.cpp
index 58aec58909f..a2c7a804538 100644
--- a/src/mongo/db/commands.cpp
+++ b/src/mongo/db/commands.cpp
@@ -54,6 +54,8 @@
 #include "mongo/db/jsobj.h"
 #include "mongo/db/namespace_string.h"
 #include "mongo/db/read_write_concern_defaults.h"
+#include "mongo/idl/basic_types_gen.h"
+#include "mongo/idl/idl_parser.h"
 #include "mongo/logv2/log.h"
 #include "mongo/rpc/factory.h"
 #include "mongo/rpc/metadata/client_metadata.h"
@@ -321,6 +323,20 @@ bool CommandHelpers::appendCommandStatusNoThrow(BSONObjBuilder& result, const St
     if (auto extraInfo = status.extraInfo()) {
         extraInfo->serialize(&result);
     }
+    // If the command has errored, assert that it satisfies the IDL-defined requirements on a
+    // command error reply.
+    if (!status.isOK()) {
+        try {
+            ErrorReply::parse(IDLParserErrorContext("appendCommandStatusNoThrow"),
+                              result.asTempObj());
+        } catch (const DBException&) {
+            invariant(false,
+                      "invalid error-response to a command constructed in "
+                      "CommandHelpers::appendComandStatusNoThrow. All erroring command responses "
+                      "must comply with the format specified by the IDL-defined struct ErrorReply, "
+                      "defined in idl/basic_types.idl");
+        }
+    }
     return status.isOK();
 }
 
diff --git a/src/mongo/db/commands_test.cpp b/src/mongo/db/commands_test.cpp
index 89b7b38c12f..8fecc92a2ff 100644
--- a/src/mongo/db/commands_test.cpp
+++ b/src/mongo/db/commands_test.cpp
@@ -36,6 +36,7 @@
 #include "mongo/db/service_context_test_fixture.h"
 #include "mongo/rpc/factory.h"
 #include "mongo/rpc/op_msg_rpc_impls.h"
+#include "mongo/unittest/death_test.h"
 #include "mongo/unittest/unittest.h"
 
 namespace mongo {
@@ -69,14 +70,14 @@ TEST(Commands, appendCommandStatusNoOverwrite) {
     BSONObjBuilder actualResult;
     actualResult.append("a", "b");
     actualResult.append("c", "d");
-    actualResult.append("ok", "not ok");
+    actualResult.append("ok", 0.0);
     const Status status(ErrorCodes::InvalidLength, "Response payload too long");
     CommandHelpers::appendCommandStatusNoThrow(actualResult, status);
 
     BSONObjBuilder expectedResult;
     expectedResult.append("a", "b");
     expectedResult.append("c", "d");
-    expectedResult.append("ok", "not ok");
+    expectedResult.append("ok", 0.0);
     expectedResult.append("errmsg", status.reason());
     expectedResult.append("code", status.code());
     expectedResult.append("codeName", ErrorCodes::errorString(status.code()));
@@ -99,6 +100,30 @@ TEST(Commands, appendCommandStatusErrorExtraInfo) {
     ASSERT_BSONOBJ_EQ(actualResult.obj(), expectedResult.obj());
 }
 
+DEATH_TEST(Commands, appendCommandStatusInvalidOkValue, "invariant") {
+    BSONObjBuilder actualResult;
+    actualResult.append("a", "b");
+    actualResult.append("c", "d");
+    actualResult.append("ok", "yes");
+    const Status status(ErrorCodes::InvalidLength, "fake error for test");
+
+    // An "ok" value other than 1.0 or 0.0 is not allowed and should crash.
+    CommandHelpers::appendCommandStatusNoThrow(actualResult, status);
+}
+
+DEATH_TEST(Commands, appendCommandStatusNoCodeName, "invariant") {
+    BSONObjBuilder actualResult;
+    actualResult.append("a", "b");
+    actualResult.append("code", ErrorCodes::InvalidLength);
+    actualResult.append("ok", 1.0);
+    const Status status(ErrorCodes::InvalidLength, "Response payload too long");
+
+    // If the result already has an error code, we don't move any code or codeName over from the
+    // status. Therefore, if the result has an error code but no codeName, we're missing a
+    // required field and should crash.
+    CommandHelpers::appendCommandStatusNoThrow(actualResult, status);
+}
+
 class ParseNsOrUUID : public ServiceContextTest {
 public:
     ParseNsOrUUID() : opCtxPtr(makeOperationContext()), opCtx(opCtxPtr.get()) {}
diff --git a/src/mongo/idl/SConscript b/src/mongo/idl/SConscript
index 30847441bfb..53ff5606def 100644
--- a/src/mongo/idl/SConscript
+++ b/src/mongo/idl/SConscript
@@ -51,6 +51,7 @@ env.Library(
     ],
     LIBDEPS=[
         '$BUILD_DIR/mongo/base',
+        'idl_parser',
     ],
 )
 
diff --git a/src/mongo/idl/basic_types.idl b/src/mongo/idl/basic_types.idl
index 2a5a569573b..063598e8d5d 100644
--- a/src/mongo/idl/basic_types.idl
+++ b/src/mongo/idl/basic_types.idl
@@ -196,6 +196,21 @@ types:
         deserializer: "mongo::FeatureCompatibilityVersionParser::parseVersion"
 
 structs:
+
     OkReply:
         description: "Shared by commands that reply with just {ok: 1} and no additional information"
-        strict: true
\ No newline at end of file
+        strict: true
+
+    ErrorReply:
+        description: "Error Reply structure shared by all commands"
+        strict: false
+        fields:
+            ok:
+                type: safeDouble 
+                validator: { gte: 0.0, lte: 0.0 }
+            code: int
+            codeName: string
+            errmsg: string
+            errorLabels:
+                type: array<string>
+                optional: true
diff --git a/src/mongo/idl/idl_test.cpp b/src/mongo/idl/idl_test.cpp
index 26397436f6e..6bfb54782f9 100644
--- a/src/mongo/idl/idl_test.cpp
+++ b/src/mongo/idl/idl_test.cpp
@@ -2945,5 +2945,60 @@ TEST(IDLTypeCommand, TestUnderscoreCommand) {
     }
 }
 
+TEST(IDLTypeCommand, TestErrorReplyStruct) {
+    // Correctly parse all required fields.
+    {
+        IDLParserErrorContext ctxt("root");
+
+        auto errorDoc = BSON("ok" << 0.0 << "code" << 123456 << "codeName"
+                                  << "blah blah"
+                                  << "errmsg"
+                                  << "This is an error Message"
+                                  << "errorLabels"
+                                  << BSON_ARRAY("label1"
+                                                << "label2"));
+        auto errorReply = ErrorReply::parse(ctxt, errorDoc);
+        ASSERT_BSONOBJ_EQ(errorReply.toBSON(), errorDoc);
+    }
+    // Non-strictness: ensure we parse even if input has extra fields.
+    {
+        IDLParserErrorContext ctxt("root");
+
+        auto errorDoc = BSON("a"
+                             << "b"
+                             << "ok" << 0.0 << "code" << 123456 << "codeName"
+                             << "blah blah"
+                             << "errmsg"
+                             << "This is an error Message");
+        auto errorReply = ErrorReply::parse(ctxt, errorDoc);
+        ASSERT_BSONOBJ_EQ(errorReply.toBSON(),
+                          BSON("ok" << 0.0 << "code" << 123456 << "codeName"
+                                    << "blah blah"
+                                    << "errmsg"
+                                    << "This is an error Message"));
+    }
+    // Ensure that we fail to parse if any required fields are missing.
+    {
+        IDLParserErrorContext ctxt("root");
+
+        auto missingOk = BSON("code" << 123456 << "codeName"
+                                     << "blah blah"
+                                     << "errmsg"
+                                     << "This is an error Message");
+        auto missingCode = BSON("ok" << 0.0 << "codeName"
+                                     << "blah blah"
+                                     << "errmsg"
+                                     << "This is an error Message");
+        auto missingCodeName = BSON("ok" << 0.0 << "code" << 123456 << "errmsg"
+                                         << "This is an error Message");
+        auto missingErrmsg = BSON("ok" << 0.0 << "code" << 123456 << "codeName"
+                                       << "blah blah");
+        ASSERT_THROWS(ErrorReply::parse(ctxt, missingOk), AssertionException);
+        ASSERT_THROWS(ErrorReply::parse(ctxt, missingCode), AssertionException);
+        ASSERT_THROWS(ErrorReply::parse(ctxt, missingCodeName), AssertionException);
+        ASSERT_THROWS(ErrorReply::parse(ctxt, missingErrmsg), AssertionException);
+    }
+}
+
 }  // namespace
 }  // namespace mongo
diff --git a/src/mongo/rpc/SConscript b/src/mongo/rpc/SConscript
index 43f089fa02a..d9f13bcfeb7 100644
--- a/src/mongo/rpc/SConscript
+++ b/src/mongo/rpc/SConscript
@@ -62,6 +62,7 @@ env.Library(
         '$BUILD_DIR/mongo/base',
         '$BUILD_DIR/mongo/db/dbmessage',
         '$BUILD_DIR/mongo/db/server_options_core',
+        '$BUILD_DIR/mongo/idl/basic_types',
         '$BUILD_DIR/mongo/s/common_s',
         '$BUILD_DIR/mongo/util/net/network',
         'metadata',
diff --git a/src/mongo/rpc/reply_builder_interface.cpp b/src/mongo/rpc/reply_builder_interface.cpp
index 6f4eddbbebe..1871d70af7a 100644
--- a/src/mongo/rpc/reply_builder_interface.cpp
+++ b/src/mongo/rpc/reply_builder_interface.cpp
@@ -35,6 +35,7 @@
 
 #include "mongo/base/status_with.h"
 #include "mongo/db/jsobj.h"
+#include "mongo/idl/basic_types_gen.h"
 
 namespace mongo {
 namespace rpc {
@@ -73,6 +74,17 @@ BSONObj augmentReplyWithStatus(const Status& status, BSONObj reply) {
         extraInfo->serialize(&bob);
     }
 
+    // Ensure the error reply satisfies the IDL-defined requirements.
+    try {
+        ErrorReply::parse(IDLParserErrorContext("augmentReplyWithStatus"), bob.asTempObj());
+    } catch (const DBException&) {
+        invariant(false,
+                  "invalid error-response to a command constructed in "
+                  "rpc::augmentReplyWithStatus. All erroring command responses "
+                  "must comply with the format specified by the IDL-defined struct ErrorReply, "
+                  "defined in idl/basic_types.idl");
+    }
+
     return bob.obj();
 }