-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(NODE-7298): ensure commonWireVersion is computed from server maxWireVersion #4805
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
base: main
Are you sure you want to change the base?
Conversation
|
Fantastic job @crehbichler, thanks a lot! Can you please also add integration test for the specific scenario you've found in the jira ticket, i.e. |
…ge using provided read preference
|
Added! I wasn’t 100% sure whether to place the integration test in Also, in the test I am checking the server address (I used the hello command) because the correct $readPreference was already attached to the command before the fix (just the wrong server was being selected). Please let me know if this is fine or if I should adjust anything. |
| heartbeatFrequencyMS: number; | ||
| localThresholdMS: number; | ||
| commonWireVersion: number; | ||
| commonWireVersion: number | null; |
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.
Technically TopologyDescription is public 🫤 .
So, widening the type like this is technically a breaking change. Is it possible to fix this bug in a way that doesn't change the type of commonWireVersion?
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.
(Any type/class/object marked with the @public tag is a part of our public API).
Description
Summary of Changes
The TopologyDescription constructor initialized commonWireVersion to 0, but the update() method only updates this field when its previous value is null. As a result, commonWireVersion remained stuck at 0 even when all servers reported valid non-zero maxWireVersion values.
This caused secondaryWritableServerSelector() to treat the cluster as pre–MongoDB 5.0 (because wireVersion < 13) and always fall back to primary read preference. User-provided read preferences (e.g. secondary, secondaryPreferred) were therefore ignored for aggregation pipelines containing write stages ($merge and $out).
The fix initializes commonWireVersion to null, allowing it to be correctly updated to the minimum non-zero maxWireVersion across server descriptions.
A new test file
test/unit/sdam/topology_description.test.tshas been added to cover initialization and update behavior.Release Highlight
Release notes highlight
Double check the following
npm run check:lint)type(NODE-xxxx)[!]: description