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-3921): error on invalid TLS option combinations #3405

Merged
merged 4 commits into from Oct 4, 2022

Conversation

biniona-mongodb
Copy link
Contributor

@biniona-mongodb biniona-mongodb commented Sep 11, 2022

Description

Unskip tests to ensure that specifying mutually exclusive TLS options raises an error.

What is changing?

  • Update TLS check to use allOptions object rather than mongoOptions object. This was done as mongoOptions object does not contain options with falsy values.
  • Move TLS check before population of mongoOptions object.
  • Remove export of function not used outside module.
Is there new documentation needed for these changes?

None

What is the motivation for this change?

https://jira.mongodb.org/browse/NODE-3921

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>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@biniona-mongodb biniona-mongodb changed the title fix(NODE-3921): unskip fix(NODE-3921): unskip TLS options tests Sep 11, 2022
if (Reflect.has(options, a) && Reflect.has(options, b)) {
throw new MongoParseError(`The '${a}' option cannot be used with '${b}'`);
if (allOptions.has(a) && allOptions.has(b)) {
throw new MongoAPIError(`The '${a}' option cannot be used with the '${b}' option`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See this thread for context on why this error type was changed

@@ -369,6 +368,8 @@ export function parseOptions(
}
}

checkTLSOptions(allOptions);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed check to use allOptions from mongoOptions as mongoOptions object does not have key value pairs when value would be falsy.

*/
export function checkTLSOptions(options: AnyOptions): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove export because AFAICT checkTLSOptions is not used outside this module and is not included in public API.

@biniona-mongodb biniona-mongodb marked this pull request as ready for review September 11, 2022 19:15
@bajanam bajanam added the External Submission PR submitted from outside the team label Sep 16, 2022
nbbeeken
nbbeeken previously approved these changes Oct 3, 2022
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks for the help here!

@nbbeeken nbbeeken added the Team Review Needs review from team label Oct 3, 2022
@baileympearson
Copy link
Contributor

hey @biniona-mongodb - thanks for the fix. Once the merge conflicts on this PR have been resolved, this should be good go to.

if you don't have time, we can always resolve the conflicts as well. just let us know

# Conflicts:
#	src/connection_string.ts
@biniona-mongodb
Copy link
Contributor Author

biniona-mongodb commented Oct 4, 2022

Hey @baileympearson and @nbbeeken, I just resolved the conflicts. Let me know if you'd like to see any additional changes 🙂.

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.

@nbbeeken I logged the flaky tests and reran the failures, CI is green now

@nbbeeken nbbeeken changed the title fix(NODE-3921): unskip TLS options tests fix(NODE-3921): error on invalid TLS option combinations Oct 4, 2022
@nbbeeken nbbeeken merged commit 1a550df into mongodb:main Oct 4, 2022
@biniona-mongodb biniona-mongodb deleted the NODE-3921 branch October 4, 2022 14:21
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
External Submission PR submitted from outside the team Team Review Needs review from team
Projects
None yet
4 participants