feat(NODE-4202): add FLE 2 behavior for create/drop collection #3218

addaleax · 2022-04-26T14:43:06Z

Description

See NODE-4202 / mongodb/specifications@0142955

What is changing?

Collection creation and dropping are extended to account for FLE2 properties according to spec.

Is there new documentation needed for these changes?

What is the motivation for this change?

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/operations/create_collection.ts

test/tools/spec-runner/index.js

durran · 2022-04-27T11:14:31Z

@addaleax Notes from our chat. Let's dry up the repeating calls to create collection and drop collection so we don't have the same code running for each fle collection. For the additional pings in the test runner we can add an option to the test runner's MongoClient: [Symbol.for('@@mdb.skipPingOnConnect')]: true that will skip the ping.

addaleax · 2022-04-27T11:52:27Z

Let's dry up the repeating calls to create collection and drop collection so we don't have the same code running for each fle collection.

@durran After thinking about using something like promisify/callbackify or using a custom async.series() equivalent, I’ve kind of come to the conclusion that it would really be easiest to just write async/await code here. I feel like that should be an acceptable way of doing things here.

For the additional pings in the test runner we can add an option to the test runner's MongoClient: [Symbol.for('@@mdb.skipPingOnConnect')]: true that will skip the ping.

This option is actually already set also for the legacy test runner:

node-mongodb-native/test/tools/spec-runner/index.js

Line 349 in 441fc63

[Symbol.for('@@mdb.skipPingOnConnect')]: true

I’ll try to figure out why this is happening (unless you happen to know)

EDIT: Ok, I think I figured it out – needed to pass the symbol down to the internal client created for the AutoEncrypter instance :)

src/encrypter.ts

src/operations/create_collection.ts

src/operations/drop.ts

test/tools/spec-runner/index.js

nbbeeken · 2022-04-29T19:25:20Z

src/operations/create_collection.ts

