Assert unset BulkWriteException.partialResult in CRUD prose tests
#1785
Conversation
Identifies a bug in the C driver implementation.
`BulkWriteError` is referenced in CRUD spec, but not the `MongoClient.bulkWrite` spec.
kevinAlbs
requested review from
nbbeeken,
isabelatkinson and
jmikola
and removed request for
a team and
nbbeeken
source/crud/tests/README.md
Outdated
| @@ -583,7 +583,7 @@ InsertOne { | |||
| ``` | |||
|
|
|||
| Execute `bulkWrite` on `client` with `largeDocumentModel`. Assert that an error (referred to as `error`) is returned. | |||
| Assert that `error` is a client error. | |||
| Assert that `error` is a client error. Assert the returned `BulkWriteException.partialResult` is unset. | |||
There was a problem hiding this comment.
The Rust driver returns an InvalidArgument error rather than a BulkWriteError for these cases in line with the language from this section of the spec:
When a top-level error is encountered and individual results and/or errors have already been observed, drivers MUST embed the top-level error within a
BulkWriteExceptionas the error field to retain this information. Otherwise, drivers MAY throw an exception containing only the top-level error.
i.e., the invalid argument error is the "top-level" error, and no other errors or results have been observed, so we don't construct a wrapper BulkWriteError.
I'm still in favor of adding this assertion, but I think we should relax the language to be something like: "If a BulkWriteException was thrown, assert that partialResult is unset." I'll likely add assertions to the Rust tests that the error kind is indeed InvalidArgument.
Ditto for the other additions in this PR.
There was a problem hiding this comment.
Great catch. Updated.
Driver is not required to raise `BulkWriteException` for a top-level error when no other results have been reported. Make the assertion optional.
kevinAlbs
changed the title
BulkWriteResult.partialResult in CRUD prose testsBulkWriteException.partialResult in CRUD prose tests
Summary
Assert unset
BulkWriteException.partialResultin CRUD prose tests.Background & Motivation
CDRIVER-5969 identified an implementation bug. Empty (rather than unset) results were sometimes returned on client errors.
The assert is optional. These cases do not require throwing a
BulkWriteException. Quoting spec:If acceptable, will file a DRIVERS ticket for the test tweak (will likely be a
spec-festchange).Please complete the following before merging:
[ ] Update changelog.(Tests only)[ ] Test these changes against all server versions and topologies (including standalone, replica set, sharded(Tested on all but serverless. C does not test serverless).clusters, and serverless).