diff options
author | Moustafa Maher <m.maher@10gen.com> | 2021-01-11 14:18:54 +0000 |
---|---|---|
committer | Evergreen Agent <no-reply@evergreen.mongodb.com> | 2021-01-14 05:29:35 +0000 |
commit | fd1c62d46e4305298fffc02655428808acf266cb (patch) | |
tree | 6080b077325c34ee6c55c7d9983fd95c1887614e | |
parent | a004d61558d3b0cbe123b07cd475f45a084bff89 (diff) | |
download | mongo-fd1c62d46e4305298fffc02655428808acf266cb.tar.gz |
SERVER-52547 Update Txn cmds to to inherit from IDL-generated base classes
-rw-r--r-- | etc/backports_required_for_multiversion_tests.yml | 2 | ||||
-rw-r--r-- | jstests/core/txns/errors_on_committed_transaction.js | 2 | ||||
-rw-r--r-- | jstests/core/txns/no_writes_to_config_transactions_with_prepared_transaction.js | 2 | ||||
-rw-r--r-- | src/mongo/db/commands.h | 60 | ||||
-rw-r--r-- | src/mongo/db/commands/txn_cmds.cpp | 278 | ||||
-rw-r--r-- | src/mongo/db/commands/txn_cmds.idl | 8 | ||||
-rw-r--r-- | src/mongo/s/commands/cluster_abort_transaction_cmd.cpp | 29 | ||||
-rw-r--r-- | src/mongo/s/commands/cluster_commit_transaction_cmd.cpp | 29 |
8 files changed, 258 insertions, 152 deletions
diff --git a/etc/backports_required_for_multiversion_tests.yml b/etc/backports_required_for_multiversion_tests.yml index c243835256b..6d9d70eef66 100644 --- a/etc/backports_required_for_multiversion_tests.yml +++ b/etc/backports_required_for_multiversion_tests.yml @@ -114,6 +114,8 @@ all: test_file: jstests/sharding/change_stream_empty_apply_ops.js - ticket: SERVER-52953 test_file: jstests/core/geo_near_point_query.js + - ticket: SERVER-52547 + test_file: jstests/core/txns/errors_on_committed_transaction.js # Tests that should only be excluded from particular suites should be listed under that suite. suites: diff --git a/jstests/core/txns/errors_on_committed_transaction.js b/jstests/core/txns/errors_on_committed_transaction.js index 2734f7fa11a..affe21a5115 100644 --- a/jstests/core/txns/errors_on_committed_transaction.js +++ b/jstests/core/txns/errors_on_committed_transaction.js @@ -68,7 +68,7 @@ jsTestLog("Test that calling abort with invalid fields on a committed transactio assert.commandFailedWithCode( sessionDB.adminCommand( {abortTransaction: 1, invalidField: 1, txnNumber: txnNumber, autocommit: false}), - ErrorCodes.TransactionCommitted); + 40415 /* IDL unknown field error */); session.endSession(); }()); diff --git a/jstests/core/txns/no_writes_to_config_transactions_with_prepared_transaction.js b/jstests/core/txns/no_writes_to_config_transactions_with_prepared_transaction.js index 4302a131c36..210e898f3f9 100644 --- a/jstests/core/txns/no_writes_to_config_transactions_with_prepared_transaction.js +++ b/jstests/core/txns/no_writes_to_config_transactions_with_prepared_transaction.js @@ -93,7 +93,7 @@ assert.commandWorked(sessionColl2.insert({_id: 1})); PrepareHelpers.prepareTransaction(session2); assert.commandWorked(sessionDB.adminCommand( - {abortTransaction: 1, txnNumber: NumberLong(0), stmtid: NumberInt(2), autocommit: false})); + {abortTransaction: 1, txnNumber: NumberLong(0), stmtId: NumberInt(2), autocommit: false})); session.endSession(); assert.commandWorked(session2.abortTransaction_forTesting()); diff --git a/src/mongo/db/commands.h b/src/mongo/db/commands.h index dfb4cdba633..dc1a53f2e8f 100644 --- a/src/mongo/db/commands.h +++ b/src/mongo/db/commands.h @@ -923,13 +923,69 @@ public: bool runWithReplyBuilder(OperationContext* opCtx, const std::string& db, const BSONObj& cmdObj, - rpc::ReplyBuilderInterface* replyBuilder) final { + rpc::ReplyBuilderInterface* replyBuilder) override { auto result = replyBuilder->getBodyBuilder(); return run(opCtx, db, cmdObj, result); } }; /** + * A CRTP base class for BasicCommandWithRequestParser, which simplifies writing commands that + * accept requests generated by IDL to enforce API versioning and to overcome the complexity + * to standardize the output generated by commands using IDL. + * + * Derive from it as follows: + * + * class MyCommand : public BasicCommandWithRequestParser<MyCommand> {...}; + * + * The 'Derived' type parameter must have: + * + * - 'Request' naming a usable request type. + * A usable Request type must have: + * + * - a static member factory function 'parse', callable as: + * + * const IDLParserErrorContext& idlCtx = ...; + * const OpMsgRequest& opMsgRequest = ...; + * Request r = Request::parse(idlCtx, opMsgRequest); + * + * which enables it to be parsed as an IDL command. + * + * - a 'constexpr StringData kCommandName' member. + * + * - validateResult: that has a custom logic to validate the result BSON object + * to enforce API versioning. + * + */ +template <typename Derived> +class BasicCommandWithRequestParser : public BasicCommand { +public: + // Commands that only have a single name don't need to define any constructors. + BasicCommandWithRequestParser() : BasicCommand(Derived::Request::kCommandName) {} + + bool runWithReplyBuilder(OperationContext* opCtx, + const std::string& db, + const BSONObj& cmdObj, + rpc::ReplyBuilderInterface* replyBuilder) final { + auto result = replyBuilder->getBodyBuilder(); + + // To enforce API versioning + using RequestType = typename Derived::Request; + auto request = RequestType::parse( + IDLParserErrorContext(Derived::Request::kCommandName, + APIParameters::get(opCtx).getAPIStrict().value_or(false)), + cmdObj); + + auto cmdDone = run(opCtx, db, request.toBSON(cmdObj), result); + validateResult(result.asTempObj()); + return cmdDone; + } + + // Custom logic to validate results to enforce API versioning. + virtual void validateResult(const BSONObj& resultObj) = 0; +}; + +/** * Deprecated. Do not add new subclasses. */ class ErrmsgCommandDeprecated : public BasicCommand { @@ -952,7 +1008,7 @@ class ErrmsgCommandDeprecated : public BasicCommand { * * class MyCommand : public TypedCommand<MyCommand> {...}; * - * The 'Derived' type paramter must have: + * The 'Derived' type parameter must have: * * - 'Request' naming a usable request type. * A usable Request type must have: diff --git a/src/mongo/db/commands/txn_cmds.cpp b/src/mongo/db/commands/txn_cmds.cpp index 49ce490a353..6ac83c5166b 100644 --- a/src/mongo/db/commands/txn_cmds.cpp +++ b/src/mongo/db/commands/txn_cmds.cpp @@ -57,106 +57,100 @@ MONGO_FAIL_POINT_DEFINE(hangBeforeAbortingTxn); // on stale version and snapshot errors. MONGO_FAIL_POINT_DEFINE(dontRemoveTxnCoordinatorOnAbort); -class CmdCommitTxn : public BasicCommand { +class CmdCommitTxn final : public CommitTransactionCmdVersion1Gen<CmdCommitTxn> { public: - CmdCommitTxn() : BasicCommand("commitTransaction") {} + CmdCommitTxn() = default; - const std::set<std::string>& apiVersions() const { - return kApiVersions1; - } - - AllowedOnSecondary secondaryAllowed(ServiceContext*) const override { + AllowedOnSecondary secondaryAllowed(ServiceContext*) const final { return AllowedOnSecondary::kNever; } - virtual bool adminOnly() const { + bool adminOnly() const final { return true; } - bool supportsWriteConcern(const BSONObj& cmd) const override { + bool collectsResourceConsumptionMetrics() const final { return true; } - bool collectsResourceConsumptionMetrics() const override { - return true; - } - - std::string help() const override { + std::string help() const final { return "Commits a transaction"; } + class Invocation final : public InvocationBaseGen { + public: + using InvocationBaseGen::InvocationBaseGen; - Status checkAuthForOperation(OperationContext* opCtx, - const std::string& dbname, - const BSONObj& cmdObj) const override { - return Status::OK(); - } - - bool run(OperationContext* opCtx, - const std::string& dbname, - const BSONObj& cmdObj, - BSONObjBuilder& result) override { - IDLParserErrorContext ctx("commitTransaction"); - auto cmd = CommitTransaction::parse(ctx, cmdObj); - - auto txnParticipant = TransactionParticipant::get(opCtx); - uassert(ErrorCodes::CommandFailed, - "commitTransaction must be run within a transaction", - txnParticipant); - - LOGV2_DEBUG(20507, - 3, - "Received commitTransaction for transaction with txnNumber " - "{txnNumber} on session {sessionId}", - "Received commitTransaction", - "txnNumber"_attr = opCtx->getTxnNumber(), - "sessionId"_attr = opCtx->getLogicalSessionId()->toBSON()); - - // commitTransaction is retryable. - if (txnParticipant.transactionIsCommitted()) { - // We set the client last op to the last optime observed by the system to ensure that - // we wait for the specified write concern on an optime greater than or equal to the - // commit oplog entry. - auto& replClient = repl::ReplClientInfo::forClient(opCtx->getClient()); - replClient.setLastOpToSystemLastOpTime(opCtx); - if (MONGO_unlikely( - participantReturnNetworkErrorForCommitAfterExecutingCommitLogic.shouldFail())) { - uasserted(ErrorCodes::HostUnreachable, - "returning network error because failpoint is on"); - } - + bool supportsWriteConcern() const final { return true; } - uassert(ErrorCodes::NoSuchTransaction, - "Transaction isn't in progress", - txnParticipant.transactionIsOpen()); + NamespaceString ns() const final { + return NamespaceString(request().getDbName()); + } - CurOpFailpointHelpers::waitWhileFailPointEnabled( - &hangBeforeCommitingTxn, opCtx, "hangBeforeCommitingTxn"); + void doCheckAuthorization(OperationContext* opCtx) const final {} + + Reply typedRun(OperationContext* opCtx) final { + auto txnParticipant = TransactionParticipant::get(opCtx); + uassert(ErrorCodes::CommandFailed, + "commitTransaction must be run within a transaction", + txnParticipant); + + LOGV2_DEBUG(20507, + 3, + "Received commitTransaction for transaction with txnNumber " + "{txnNumber} on session {sessionId}", + "Received commitTransaction", + "txnNumber"_attr = opCtx->getTxnNumber(), + "sessionId"_attr = opCtx->getLogicalSessionId()->toBSON()); + + // commitTransaction is retryable. + if (txnParticipant.transactionIsCommitted()) { + // We set the client last op to the last optime observed by the system to ensure + // that we wait for the specified write concern on an optime greater than or equal + // to the commit oplog entry. + auto& replClient = repl::ReplClientInfo::forClient(opCtx->getClient()); + replClient.setLastOpToSystemLastOpTime(opCtx); + if (MONGO_unlikely(participantReturnNetworkErrorForCommitAfterExecutingCommitLogic + .shouldFail())) { + uasserted(ErrorCodes::HostUnreachable, + "returning network error because failpoint is on"); + } + + return Reply(); + } - auto optionalCommitTimestamp = cmd.getCommitTimestamp(); - if (optionalCommitTimestamp) { - // commitPreparedTransaction will throw if the transaction is not prepared. - txnParticipant.commitPreparedTransaction(opCtx, optionalCommitTimestamp.get(), {}); - } else { - if (ShardingState::get(opCtx)->canAcceptShardedCommands().isOK() || - serverGlobalParams.clusterRole == ClusterRole::ConfigServer) { - TransactionCoordinatorService::get(opCtx)->cancelIfCommitNotYetStarted( - opCtx, *opCtx->getLogicalSessionId(), *opCtx->getTxnNumber()); + uassert(ErrorCodes::NoSuchTransaction, + "Transaction isn't in progress", + txnParticipant.transactionIsOpen()); + + CurOpFailpointHelpers::waitWhileFailPointEnabled( + &hangBeforeCommitingTxn, opCtx, "hangBeforeCommitingTxn"); + + auto optionalCommitTimestamp = request().getCommitTimestamp(); + if (optionalCommitTimestamp) { + // commitPreparedTransaction will throw if the transaction is not prepared. + txnParticipant.commitPreparedTransaction(opCtx, optionalCommitTimestamp.get(), {}); + } else { + if (ShardingState::get(opCtx)->canAcceptShardedCommands().isOK() || + serverGlobalParams.clusterRole == ClusterRole::ConfigServer) { + TransactionCoordinatorService::get(opCtx)->cancelIfCommitNotYetStarted( + opCtx, *opCtx->getLogicalSessionId(), *opCtx->getTxnNumber()); + } + + // commitUnpreparedTransaction will throw if the transaction is prepared. + txnParticipant.commitUnpreparedTransaction(opCtx); } - // commitUnpreparedTransaction will throw if the transaction is prepared. - txnParticipant.commitUnpreparedTransaction(opCtx); - } + if (MONGO_unlikely( + participantReturnNetworkErrorForCommitAfterExecutingCommitLogic.shouldFail())) { + uasserted(ErrorCodes::HostUnreachable, + "returning network error because failpoint is on"); + } - if (MONGO_unlikely( - participantReturnNetworkErrorForCommitAfterExecutingCommitLogic.shouldFail())) { - uasserted(ErrorCodes::HostUnreachable, - "returning network error because failpoint is on"); + return Reply(); } - - return true; - } + }; } commitTxn; @@ -165,94 +159,90 @@ static const Status kOnlyTransactionsReadConcernsSupported{ static const Status kDefaultReadConcernNotPermitted{ErrorCodes::InvalidOptions, "default read concern not permitted"}; -class CmdAbortTxn : public BasicCommand { +class CmdAbortTxn final : public AbortTransactionCmdVersion1Gen<CmdAbortTxn> { public: - CmdAbortTxn() : BasicCommand("abortTransaction") {} + CmdAbortTxn() = default; - virtual const std::set<std::string>& apiVersions() const { - return kApiVersions1; - }; - - AllowedOnSecondary secondaryAllowed(ServiceContext*) const override { + AllowedOnSecondary secondaryAllowed(ServiceContext*) const final { return AllowedOnSecondary::kNever; } - virtual bool adminOnly() const { - return true; - } - - bool supportsWriteConcern(const BSONObj& cmd) const override { + bool adminOnly() const final { return true; } - bool collectsResourceConsumptionMetrics() const override { + bool collectsResourceConsumptionMetrics() const final { return true; } - ReadConcernSupportResult supportsReadConcern(const BSONObj& cmdObj, - repl::ReadConcernLevel level) const override { - // abortTransaction commences running inside a transaction (even though the transaction will - // be ended by the time it completes). Therefore it needs to accept any readConcern which - // is valid within a transaction. However it is not appropriate to apply the default - // readConcern, since the readConcern of the transaction (set by the first operation) is - // what must apply. - return {{!isReadConcernLevelAllowedInTransaction(level), - kOnlyTransactionsReadConcernsSupported}, - {kDefaultReadConcernNotPermitted}}; - } - - std::string help() const override { + std::string help() const final { return "Aborts a transaction"; } - Status checkAuthForOperation(OperationContext* opCtx, - const std::string& dbname, - const BSONObj& cmdObj) const override { - return Status::OK(); - } + class Invocation final : public InvocationBaseGen { + public: + using InvocationBaseGen::InvocationBaseGen; - bool run(OperationContext* opCtx, - const std::string& dbname, - const BSONObj& cmdObj, - BSONObjBuilder& result) override { - auto txnParticipant = TransactionParticipant::get(opCtx); - uassert(ErrorCodes::CommandFailed, - "abortTransaction must be run within a transaction", - txnParticipant); - - LOGV2_DEBUG(20508, - 3, - "Received abortTransaction for transaction with txnNumber {txnNumber} " - "on session {sessionId}", - "Received abortTransaction", - "txnNumber"_attr = opCtx->getTxnNumber(), - "sessionId"_attr = opCtx->getLogicalSessionId()->toBSON()); - - uassert(ErrorCodes::NoSuchTransaction, - "Transaction isn't in progress", - txnParticipant.transactionIsOpen()); - - CurOpFailpointHelpers::waitWhileFailPointEnabled( - &hangBeforeAbortingTxn, opCtx, "hangBeforeAbortingTxn"); - - if (!MONGO_unlikely(dontRemoveTxnCoordinatorOnAbort.shouldFail()) && - (ShardingState::get(opCtx)->canAcceptShardedCommands().isOK() || - serverGlobalParams.clusterRole == ClusterRole::ConfigServer)) { - TransactionCoordinatorService::get(opCtx)->cancelIfCommitNotYetStarted( - opCtx, *opCtx->getLogicalSessionId(), *opCtx->getTxnNumber()); + bool supportsWriteConcern() const final { + return true; } - txnParticipant.abortTransaction(opCtx); + ReadConcernSupportResult supportsReadConcern(repl::ReadConcernLevel level) const final { + // abortTransaction commences running inside a transaction (even though the transaction + // will be ended by the time it completes). Therefore it needs to accept any + // readConcern which is valid within a transaction. However it is not appropriate to + // apply the default readConcern, since the readConcern of the transaction (set by the + // first operation) is what must apply. + return {{!isReadConcernLevelAllowedInTransaction(level), + kOnlyTransactionsReadConcernsSupported}, + {kDefaultReadConcernNotPermitted}}; + } - if (MONGO_unlikely( - participantReturnNetworkErrorForAbortAfterExecutingAbortLogic.shouldFail())) { - uasserted(ErrorCodes::HostUnreachable, - "returning network error because failpoint is on"); + NamespaceString ns() const final { + return NamespaceString(request().getDbName()); } - return true; - } + void doCheckAuthorization(OperationContext* opCtx) const final {} + + Reply typedRun(OperationContext* opCtx) final { + auto txnParticipant = TransactionParticipant::get(opCtx); + uassert(ErrorCodes::CommandFailed, + "abortTransaction must be run within a transaction", + txnParticipant); + + LOGV2_DEBUG(20508, + 3, + "Received abortTransaction for transaction with txnNumber {txnNumber} " + "on session {sessionId}", + "Received abortTransaction", + "txnNumber"_attr = opCtx->getTxnNumber(), + "sessionId"_attr = opCtx->getLogicalSessionId()->toBSON()); + + uassert(ErrorCodes::NoSuchTransaction, + "Transaction isn't in progress", + txnParticipant.transactionIsOpen()); + CurOpFailpointHelpers::waitWhileFailPointEnabled( + &hangBeforeAbortingTxn, opCtx, "hangBeforeAbortingTxn"); + + if (!MONGO_unlikely(dontRemoveTxnCoordinatorOnAbort.shouldFail()) && + (ShardingState::get(opCtx)->canAcceptShardedCommands().isOK() || + serverGlobalParams.clusterRole == ClusterRole::ConfigServer)) { + TransactionCoordinatorService::get(opCtx)->cancelIfCommitNotYetStarted( + opCtx, *opCtx->getLogicalSessionId(), *opCtx->getTxnNumber()); + } + + txnParticipant.abortTransaction(opCtx); + + if (MONGO_unlikely( + participantReturnNetworkErrorForAbortAfterExecutingAbortLogic.shouldFail())) { + uasserted(ErrorCodes::HostUnreachable, + "returning network error because failpoint is on"); + } + + return Reply(); + } + }; } abortTxn; } // namespace diff --git a/src/mongo/db/commands/txn_cmds.idl b/src/mongo/db/commands/txn_cmds.idl index 7a1bf5783bb..125dcc4cd4d 100644 --- a/src/mongo/db/commands/txn_cmds.idl +++ b/src/mongo/db/commands/txn_cmds.idl @@ -82,6 +82,9 @@ commands: description: "commitTransaction Command" command_name: commitTransaction namespace: ignored + cpp_name: CommitTransaction + strict: true + api_version: "1" fields: commitTimestamp: description: "Timestamp at which to commit the transaction. Required for prepared @@ -93,8 +96,13 @@ commands: progress on commit by processing using the info in the recoveryToken." optional: true type: TxnRecoveryToken + reply_type: OkReply abortTransaction: description: "abortTransaction Command" command_name: abortTransaction namespace: ignored + cpp_name: AbortTransaction + strict: true + api_version: "1" + reply_type: OkReply
\ No newline at end of file diff --git a/src/mongo/s/commands/cluster_abort_transaction_cmd.cpp b/src/mongo/s/commands/cluster_abort_transaction_cmd.cpp index 9259f9c108e..4ff806bede4 100644 --- a/src/mongo/s/commands/cluster_abort_transaction_cmd.cpp +++ b/src/mongo/s/commands/cluster_abort_transaction_cmd.cpp @@ -33,6 +33,7 @@ #include "mongo/db/commands.h" #include "mongo/db/transaction_validation.h" +#include "mongo/rpc/get_status_from_command_result.h" #include "mongo/s/cluster_commands_helpers.h" #include "mongo/s/transaction_router.h" @@ -47,9 +48,33 @@ static const Status kDefaultReadConcernNotPermitted{ErrorCodes::InvalidOptions, /** * Implements the abortTransaction command on mongos. */ -class ClusterAbortTransactionCmd : public BasicCommand { +class ClusterAbortTransactionCmd + : public BasicCommandWithRequestParser<ClusterAbortTransactionCmd> { public: - ClusterAbortTransactionCmd() : BasicCommand("abortTransaction") {} + using BasicCommandWithRequestParser::BasicCommandWithRequestParser; + using Request = AbortTransaction; + using Reply = OkReply; + + void validateResult(const BSONObj& resultObj) final { + auto ctx = IDLParserErrorContext("AbortReply"); + auto status = getStatusFromCommandResult(resultObj); + auto wcStatus = getWriteConcernStatusFromCommandResult(resultObj); + + if (!wcStatus.isOK()) { + if (wcStatus.code() == ErrorCodes::TypeMismatch) { + // Result has "writeConcerError" field but it is not valid wce object. + uassertStatusOK(wcStatus); + } + } + + if (!status.isOK()) { + // Will throw if the result doesn't match the ErrorReply. + ErrorReply::parse(ctx, resultObj); + } else { + // Will throw if the result doesn't match the abortReply. + Reply::parse(ctx, resultObj); + } + } const std::set<std::string>& apiVersions() const { return kApiVersions1; diff --git a/src/mongo/s/commands/cluster_commit_transaction_cmd.cpp b/src/mongo/s/commands/cluster_commit_transaction_cmd.cpp index bbcfccd0dee..f393e22bdaf 100644 --- a/src/mongo/s/commands/cluster_commit_transaction_cmd.cpp +++ b/src/mongo/s/commands/cluster_commit_transaction_cmd.cpp @@ -33,6 +33,7 @@ #include "mongo/db/commands.h" #include "mongo/db/commands/txn_cmds_gen.h" +#include "mongo/rpc/get_status_from_command_result.h" #include "mongo/s/cluster_commands_helpers.h" #include "mongo/s/transaction_router.h" @@ -42,9 +43,33 @@ namespace { /** * Implements the commitTransaction command on mongos. */ -class ClusterCommitTransactionCmd : public BasicCommand { +class ClusterCommitTransactionCmd + : public BasicCommandWithRequestParser<ClusterCommitTransactionCmd> { public: - ClusterCommitTransactionCmd() : BasicCommand("commitTransaction") {} + using BasicCommandWithRequestParser::BasicCommandWithRequestParser; + using Request = CommitTransaction; + using Reply = OkReply; + + void validateResult(const BSONObj& resultObj) final { + auto ctx = IDLParserErrorContext("CommitReply"); + auto status = getStatusFromCommandResult(resultObj); + auto wcStatus = getWriteConcernStatusFromCommandResult(resultObj); + + if (!wcStatus.isOK()) { + if (wcStatus.code() == ErrorCodes::TypeMismatch) { + // Result has "writeConcerError" field but it is not valid wce object. + uassertStatusOK(wcStatus); + } + } + + if (!status.isOK()) { + // Will throw if the result doesn't match the ErrorReply. + ErrorReply::parse(ctx, resultObj); + } else { + // Will throw if the result doesn't match the committReply. + Reply::parse(ctx, resultObj); + } + } const std::set<std::string>& apiVersions() const { return kApiVersions1; |