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

baileympearson · 2022-02-08T21:00:17Z

Description

What is changing?

This PR does two things:

fixes options parsing for arrays of options.
refactors how we parse key-value pairs of options for a particular option. In particular, it removes logic from parsing readPreference that was converting string values to native JS types, causing spec tests to fail

Is there new documentation needed for these changes?

No.

Double check the following

Ran npm run check:lint script
Self-review completed using the steps outlined here
PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
Changes are covered by tests
New TODOs have a related JIRA ticket

src/connection_string.ts

durran · 2022-02-09T15:28:36Z

CSFLE failures unrelated and fixed in #3070

src/connection_string.ts

dariakp

Please make sure to either unskip or update the TODO tagged to this ticket in uri_options.spec.test

durran · 2022-02-09T20:35:26Z

There's an eslint error on latest: https://spruce.mongodb.com/task/mongo_node_driver_next_lint_run_checks_patch_44bbd6e94d58fac9eb07a07e5437662eb6409dc5_62041da20305b97e46960d87_22_02_09_20_01_39/logs?execution=0

src/connection_string.ts

nbbeeken

LGTM

dariakp · 2022-02-11T18:50:12Z

src/connection_string.ts

+        for (const [key, _value] of entriesFromString(value)) {
+          if (validKeys.includes(key)) {
+            if (key === 'CANONICALIZE_HOST_NAME') {
+              mechanismProperties[key] = getBoolean(key, _value);


Just a note that this is probably going to conflict with Durran's PR to expand the CANONICALIZE_HOST_NAME options (#3131)

dariakp · 2022-02-11T18:51:09Z

src/connection_string.ts

@@ -631,7 +634,28 @@ export const OPTIONS = {
    target: 'credentials',
    transform({ options, values: [value] }): MongoCredentials {
      if (typeof value === 'string') {
-        value = toRecord(value);
+        const validKeys = [


Are we going to intentionally silently ignore invalid keys here? Does this actually produce the same behavior as before?

This array might be unnecessary, the idea was to only translate the type of the keys we know about and leave whatever remains as is (strings). I spoke generally about the options, so that might be where the inspiration came from but I think CANONICALIZE_HOST_NAME, and gssapiCannoncializeHostname (which we need to add here, whoops!) are the only ones that need translating to boolean. the rest can remain as strings

baileympearson commented Feb 9, 2022

View reviewed changes

src/connection_string.ts Show resolved Hide resolved

baileympearson commented Feb 9, 2022

View reviewed changes

src/connection_string.ts Show resolved Hide resolved

baileympearson marked this pull request as ready for review February 9, 2022 14:50

durran added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Feb 9, 2022

durran requested changes Feb 9, 2022

View reviewed changes

src/connection_string.ts Outdated Show resolved Hide resolved

dariakp requested changes Feb 9, 2022

View reviewed changes

baileympearson force-pushed the NODE-3813-parse-readpreference-tags-correctly branch from 6545f24 to 991c0eb Compare February 9, 2022 20:01

baileympearson requested review from dariakp and durran February 9, 2022 20:13

durran previously approved these changes Feb 10, 2022

View reviewed changes

durran added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Feb 10, 2022

nbbeeken reviewed Feb 10, 2022

View reviewed changes

src/connection_string.ts Outdated Show resolved Hide resolved

nbbeeken requested changes Feb 10, 2022

View reviewed changes

src/connection_string.ts Outdated Show resolved Hide resolved

baileympearson dismissed durran’s stale review via c996fa4 February 10, 2022 18:52

baileympearson force-pushed the NODE-3813-parse-readpreference-tags-correctly branch from a9195e3 to c996fa4 Compare February 10, 2022 18:52

baileympearson commented Feb 10, 2022

View reviewed changes

src/connection_string.ts Show resolved Hide resolved

baileympearson requested review from nbbeeken and durran February 10, 2022 19:21

nbbeeken requested changes Feb 10, 2022

View reviewed changes

src/connection_string.ts Outdated Show resolved Hide resolved

src/connection_string.ts Outdated Show resolved Hide resolved

nbbeeken requested changes Feb 10, 2022

View reviewed changes

src/connection_string.ts Outdated Show resolved Hide resolved

baileympearson requested a review from nbbeeken February 11, 2022 17:03

nbbeeken requested changes Feb 11, 2022

View reviewed changes

src/connection_string.ts Outdated Show resolved Hide resolved

baileympearson added 5 commits February 11, 2022 13:20

fix parsing of read preference tags

3e2c58f

refactor the parsing a bit

ae98239

clean up logic when flattening options object

c9c0b56

remove console.error

4295b5a

fix implicit type conversion issue in options parser

3fe640d

baileympearson added 7 commits February 11, 2022 13:20

Refactor toArray to use OneOrMore utility type

d23b27f

Validate authMechanismProperties

43a6a3d

rename parseobjectstring

f48e477

create authMechanismProperties object as Object.null

b8b8a03

use emitwarningonce instead of emitwarning

aa5fda7

Address PR comments

98fb89e

Update import

98b1448

baileympearson force-pushed the NODE-3813-parse-readpreference-tags-correctly branch from c93902e to 98b1448 Compare February 11, 2022 18:20

Add lint rule to prevent '.' imports

9edcfe6

nbbeeken previously approved these changes Feb 11, 2022

View reviewed changes

dariakp reviewed Feb 11, 2022

View reviewed changes

revert new logic when parsing authMechanismProperties

54c5560

baileympearson dismissed nbbeeken’s stale review via 54c5560 February 11, 2022 20:11

dariakp approved these changes Feb 11, 2022

View reviewed changes

nbbeeken approved these changes Feb 11, 2022

View reviewed changes

nbbeeken changed the title ~~fix(NODE-3813): fix parsing of read preference tags~~ fix(NODE-3813): unexpected type conversion of read preference tags Feb 11, 2022

nbbeeken merged commit 3e7b894 into main Feb 11, 2022

nbbeeken deleted the NODE-3813-parse-readpreference-tags-correctly branch February 11, 2022 23:24

baileympearson mentioned this pull request Apr 7, 2022

fix(NODE-4031): options parsing for array options #3193

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

baileympearson commented Feb 8, 2022 •

edited

Loading

durran commented Feb 9, 2022

dariakp left a comment

durran commented Feb 9, 2022

nbbeeken left a comment

dariakp Feb 11, 2022 •

edited

Loading

dariakp Feb 11, 2022

nbbeeken Feb 11, 2022

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

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

Conversation

baileympearson commented Feb 8, 2022 • edited Loading

Description

What is changing?

Is there new documentation needed for these changes?

Double check the following

durran commented Feb 9, 2022

dariakp left a comment

Choose a reason for hiding this comment

durran commented Feb 9, 2022

nbbeeken left a comment

Choose a reason for hiding this comment

dariakp Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

dariakp Feb 11, 2022

Choose a reason for hiding this comment

nbbeeken Feb 11, 2022

Choose a reason for hiding this comment

baileympearson commented Feb 8, 2022 •

edited

Loading

dariakp Feb 11, 2022 •

edited

Loading