diff options
author | Dorothy Chen <dorothy.chen@mongodb.com> | 2016-08-23 15:42:00 -0400 |
---|---|---|
committer | Dorothy Chen <dorothy.chen@mongodb.com> | 2016-08-26 15:14:07 -0400 |
commit | 6f03bed78373f186632f8d6f8a2d4fdc3e5177ee (patch) | |
tree | 38e3fcb1742cbf449065d2f0ff52f10f680971bd | |
parent | cac902c5ea7f4f01db6826eb3a0ed32083fd6dac (diff) | |
download | mongo-6f03bed78373f186632f8d6f8a2d4fdc3e5177ee.tar.gz |
SERVER-23501 include stringified error code in erroring command replies
-rw-r--r-- | src/mongo/db/SConscript | 14 | ||||
-rw-r--r-- | src/mongo/db/auth/sasl_commands.cpp | 24 | ||||
-rw-r--r-- | src/mongo/db/catalog/apply_ops.cpp | 3 | ||||
-rw-r--r-- | src/mongo/db/commands.cpp | 5 | ||||
-rw-r--r-- | src/mongo/db/commands/get_last_error.cpp | 4 | ||||
-rw-r--r-- | src/mongo/db/commands/index_filter_commands.cpp | 23 | ||||
-rw-r--r-- | src/mongo/db/commands/plan_cache_commands.cpp | 22 | ||||
-rw-r--r-- | src/mongo/db/commands_test.cpp | 77 | ||||
-rw-r--r-- | src/mongo/db/lasterror.cpp | 4 | ||||
-rw-r--r-- | src/mongo/dbtests/gle_test.cpp | 8 | ||||
-rw-r--r-- | src/mongo/rpc/reply_builder_interface.cpp | 2 | ||||
-rw-r--r-- | src/mongo/rpc/reply_builder_interface.h | 4 | ||||
-rw-r--r-- | src/mongo/rpc/reply_builder_test.cpp | 42 | ||||
-rw-r--r-- | src/mongo/rpc/write_concern_error_detail.cpp | 5 | ||||
-rw-r--r-- | src/mongo/s/commands/commands_public.cpp | 2 | ||||
-rw-r--r-- | src/mongo/s/write_ops/batched_command_response_test.cpp | 7 |
16 files changed, 178 insertions, 68 deletions
diff --git a/src/mongo/db/SConscript b/src/mongo/db/SConscript index 13152dcc249..8dad1e9d8a5 100644 --- a/src/mongo/db/SConscript +++ b/src/mongo/db/SConscript @@ -554,6 +554,20 @@ env.Library( ], ) +env.CppUnitTest( + target="commands_test", + source=[ + "commands_test.cpp", + ], + LIBDEPS=[ + "commands", + "commands_test_crutch", + "service_context_noop_init", + "auth/authorization_manager_mock_init", + "repl/replmocks", + ], +) + # TODO: This library should probably be folded into catalog/catalog, # with which it is currently circular. env.Library( diff --git a/src/mongo/db/auth/sasl_commands.cpp b/src/mongo/db/auth/sasl_commands.cpp index 37cc1984923..ccf26f11643 100644 --- a/src/mongo/db/auth/sasl_commands.cpp +++ b/src/mongo/db/auth/sasl_commands.cpp @@ -164,14 +164,6 @@ Status extractMechanism(const BSONObj& cmdObj, std::string* mechanism) { return bsonExtractStringField(cmdObj, saslCommandMechanismFieldName, mechanism); } -void addStatus(const Status& status, BSONObjBuilder* builder) { - builder->append("ok", status.isOK() ? 1.0 : 0.0); - if (!status.isOK()) - builder->append(saslCommandCodeFieldName, status.code()); - if (!status.reason().empty()) - builder->append(saslCommandErrmsgFieldName, status.reason()); -} - Status doSaslStep(const Client* client, SaslAuthenticationSession* session, const BSONObj& cmdObj, @@ -301,7 +293,7 @@ bool CmdSaslStart::run(OperationContext* txn, session->setOpCtxt(txn); Status status = doSaslStart(client, session, db, cmdObj, &result); - addStatus(status, &result); + appendCommandStatus(result, status); if (session->isDone()) { audit::logAuthentication(client, @@ -332,8 +324,8 @@ bool CmdSaslContinue::run(OperationContext* txn, AuthenticationSession::swap(client, sessionGuard); if (!sessionGuard || sessionGuard->getType() != AuthenticationSession::SESSION_TYPE_SASL) { - addStatus(Status(ErrorCodes::ProtocolError, "No SASL session state found"), &result); - return false; + return appendCommandStatus( + result, Status(ErrorCodes::ProtocolError, "No SASL session state found")); } SaslAuthenticationSession* session = @@ -342,16 +334,16 @@ bool CmdSaslContinue::run(OperationContext* txn, // Authenticating the __system@local user to the admin database on mongos is required // by the auth passthrough test suite. if (session->getAuthenticationDatabase() != db && !Command::testCommandsEnabled) { - addStatus(Status(ErrorCodes::ProtocolError, - "Attempt to switch database target during SASL authentication."), - &result); - return false; + return appendCommandStatus( + result, + Status(ErrorCodes::ProtocolError, + "Attempt to switch database target during SASL authentication.")); } session->setOpCtxt(txn); Status status = doSaslContinue(client, session, cmdObj, &result); - addStatus(status, &result); + appendCommandStatus(result, status); if (session->isDone()) { audit::logAuthentication(client, diff --git a/src/mongo/db/catalog/apply_ops.cpp b/src/mongo/db/catalog/apply_ops.cpp index 28be87cf6de..5b783fff7f4 100644 --- a/src/mongo/db/catalog/apply_ops.cpp +++ b/src/mongo/db/catalog/apply_ops.cpp @@ -170,6 +170,8 @@ Status _applyOps(OperationContext* txn, ab.append(false); result->append("applied", ++(*numApplied)); result->append("code", ex.getCode()); + result->append("codeName", + ErrorCodes::errorString(ErrorCodes::fromInt(ex.getCode()))); result->append("errmsg", ex.what()); result->append("results", ab.arr()); return Status(ErrorCodes::UnknownError, ""); @@ -334,6 +336,7 @@ Status applyOps(OperationContext* txn, ab.append(false); result->append("applied", numApplied); result->append("code", ex.getCode()); + result->append("codeName", ErrorCodes::errorString(ErrorCodes::fromInt(ex.getCode()))); result->append("errmsg", ex.what()); result->append("results", ab.arr()); return Status(ErrorCodes::UnknownError, ""); diff --git a/src/mongo/db/commands.cpp b/src/mongo/db/commands.cpp index 70989b0db9f..bb7bee083ec 100644 --- a/src/mongo/db/commands.cpp +++ b/src/mongo/db/commands.cpp @@ -173,6 +173,7 @@ bool Command::appendCommandStatus(BSONObjBuilder& result, const Status& status) BSONObj tmp = result.asTempObj(); if (!status.isOK() && !tmp.hasField("code")) { result.append("code", status.code()); + result.append("codeName", ErrorCodes::errorString(status.code())); } return status.isOK(); } @@ -180,12 +181,12 @@ bool Command::appendCommandStatus(BSONObjBuilder& result, const Status& status) void Command::appendCommandStatus(BSONObjBuilder& result, bool ok, const std::string& errmsg) { BSONObj tmp = result.asTempObj(); bool have_ok = tmp.hasField("ok"); - bool have_errmsg = tmp.hasField("errmsg"); + bool need_errmsg = !ok && !tmp.hasField("errmsg"); if (!have_ok) result.append("ok", ok ? 1.0 : 0.0); - if (!ok && !have_errmsg) { + if (need_errmsg) { result.append("errmsg", errmsg); } } diff --git a/src/mongo/db/commands/get_last_error.cpp b/src/mongo/db/commands/get_last_error.cpp index 899d081926d..e71dcf15210 100644 --- a/src/mongo/db/commands/get_last_error.cpp +++ b/src/mongo/db/commands/get_last_error.cpp @@ -250,6 +250,7 @@ public: if (electionId != OID()) { errmsg = "wElectionId passed but no replication active"; result.append("code", ErrorCodes::BadValue); + result.append("codeName", ErrorCodes::errorString(ErrorCodes::BadValue)); return false; } } else { @@ -258,6 +259,8 @@ public: << repl::getGlobalReplicationCoordinator()->getElectionId(); errmsg = "election occurred after write"; result.append("code", ErrorCodes::WriteConcernFailed); + result.append("codeName", + ErrorCodes::errorString(ErrorCodes::WriteConcernFailed)); return false; } } @@ -278,6 +281,7 @@ public: dassert(!status.isOK()); result.append("errmsg", "timed out waiting for slaves"); result.append("code", status.code()); + result.append("codeName", ErrorCodes::errorString(status.code())); return true; } diff --git a/src/mongo/db/commands/index_filter_commands.cpp b/src/mongo/db/commands/index_filter_commands.cpp index 04a826b28d3..5a8b98f4390 100644 --- a/src/mongo/db/commands/index_filter_commands.cpp +++ b/src/mongo/db/commands/index_filter_commands.cpp @@ -58,20 +58,6 @@ using std::vector; using namespace mongo; /** - * Utility function to extract error code and message from status - * and append to BSON results. - */ -void addStatus(const Status& status, BSONObjBuilder& builder) { - builder.append("ok", status.isOK() ? 1.0 : 0.0); - if (!status.isOK()) { - builder.append("code", status.code()); - } - if (!status.reason().empty()) { - builder.append("errmsg", status.reason()); - } -} - -/** * Retrieves a collection's query settings and plan cache from the database. */ static Status getQuerySettingsAndPlanCache(OperationContext* txn, @@ -135,15 +121,8 @@ bool IndexFilterCommand::run(OperationContext* txn, string& errmsg, BSONObjBuilder& result) { string ns = parseNs(dbname, cmdObj); - Status status = runIndexFilterCommand(txn, ns, cmdObj, &result); - - if (!status.isOK()) { - addStatus(status, result); - return false; - } - - return true; + return appendCommandStatus(result, status); } diff --git a/src/mongo/db/commands/plan_cache_commands.cpp b/src/mongo/db/commands/plan_cache_commands.cpp index dd51739de13..0fb1a5b1d57 100644 --- a/src/mongo/db/commands/plan_cache_commands.cpp +++ b/src/mongo/db/commands/plan_cache_commands.cpp @@ -53,19 +53,6 @@ using std::string; using std::unique_ptr; using namespace mongo; -/** - * Utility function to extract error code and message from status - * and append to BSON results. - */ -void addStatus(const Status& status, BSONObjBuilder& builder) { - builder.append("ok", status.isOK() ? 1.0 : 0.0); - if (!status.isOK()) { - builder.append("code", status.code()); - } - if (!status.reason().empty()) { - builder.append("errmsg", status.reason()); - } -} /** * Retrieves a collection's plan cache from the database. @@ -129,15 +116,8 @@ bool PlanCacheCommand::run(OperationContext* txn, string& errmsg, BSONObjBuilder& result) { string ns = parseNs(dbname, cmdObj); - Status status = runPlanCacheCommand(txn, ns, cmdObj, &result); - - if (!status.isOK()) { - addStatus(status, result); - return false; - } - - return true; + return appendCommandStatus(result, status); } diff --git a/src/mongo/db/commands_test.cpp b/src/mongo/db/commands_test.cpp new file mode 100644 index 00000000000..5b910cc308f --- /dev/null +++ b/src/mongo/db/commands_test.cpp @@ -0,0 +1,77 @@ +/** + * Copyright (C) 2016 MongoDB Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + * + * As a special exception, the copyright holders give permission to link the + * code of portions of this program with the OpenSSL library under certain + * conditions as described in each individual source file and distribute + * linked combinations including the program with the OpenSSL library. You + * must comply with the GNU Affero General Public License in all respects + * for all of the code used other than as permitted herein. If you modify + * file(s) with this exception, you may extend this exception to your + * version of the file(s), but you are not obligated to do so. If you do not + * wish to do so, delete this exception statement from your version. If you + * delete this exception statement from all source files in the program, + * then also delete it in the license file. + */ + +#include "mongo/db/commands.h" +#include "mongo/platform/basic.h" +#include "mongo/unittest/unittest.h" + +namespace mongo { + +TEST(Commands, appendCommandStatusOK) { + BSONObjBuilder actualResult; + Command::appendCommandStatus(actualResult, Status::OK()); + + BSONObjBuilder expectedResult; + expectedResult.append("ok", 1.0); + + ASSERT_BSONOBJ_EQ(actualResult.obj(), expectedResult.obj()); +} + +TEST(Commands, appendCommandStatusError) { + BSONObjBuilder actualResult; + const Status status(ErrorCodes::InvalidLength, "Response payload too long"); + Command::appendCommandStatus(actualResult, status); + + BSONObjBuilder expectedResult; + expectedResult.append("ok", 0.0); + expectedResult.append("errmsg", status.reason()); + expectedResult.append("code", status.code()); + expectedResult.append("codeName", ErrorCodes::errorString(status.code())); + + ASSERT_BSONOBJ_EQ(actualResult.obj(), expectedResult.obj()); +} + +TEST(Commands, appendCommandStatusNoOverwrite) { + BSONObjBuilder actualResult; + actualResult.append("a", "b"); + actualResult.append("c", "d"); + actualResult.append("ok", "not ok"); + const Status status(ErrorCodes::InvalidLength, "Response payload too long"); + Command::appendCommandStatus(actualResult, status); + + BSONObjBuilder expectedResult; + expectedResult.append("a", "b"); + expectedResult.append("c", "d"); + expectedResult.append("ok", "not ok"); + expectedResult.append("errmsg", status.reason()); + expectedResult.append("code", status.code()); + expectedResult.append("codeName", ErrorCodes::errorString(status.code())); + + ASSERT_BSONOBJ_EQ(actualResult.obj(), expectedResult.obj()); +} +} // namespace mongo diff --git a/src/mongo/db/lasterror.cpp b/src/mongo/db/lasterror.cpp index 60d0e59acc3..8789609e2cd 100644 --- a/src/mongo/db/lasterror.cpp +++ b/src/mongo/db/lasterror.cpp @@ -83,8 +83,10 @@ bool LastError::appendSelf(BSONObjBuilder& b, bool blankErr) const { b.append("err", _msg); } - if (_code) + if (_code) { b.append("code", _code); + b.append("codeName", ErrorCodes::errorString(ErrorCodes::fromInt(_code))); + } if (_updatedExisting != NotUpdate) b.appendBool("updatedExisting", _updatedExisting == True); if (!_upsertedId.isEmpty()) { diff --git a/src/mongo/dbtests/gle_test.cpp b/src/mongo/dbtests/gle_test.cpp index 18c721a404f..026933c3d22 100644 --- a/src/mongo/dbtests/gle_test.cpp +++ b/src/mongo/dbtests/gle_test.cpp @@ -103,8 +103,12 @@ public: // insert dup client.insert(_ns, BSON("_id" << 1)); // Make sure there was an error - gleString = client.getLastError(); - ASSERT_NOT_EQUALS(gleString, ""); + + BSONObj info = client.getLastErrorDetailed(); + ASSERT_NOT_EQUALS(info["err"].String(), ""); + ASSERT_EQUALS(info["ok"].Double(), 1.0); + ASSERT_EQUALS(info["code"].Int(), 11000); + ASSERT_EQUALS(info["codeName"].String(), "DuplicateKey"); } }; diff --git a/src/mongo/rpc/reply_builder_interface.cpp b/src/mongo/rpc/reply_builder_interface.cpp index 63e147aab05..34a99cd4176 100644 --- a/src/mongo/rpc/reply_builder_interface.cpp +++ b/src/mongo/rpc/reply_builder_interface.cpp @@ -43,6 +43,7 @@ namespace rpc { namespace { const char kOKField[] = "ok"; const char kCodeField[] = "code"; +const char kCodeNameField[] = "codeName"; const char kErrorField[] = "errmsg"; // similar to appendCommandStatus (duplicating logic here to avoid cyclic library @@ -68,6 +69,7 @@ BSONObj augmentReplyWithStatus(const Status& status, const BSONObj& reply) { if (!reply.hasField(kCodeField)) { bob.append(kCodeField, status.code()); + bob.append(kCodeNameField, ErrorCodes::errorString(status.code())); } return bob.obj(); diff --git a/src/mongo/rpc/reply_builder_interface.h b/src/mongo/rpc/reply_builder_interface.h index 1dc021faa49..2cb72066d8c 100644 --- a/src/mongo/rpc/reply_builder_interface.h +++ b/src/mongo/rpc/reply_builder_interface.h @@ -79,6 +79,7 @@ public: * reply will be set to the contained BSONObj, augmented with the element {ok, 1.0} if it * does not already have an "ok" field. If a disengaged StatusWith<BSONObj> is passed, the * command reply will be set to {ok: 0.0, code: <code of status>, + * codeName: <name of status code>, * errmsg: <reason of status>} */ ReplyBuilderInterface& setCommandReply(StatusWith<BSONObj> commandReply); @@ -86,7 +87,8 @@ public: /** * Sets the reply for this command. The status parameter must be non-OK. The reply for * this command will be set to an object containing all the fields in extraErrorInfo, - * augmented with {ok: 0.0} , {code: <code of status>}, and {errmsg: <reason of status>}. + * augmented with {ok: 0.0} , {code: <code of status>}, {codeName: <name of status code>}, + * and {errmsg: <reason of status>}. * If any of the fields "ok", "code", or "errmsg" already exist in extraErrorInfo, they * will be left as-is in the command reply. This use of this form is intended for * interfacing with legacy code that adds additional data to a failed command reply and diff --git a/src/mongo/rpc/reply_builder_test.cpp b/src/mongo/rpc/reply_builder_test.cpp index c494c080ec9..4aa6a9758e0 100644 --- a/src/mongo/rpc/reply_builder_test.cpp +++ b/src/mongo/rpc/reply_builder_test.cpp @@ -86,6 +86,48 @@ BSONObj buildCommand() { return commandReplyBob.obj(); } +BSONObj buildErrReply(const Status status, const BSONObj& extraInfo = {}) { + BSONObjBuilder bob; + bob.appendElements(extraInfo); + bob.append("ok", 0.0); + bob.append("errmsg", status.reason()); + bob.append("code", status.code()); + bob.append("codeName", ErrorCodes::errorString(status.code())); + return bob.obj(); +} + +TEST(CommandReplyBuilder, CommandError) { + const Status status(ErrorCodes::InvalidLength, "Response payload too long"); + BSONObj metadata = buildMetadata(); + rpc::CommandReplyBuilder replyBuilder; + replyBuilder.setCommandReply(status); + replyBuilder.setMetadata(metadata); + auto msg = replyBuilder.done(); + + rpc::CommandReply parsed(&msg); + + ASSERT_BSONOBJ_EQ(parsed.getMetadata(), metadata); + ASSERT_BSONOBJ_EQ(parsed.getCommandReply(), buildErrReply(status)); +} + +TEST(LegacyReplyBuilder, CommandError) { + const Status status(ErrorCodes::InvalidLength, "Response payload too long"); + BSONObj metadata = buildMetadata(); + BSONObjBuilder extra; + extra.append("a", "b"); + extra.append("c", "d"); + const BSONObj extraObj = extra.obj(); + rpc::LegacyReplyBuilder replyBuilder; + replyBuilder.setCommandReply(status, extraObj); + replyBuilder.setMetadata(metadata); + auto msg = replyBuilder.done(); + + rpc::LegacyReply parsed(&msg); + + ASSERT_BSONOBJ_EQ(parsed.getMetadata(), metadata); + ASSERT_BSONOBJ_EQ(parsed.getCommandReply(), buildErrReply(status, extraObj)); +} + TEST(CommandReplyBuilder, MemAccess) { BSONObj metadata = buildMetadata(); BSONObj commandReply = buildCommand(); diff --git a/src/mongo/rpc/write_concern_error_detail.cpp b/src/mongo/rpc/write_concern_error_detail.cpp index fcad013be5e..1b63898daeb 100644 --- a/src/mongo/rpc/write_concern_error_detail.cpp +++ b/src/mongo/rpc/write_concern_error_detail.cpp @@ -40,6 +40,7 @@ using std::string; namespace { const BSONField<int> errCode("code"); +const BSONField<string> errCodeName("codeName"); const BSONField<BSONObj> errInfo("errInfo"); const BSONField<string> errMessage("errmsg"); @@ -67,8 +68,10 @@ bool WriteConcernErrorDetail::isValid(string* errMsg) const { BSONObj WriteConcernErrorDetail::toBSON() const { BSONObjBuilder builder; - if (_isErrCodeSet) + if (_isErrCodeSet) { builder.append(errCode(), _errCode); + builder.append(errCodeName(), ErrorCodes::errorString(ErrorCodes::fromInt(_errCode))); + } if (_isErrInfoSet) builder.append(errInfo(), _errInfo); diff --git a/src/mongo/s/commands/commands_public.cpp b/src/mongo/s/commands/commands_public.cpp index 67d2ba99338..a32ac51449d 100644 --- a/src/mongo/s/commands/commands_public.cpp +++ b/src/mongo/s/commands/commands_public.cpp @@ -327,6 +327,7 @@ public: BSONObjBuilder b; b.append("errmsg", e.toString()); b.append("code", e.getCode()); + b.append("codeName", ErrorCodes::errorString(ErrorCodes::fromInt(e.getCode()))); return b.obj(); } } @@ -494,6 +495,7 @@ public: int code = getUniqueCodeFromCommandResults(results); if (code != 0) { output.append("code", code); + output.append("codeName", ErrorCodes::errorString(ErrorCodes::fromInt(code))); } if (errored) { diff --git a/src/mongo/s/write_ops/batched_command_response_test.cpp b/src/mongo/s/write_ops/batched_command_response_test.cpp index 400962b3006..915ba258c30 100644 --- a/src/mongo/s/write_ops/batched_command_response_test.cpp +++ b/src/mongo/s/write_ops/batched_command_response_test.cpp @@ -50,8 +50,11 @@ TEST(BatchedCommandResponse, Basic) { << WriteErrorDetail::errInfo(BSON("more info" << 1)) << WriteErrorDetail::errMessage("index 1 failed too"))); - BSONObj writeConcernError(BSON("code" << 8 << "errInfo" << BSON("a" << 1) << "errmsg" - << "norepl")); + BSONObj writeConcernError(BSON( + "code" << 8 << "codeName" << ErrorCodes::errorString(ErrorCodes::fromInt(8)) << "errInfo" + << BSON("a" << 1) + << "errmsg" + << "norepl")); BSONObj origResponseObj = BSON(BatchedCommandResponse::ok(false) << BatchedCommandResponse::errCode(-1) |