Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(NODE-5925): driver throws error when non-read operation in a transaction has a ReadPreferenceMode other than 'primary' #4075

Merged
merged 8 commits into from
Apr 15, 2024
Merged

fix(NODE-5925): driver throws error when non-read operation in a transaction has a ReadPreferenceMode other than 'primary' #4075

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
082a3be
fix(NODE-5925): Attempting a read on txn with a non-primary readPrefe…
aditi-khare-mongoDB Apr 11, 2024
2138e47
updated commandIsReadOperation to be inspired by READ_OPERATION aspect
aditi-khare-mongoDB Apr 11, 2024
fe5cfb2
removed extraneous code and started using aspect.READ_OPERATION direc…
aditi-khare-mongoDB Apr 11, 2024
8389a35
Merge branch 'main' into NODE-5925/no-secondary-readpref-on-txn
aditi-khare-mongoDB Apr 11, 2024
4b4ccdf
throw error on runCommand too
aditi-khare-mongoDB Apr 11, 2024
22d73a9
Merge branch 'main' into NODE-5925/no-secondary-readpref-on-txn
aditi-khare-mongoDB Apr 11, 2024
565b193
pr requested changes
aditi-khare-mongoDB Apr 15, 2024
96d07c2
Merge branch 'main' into NODE-5925/no-secondary-readpref-on-txn
aditi-khare-mongoDB Apr 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/operations/execute_operation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,11 @@ export async function executeOperation<
const readPreference = operation.readPreference ?? ReadPreference.primary;
const inTransaction = !!session?.inTransaction();

if (inTransaction && !readPreference.equals(ReadPreference.primary)) {
if (
inTransaction &&
!readPreference.equals(ReadPreference.primary) &&
(operation.hasAspect(Aspect.READ_OPERATION) || operation.commandName === 'runCommand')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(operation.hasAspect(Aspect.READ_OPERATION) || operation.commandName === 'runCommand')
(operation.hasAspect(Aspect.READ_OPERATION) || operation.commandName === 'runCommand')

Very minor but a handful of lines down we check for read/write aspect, can we move those variable declarations up here and reuse the result of this call? I figure we can just reuse the same work once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I've moved the isReadOperation up in the function!

) {
throw new MongoTransactionError(
`Read preference in a transaction must be primary, not: ${readPreference.mode}`
);
Expand Down
2 changes: 1 addition & 1 deletion src/sessions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ export class ClientSession extends TypedEventEmitter<ClientSessionEvents> {

this.operationTime = undefined;
this.owner = options.owner;
this.defaultTransactionOptions = Object.assign({}, options.defaultTransactionOptions);
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
this.defaultTransactionOptions = { ...options.defaultTransactionOptions };
this.transaction = new Transaction();
}

Expand Down
4 changes: 1 addition & 3 deletions test/integration/transactions/transactions.spec.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ import { loadSpecTests } from '../../spec';
import { runUnifiedSuite } from '../../tools/unified-spec-runner/runner';

const SKIPPED_TESTS = [
// TODO(NODE-5925) - secondary read preference not allowed in transactions.
'readPreference inherited from defaultTransactionOptions',
// TODO(NODE-5924) - Fix modification of readConcern object post message send.
'readConcern local in defaultTransactionOptions',
'defaultTransactionOptions override client options',
Expand All @@ -17,7 +15,7 @@ const SKIPPED_TESTS = [
describe('Transactions Spec Unified Tests', function () {
runUnifiedSuite(loadSpecTests(path.join('transactions', 'unified')), test => {
return SKIPPED_TESTS.includes(test.description)
? 'TODO(NODE-5924/NODE-5925): Skipping failing transaction tests'
? 'TODO(NODE-5924): Skipping failing transaction tests'
: false;
});
});
5 changes: 2 additions & 3 deletions test/tools/unified-spec-runner/entities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,9 +620,8 @@ export class EntitiesMap<E = Entity> extends Map<string, E> {
WriteConcern.fromOptions(defaultOptions);
}
if (defaultOptions.readPreference) {
options.defaultTransactionOptions.readPreference = ReadPreference.fromOptions(
defaultOptions.readPreference
);
options.defaultTransactionOptions.readPreference =
ReadPreference.fromOptions(defaultOptions);
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
}
if (typeof defaultOptions.maxCommitTimeMS === 'number') {
options.defaultTransactionOptions.maxCommitTimeMS = defaultOptions.maxCommitTimeMS;
Expand Down
Loading