-
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-3921): error on invalid TLS option combinations #3405
Conversation
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`); |
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.
See this thread for context on why this error type was changed
@@ -369,6 +368,8 @@ export function parseOptions( | |||
} | |||
} | |||
|
|||
checkTLSOptions(allOptions); |
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.
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 { |
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.
Remove export
because AFAICT checkTLSOptions
is not used outside this module and is not included in public API.
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.
LGTM, Thanks for the help here!
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
Hey @baileympearson and @nbbeeken, I just resolved the conflicts. Let me know if you'd like to see any additional changes 🙂. |
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.
@nbbeeken I logged the flaky tests and reran the failures, CI is green now
Description
Unskip tests to ensure that specifying mutually exclusive TLS options raises an error.
What is changing?
allOptions
object rather thanmongoOptions
object. This was done asmongoOptions
object does not contain options with falsy values.mongoOptions
object.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
npm run check:lint
script<type>(NODE-xxxx)<!>: <description>