-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
fix(NODE-5925): driver throws error when non-read operation in a transaction has a ReadPreferenceMode other than 'primary' #4075
Conversation
…rence throws an error
…tly in execute operation error check"
src/operations/execute_operation.ts
Outdated
if ( | ||
inTransaction && | ||
!readPreference.equals(ReadPreference.primary) && | ||
(operation.hasAspect(Aspect.READ_OPERATION) || operation.commandName === 'runCommand') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
There was a problem hiding this comment.
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!
5b89077
to
565b193
Compare
Description
Within a transaction, readPreferences != 'primary' are not supported, and our error logic must support that to comply with spec. An error should only be thrown in these circumstances when a read operation is being executed.
What is changing?
In a transaction, when a user provides a readPreference != 'primary' and then tries to perform a command that involves a read, the Node Driver will now throw a
Is there new documentation needed for these changes?
Yes, update Transactions Node.js Manual to reflect desired readPreference inputs.
What is the motivation for this change?
See Release Highlights
Release Highlight
Don't throw error when non-read operation in a transaction has a
ReadPreferenceMode
other than'primary'
The following error will now only be thrown when a user provides a
ReadPreferenceMode
other thanprimary
and then tries to perform a command that involves a read:Prior to this change, the Node Driver would incorrectly throw this error even when the operation does not perform a read.
Note: a
RunCommandOperation
is treated as a read operation for this error.Double check the following
npm run check:lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript