From 71bb195e470f95691c4e0e65098cdeb474d19fb1 Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 2 Aug 2023 13:52:20 -0400 Subject: [PATCH 1/6] feat(NODE-3989)!: only accept true and false for boolean options --- src/connection_string.ts | 28 ++++++--------------- test/unit/connection_string.test.ts | 39 +++++++++++++++++++++++------ 2 files changed, 39 insertions(+), 28 deletions(-) diff --git a/src/connection_string.ts b/src/connection_string.ts index 0d8e511eb0e..7633212194b 100644 --- a/src/connection_string.ts +++ b/src/connection_string.ts @@ -33,7 +33,6 @@ import type { TagSet } from './sdam/server_description'; import { DEFAULT_PK_FACTORY, emitWarning, - emitWarningOnce, HostAddress, isRecord, matchesParentDomain, @@ -162,29 +161,16 @@ function checkTLSOptions(allOptions: CaseInsensitiveMap): void { check('tlsAllowInvalidCertificates', 'tlsDisableOCSPEndpointCheck'); check('tlsDisableCertificateRevocationCheck', 'tlsDisableOCSPEndpointCheck'); } - -const TRUTHS = new Set(['true', 't', '1', 'y', 'yes']); -const FALSEHOODS = new Set(['false', 'f', '0', 'n', 'no', '-1']); function getBoolean(name: string, value: unknown): boolean { if (typeof value === 'boolean') return value; - const valueString = String(value).toLowerCase(); - if (TRUTHS.has(valueString)) { - if (valueString !== 'true') { - emitWarningOnce( - `deprecated value for ${name} : ${valueString} - please update to ${name} : true instead` - ); - } - return true; - } - if (FALSEHOODS.has(valueString)) { - if (valueString !== 'false') { - emitWarningOnce( - `deprecated value for ${name} : ${valueString} - please update to ${name} : false instead` - ); - } - return false; + switch (value) { + case 'true': + return true; + case 'false': + return false; + default: + throw new MongoParseError(`${name} must be either "true" or "false"`); } - throw new MongoParseError(`Expected ${name} to be stringified boolean value, got: ${value}`); } function getIntFromOptions(name: string, value: unknown): number { diff --git a/test/unit/connection_string.test.ts b/test/unit/connection_string.test.ts index 0439d49984c..f8e5c2199bd 100644 --- a/test/unit/connection_string.test.ts +++ b/test/unit/connection_string.test.ts @@ -208,13 +208,38 @@ describe('Connection String', function () { }); }); - it('should parse boolean values', function () { - let options = parseOptions('mongodb://hostname?retryWrites=1'); - expect(options.retryWrites).to.equal(true); - options = parseOptions('mongodb://hostname?retryWrites=false'); - expect(options.retryWrites).to.equal(false); - options = parseOptions('mongodb://hostname?retryWrites=t'); - expect(options.retryWrites).to.equal(true); + context('boolean options', function () { + const valuesExpectations: { value: string; expectation: 'error' | boolean }[] = [ + { value: 'true', expectation: true }, + { value: 'false', expectation: false }, + { value: '-1', expectation: 'error' }, + { value: '1', expectation: 'error' }, + { value: '0', expectation: 'error' }, + { value: 't', expectation: 'error' }, + { value: 'f', expectation: 'error' }, + { value: 'n', expectation: 'error' }, + { value: 'y', expectation: 'error' }, + { value: 'yes', expectation: 'error' }, + { value: 'no', expectation: 'error' }, + { value: 'unknown', expectation: 'error' } + ]; + for (const { value, expectation } of valuesExpectations) { + const connString = `mongodb://hostname?retryWrites=${value}`; + context(`when provided '${value}'`, function () { + if (expectation === 'error') { + it('throws MongoParseError', function () { + expect(() => { + parseOptions(connString); + }).to.throw(MongoParseError); + }); + } else { + it(`parses as ${expectation}`, function () { + const options = parseOptions(connString); + expect(options).to.have.property('retryWrites', expectation); + }); + } + }); + } }); it('should parse compression options', function () { From 7969c7be08cdfdaa14fa9ee535d699d3581f552c Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 2 Aug 2023 13:58:39 -0400 Subject: [PATCH 2/6] test(NODE-3989): update unit test --- test/unit/connection_string.test.ts | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/test/unit/connection_string.test.ts b/test/unit/connection_string.test.ts index f8e5c2199bd..dc2cf67381b 100644 --- a/test/unit/connection_string.test.ts +++ b/test/unit/connection_string.test.ts @@ -226,17 +226,19 @@ describe('Connection String', function () { for (const { value, expectation } of valuesExpectations) { const connString = `mongodb://hostname?retryWrites=${value}`; context(`when provided '${value}'`, function () { - if (expectation === 'error') { - it('throws MongoParseError', function () { - expect(() => { - parseOptions(connString); - }).to.throw(MongoParseError); - }); - } else { - it(`parses as ${expectation}`, function () { - const options = parseOptions(connString); - expect(options).to.have.property('retryWrites', expectation); - }); + switch (expectation) { + case 'error': + it('throws MongoParseError', function () { + expect(() => { + parseOptions(connString); + }).to.throw(MongoParseError); + }); + break; + default: + it(`parses as ${expectation}`, function () { + const options = parseOptions(connString); + expect(options).to.have.property('retryWrites', expectation); + }); } }); } From 6d4d24429f3c38ae71086b6de0c3b60c2e1a78ae Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 2 Aug 2023 14:14:40 -0400 Subject: [PATCH 3/6] test(NODE-3989): skip spec test --- test/unit/assorted/uri_options.spec.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/unit/assorted/uri_options.spec.test.ts b/test/unit/assorted/uri_options.spec.test.ts index 8420ff38709..93927ec3f34 100644 --- a/test/unit/assorted/uri_options.spec.test.ts +++ b/test/unit/assorted/uri_options.spec.test.ts @@ -14,7 +14,10 @@ describe('URI option spec tests', function () { 'tlsDisableCertificateRevocationCheck can be set to true', 'tlsDisableCertificateRevocationCheck can be set to false', 'tlsDisableOCSPEndpointCheck can be set to true', - 'tlsDisableOCSPEndpointCheck can be set to false' + 'tlsDisableOCSPEndpointCheck can be set to false', + + // Skipped because we now only accept true/false for boolean options + 'Invalid loadBalanced value' ]; const testsThatDoNotThrowOnWarn = [ From 5d8731d088fd108e09020e1819846cc9f73a1124 Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 3 Aug 2023 09:50:01 -0400 Subject: [PATCH 4/6] Update test/unit/connection_string.test.ts Co-authored-by: Durran Jordan --- test/unit/connection_string.test.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/unit/connection_string.test.ts b/test/unit/connection_string.test.ts index dc2cf67381b..5a23b8c7d16 100644 --- a/test/unit/connection_string.test.ts +++ b/test/unit/connection_string.test.ts @@ -226,15 +226,13 @@ describe('Connection String', function () { for (const { value, expectation } of valuesExpectations) { const connString = `mongodb://hostname?retryWrites=${value}`; context(`when provided '${value}'`, function () { - switch (expectation) { - case 'error': + if (expectation === 'error') { it('throws MongoParseError', function () { expect(() => { parseOptions(connString); }).to.throw(MongoParseError); }); - break; - default: + } else { it(`parses as ${expectation}`, function () { const options = parseOptions(connString); expect(options).to.have.property('retryWrites', expectation); From 0d153589866dbb2e9b1059935fac81fa8c75c205 Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 3 Aug 2023 10:08:28 -0400 Subject: [PATCH 5/6] test(NODE-3989): expand on skipp comment --- test/unit/assorted/uri_options.spec.test.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/test/unit/assorted/uri_options.spec.test.ts b/test/unit/assorted/uri_options.spec.test.ts index 93927ec3f34..f6ac356df9f 100644 --- a/test/unit/assorted/uri_options.spec.test.ts +++ b/test/unit/assorted/uri_options.spec.test.ts @@ -16,17 +16,16 @@ describe('URI option spec tests', function () { 'tlsDisableOCSPEndpointCheck can be set to true', 'tlsDisableOCSPEndpointCheck can be set to false', - // Skipped because we now only accept true/false for boolean options + // NOTE(NODE-3989): Skipped because we now only accept true/false for boolean options. The spec expects us to + // warn on other accepted but deprecated values, but as of NODE-3989, we now throw on these + // values 'Invalid loadBalanced value' ]; const testsThatDoNotThrowOnWarn = [ // TODO(NODE-3923): compression option validation 'Too high zlibCompressionLevel causes a warning', - 'Too low zlibCompressionLevel causes a warning', - - // TODO(NODE-3989): Fix legacy boolean parsing - 'Invalid loadBalanced value' + 'Too low zlibCompressionLevel causes a warning' ]; for (const suite of suites) { From 765d643810924a6c1fe4e2cb694a5914cfc5147a Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 3 Aug 2023 15:26:42 -0400 Subject: [PATCH 6/6] style(NODE-3989): eslint --- test/unit/connection_string.test.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/unit/connection_string.test.ts b/test/unit/connection_string.test.ts index 5a23b8c7d16..f8e5c2199bd 100644 --- a/test/unit/connection_string.test.ts +++ b/test/unit/connection_string.test.ts @@ -227,16 +227,16 @@ describe('Connection String', function () { const connString = `mongodb://hostname?retryWrites=${value}`; context(`when provided '${value}'`, function () { if (expectation === 'error') { - it('throws MongoParseError', function () { - expect(() => { - parseOptions(connString); - }).to.throw(MongoParseError); - }); + it('throws MongoParseError', function () { + expect(() => { + parseOptions(connString); + }).to.throw(MongoParseError); + }); } else { - it(`parses as ${expectation}`, function () { - const options = parseOptions(connString); - expect(options).to.have.property('retryWrites', expectation); - }); + it(`parses as ${expectation}`, function () { + const options = parseOptions(connString); + expect(options).to.have.property('retryWrites', expectation); + }); } }); }