Repository navigation
DRIVERS-3684 Mandate reporting write concern errors - #1998
Conversation
| expectError: | ||
| isError: true |
There was a problem hiding this comment.
Note: the unified test runner spec doesn't currently support asserting on error codes for bulk errors. The purpose of these tests is to ensure that drivers aren't swallowing write concern errors, so it should be sufficient in these cases just to assert that an error was returned.
There was a problem hiding this comment.
🟡 Changes recommended
Missing data isolation in both bulk-write fixtures allows duplicate-key errors to mask failures to report write concern errors.
1 open issue
What changed in this PR
This PR strengthens the MongoDB driver specification by requiring write concern errors to be reported and adding cross-driver tests for that behavior.
Changes:
- Changes write concern error reporting from SHOULD to MUST for methods accepting a
WriteConcern. - Adds failpoint-based unified tests and generated JSON across write-capable operations.
- Documents the new test suite and updates the specification changelog.
| File | Description |
|---|---|
| source/read-write-concern/tests/write-concern-errors/mapReduce.yml | Tests error reporting for mapReduce output writes. |
| source/read-write-concern/tests/write-concern-errors/mapReduce.json | Generated mapReduce test. |
| source/read-write-concern/tests/write-concern-errors/index-management.yml | Tests index creation and deletion errors. |
| source/read-write-concern/tests/write-concern-errors/index-management.json | Generated index-management tests. |
| source/read-write-concern/tests/write-concern-errors/gridfs.yml | Tests chunk and file-document upload errors. |
| source/read-write-concern/tests/write-concern-errors/gridfs.json | Generated GridFS tests. |
| source/read-write-concern/tests/write-concern-errors/findAndModify.yml | Tests find-and-modify operation errors. |
| source/read-write-concern/tests/write-concern-errors/findAndModify.json | Generated find-and-modify tests. |
| source/read-write-concern/tests/write-concern-errors/db-coll-management.yml | Tests database and collection management errors. |
| source/read-write-concern/tests/write-concern-errors/db-coll-management.json | Generated database and collection tests. |
| source/read-write-concern/tests/write-concern-errors/crud.yml | Tests insert, update, replace, and delete errors. |
| source/read-write-concern/tests/write-concern-errors/crud.json | Generated CRUD tests. |
| source/read-write-concern/tests/write-concern-errors/client-side-encryption.yml | Tests data-key rewrapping errors. |
| source/read-write-concern/tests/write-concern-errors/client-side-encryption.json | Generated encryption test. |
| source/read-write-concern/tests/write-concern-errors/client-bulkWrite.yml | Tests client bulk-write errors on MongoDB 8.0+. |
| source/read-write-concern/tests/write-concern-errors/client-bulkWrite.json | Generated client bulk-write test. |
| source/read-write-concern/tests/write-concern-errors/bulkWrite.yml | Tests collection bulk-write errors. |
| source/read-write-concern/tests/write-concern-errors/bulkWrite.json | Generated collection bulk-write test. |
| source/read-write-concern/tests/write-concern-errors/aggregate.yml | Tests aggregation errors with $out and $merge. |
| source/read-write-concern/tests/write-concern-errors/aggregate.json | Generated aggregation tests. |
| source/read-write-concern/tests/README.md | Describes the write concern error suite. |
| source/read-write-concern/read-write-concern.md | Mandates reporting and updates test requirements and changelog. |
Review effort: Balanced
|
Assigned |
kevinAlbs
left a comment
There was a problem hiding this comment.
A not-yet-well-reviewed C driver draft implementation passes these tests: mongodb/mongo-c-driver#2517
Most of my suggestions are test tweaks and coverage additions. I think the only substantial comment is a wording change to indicate writeConcernError must always be parsed, unless it is a generic runCommand API.
| provider: local | ||
|
|
||
| tests: | ||
| - description: rewrapManyDataKey reports a writeConcernError |
There was a problem hiding this comment.
Suggest testing additional ClientEncryption operations that send write commands: createDataKey, deleteKey, addKeyAltName, and removeKeyAltName. These operations are already referenced in other unified tests, so I expect are already supported by test runners.
kevinAlbs
left a comment
There was a problem hiding this comment.
LGTM. Latest tests pass in C driver implementation locally, and are running in Evergreen.
Aggregate fell back to Server::executeCommand() whenever a read preference was set. Collection always sets one, so a writeConcernError returned for a pipeline ending in $out or $merge was never reported. A primary read preference is the default for executeReadWriteCommand(), so drop it and use that method, which reports the error. Update the tests/specifications submodule to the DRIVERS-3684 branch (mongodb/specifications#1998) and add the new write-concern-errors suite. Skip it until the specifications PR is merged, since its failpoints omit errmsg and the runner does not support dropIndexes, dropDatabase on a client and mapReduce.

Please complete the following before merging:
clusters).
Rust implementation: mongodb/mongo-rust-driver#1840