feat(NODE-6773): add support for $lookup on encrypted collections #4427

nbbeeken · 2025-02-19T23:08:15Z

Description

What is changing?

adds tests for lookup support

Is there new documentation needed for these changes?

What is the motivation for this change?

Release Highlight

Add support for $lookup on encrypted collections

After upgrading to mongodb-client-encryption 6.3.0 or later, this version of the driver will now feed multiple collection schema data into the encryption library so that $lookup aggregation stages can be used on encrypted collections. 🔒 🎉

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
- Example: feat(NODE-1234)!: rewriting everything in coffeescript
Changes are covered by tests
New TODOs have a related JIRA ticket

nbbeeken · 2025-02-21T15:54:13Z

Will update the bindings when they are available, we can still start reviewing what is in here in the meantime

baileympearson

There are pending spec test changes from Maxim, I'll re-review the tests when Kevin has updated the prose tests in his PR.

baileympearson · 2025-02-24T17:18:35Z

src/client-side-encryption/auto_encrypter.ts

@@ -239,6 +239,8 @@ export class AutoEncrypter {
    this._kmsProviders = options.kmsProviders || {};

    const mongoCryptOptions: MongoCryptOptions = {
+      //@ts-expect-error: not yet in the defs


TODO: remove before merging.

baileympearson · 2025-02-24T17:21:27Z

src/client-side-encryption/state_machine.ts

-            context.addMongoOperationResponse(collInfo);
+
+          for await (const collInfo of collInfoCursor) {
+            context.addMongoOperationResponse(serialize(collInfo));


addMongoOperationResponse squashes errors (oops).

Can we add error handling, similar to C's approach? This will require bindings changes.

kevinAlbs/mongo-c-driver@eb18fc8

What bindings changes are needed? Can the context.state be checked after each call here?

I suppose, but I think that goes against our existing patterns in mongodb-client-encryption.

We don't provide a wrapper around mongocrypt_status_message, so we wouldn't be able to throw the error message that libmongocrypt generates.

Most (all?) other bindings functions that call libmongocrypt functions that can error handle the error inline and throw an error if we encounter one, instead of returning nothing and relying on the state machine to handle it.

Ex:

if (!mongocrypt_ctx_encrypt_init(context.get(), ns.c_str(), ns.size(), binaryCommand.get())) { throw TypeError::New(Env(), errorStringFromStatus(context.get())); }

so I'm imagining something like:

void MongoCryptContext::AddMongoOperationResponse(const CallbackInfo& info) { Uint8Array buffer = Uint8ArrayFromValue(info[0], "buffer"); std::unique_ptr<mongocrypt_binary_t, MongoCryptBinaryDeleter> reply_bson( Uint8ArrayToBinary(buffer)); if (!mongocrypt_ctx_mongo_feed(context(), reply_bson.get())) { throw Error::New(Env(), errorStringFromStatus(context())); } }

Why isn't this problematic for existing driver releases that don't expect this method to throw?

From slack: the impact of a bindings change is multi-version and multi-codepath and released drivers won't have handling to throw the appropriate error.

I added handling for an error status in the collection info loop and we do have access to the error message via status.

What kind of errors could be encountered here?

baileympearson · 2025-02-24T17:22:47Z

src/client-side-encryption/state_machine.ts

    client: MongoClient,
    ns: string,
    filter: Document,
    options?: { timeoutContext?: TimeoutContext } & Abortable
-  ): Promise<Uint8Array | null> {
+  ): ListCollectionsCursor<CollectionInfo> {


(optional)

Suggested change

): ListCollectionsCursor<CollectionInfo> {

): AsyncIterable<CollectionInfo> {

I don't see an issue with a future change here that uses other cursor APIs, so I see this as friction for the next person. What's the gain?

Just preference, no need to leak the implementation if it isn't needed.

test/integration/client-side-encryption/client_side_encryption.prose.25.lookup.test.ts

baileympearson · 2025-02-24T17:42:15Z

test/integration/client-side-encryption/client_side_encryption.prose.25.lookup.test.ts

+      for (const { name, document } of collections) {
+        const { insertedId } = await encryptedClient.db('db').collection(name).insertOne(document);
+
+        if (name.startsWith('no_')) continue;
+
+        expect(await client.db('db').collection(name).findOne(insertedId))
+          .to.have.property(Object.keys(document)[0])
+          .that.has.property('_bsontype', 'Binary');


How would you feel about removing the looping in favor of simply creating each collection explicitly and inserting documents explicitly? I think complexity-wise it's about the same but the explicit approach is easier to compare with the spec test line-by-line.

I wouldn't be in favor, loops are a reasonable reduction of verbatim code complexity, and table testing is a common pattern so you know X is applied the same way to all inputs, rather than missing a small deviation between actual code paths.

loops are a reasonable reduction of verbatim code complexity

But here all we should need is 6 create collections calls, and 4 insert calls? I don't think a loop reduces much complexity here. And as a reviewer, comparing this with the spec, I'd much rather compare line-by-line than need to cross reference the spec, the table, and the multiple loops used here.

table testing is a common pattern so you know X is applied the same way to all inputs

Table testing is great for pure input -> output testing but breaks down in its efficacy when the test logic isn't identical for each test case. In this case (and the table tests below), the loop has different logic for different tests, which obfuscates both the intent of the table testing matrix and the actual test logic.

baileympearson · 2025-02-24T17:44:04Z

test/integration/client-side-encryption/client_side_encryption.prose.25.lookup.test.ts

+        await client.close();
+      });
+
+      it(title.slice(title.indexOf(':') + 1).trim(), metadata ?? defaultMetadata, async () => {


Similar to the above, how would you feel about just writing 9 test cases explictly?

This makes it easier to compare the implementation to the spec.

Would not be in favor for the same reasons. Also seems like this approach was reasonably grok-able by someone not familiar with our tests: https://mongodb.slack.com/archives/C0668RJBA0J/p1740413020860229?thread_ts=1740405607.463789&cid=C0668RJBA0J

So, I'm not feeling particularly strong enough to continue arguing this, but as someone intimately familiar with our tests, I am telling you that this was annoying to grok. Especially compared to something like

node-mongodb-native/test/manual/search-index-management.prose.test.ts

Line 138 in cb6a7ba

async function () {

baileympearson · 2025-02-24T17:46:49Z

test/integration/client-side-encryption/client_side_encryption.prose.25.lookup.test.ts

+    expected: Document,
+    metadata?: MongoDBMetadataUI
+  ) {
+    describe(title.slice(0, title.indexOf(':')), function () {


I think we've discussed ensuring that prose test titles match the spec, to make it possible to search test output. (i.e., with this structure, searching Case 3: db.no_schemajoinsdb.csfle`` returns nothing because it is split across the describe + test)

Do you remember this as well or am I imagining things?

It is still searchable without the "case" portion, I don't think it would cause too much pain to find.

Happy to change it but I need something to put in the suite name that changes for each test so I bind the correct before and afters.

I don't recall an explicit discussion on this but we have been doing it so we can tie things together, you wouldn't spend more than a few moments cutting down you're query in vscode/parsely/mocha to find/filter the test

nbbeeken mentioned this pull request Feb 19, 2025

feat(NODE-6773): add multiple collinfo option mongodb-js/mongodb-client-encryption#70

Open

5 tasks

nbbeeken force-pushed the NODE-6774-fle-lookup branch 2 times, most recently from 813223f to ffb01de Compare February 21, 2025 15:26

nbbeeken marked this pull request as ready for review February 21, 2025 15:53

nbbeeken requested a review from a team as a code owner February 21, 2025 15:53

baileympearson changed the title ~~feat(NODE-6774): add support for $lookup on encrypted collections~~ feat(NODE-6773): add support for $lookup on encrypted collections Feb 24, 2025

baileympearson requested changes Feb 24, 2025

View reviewed changes

nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Feb 24, 2025

nbbeeken assigned baileympearson Feb 24, 2025

nbbeeken added the wip label Feb 24, 2025

nbbeeken added 9 commits February 24, 2025 17:18

feat(NODE-6774): support $lookup on encrypted collections

28fd864

chore: add options

f31c0f8

test: multiple collection fails with a specific error when not enabled

8be80da

chore: fixes

54c59df

chore: try fixing tests

99ded3a

chore: version filter

4551856

chore: lint

b076b57

chore: wrong filter

9aa5279

chore: more metadata

3d579b4

nbbeeken force-pushed the NODE-6774-fle-lookup branch from ab77f26 to 3d579b4 Compare February 24, 2025 22:18

chore: run on cryptd too

715c23d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NODE-6773): add support for $lookup on encrypted collections #4427

feat(NODE-6773): add support for $lookup on encrypted collections #4427

nbbeeken commented Feb 19, 2025 •

edited

Loading

nbbeeken commented Feb 21, 2025

baileympearson left a comment

baileympearson Feb 24, 2025

baileympearson Feb 24, 2025

nbbeeken Feb 24, 2025

baileympearson Feb 24, 2025

nbbeeken Feb 24, 2025

nbbeeken Feb 24, 2025

baileympearson Feb 24, 2025

nbbeeken Feb 24, 2025

baileympearson Feb 24, 2025

baileympearson Feb 24, 2025

nbbeeken Feb 24, 2025

baileympearson Feb 24, 2025 •

edited by nbbeeken

Loading

baileympearson Feb 24, 2025

nbbeeken Feb 24, 2025

baileympearson Feb 24, 2025

baileympearson Feb 24, 2025

nbbeeken Feb 24, 2025

	): ListCollectionsCursor<CollectionInfo> {
	): AsyncIterable<CollectionInfo> {

feat(NODE-6773): add support for $lookup on encrypted collections #4427

Are you sure you want to change the base?

feat(NODE-6773): add support for $lookup on encrypted collections #4427

Conversation

nbbeeken commented Feb 19, 2025 • edited Loading

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Release Highlight

Add support for $lookup on encrypted collections

Double check the following

nbbeeken commented Feb 21, 2025

baileympearson 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

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

baileympearson Feb 24, 2025 • edited by nbbeeken Loading

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

nbbeeken commented Feb 19, 2025 •

edited

Loading

baileympearson Feb 24, 2025 •

edited by nbbeeken

Loading