Skip to content

Conversation

@mdb-ad
Copy link
Contributor

@mdb-ad mdb-ad commented Mar 17, 2025

Add spec test for client bulkWrite QE.

C driver implementation

libmongocrypt requirement: 1.10
Drivers changes required:

Please complete the following before merging:

  • Update changelog.
  • Test changes in at least one language driver. (C driver)
  • Test these changes against all server versions and topologies (including standalone, replica set, sharded
    clusters, and serverless). (Note: C driver doesn't currently test CSFLE with sharded)
@mdb-ad mdb-ad marked this pull request as ready for review April 18, 2025 18:00
@mdb-ad mdb-ad requested a review from a team as a code owner April 18, 2025 18:00
@mdb-ad mdb-ad requested review from nbbeeken and removed request for a team April 18, 2025 18:00
@kevinAlbs kevinAlbs self-requested a review April 18, 2025 18:05
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be out of the loop on some of the context, are drivers changes needed to get these passing? Is there a specific libmongocrypt version needed?

There seems to be more unified test changes in the C driver than are here, are those just extra test sync(s) or are they new tests?

Node.js Test Failures
 1) CRUD unified client bulkWrite with queryable encryption client bulkWrite QE replaceOne: MongoBulkWriteError: Document failed validation at LegacyOrderedBulkOperation.handleWriteError (src/bulk/common.ts:1224:13) at executeCommands (src/bulk/common.ts:587:19) at processTicksAndRejections (node:internal/process/task_queues:105:5) at async BulkWriteShimOperation.execute (src/bulk/common.ts:882:12) at async tryOperation (src/operations/execute_operation.ts:283:14) at async executeOperation (src/operations/execute_operation.ts:115:12) at async LegacyOrderedBulkOperation.execute (src/bulk/common.ts:1211:12) at async BulkWriteOperation.execute (src/operations/bulk_write.ts:60:12) at async InsertManyOperation.execute (src/operations/insert.ts:147:19) at async tryOperation (src/operations/execute_operation.ts:283:14) at async executeOperation (src/operations/execute_operation.ts:115:12) at async Collection.insertMany (src/collection.ts:307:12) at async insertMany (test/tools/unified-spec-runner/operations.ts:411:10) at async executeOperationAndCheck (test/tools/unified-spec-runner/operations.ts:1039:14) at async runUnifiedTest (test/tools/unified-spec-runner/runner.ts:226:9) at async Context.<anonymous> (test/tools/unified-spec-runner/runner.ts:336:13) 2) CRUD unified client bulkWrite with queryable encryption client bulkWrite QE with multiple replace fails: AssertionError: Operation clientBulkWrite succeeded but was not supposed to at executeOperationAndCheck (test/tools/unified-spec-runner/operations.ts:1057:12) at processTicksAndRejections (node:internal/process/task_queues:105:5) at async runUnifiedTest (test/tools/unified-spec-runner/runner.ts:226:9) at async Context.<anonymous> (test/tools/unified-spec-runner/runner.ts:336:13) 
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest updating this PR description to give an overview of needed changes. I expect implementors will refer to this PR (and the C driver PR).

C driver implementation

Suggest linking to the C driver PR (once created) rather than the Evergreen patch. The patch appears to include unrelated spec tests.

@mdb-ad mdb-ad requested a review from kevinAlbs July 17, 2025 06:07
@mdb-ad mdb-ad requested a review from a team as a code owner July 23, 2025 23:25
@mdb-ad mdb-ad requested review from katcharov and removed request for a team July 23, 2025 23:25
@baileympearson
Copy link
Contributor

@kevinAlbs @mdb-ad Is there any chance https://jira.mongodb.org/browse/DRIVERS-2859 can be completed before DRIVERS-2888? It makes much more sense to make the change in once place than making the changes in every driver. This is especially true because drivers supporting non-document sequences for client bulk write will be temporary until https://jira.mongodb.org/browse/DRIVERS-2859 is addressed. So making drivers implement the changes before https://jira.mongodb.org/browse/DRIVERS-2859 will cost extra work in every driver, extra work updating the spec to allow (or enforce) document sequences with bulk write after https://jira.mongodb.org/browse/DRIVERS-2859 is implemented, and then again extra work in drivers to support document sequencing for bulkWrite with QE.

From a more selfish, Node-specific perspective - our bulkWrite implementation is tightly coupled to document sequences and making this change might be non-trivial. I'd rather not refactor our bulk write implementation unless absolutely necessary.

@kevinAlbs
Copy link
Contributor

drivers supporting non-document sequences for client bulk write will be temporary until https://jira.mongodb.org/browse/DRIVERS-2859 is addressed

The C driver only required a minor change to support QE with MongoClient.bulkWrite. Supporting CSFLE with MongoCollection.bulkWrite already requires passing all write documents in a non-document sequence, and the C driver reuses it.

I propose: proceed with this PR to unblock some drivers with QE, but add a blurb suggesting drivers wait for DRIVERS-2859:

Auto-Encryption

libmongocrypt does not-yet support OP_MSG document sequences (DRIVERS-2859). Drivers requiring significant changes to pass a bulkWrite command to libmongocrypt may prefer to wait until DRIVERS-2859 is implemented to support automatic encryption.

@mdb-ad mdb-ad requested a review from isabelatkinson August 14, 2025 05:26
@mdb-ad mdb-ad requested a review from a team as a code owner August 21, 2025 05:47
@mdb-ad mdb-ad requested a review from kevinAlbs August 21, 2025 05:51
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the prose tests numbering restored.

mdb-ad and others added 3 commits August 21, 2025 11:03
Copy link
Contributor

@isabelatkinson isabelatkinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

Co-authored-by: Kevin Albertson <kevin.eric.albertson@gmail.com>
@mdb-ad mdb-ad requested a review from kevinAlbs August 22, 2025 18:48
@mdb-ad mdb-ad merged commit 5ef7b1b into mongodb:master Aug 22, 2025
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

6 participants