feat(NODE-5994)!: Remove metadata-related properties from public driver API #4716
Conversation
baileympearson
force-pushed
the
NODE-5994
branch
from
d26d719 to
4a94786
Compare
| env?: { | ||
| name: 'aws.lambda' | 'gcp.func' | 'azure.func' | 'vercel'; | ||
| name?: 'aws.lambda' | 'gcp.func' | 'azure.func' | 'vercel'; | ||
| timeout_sec?: Int32; | ||
| memory_mb?: Int32; | ||
| region?: string; | ||
| url?: string; | ||
| container?: { | ||
| runtime?: string; | ||
| orchestrator?: string; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
The changes here align this interface with the specification.
| if (typeof callback === 'function') callback(undefined, true); | ||
| } | ||
|
|
||
| get clientMetadata(): ClientMetadata { |
There was a problem hiding this comment.
Happy to put this back, although we'd have to change it to return a promise. This is dead code in src/, although I did find a few usages in tests (which I worked around by checking for the metadata on the options object instead).
| // make metadata = 507 bytes, so it takes up entire LimitedSizeDocument | ||
| // make a metadata object that, with just the name and appName, is already at capacity. | ||
| const longAppName = 's'.repeat(493); | ||
| const metadata = makeClientMetadata( |
There was a problem hiding this comment.
The changes like this one in this file are a result of making addContainerMetadata internal to the client metadata module.
makeClientMetadata truncates the appname, so creating a long appName isn't sufficient to make the metadata exceed the handshake's max size anymore.
baileympearson
force-pushed
the
NODE-5994
branch
from
4a94786 to
c238206
Compare
|
|
||
| import { | ||
| type Collection, | ||
| type CommandFailedEvent, |
There was a problem hiding this comment.
Two tests related to the appName in this file broke with the changes in this PR. I decided to just refactor this file to async-await while I was working in it.
| writeConcern: { w: 1, wtimeoutMS: 1000, fsync: true, j: true }, | ||
| readPreference: 'nearest', | ||
| readPreferenceTags: { loc: 'ny' }, | ||
| readPreferenceTags: [{ loc: 'ny' }], |
There was a problem hiding this comment.
drive-by: this is the correct way to specify readpreferencetags
| 'Invalid scheme, expected connection string to start with "mongodb://" or "mongodb+srv://"' | ||
| ); | ||
| } | ||
| it('Should fail due to wrong uri user:password@localhost', function () { |
There was a problem hiding this comment.
this test can run everywhere.
|
|
||
| client.connect(function (err) { | ||
| expect(err).to.exist; | ||
| it('correctly error out when no socket available on MongoClient `connect`', async function () { |
There was a problem hiding this comment.
this test can run everywhere.
| metadata: { | ||
| requires: { | ||
| topology: ['single'] | ||
| it('Should correctly pass through socketTimeoutMS and connectTimeoutMS', async function () { |
There was a problem hiding this comment.
this test can run everywhere.
| } | ||
| }); | ||
| // TODO(NODE-7219): remove unnecessary test | ||
| // it('should open a new MongoClient connection', { |
There was a problem hiding this comment.
we have plenty of coverage for this test elsewhere
There was a problem hiding this comment.
I'm not sure I understand what's left for the TODO if this is intentionally commented out here – would it be about removing the comments?
There was a problem hiding this comment.
To aid in reviewing the test refactors that Sergey and Pavel are doing, we've decided to comment out unnecessary tests instead of deleting them. Otherwise, the diff gets shifted and harder to read.
| } | ||
| ); | ||
| } | ||
| it('should error on unexpected options', async function () { |
There was a problem hiding this comment.
this test can run everywhere.
durran
added
the
Primary Review
| } | ||
| }); | ||
| // TODO(NODE-7219): remove unnecessary test | ||
| // it('should open a new MongoClient connection', { |
There was a problem hiding this comment.
I'm not sure I understand what's left for the TODO if this is intentionally commented out here – would it be about removing the comments?
Description
Summary of Changes
This PR makes all metadata properties in the driver internal. Additionally, it removes some unnecessary properties (
additionalDriverInfo,extendedMetadata) in favor ofmetadata.I also took this opportunity to clean up the client metadata module. Specifically:
makeClientMetadata. The only reason we exported this function and called it separate frommakeClientMetadatawas thatMongoClient.options.metadatawas public, which meant that we could not change the type fromClientMetadata -> Promise<ClientMetadata>.interface ClientMetadatais internal, I updated it so that it matches the specification.Notes for Reviewers
What is the motivation for this change?
Release Highlight
Internal ClientMetadata properties have been removed
Previous versions of the driver unintentionally made properties used when constructing client metadata public. These properties have now been made internal. The full list of properties is:
Double check the following
npm run check:lint)type(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript