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

feat: Add MongoOption builder logic #2623

Merged
merged 26 commits into from
Dec 8, 2020
Merged

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Nov 17, 2020

Parsing options from URI and options object we get a final MongoOptions shaped object, one downside here is that the building process isn't typechecked, doesn't seem like there's an easy way to do that. But the final product is an options property on MongoClient that carries all the top level options specified by the user.

A part of this work revealed a few issues:

  • Aliased options are hard to resolve in the current logic structure, minPoolSize and minSize might supersede one another and there's no clear way to give priority but also to communicate that priority. I removed the aliases minSize and poolSize.
  • This is a good opportunity to remove a number of deprecated or no longer used options, ha, haInterval, domainsEnabled, useUnifiedTopology, useNewUrlParser, appname (not appName), and validateOptions
  • Do we want to consider removing ssl options in favor of the tls versions?

NODE-2699, NODE-2871

@nbbeeken nbbeeken force-pushed the NODE-2699/MongoOptions-builder branch from b661080 to 0875e3a Compare November 18, 2020 18:08
@nbbeeken nbbeeken force-pushed the NODE-2699/MongoOptions-builder branch from a545f57 to dcc1e8f Compare November 19, 2020 18:03
Base automatically changed from NODE-2698/MongoOptions to master November 19, 2020 21:04
@nbbeeken nbbeeken force-pushed the NODE-2699/MongoOptions-builder branch from dcc1e8f to f311817 Compare November 19, 2020 21:39
@nbbeeken nbbeeken marked this pull request as ready for review November 19, 2020 22:19
src/cmap/auth/defaultAuthProviders.ts Outdated Show resolved Hide resolved
src/cmap/auth/mongo_credentials.ts Outdated Show resolved Hide resolved
src/cmap/auth/mongo_credentials.ts Outdated Show resolved Hide resolved
src/mongo_client.ts Outdated Show resolved Hide resolved
test/functional/mongo_client.test.js Outdated Show resolved Hide resolved
src/connection_string.ts Outdated Show resolved Hide resolved
src/connection_string.ts Outdated Show resolved Hide resolved
src/connection_string.ts Outdated Show resolved Hide resolved
src/connection_string.ts Show resolved Hide resolved
src/connection_string.ts Outdated Show resolved Hide resolved
@nbbeeken nbbeeken force-pushed the NODE-2699/MongoOptions-builder branch from 22bdd05 to 5816fa2 Compare November 23, 2020 16:28
@nbbeeken nbbeeken changed the base branch from master to chore/remove-old-options November 23, 2020 16:29
@nbbeeken nbbeeken force-pushed the NODE-2699/MongoOptions-builder branch 3 times, most recently from 82d2d81 to 3b0430e Compare November 23, 2020 21:59
Base automatically changed from chore/remove-old-options to master November 24, 2020 15:38
@nbbeeken nbbeeken force-pushed the NODE-2699/MongoOptions-builder branch from 3b0430e to 956c6a0 Compare November 24, 2020 15:43
Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

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

Nice work, lot's to review! Did a first pass and had a few questions.

test/unit/core/connection_string.test.js Show resolved Hide resolved
test/unit/core/connection_string.test.js Show resolved Hide resolved
src/connection_string.ts Show resolved Hide resolved
src/connection_string.ts Outdated Show resolved Hide resolved
src/connection_string.ts Outdated Show resolved Hide resolved
src/mongo_client.ts Outdated Show resolved Hide resolved
src/connection_string.ts Outdated Show resolved Hide resolved
src/connection_string.ts Outdated Show resolved Hide resolved
src/connection_string.ts Outdated Show resolved Hide resolved
@nbbeeken nbbeeken force-pushed the NODE-2699/MongoOptions-builder branch from 93df27c to 73988a7 Compare December 2, 2020 22:23
Comment on lines +149 to +159
static merge(
creds: MongoCredentials,
options: Partial<MongoCredentialsOptions>
): MongoCredentials {
return new MongoCredentials({
username: options.username ?? creds.username,
password: options.password ?? creds.password,
mechanism: options.mechanism ?? creds.mechanism,
mechanismProperties: options.mechanismProperties ?? creds.mechanismProperties,
source: options.source ?? creds.source ?? options.db
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mbroadst
Sorry about my git fu, I can't reply to your comment about the credentials issue. I've gone ahead with the merge solution, lmk what you think. For what its worth I think we were safe with the spread solution because typescript would not permit us to have a property missing, it just so happens that MongoCredentials and MongoCredentialOptions share all their properties and mandate that they are defined, but ofc better to be safe and box up more complex functionality, so .merge seems like a good solution .

@nbbeeken nbbeeken force-pushed the NODE-2699/MongoOptions-builder branch from 1dd1b8d to fe8dffd Compare December 3, 2020 23:37
Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Member

@mbroadst mbroadst left a comment

Choose a reason for hiding this comment

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

LGTM 👏 👏

@nbbeeken nbbeeken merged commit cb9ee9e into master Dec 8, 2020
@nbbeeken nbbeeken deleted the NODE-2699/MongoOptions-builder branch December 8, 2020 15:25
johntseng added a commit to johntseng/webonary that referenced this pull request Jun 17, 2022
MongoClientOptions.useNewUrlParser has been removed: mongodb/node-mongodb-native#2623
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants