From d12213c0198b2a2adfa033a8661e026c9e9c7f7f Mon Sep 17 00:00:00 2001 From: George Wangensteen Date: Mon, 26 Oct 2020 20:40:49 +0000 Subject: SERVER-51374 Create IDL definition for ErrorReply --- src/mongo/db/commands.cpp | 16 +++++++++ src/mongo/db/commands_test.cpp | 29 ++++++++++++++-- src/mongo/idl/SConscript | 1 + src/mongo/idl/basic_types.idl | 17 +++++++++- src/mongo/idl/idl_test.cpp | 55 +++++++++++++++++++++++++++++++ src/mongo/rpc/SConscript | 1 + src/mongo/rpc/reply_builder_interface.cpp | 12 +++++++ 7 files changed, 128 insertions(+), 3 deletions(-) (limited to 'src') 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 + 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(); } -- cgit v1.2.1