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

fix(NODE-3813): unexpected type conversion of read preference tags #3138

Merged
merged 14 commits into from
Feb 11, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,17 @@
"global"
],
"@typescript-eslint/no-explicit-any": "off",
"no-restricted-imports": [
"error",
{
"paths": [
{
"name": ".",
"message": "Please import directly from the relevant file instead."
}
]
}
],
"no-restricted-syntax": [
"error",
{
Expand Down
97 changes: 56 additions & 41 deletions src/connection_string.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
ServerApi,
ServerApiVersion
} from './mongo_client';
import type { OneOrMore } from './mongo_types';
import { PromiseProvider } from './promise_provider';
import { ReadConcern, ReadConcernLevel } from './read_concern';
import { ReadPreference, ReadPreferenceMode } from './read_preference';
Expand All @@ -28,6 +29,7 @@ import {
Callback,
DEFAULT_PK_FACTORY,
emitWarning,
emitWarningOnce,
HostAddress,
isRecord,
makeClientMetadata,
Expand Down Expand Up @@ -184,8 +186,22 @@ 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)) return true;
if (FALSEHOODS.has(valueString)) return false;
if (TRUTHS.has(valueString)) {
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
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;
}
throw new MongoParseError(`Expected ${name} to be stringified boolean value, got: ${value}`);
}

Expand All @@ -204,31 +220,24 @@ function getUint(name: string, value: unknown): number {
return parsedValue;
}