-    const db = this.db;
-    const name = this.name;
-    const options = this.options;
+    (async () => {


Unfortunately we can't use async/await just yet in the driver since we support providing third party promise libs (I know..), I'm sorry I think we have to revert to the callback style.

@durran am I missing something that makes this API exempt? I looked into FLE and it has its own promise-if-no-callback logic that will use the global promise but I don't think the driver will hit that since it will always pass in a callback to the library

@nbbeeken But aren’t third-party promise libs only relevant for the public API? This async function is not externally visible, and even if a client provides a different promise library, they can’t expect that other libraries (or even Node.js itself) don’t use async/await at least internally, right?

This is a really fair point 🤔 I also thought we could consider that new APIs don't have to abide by the deprecated option. We actually already have a case where snappy 7+ will use native promises, and we callbackify the API. I'm onboard then. This would be the first time we ship async await in the driver, so lemme get the rest of the team's attention. 2017 here we come :)

cc: @dariakp @baileympearson

src/operations/create_collection.ts

src/operations/drop.ts

Co-authored-by: Neal Beeken <[email protected]>

nbbeeken

LGTM

addaleax · 2022-05-02T15:13:09Z

src/operations/create_collection.ts

+
+      if (encryptedFields) {
+        // Create the required index for FLE2 support.
+        const createIndexOp = new CreateIndexOperation(db, name, { __safeContent__: 1 }, {});


Heads up: This may need to become a unique index to avoid easy-to-make user errors based on the slack conversation in https://mongodb.slack.com/archives/C0316KB5VM4/p1651503928461139?thread_ts=1651261523.846079&cid=C0316KB5VM4

Should we do this now or wait until it is spec'ed. I think it's pretty much assured we need to set unique. Is there a ticket to block on?

@nbbeeken @durran … and we decided not to do this since it would not work on sharded setups 🙂 So let’s stick to the spec here for now. We’ll probably have an ongoing conversation about this since I do think it’s a footgun, but this probably isn’t the solution.

Oh sorry - I had thought when I read the thread we wanted it and wanted to act quickly. :)

I thought so too! But yeah, makes sense that this doesn’t work :)

nbbeeken · 2022-05-02T19:19:32Z

src/operations/create_collection.ts

+
+      if (encryptedFields) {
+        // Create the required index for FLE2 support.
+        const createIndexOp = new CreateIndexOperation(db, name, { __safeContent__: 1 }, {});


Should we do this now or wait until it is spec'ed. I think it's pretty much assured we need to set unique. Is there a ticket to block on?

src/operations/create_collection.ts

This reverts commit 69db5e2.

dariakp

just one question here

src/operations/drop.ts

dariakp

just one question here

…tedFieldsMap

addaleax · 2022-05-03T16:48:27Z

src/operations/create_collection.ts

+
+        if (!options.encryptedFields) {
+          this.options = { ...this.options, encryptedFields };
+        }


Sorry, added this very late but I think this is a definite omission in the spec tests – opened mongodb/specifications#1205 so this doesn’t happen to anybody else

And now the spec tests are failing because the promoteValues fix from mongodb/libmongocrypt#324 is missing – I think that’s okay for me, unless it’s important that these tests here are skipped until that PR is merged & released

I think that's ok

baileympearson

Per Neal's direction, I only looked at the changes in create_collection.ts and the async await code. 👍 for a good idea here, LGTM

dariakp · 2022-05-03T17:38:56Z

src/operations/create_collection.ts

+
+        if (!options.encryptedFields) {
+          this.options = { ...this.options, encryptedFields };
+        }


I think that's ok

feat(NODE-4202): add FLE 2 behavior for create/drop collection

0029494

addaleax commented Apr 26, 2022

View reviewed changes

src/operations/create_collection.ts Show resolved Hide resolved

test/tools/spec-runner/index.js Show resolved Hide resolved

test/tools/spec-runner/index.js Show resolved Hide resolved

test/tools/spec-runner/index.js Show resolved Hide resolved

nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Apr 26, 2022

nbbeeken requested a review from durran April 26, 2022 16:04

fixup: async/await

263e240

fixup: forward skipPingOnConnect to internal client for FLE

d8d0f99

addaleax commented Apr 27, 2022

View reviewed changes

src/encrypter.ts Show resolved Hide resolved

durran reviewed Apr 27, 2022

View reviewed changes

src/operations/create_collection.ts Show resolved Hide resolved

durran requested changes Apr 27, 2022

View reviewed changes

src/operations/drop.ts Show resolved Hide resolved

src/operations/drop.ts Show resolved Hide resolved

test/tools/spec-runner/index.js Show resolved Hide resolved

test/tools/spec-runner/index.js Show resolved Hide resolved

durran previously approved these changes Apr 27, 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 Apr 27, 2022

alenakhineika mentioned this pull request Apr 28, 2022

feat: visually identify FLE2 collections MONGOSH-1204 mongodb-js/mongosh#1270

Merged

durran requested review from dariakp, nbbeeken and baileympearson April 28, 2022 15:28

nbbeeken requested changes Apr 29, 2022

View reviewed changes

addaleax dismissed durran’s stale review via 75c02b7 May 2, 2022 09:15

addaleax and others added 2 commits May 2, 2022 11:15

Apply suggestions from code review

75c02b7

Co-authored-by: Neal Beeken <[email protected]>

fixup: lint after review comment

7fbb5ce

nbbeeken approved these changes May 2, 2022

View reviewed changes

addaleax commented May 2, 2022

View reviewed changes

nbbeeken requested changes May 2, 2022

View reviewed changes

feat(NODE-4202): make safe content index unique

69db5e2

durran reviewed May 2, 2022

View reviewed changes

src/operations/create_collection.ts Outdated Show resolved Hide resolved

Revert "feat(NODE-4202): make safe content index unique"

4406524

This reverts commit 69db5e2.

dariakp reviewed May 3, 2022

View reviewed changes

src/operations/drop.ts Show resolved Hide resolved

dariakp reviewed May 3, 2022

View reviewed changes

addaleax added 2 commits May 3, 2022 18:16

fixup: fix review-comment error instanceof checks

244268e

fixup: pass encryptedFields in create if it was passed through encryp…

08cc07e

…tedFieldsMap

addaleax commented May 3, 2022

View reviewed changes

fixup: parse fle CreateCollection spec tests with non-relaxed EJSON

55cfc4e

baileympearson approved these changes May 3, 2022

View reviewed changes

nbbeeken approved these changes May 3, 2022

View reviewed changes

dariakp approved these changes May 3, 2022

View reviewed changes

durran merged commit 6d3947b into mongodb:main May 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NODE-4202): add FLE 2 behavior for create/drop collection #3218

feat(NODE-4202): add FLE 2 behavior for create/drop collection #3218

addaleax commented Apr 26, 2022

durran commented Apr 27, 2022

addaleax commented Apr 27, 2022 •

edited

Loading

nbbeeken Apr 29, 2022

addaleax May 2, 2022 •

edited

Loading

nbbeeken May 2, 2022

nbbeeken left a comment

addaleax May 2, 2022

nbbeeken May 2, 2022

addaleax May 3, 2022

durran May 3, 2022

addaleax May 3, 2022

nbbeeken May 2, 2022

dariakp left a comment

dariakp left a comment

addaleax May 3, 2022

addaleax May 3, 2022

dariakp May 3, 2022

baileympearson left a comment

dariakp May 3, 2022

feat(NODE-4202): add FLE 2 behavior for create/drop collection #3218

feat(NODE-4202): add FLE 2 behavior for create/drop collection #3218

Conversation

addaleax commented Apr 26, 2022

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Double check the following

durran commented Apr 27, 2022

addaleax commented Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

addaleax May 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbbeeken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baileympearson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Apr 27, 2022 •

edited

Loading

addaleax May 2, 2022 •

edited

Loading