From f4f01929183e97df1c555372c742968901ee69fb Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Mon, 17 Apr 2023 13:00:56 -0400 Subject: [PATCH 1/4] prevent warning when default is used --- src/connection_string.ts | 61 +++++++++++++++-------------- test/tools/uri_spec_runner.ts | 2 +- test/unit/connection_string.test.ts | 33 ++++++++++++++++ test/unit/mongo_client.test.js | 12 ++---- 4 files changed, 68 insertions(+), 40 deletions(-) diff --git a/src/connection_string.ts b/src/connection_string.ts index c56c0e48b8d..85843ad734d 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -316,15 +316,11 @@ export function parseOptions( // All option collection - const allOptions = new CaseInsensitiveMap(); + const allProvidedOptions = new CaseInsensitiveMap(); - const allKeys = new Set([ - ...urlOptions.keys(), - ...objectOptions.keys(), - ...DEFAULT_OPTIONS.keys() - ]); + const allProvidedKeys = new Set([...urlOptions.keys(), ...objectOptions.keys()]); - for (const key of allKeys) { + for (const key of allProvidedKeys) { const values = []; const objectOptionValue = objectOptions.get(key); if (objectOptionValue != null) { @@ -334,30 +330,29 @@ export function parseOptions( if (urlValue != null) { values.push(...urlValue); } - const defaultOptionsValue = DEFAULT_OPTIONS.get(key); - if (defaultOptionsValue != null) { - values.push(defaultOptionsValue); - } - allOptions.set(key, values); + allProvidedOptions.set(key, values); } - if (allOptions.has('tlsCertificateKeyFile') && !allOptions.has('tlsCertificateFile')) { - allOptions.set('tlsCertificateFile', allOptions.get('tlsCertificateKeyFile')); + if ( + allProvidedOptions.has('tlsCertificateKeyFile') && + !allProvidedOptions.has('tlsCertificateFile') + ) { + allProvidedOptions.set('tlsCertificateFile', allProvidedOptions.get('tlsCertificateKeyFile')); } - if (allOptions.has('tls') || allOptions.has('ssl')) { - const tlsAndSslOpts = (allOptions.get('tls') || []) - .concat(allOptions.get('ssl') || []) + if (allProvidedOptions.has('tls') || allProvidedOptions.has('ssl')) { + const tlsAndSslOpts = (allProvidedOptions.get('tls') || []) + .concat(allProvidedOptions.get('ssl') || []) .map(getBoolean.bind(null, 'tls/ssl')); if (new Set(tlsAndSslOpts).size !== 1) { throw new MongoParseError('All values of tls/ssl must be the same.'); } } - checkTLSOptions(allOptions); + checkTLSOptions(allProvidedOptions); const unsupportedOptions = setDifference( - allKeys, + allProvidedKeys, Array.from(Object.keys(OPTIONS)).map(s => s.toLowerCase()) ); if (unsupportedOptions.size !== 0) { @@ -371,9 +366,20 @@ export function parseOptions( // Option parsing and setting for (const [key, descriptor] of Object.entries(OPTIONS)) { - const values = allOptions.get(key); - if (!values || values.length === 0) continue; - setOption(mongoOptions, key, descriptor, values); + const values = allProvidedOptions.get(key); + if (!values || values.length === 0) { + if (DEFAULT_OPTIONS.has(key)) { + setOption(mongoOptions, key, descriptor, [DEFAULT_OPTIONS.get(key)]); + } + } else { + const { deprecated } = descriptor; + if (deprecated) { + const deprecatedMsg = typeof deprecated === 'string' ? `: ${deprecated}` : ''; + emitWarning(`${key} is a deprecated option${deprecatedMsg}`); + } + + setOption(mongoOptions, key, descriptor, values); + } } if (mongoOptions.credentials) { @@ -383,7 +389,7 @@ export function parseOptions( const isOidc = mongoOptions.credentials.mechanism === AuthMechanism.MONGODB_OIDC; if ( (isGssapi || isX509) && - allOptions.has('authSource') && + allProvidedOptions.has('authSource') && mongoOptions.credentials.source !== '$external' ) { // If authSource was explicitly given and its incorrect, we error @@ -395,7 +401,7 @@ export function parseOptions( if ( !(isGssapi || isX509 || isAws || isOidc) && mongoOptions.dbName && - !allOptions.has('authSource') + !allProvidedOptions.has('authSource') ) { // inherit the dbName unless GSSAPI or X509, then silently ignore dbName // and there was no specific authSource given @@ -571,14 +577,9 @@ function setOption( descriptor: OptionDescriptor, values: unknown[] ) { - const { target, type, transform, deprecated } = descriptor; + const { target, type, transform } = descriptor; const name = target ?? key; - if (deprecated) { - const deprecatedMsg = typeof deprecated === 'string' ? `: ${deprecated}` : ''; - emitWarning(`${key} is a deprecated option${deprecatedMsg}`); - } - switch (type) { case 'boolean': mongoOptions[name] = getBoolean(name, values[0]); diff --git a/test/tools/uri_spec_runner.ts b/test/tools/uri_spec_runner.ts index 492043aa11b..e7f493f3897 100644 --- a/test/tools/uri_spec_runner.ts +++ b/test/tools/uri_spec_runner.ts @@ -336,7 +336,7 @@ export function executeUriValidationTest( case 'compressors': expect(options, `${errorMessage} ${optionKey}`) .to.have.property(optionKey) - .deep.equal(optionValue.concat('none')); // TODO(NODE-3923): remove unnecessary appending + .deep.equal(optionValue); break; case 'replicaset': // replicaset appears with both casings in the test expectations expect(options, `${errorMessage} replicaSet`) diff --git a/test/unit/connection_string.test.ts b/test/unit/connection_string.test.ts index 4a33259bb9b..f9284459cac 100644 --- a/test/unit/connection_string.test.ts +++ b/test/unit/connection_string.test.ts @@ -14,6 +14,7 @@ import { MongoOptions, MongoParseError, MongoRuntimeError, + OPTIONS, parseOptions, resolveSRVRecord } from '../mongodb'; @@ -641,4 +642,36 @@ describe('Connection String', function () { ]); }); }); + + context('default deprecated values', () => { + afterEach(() => sinon.restore()); + before('ensure that `keepAlive` is deprecated', () => { + const { deprecated } = OPTIONS.keepAlive; + expect(deprecated).to.exist; + }); + context('when no value is provided', () => { + it('uses the default value', () => { + const options = parseOptions('mongodb://localhost:27017'); + expect(options).to.have.property('keepAlive', true); + }); + it('does not emit a deprecation warning', async () => { + const spy = sinon.spy(process, 'emitWarning'); + parseOptions('mongodb://localhost:27017'); + expect(spy.called).to.be.false; + }); + }); + + context('when a value is provided', () => { + it('uses the provided value', () => { + const options = parseOptions('mongodb://localhost:27017?keepAlive=false'); + expect(options).to.have.property('keepAlive', false); + }); + it('emits a deprecation warning', async () => { + const spy = sinon.spy(process, 'emitWarning'); + parseOptions('mongodb://localhost:27017?keepAlive=false'); + expect(spy.called, 'expected a warning to be emitted, but none was').to.be.true; + expect(spy.getCalls()[0].args[0]).to.match(/keepAlive is a deprecated option/); + }); + }); + }); }); diff --git a/test/unit/mongo_client.test.js b/test/unit/mongo_client.test.js index 1ff539aaf3b..0a2e6130bf1 100644 --- a/test/unit/mongo_client.test.js +++ b/test/unit/mongo_client.test.js @@ -488,9 +488,7 @@ describe('MongoOptions', function () { const clientViaOpt = new MongoClient('mongodb://localhost', { compressors: ['zlib', 'snappy'] }); - expect(clientViaOpt.options) - .to.have.property('compressors') - .deep.equal(['zlib', 'snappy', 'none']); + expect(clientViaOpt.options).to.have.property('compressors').deep.equal(['zlib', 'snappy']); }); it('can be set when passed in as a comma-delimited string in the options object or URI', function () { @@ -498,12 +496,8 @@ describe('MongoOptions', function () { compressors: 'zlib,snappy' }); const clientViaUri = new MongoClient('mongodb://localhost?compressors=zlib,snappy'); - expect(clientViaOpt.options) - .to.have.property('compressors') - .deep.equal(['zlib', 'snappy', 'none']); - expect(clientViaUri.options) - .to.have.property('compressors') - .deep.equal(['zlib', 'snappy', 'none']); + expect(clientViaOpt.options).to.have.property('compressors').deep.equal(['zlib', 'snappy']); + expect(clientViaUri.options).to.have.property('compressors').deep.equal(['zlib', 'snappy']); }); it('should validate that a string or an array of strings is provided as input', function () { From 65f5357eb6a6b0aeb8b2bd4bbed94a03a1416614 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Mon, 17 Apr 2023 14:33:30 -0400 Subject: [PATCH 2/4] add typing to caseinsensitivemaps --- src/connection_string.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/connection_string.ts b/src/connection_string.ts index 85843ad734d..704be1183f0 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -263,7 +263,7 @@ export function parseOptions( mongoOptions.hosts = isSRV ? [] : hosts.map(HostAddress.fromString); - const urlOptions = new CaseInsensitiveMap(); + const urlOptions = new CaseInsensitiveMap(); if (url.pathname !== '/' && url.pathname !== '') { const dbName = decodeURIComponent( @@ -298,7 +298,7 @@ export function parseOptions( } } - const objectOptions = new CaseInsensitiveMap( + const objectOptions = new CaseInsensitiveMap( Object.entries(options).filter(([, v]) => v != null) ); @@ -326,10 +326,9 @@ export function parseOptions( if (objectOptionValue != null) { values.push(objectOptionValue); } - const urlValue = urlOptions.get(key); - if (urlValue != null) { - values.push(...urlValue); - } + + const urlValues = urlOptions.get(key) ?? []; + values.push(...urlValues); allProvidedOptions.set(key, values); } From 8abb8694e92ca791c271975e240e0fe4c340edfb Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Mon, 17 Apr 2023 14:34:57 -0400 Subject: [PATCH 3/4] add typing to caseinsensitivemaps --- src/connection_string.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/connection_string.ts b/src/connection_string.ts index 704be1183f0..59ba73d6405 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -316,7 +316,7 @@ export function parseOptions( // All option collection - const allProvidedOptions = new CaseInsensitiveMap(); + const allProvidedOptions = new CaseInsensitiveMap(); const allProvidedKeys = new Set([...urlOptions.keys(), ...objectOptions.keys()]); From 52b31cadc9ae41ce5b4f34d4ea092a6e1d08f760 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Mon, 17 Apr 2023 16:29:13 -0400 Subject: [PATCH 4/4] integration test for compression --- .../mongodb-handshake.test.ts | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/test/integration/mongodb-handshake/mongodb-handshake.test.ts b/test/integration/mongodb-handshake/mongodb-handshake.test.ts index fa4b7177921..154a6b991ae 100644 --- a/test/integration/mongodb-handshake/mongodb-handshake.test.ts +++ b/test/integration/mongodb-handshake/mongodb-handshake.test.ts @@ -1,5 +1,5 @@ import { expect } from 'chai'; -import * as sinon from 'sinon'; +import Sinon, * as sinon from 'sinon'; import { Connection, @@ -11,6 +11,8 @@ import { describe('MongoDB Handshake', () => { let client; + afterEach(() => client.close()); + context('when hello is too large', () => { before(() => { sinon.stub(Connection.prototype, 'command').callsFake(function (ns, cmd, options, callback) { @@ -42,4 +44,21 @@ describe('MongoDB Handshake', () => { expect(error).to.match(/client metadata document must be less/); }); }); + + context('when compressors are provided on the mongo client', () => { + let spy: Sinon.SinonSpy; + before(() => { + spy = sinon.spy(Connection.prototype, 'command'); + }); + + after(() => sinon.restore()); + + it('constructs a handshake with the specified compressors', async function () { + client = this.configuration.newClient({ compressors: ['snappy'] }); + await client.connect(); + expect(spy.called).to.be.true; + const handshakeDoc = spy.getCall(0).args[1]; + expect(handshakeDoc).to.have.property('compression').to.deep.equal(['snappy']); + }); + }); });