function toRecord(value: string): Record<string, any> {
const record = Object.create(null);
/** Wrap a single value in an array if the value is not an array */
function toArray<T>(value: OneOrMore<T>): T[] {
return Array.isArray(value) ? value : [value];
}

function* entriesFromString(value: string) {
const keyValuePairs = value.split(',');
for (const keyValue of keyValuePairs) {
const [key, value] = keyValue.split(':');
if (value == null) {
throw new MongoParseError('Cannot have undefined values in key value pairs');
}
try {
// try to get a boolean
record[key] = getBoolean('', value);
} catch {
try {
// try to get a number
record[key] = getInt('', value);
} catch {
// keep value as a string
record[key] = value;
}
}

yield [key, value];
}
return record;
}

class CaseInsensitiveMap extends Map<string, any> {
class CaseInsensitiveMap<Value = any> extends Map<string, Value> {
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
constructor(entries: Array<[string, any]> = []) {
super(entries.map(([k, v]) => [k.toLowerCase(), v]));
}
Expand Down Expand Up @@ -262,7 +271,7 @@ export function parseOptions(
const mongoOptions = Object.create(null);
mongoOptions.hosts = isSRV ? [] : hosts.map(HostAddress.fromString);

const urlOptions = new CaseInsensitiveMap();
const urlOptions = new CaseInsensitiveMap<any[]>();

if (url.pathname !== '/' && url.pathname !== '') {
const dbName = decodeURIComponent(
Expand Down Expand Up @@ -324,16 +333,11 @@ export function parseOptions(
]);

for (const key of allKeys) {
const values = [];
if (objectOptions.has(key)) {
values.push(objectOptions.get(key));
}
if (urlOptions.has(key)) {
values.push(...urlOptions.get(key));
}
if (DEFAULT_OPTIONS.has(key)) {
values.push(DEFAULT_OPTIONS.get(key));
}
const values = [objectOptions, urlOptions, DEFAULT_OPTIONS].flatMap(optionsObject => {
const options = optionsObject.get(key) ?? [];
return toArray(options);
});

allOptions.set(key, values);
}

Expand Down Expand Up @@ -478,12 +482,11 @@ export function parseOptions(
throw new MongoParseError('Can only specify both of proxy username/password or neither');
}

if (
urlOptions.get('proxyHost')?.length > 1 ||
urlOptions.get('proxyPort')?.length > 1 ||
urlOptions.get('proxyUsername')?.length > 1 ||
urlOptions.get('proxyPassword')?.length > 1
) {
const proxyOptions = ['proxyHost', 'proxyPort', 'proxyUsername', 'proxyPassword'].map(
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
key => urlOptions.get(key) ?? []
);

if (proxyOptions.some(options => options.length > 1)) {
throw new MongoParseError(
'Proxy options cannot be specified multiple times in the connection string'
);
Expand Down Expand Up @@ -629,14 +632,26 @@ export const OPTIONS = {
},
authMechanismProperties: {
target: 'credentials',
transform({ options, values: [value] }): MongoCredentials {
if (typeof value === 'string') {
value = toRecord(value);
transform({ options, values: [optionValue] }): MongoCredentials {
if (typeof optionValue === 'string') {
const mechanismProperties = Object.create(null);

for (const [key, value] of entriesFromString(optionValue)) {
try {
mechanismProperties[key] = getBoolean(key, value);
} catch {
mechanismProperties[key] = value;
}
}

return MongoCredentials.merge(options.credentials, {
mechanismProperties
});
}
if (!isRecord(value)) {
if (!isRecord(optionValue)) {
throw new MongoParseError('AuthMechanismProperties must be an object');
}
return MongoCredentials.merge(options.credentials, { mechanismProperties: value });
return MongoCredentials.merge(options.credentials, { mechanismProperties: optionValue });
}
},
authSource: {
Expand Down Expand Up @@ -965,7 +980,7 @@ export const OPTIONS = {
for (const tag of values) {
const readPreferenceTag: TagSet = Object.create(null);
if (typeof tag === 'string') {
for (const [k, v] of Object.entries(toRecord(tag))) {
for (const [k, v] of entriesFromString(tag)) {
readPreferenceTag[k] = v;
}
}
Expand Down
6 changes: 6 additions & 0 deletions test/tools/uri_spec_runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,12 @@ export function executeUriValidationTest(
.to.have.nested.property(expectedProp)
.deep.equal(optionValue);
break;
case 'maxStalenessSeconds':
expectedProp = 'readPreference.maxStalenessSeconds';
expect(options, `${errorMessage} ${optionKey} -> ${expectedProp}`)
.to.have.nested.property(expectedProp)
.deep.equal(optionValue);
break;

//** WRITE CONCERN OPTIONS **/
case 'w':
Expand Down
5 changes: 1 addition & 4 deletions test/unit/assorted/uri_options.spec.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,7 @@ 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',

// TODO(NODE-3813): read preference tag issue: parsing rack:1 as rack:true
'Valid read preference options are parsed correctly'
'tlsDisableOCSPEndpointCheck can be set to false'
];

const testsThatDoNotThrowOnWarn = [
Expand Down
19 changes: 14 additions & 5 deletions test/unit/connection_string.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,20 @@ describe('Connection String', function () {
expect(options.hosts[0].port).to.equal(27017);
});

it('should parse multiple readPreferenceTags', function () {
const options = parseOptions(
'mongodb://hostname?readPreferenceTags=bar:foo&readPreferenceTags=baz:bar'
);
expect(options.readPreference.tags).to.deep.equal([{ bar: 'foo' }, { baz: 'bar' }]);
context('readPreferenceTags', function () {
it('should parse multiple readPreferenceTags when passed in the uri', () => {
const options = parseOptions(
'mongodb://hostname?readPreferenceTags=bar:foo&readPreferenceTags=baz:bar'
);
expect(options.readPreference.tags).to.deep.equal([{ bar: 'foo' }, { baz: 'bar' }]);
});

it('should parse multiple readPreferenceTags when passed in options object', () => {
const options = parseOptions('mongodb://hostname?', {
readPreferenceTags: [{ bar: 'foo' }, { baz: 'bar' }]
});
expect(options.readPreference.tags).to.deep.equal([{ bar: 'foo' }, { baz: 'bar' }]);
});
});

it('should parse boolean values', function () {
Expand Down