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.

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-4647): improve error message #3409

Merged
merged 2 commits into from
Sep 28, 2022
Merged

Conversation

LinusU
Copy link
Contributor

@LinusU LinusU commented Sep 13, 2022

Description

What is changing?

The error message when trying to to an update op with both a hint and a write concern with w: 0.

Is there new documentation needed for these changes?

I don't believe so.

What is the motivation for this change?

The previous error message led the user to believe that the server was too old, even when that was in fact not the case. The code now only gives the old message when the server is old, and gives a new message if that is not the case.

We encountered this error today and it was hard to track down the actual source.

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: <type>(NODE-xxxx)<!>: <description> (not sure if this is correct since it's only a step in the right direction, doesn't close the entire bug)
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket (not new but points to an existing ticket)

Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

Hey @LinusU - thanks for submitting this change. I think this is an easy win. I left two suggestions. Wire version 5 corresponds to server 3.4, which is below our minimum supported server version, so we can remove the check altogether 😄

Also - thanks for the TODO comment! I suggested a reformat of the TODO so that it aligns with our convention (TODO(<jira ticket number): <description).

If you aren't able to make these changes, let me know. I'm happy to take over the work, but if you'd like to make the changes, go for it 🙂

src/operations/update.ts Outdated Show resolved Hide resolved
Comment on lines 128 to 137
if (maxWireVersion(server) < 5) {
callback(new MongoCompatibilityError(`Servers < 3.4 do not support hint on update`));
} else {
// TODO: https://jira.mongodb.org/browse/NODE-3541
callback(
new MongoCompatibilityError(
`This Node.js driver do not support hint together with unacknowledged writes`
)
);
}
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
if (maxWireVersion(server) < 5) {
callback(new MongoCompatibilityError(`Servers < 3.4 do not support hint on update`));
} else {
// TODO: https://jira.mongodb.org/browse/NODE-3541
callback(
new MongoCompatibilityError(
`This Node.js driver do not support hint together with unacknowledged writes`
)
);
}
// TODO(NODE-3541): support unacknowledged writes in CRUD operations
callback(
new MongoCompatibilityError(
`This Node.js driver does not support hint together with unacknowledged writes`
)
);

@baileympearson baileympearson added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Sep 15, 2022
@LinusU
Copy link
Contributor Author

LinusU commented Sep 15, 2022

@baileympearson thank you for reviewing, with the information you provided I managed to implement the entire feature instead of just updating the error message!

👉 #3415

@baileympearson baileympearson changed the title fix(NODE-3541): improve error message fix(NODE-4647): improve error message Sep 16, 2022
Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

can we fix the error message in delete as well?

@baileympearson
Copy link
Contributor

Hey @LinusU - are you able to make the changes I requested? If not, I'm happy to take over the PR from here if that's okay with you.

@dariakp dariakp added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Sep 28, 2022
@baileympearson baileympearson merged commit 0d3c02e into mongodb:main Sep 28, 2022
@LinusU
Copy link
Contributor Author

LinusU commented Sep 28, 2022

@baileympearson sorry for not getting back to you earlier, and thank you so much for landing this PR! ❤️

@LinusU LinusU deleted the patch-1 branch September 28, 2022 19:56
ZLY201 pushed a commit to ZLY201/node-mongodb-native that referenced this pull request Nov 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants