diff options
author | A. Jesse Jiryu Davis <jesse@mongodb.com> | 2021-05-04 15:21:10 -0400 |
---|---|---|
committer | Evergreen Agent <no-reply@evergreen.mongodb.com> | 2021-05-04 21:50:07 +0000 |
commit | 4aa27885874b90e098c1225fccb10f4daa3b3d38 (patch) | |
tree | 6bc40bf2aaa6e275c1644114c2c80c7b09c7d257 | |
parent | 882fa82e21f412e6c5743e99d2d97861b28bd935 (diff) | |
download | mongo-4aa27885874b90e098c1225fccb10f4daa3b3d38.tar.gz |
SERVER-55614 Make API params optional for cmds in txns
-rw-r--r-- | jstests/core/txns/api_params_transaction.js | 88 | ||||
-rw-r--r-- | jstests/core/txns/transaction_continuing_cmds_refuse_api_params.js | 68 | ||||
-rw-r--r-- | jstests/noPassthrough/require_api_version.js | 25 | ||||
-rw-r--r-- | jstests/sharding/libs/mongos_api_params_util.js | 78 | ||||
-rw-r--r-- | src/mongo/base/error_codes.yml | 2 | ||||
-rw-r--r-- | src/mongo/db/api_parameters.cpp | 6 | ||||
-rw-r--r-- | src/mongo/db/api_parameters.h | 2 | ||||
-rw-r--r-- | src/mongo/db/initialize_api_parameters.cpp | 1 | ||||
-rw-r--r-- | src/mongo/db/s/transaction_coordinator.cpp | 6 | ||||
-rw-r--r-- | src/mongo/db/s/transaction_coordinator_util.cpp | 18 | ||||
-rw-r--r-- | src/mongo/db/s/transaction_coordinator_util.h | 2 | ||||
-rw-r--r-- | src/mongo/db/s/txn_two_phase_commit_cmds.cpp | 4 | ||||
-rw-r--r-- | src/mongo/db/service_entry_point_common.cpp | 20 | ||||
-rw-r--r-- | src/mongo/db/transaction_participant.cpp | 3 | ||||
-rw-r--r-- | src/mongo/executor/remote_command_request.cpp | 3 | ||||
-rw-r--r-- | src/mongo/s/transaction_router.cpp | 21 | ||||
-rw-r--r-- | src/mongo/s/transaction_router_test.cpp | 25 |
17 files changed, 249 insertions, 123 deletions
diff --git a/jstests/core/txns/api_params_transaction.js b/jstests/core/txns/api_params_transaction.js new file mode 100644 index 00000000000..a809f8a701c --- /dev/null +++ b/jstests/core/txns/api_params_transaction.js @@ -0,0 +1,88 @@ +/** + * Tests passing API parameters into transaction-continuing commands. + * @tags: [uses_transactions, requires_fcv_47] + */ + +(function() { +"use strict"; + +load("jstests/libs/fixture_helpers.js"); // For FixtureHelpers.isMongos(). + +const dbName = jsTestName(); +const collName = "test"; + +const testDB = db.getSiblingDB(dbName); +const testColl = testDB.getCollection(collName); + +testColl.drop({writeConcern: {w: "majority"}}); +assert.commandWorked( + testDB.runCommand({create: testColl.getName(), writeConcern: {w: "majority"}})); + +const apiParamCombos = [ + {}, + {apiVersion: "1"}, + {apiVersion: "1", apiDeprecationErrors: true}, + {apiVersion: "1", apiDeprecationErrors: false}, + {apiVersion: "1", apiStrict: true}, + {apiVersion: "1", apiStrict: true, apiDeprecationErrors: true}, + {apiVersion: "1", apiStrict: true, apiDeprecationErrors: false}, + {apiVersion: "1", apiStrict: false}, + {apiVersion: "1", apiStrict: false, apiDeprecationErrors: true}, + {apiVersion: "1", apiStrict: false, apiDeprecationErrors: false} +]; + +function addApiParams(obj, params) { + return Object.assign(Object.assign({}, obj), params); +} + +for (const txnInitiatingParams of apiParamCombos) { + for (const txnContinuingParams of apiParamCombos) { + for (const txnEndingCmdName of ["commitTransaction", "abortTransaction"]) { + // TODO (SERVER-56550): Remove "!txnContinuingParams.apiVersion". + const compatibleParams = + !txnContinuingParams.apiVersion || txnContinuingParams === txnInitiatingParams; + const session = db.getMongo().startSession(); + const sessionDb = session.getDatabase(dbName); + + session.startTransaction(); + assert.commandWorked(sessionDb.runCommand( + addApiParams({insert: collName, documents: [{}, {}, {}]}, txnInitiatingParams))); + + function checkCommand(db, command) { + const commandWithParams = addApiParams(command, txnContinuingParams); + jsTestLog(`Session ${session.getSessionId().id}, ` + + `initial params: ${tojson(txnInitiatingParams)}, ` + + `continuing params: ${tojson(txnContinuingParams)}, ` + + `compatible: ${tojson(compatibleParams)}`); + jsTestLog(`Command: ${tojson(commandWithParams)}`); + const reply = db.runCommand(commandWithParams); + jsTestLog(`Reply: ${tojson(reply)}`); + + if (compatibleParams) { + assert.commandWorked(reply); + } else { + assert.commandFailedWithCode(reply, ErrorCodes.APIMismatchError); + } + } + + /* + * Check "insert" with API params in a transaction. + */ + checkCommand(sessionDb, {insert: collName, documents: [{}]}); + + /* + * Check "commitTransaction" or "abortTransaction". + */ + let txnEndingCmd = {}; + txnEndingCmd[txnEndingCmdName] = 1; + Object.assign(txnEndingCmd, + {txnNumber: session.getTxnNumber_forTesting(), autocommit: false}); + + checkCommand(session.getDatabase("admin"), txnEndingCmd); + + // Clean up. + session.abortTransaction(); + } + } +} +})(); diff --git a/jstests/core/txns/transaction_continuing_cmds_refuse_api_params.js b/jstests/core/txns/transaction_continuing_cmds_refuse_api_params.js deleted file mode 100644 index 2eb11d1f4c3..00000000000 --- a/jstests/core/txns/transaction_continuing_cmds_refuse_api_params.js +++ /dev/null @@ -1,68 +0,0 @@ -/** - * Tests that passing API parameters into transaction-continuing commands should fail. - * @tags: [uses_transactions, requires_fcv_47] - */ - -(function() { -"use strict"; - -load("jstests/libs/fixture_helpers.js"); // For FixtureHelpers.isMongos(). - -const errorCode = FixtureHelpers.isMongos(db) ? 4937701 : 4937700; -const commitTxnWithApiVersionErrorCode = FixtureHelpers.isMongos(db) ? 4937702 : 4937700; - -const dbName = jsTestName(); -const collName = "test"; - -const testDB = db.getSiblingDB(dbName); -const testColl = testDB.getCollection(collName); - -testColl.drop({writeConcern: {w: "majority"}}); -assert.commandWorked( - testDB.runCommand({create: testColl.getName(), writeConcern: {w: "majority"}})); - -const session = db.getMongo().startSession(); -const sessionAdminDB = session.getDatabase("admin"); -const sessionDB = session.getDatabase(dbName); -const sessionColl = sessionDB.getCollection(collName); - -const doc = { - x: 1 -}; - -session.startTransaction(); - -// Verify that the transaction-initiating command is allowed to specify an apiVersion. -assert.commandWorked(sessionColl.runCommand({insert: collName, documents: [doc], apiVersion: "1"})); - -// Verify that any transaction-continuing commands cannot specify API parameters. -assert.commandFailedWithCode( - sessionColl.runCommand({insert: collName, documents: [doc], apiVersion: "1"}), errorCode); -assert.commandFailedWithCode( - sessionColl.runCommand({insert: collName, documents: [doc], apiVersion: "1", apiStrict: false}), - errorCode); -assert.commandFailedWithCode( - sessionColl.runCommand( - {insert: collName, documents: [doc], apiVersion: "1", apiDeprecationErrors: false}), - errorCode); -let reply = sessionAdminDB.runCommand({ - commitTransaction: 1, - txnNumber: session.getTxnNumber_forTesting(), - autocommit: false, - apiVersion: "1" -}); -assert.commandFailedWithCode(reply, commitTxnWithApiVersionErrorCode); -reply = sessionAdminDB.runCommand({ - abortTransaction: 1, - txnNumber: session.getTxnNumber_forTesting(), - autocommit: false, - apiVersion: "1" -}); -assert.commandFailedWithCode(reply, errorCode); - -// Transaction-continuing commands without API parameters are allowed. -assert.commandWorked(sessionColl.runCommand({insert: collName, documents: [doc]})); - -assert.commandWorked(session.abortTransaction_forTesting()); -session.endSession(); -})(); diff --git a/jstests/noPassthrough/require_api_version.js b/jstests/noPassthrough/require_api_version.js index eefd34f21e0..2f496256897 100644 --- a/jstests/noPassthrough/require_api_version.js +++ b/jstests/noPassthrough/require_api_version.js @@ -47,8 +47,9 @@ function runTest(db, supportsTransctions, isMongos, writeConcern = {}, secondari if (supportsTransctions) { /* - * Transaction-starting commands must have apiVersion, transaction-continuing commands must - * not. + * Transaction-starting commands must have apiVersion, transaction-continuing commands may. + * + * TODO (SERVER-56550): Test that transaction-continuing commands fail *without* API params. */ const session = db.getMongo().startSession({causalConsistency: false}); const sessionDb = session.getDatabase(db.getName()); @@ -76,16 +77,8 @@ function runTest(db, supportsTransctions, isMongos, writeConcern = {}, secondari stmtId: NumberInt(2), autocommit: false })); - const commitTxnWithApiVersionErrorCode = isMongos ? 4937702 : 4937700; - assert.commandFailedWithCode(sessionDb.runCommand({ - commitTransaction: 1, - apiVersion: "1", - txnNumber: NumberLong(0), - autocommit: false - }), - commitTxnWithApiVersionErrorCode); assert.commandWorked(sessionDb.runCommand( - {commitTransaction: 1, txnNumber: NumberLong(0), autocommit: false})); + {commitTransaction: 1, apiVersion: "1", txnNumber: NumberLong(0), autocommit: false})); // Start a new txn so we can test abortTransaction. reply = sessionDb.runCommand({ @@ -97,16 +90,8 @@ function runTest(db, supportsTransctions, isMongos, writeConcern = {}, secondari autocommit: false }); assert.commandWorked(reply); - const abortTxnWithApiVersionErrorCode = isMongos ? 4937701 : 4937700; - assert.commandFailedWithCode(sessionDb.runCommand({ - abortTransaction: 1, - apiVersion: "1", - txnNumber: NumberLong(1), - autocommit: false - }), - abortTxnWithApiVersionErrorCode); assert.commandWorked(sessionDb.runCommand( - {abortTransaction: 1, txnNumber: NumberLong(1), autocommit: false})); + {abortTransaction: 1, apiVersion: "1", txnNumber: NumberLong(1), autocommit: false})); } assert.commandWorked( diff --git a/jstests/sharding/libs/mongos_api_params_util.js b/jstests/sharding/libs/mongos_api_params_util.js index 8d396fae984..34e28f34d8b 100644 --- a/jstests/sharding/libs/mongos_api_params_util.js +++ b/jstests/sharding/libs/mongos_api_params_util.js @@ -97,7 +97,38 @@ let MongosAPIParametersUtil = (function() { skip: "executes locally on mongos (not sent to any remote node)" }, {commandName: "_mergeAuthzCollections", skip: "internal API"}, - {commandName: "abortTransaction", skip: "prohibits API parameters"}, + { + commandName: "abortTransaction", + run: { + inAPIVersion1: true, + runsAgainstAdminDb: true, + shardCommandName: "abortTransaction", + permittedInTxn: false, // We handle the transaction manually in this test. + setUp: (context) => { + // Start a session and transaction. + const session = st.s0.startSession(); + context.lsid = session.getSessionId(); + const cmd = { + insert: "collection", + // A doc on each shard in the 2-shard configuration. + documents: [{_id: 1}, {_id: 21}], + lsid: context.lsid, + txnNumber: NumberLong(1), + autocommit: false, + startTransaction: true + }; + + assert.commandWorked( + st.s0.getDB("db").runCommand(Object.assign(cmd, context.apiParameters))); + }, + command: (context) => ({ + abortTransaction: 1, + lsid: context.lsid, + txnNumber: NumberLong(1), + autocommit: false + }) + } + }, { commandName: "addShard", run: { @@ -191,7 +222,38 @@ let MongosAPIParametersUtil = (function() { command: () => ({collStats: "collection"}), } }, - {commandName: "commitTransaction", skip: "prohibits API parameters"}, + { + commandName: "commitTransaction", + run: { + inAPIVersion1: true, + runsAgainstAdminDb: true, + shardCommandName: "commitTransaction", + permittedInTxn: false, // We handle the transaction manually in this test. + setUp: (context) => { + // Start a session and transaction. + const session = st.s0.startSession(); + context.lsid = session.getSessionId(); + const cmd = { + insert: "collection", + // A doc on each shard in the 2-shard configuration. + documents: [{_id: 1}, {_id: 21}], + lsid: context.lsid, + txnNumber: NumberLong(1), + autocommit: false, + startTransaction: true + }; + + assert.commandWorked( + st.s0.getDB("db").runCommand(Object.assign(cmd, context.apiParameters))); + }, + command: (context) => ({ + commitTransaction: 1, + lsid: context.lsid, + txnNumber: NumberLong(1), + autocommit: false + }) + } + }, {commandName: "compact", skip: "not allowed through mongos"}, { commandName: "configureFailPoint", @@ -1338,7 +1400,9 @@ let MongosAPIParametersUtil = (function() { } if (lastCommandInvocation === undefined) { - msg = `Primary didn't log ${commandName}`; + msg = `Primary didn't log ${commandName} with apiVersion ${apiVersion},` + + ` apiStrict ${apiStrict},` + + ` apiDeprecationErrors ${apiDeprecationErrors}.`; return false; } @@ -1441,7 +1505,13 @@ let MongosAPIParametersUtil = (function() { const configPrimary = st.configRS.getPrimary(); const shardZeroPrimary = st.rs0.getPrimary(); - const context = {}; + const context = { + apiParameters: { + apiVersion: apiVersion, + apiStrict: apiStrict, + apiDeprecationErrors: apiDeprecationErrors + } + }; if (runOrExplain.setUp) { jsTestLog(`setUp function for ${commandName}`); diff --git a/src/mongo/base/error_codes.yml b/src/mongo/base/error_codes.yml index 9352da0f7a1..083c75dbd9f 100644 --- a/src/mongo/base/error_codes.yml +++ b/src/mongo/base/error_codes.yml @@ -436,6 +436,8 @@ error_codes: - {code: 346,name: ChangeStreamInvalidated, extra: ChangeStreamInvalidationInfo} + - {code: 347, name: APIMismatchError, categories: [VersionedAPIError,VoteAbortError]} + # Error codes 4000-8999 are reserved. # Non-sequential error codes for compatibility only) diff --git a/src/mongo/db/api_parameters.cpp b/src/mongo/db/api_parameters.cpp index fbe130ee4de..b3731ed35a7 100644 --- a/src/mongo/db/api_parameters.cpp +++ b/src/mongo/db/api_parameters.cpp @@ -87,6 +87,12 @@ void APIParameters::appendInfo(BSONObjBuilder* builder) const { } } +BSONObj APIParameters::toBSON() const { + BSONObjBuilder bob; + appendInfo(&bob); + return bob.obj(); +} + std::size_t APIParameters::Hash::operator()(const APIParameters& params) const { size_t seed = 0; if (params.getAPIVersion()) { diff --git a/src/mongo/db/api_parameters.h b/src/mongo/db/api_parameters.h index 809cb49a902..2108281f97a 100644 --- a/src/mongo/db/api_parameters.h +++ b/src/mongo/db/api_parameters.h @@ -60,6 +60,8 @@ public: void appendInfo(BSONObjBuilder* builder) const; + BSONObj toBSON() const; + const boost::optional<std::string>& getAPIVersion() const { return _apiVersion; } diff --git a/src/mongo/db/initialize_api_parameters.cpp b/src/mongo/db/initialize_api_parameters.cpp index 588200d41b7..3962494fae6 100644 --- a/src/mongo/db/initialize_api_parameters.cpp +++ b/src/mongo/db/initialize_api_parameters.cpp @@ -107,6 +107,7 @@ void enforceRequireAPIVersion(OperationContext* opCtx, Command* command) { auto isInternalClient = !client->session() || (client->session()->getTags() & transport::Session::kInternalClient); + // TODO (SERVER-56550): Don't excuse getMore or transaction-continuing commands. if (gRequireApiVersion.load() && !opCtx->getClient()->isInDirectClient() && !isInternalClient && command->getName() != "getMore" && !opCtx->isContinuingMultiDocumentTransaction()) { uassert( diff --git a/src/mongo/db/s/transaction_coordinator.cpp b/src/mongo/db/s/transaction_coordinator.cpp index 7112a12c8f5..a551447964a 100644 --- a/src/mongo/db/s/transaction_coordinator.cpp +++ b/src/mongo/db/s/transaction_coordinator.cpp @@ -102,6 +102,7 @@ TransactionCoordinator::TransactionCoordinator(OperationContext* operationContex std::make_unique<TransactionCoordinatorMetricsObserver>()), _deadline(deadline) { + auto apiParams = APIParameters::get(operationContext); auto kickOffCommitPF = makePromiseFuture<void>(); _kickOffCommitPromise = std::move(kickOffCommitPF.promise); @@ -282,7 +283,7 @@ TransactionCoordinator::TransactionCoordinator(OperationContext* operationContex "hangBeforeWaitingForDecisionWriteConcern", std::move(opTime)); }) - .then([this] { + .then([this, apiParams] { { stdx::lock_guard<Latch> lg(_mutex); _decisionDurable = true; @@ -309,12 +310,13 @@ TransactionCoordinator::TransactionCoordinator(OperationContext* operationContex *_scheduler, _lsid, _txnNumber, + apiParams, *_participants, *_decision->getCommitTimestamp()); } case CommitDecision::kAbort: { return txn::sendAbort( - _serviceContext, *_scheduler, _lsid, _txnNumber, *_participants); + _serviceContext, *_scheduler, _lsid, _txnNumber, apiParams, *_participants); } default: MONGO_UNREACHABLE; diff --git a/src/mongo/db/s/transaction_coordinator_util.cpp b/src/mongo/db/s/transaction_coordinator_util.cpp index 9e0e6465ca4..f8e7bbb42c9 100644 --- a/src/mongo/db/s/transaction_coordinator_util.cpp +++ b/src/mongo/db/s/transaction_coordinator_util.cpp @@ -419,14 +419,17 @@ Future<void> sendCommit(ServiceContext* service, txn::AsyncWorkScheduler& scheduler, const LogicalSessionId& lsid, TxnNumber txnNumber, + const APIParameters& apiParams, const txn::ParticipantsList& participants, Timestamp commitTimestamp) { CommitTransaction commitTransaction; commitTransaction.setDbName(NamespaceString::kAdminDb); commitTransaction.setCommitTimestamp(commitTimestamp); - auto commitObj = commitTransaction.toBSON( - BSON("lsid" << lsid.toBSON() << "txnNumber" << txnNumber << "autocommit" << false - << WriteConcernOptions::kWriteConcernField << WriteConcernOptions::Majority)); + BSONObjBuilder bob(BSON("lsid" << lsid.toBSON() << "txnNumber" << txnNumber << "autocommit" + << false << WriteConcernOptions::kWriteConcernField + << WriteConcernOptions::Majority)); + apiParams.appendInfo(&bob); + auto commitObj = commitTransaction.toBSON(bob.obj()); OperationContextFn operationContextFn = [lsid, txnNumber](OperationContext* opCtx) { invariant(opCtx); @@ -451,12 +454,15 @@ Future<void> sendAbort(ServiceContext* service, txn::AsyncWorkScheduler& scheduler, const LogicalSessionId& lsid, TxnNumber txnNumber, + const APIParameters& apiParams, const txn::ParticipantsList& participants) { AbortTransaction abortTransaction; abortTransaction.setDbName(NamespaceString::kAdminDb); - auto abortObj = abortTransaction.toBSON( - BSON("lsid" << lsid.toBSON() << "txnNumber" << txnNumber << "autocommit" << false - << WriteConcernOptions::kWriteConcernField << WriteConcernOptions::Majority)); + BSONObjBuilder bob(BSON("lsid" << lsid.toBSON() << "txnNumber" << txnNumber << "autocommit" + << false << WriteConcernOptions::kWriteConcernField + << WriteConcernOptions::Majority)); + apiParams.appendInfo(&bob); + auto abortObj = abortTransaction.toBSON(bob.obj()); OperationContextFn operationContextFn = [lsid, txnNumber](OperationContext* opCtx) { invariant(opCtx); diff --git a/src/mongo/db/s/transaction_coordinator_util.h b/src/mongo/db/s/transaction_coordinator_util.h index ed0c882656a..5563e5a29e0 100644 --- a/src/mongo/db/s/transaction_coordinator_util.h +++ b/src/mongo/db/s/transaction_coordinator_util.h @@ -143,6 +143,7 @@ Future<void> sendCommit(ServiceContext* service, txn::AsyncWorkScheduler& scheduler, const LogicalSessionId& lsid, TxnNumber txnNumber, + const APIParameters& apiParams, const txn::ParticipantsList& participants, Timestamp commitTimestamp); @@ -154,6 +155,7 @@ Future<void> sendAbort(ServiceContext* service, txn::AsyncWorkScheduler& scheduler, const LogicalSessionId& lsid, TxnNumber txnNumber, + const APIParameters& apiParams, const txn::ParticipantsList& participants); /** diff --git a/src/mongo/db/s/txn_two_phase_commit_cmds.cpp b/src/mongo/db/s/txn_two_phase_commit_cmds.cpp index 1e5b1ac7288..e8105484ca2 100644 --- a/src/mongo/db/s/txn_two_phase_commit_cmds.cpp +++ b/src/mongo/db/s/txn_two_phase_commit_cmds.cpp @@ -224,6 +224,10 @@ class CoordinateCommitTransactionCmd : public TypedCommand<CoordinateCommitTrans public: using Request = CoordinateCommitTransaction; + bool acceptsAnyApiVersionParameters() const override { + return true; + } + class Invocation final : public InvocationBase { public: using InvocationBase::InvocationBase; diff --git a/src/mongo/db/service_entry_point_common.cpp b/src/mongo/db/service_entry_point_common.cpp index 31b24f5c359..abf81db6275 100644 --- a/src/mongo/db/service_entry_point_common.cpp +++ b/src/mongo/db/service_entry_point_common.cpp @@ -900,6 +900,19 @@ void CheckoutSessionAndInvokeCommand::_checkOutSession() { _sessionTxnState = std::make_unique<MongoDOperationContextSession>(opCtx); _txnParticipant.emplace(TransactionParticipant::get(opCtx)); + // TODO (SERVER-56550): Do this check even if !apiParamsFromClient.getParamsPassed(). + auto apiParamsFromClient = APIParameters::get(opCtx); + if (apiParamsFromClient.getParamsPassed()) { + auto apiParamsFromTxn = _txnParticipant->getAPIParameters(opCtx); + uassert( + ErrorCodes::APIMismatchError, + "API param conflict: {} used params {}, the transaction's first command used {}"_format( + invocation->definition()->getName(), + apiParamsFromClient.toBSON().toString(), + apiParamsFromTxn.toBSON().toString()), + apiParamsFromTxn == apiParamsFromClient); + } + if (!opCtx->getClient()->isInDirectClient()) { bool beganOrContinuedTxn{false}; // This loop allows new transactions on a session to block behind a previous prepared @@ -1547,13 +1560,6 @@ void ExecCommandDatabase::_initiateCommand() { opCtx->lockState()->setShouldConflictWithSecondaryBatchApplication(false); } - if (opCtx->inMultiDocumentTransaction() && !startTransaction) { - uassert(4937700, - "API parameters are only allowed in the first command of a multi-document " - "transaction", - !APIParameters::get(opCtx).getParamsPassed()); - } - // Remember whether or not this operation is starting a transaction, in case something later in // the execution needs to adjust its behavior based on this. opCtx->setIsStartingMultiDocumentTransaction(startTransaction); diff --git a/src/mongo/db/transaction_participant.cpp b/src/mongo/db/transaction_participant.cpp index 468b0dfe8d8..d1374aecc18 100644 --- a/src/mongo/db/transaction_participant.cpp +++ b/src/mongo/db/transaction_participant.cpp @@ -2168,7 +2168,8 @@ void TransactionParticipant::Participant::_setNewTxnNumber(OperationContext* opC "{lsid}", "New transaction started", "txnNumber"_attr = txnNumber, - "lsid"_attr = _sessionId().getId()); + "lsid"_attr = _sessionId().getId(), + "apiParameters"_attr = APIParameters::get(opCtx).toBSON()); // Abort the existing transaction if it's not prepared, committed, or aborted. if (o().txnState.isInProgress()) { diff --git a/src/mongo/executor/remote_command_request.cpp b/src/mongo/executor/remote_command_request.cpp index fc2af299615..1a62a4b30c4 100644 --- a/src/mongo/executor/remote_command_request.cpp +++ b/src/mongo/executor/remote_command_request.cpp @@ -87,8 +87,7 @@ RemoteCommandRequestBase::RemoteCommandRequestBase(RequestId requestId, cmdObj = cmdObj.addField(BSON("clientOperationKey" << operationKey.get()).firstElement()); } - if (opCtx && APIParameters::get(opCtx).getParamsPassed() && - !opCtx->isContinuingMultiDocumentTransaction()) { + if (opCtx && APIParameters::get(opCtx).getParamsPassed()) { BSONObjBuilder bob(std::move(cmdObj)); APIParameters::get(opCtx).appendInfo(&bob); cmdObj = bob.obj(); diff --git a/src/mongo/s/transaction_router.cpp b/src/mongo/s/transaction_router.cpp index a6df4060e23..57f5e636a13 100644 --- a/src/mongo/s/transaction_router.cpp +++ b/src/mongo/s/transaction_router.cpp @@ -423,9 +423,6 @@ BSONObj TransactionRouter::Participant::attachTxnFieldsIfNeeded( bool mustStartTransaction = isFirstStatementInThisParticipant && !isTransactionCommand(cmdName); if (!mustStartTransaction) { - dassert(!cmd.hasField(APIParameters::kAPIVersionFieldName)); - dassert(!cmd.hasField(APIParameters::kAPIStrictFieldName)); - dassert(!cmd.hasField(APIParameters::kAPIDeprecationErrorsFieldName)); dassert(!cmd.hasField(repl::ReadConcernArgs::kReadConcernFieldName)); } @@ -902,6 +899,16 @@ void TransactionRouter::Router::beginOrContinueTxn(OperationContext* opCtx, << o().txnNumber << " seen in session " << _sessionId()); } else if (txnNumber == o().txnNumber) { // This is the same transaction as the one in progress. + auto apiParamsFromClient = APIParameters::get(opCtx); + // TODO (SERVER-56550): Do this check even if !apiParamsFromClient.getParamsPassed(). + if (apiParamsFromClient.getParamsPassed() && + (action == TransactionActions::kContinue || action == TransactionActions::kCommit)) { + uassert(ErrorCodes::APIMismatchError, + "A transaction-continuing command must use the same API parameters as " + "the first command in the transaction", + apiParamsFromClient == o().apiParameters); + } + switch (action) { case TransactionActions::kStart: { uasserted(ErrorCodes::ConflictingOperationInProgress, @@ -909,9 +916,6 @@ void TransactionRouter::Router::beginOrContinueTxn(OperationContext* opCtx, << _sessionId() << " already started"); } case TransactionActions::kContinue: { - uassert(4937701, - "Only the first command in a transaction may specify API parameters", - !APIParameters::get(opCtx).getParamsPassed()); uassert(ErrorCodes::InvalidOptions, "Only the first command in a transaction may specify a readConcern", repl::ReadConcernArgs::get(opCtx).isEmpty()); @@ -924,9 +928,6 @@ void TransactionRouter::Router::beginOrContinueTxn(OperationContext* opCtx, break; } case TransactionActions::kCommit: - uassert(4937702, - "Only the first command in a transaction may specify API parameters", - !APIParameters::get(opCtx).getParamsPassed()); ++p().latestStmtId; _onContinue(opCtx); break; @@ -1204,8 +1205,6 @@ BSONObj TransactionRouter::Router::abortTransaction(OperationContext* opCtx) { "txnNumber"_attr = o().txnNumber, "numParticipantShards"_attr = o().participants.size()); - // Omit API parameters from abortTransaction. - IgnoreAPIParametersBlock ignoreApiParametersBlock(opCtx); const auto responses = gatherResponses(opCtx, NamespaceString::kAdminDb, ReadPreferenceSetting{ReadPreference::PrimaryOnly}, diff --git a/src/mongo/s/transaction_router_test.cpp b/src/mongo/s/transaction_router_test.cpp index 964c8a38994..b1edf1164d8 100644 --- a/src/mongo/s/transaction_router_test.cpp +++ b/src/mongo/s/transaction_router_test.cpp @@ -719,7 +719,8 @@ TEST_F(TransactionRouterTestWithDefaultSession, AttachTxnValidatesReadConcernIfA } } -TEST_F(TransactionRouterTestWithDefaultSession, CannotSpecifyAPIParametersAfterFirstStatement) { +// TODO (SERVER-56550): Test that API parameters are required in txn-continuing commands. +TEST_F(TransactionRouterTestWithDefaultSession, SameAPIParametersAfterFirstStatement) { APIParameters apiParameters = APIParameters(); apiParameters.setAPIVersion("1"); APIParameters::get(operationContext()) = apiParameters; @@ -730,11 +731,31 @@ TEST_F(TransactionRouterTestWithDefaultSession, CannotSpecifyAPIParametersAfterF operationContext(), txnNum, TransactionRouter::TransactionActions::kStart); txnRouter.setDefaultAtClusterTime(operationContext()); + // Continuing with the same API params succeeds. (Must reset readConcern from "snapshot".) + repl::ReadConcernArgs::get(operationContext()) = repl::ReadConcernArgs(); + txnRouter.beginOrContinueTxn( + operationContext(), txnNum, TransactionRouter::TransactionActions::kContinue); +} + +TEST_F(TransactionRouterTestWithDefaultSession, DifferentAPIParametersAfterFirstStatement) { + APIParameters apiParameters = APIParameters(); + apiParameters.setAPIVersion("1"); + APIParameters::get(operationContext()) = apiParameters; + TxnNumber txnNum{3}; + + auto txnRouter = TransactionRouter::get(operationContext()); + txnRouter.beginOrContinueTxn( + operationContext(), txnNum, TransactionRouter::TransactionActions::kStart); + txnRouter.setDefaultAtClusterTime(operationContext()); + + // Can't continue with different params. (Must reset readConcern from "snapshot".) + APIParameters::get(operationContext()).setAPIStrict(true); + repl::ReadConcernArgs::get(operationContext()) = repl::ReadConcernArgs(); ASSERT_THROWS_CODE( txnRouter.beginOrContinueTxn( operationContext(), txnNum, TransactionRouter::TransactionActions::kContinue), DBException, - 4937701); + ErrorCodes::APIMismatchError); } TEST_F(TransactionRouterTestWithDefaultSession, CannotSpecifyReadConcernAfterFirstStatement) { |