DRIVERS-2385: expectedError.errorResponse for asserting server-side error doc #1316
Conversation
jmikola
requested review from
benjirewis and
alcaeus
and removed request for
a team
| @@ -1,4 +1,4 @@ | |||
| SCHEMA=../schema-1.9.json | |||
| SCHEMA=../schema-1.12.json | |||
There was a problem hiding this comment.
This looks like we should pay more attention to this when reviewing changes to the unified test format as it was previously missed.
| @@ -1,4 +1,4 @@ | |||
| SCHEMA=../schema-1.9.json | |||
| SCHEMA=../schema-1.12.json | |||
There was a problem hiding this comment.
I noticed this file is quite outdated. Since DRIVERS-1767 was implemented and a CI workflow was added to test spec files against their exact schemaVersion, this file only serves two purposes:
- Validate a wide number of specs against the latest schema (not sure how useful or necessary this is)
- Test schema validation failures for
invalidtests, which are ignored by the CI workflow
FWIW, the Makefile is still quite a bit faster than the check_schema_version.sh script since it invokes ajv once for an entire directory of files. Going forward, I think it'd be best to eliminate this file and update check_schema_version.sh to also process invalid tests and possible optimize it to invoke fewer instances of ajv (e.g. collect specs per schema and run a single ajv instance per schema).
There was a problem hiding this comment.
Tracking in DRIVERS-2466.
| - { _id: 2, x: 1 } | ||
|
|
||
| tests: | ||
| - description: "modifyCollection prepareUnique violations are accessible" |
There was a problem hiding this comment.
This test covers the original impetus for this functionality (see: DRIVERS-2353). The other tests for aggregate and findOneAndUpdate are meant to provide some additional test coverage without being exhaustive.
| @@ -3749,6 +3754,8 @@ Changelog | |||
| .. | |||
| Please note schema version bumps in changelog entries where applicable. | |||
|
|
|||
| :2022-10-07: **Schema version 1.12.** | |||
There was a problem hiding this comment.
oh boy I get to do another rebase and schema version bump on #1303 🙂
There was a problem hiding this comment.
In all seriousness, if your PR is nearly ready to merge I'm happy to hold off on this for a few more days and spare you the trouble.
There was a problem hiding this comment.
ha, I appreciate it, but it really isn't too big of a deal. I am really hoping my PR is going to get merged this week but I don't want to promise it. so let's just let things play out naturally timing wise
| @@ -1079,6 +1079,11 @@ The structure of this object is as follows: | |||
| assert that the error does not contain any of the specified labels (e.g. using | |||
| the ``hasErrorLabel`` method). | |||
|
|
|||
| - ``errorResponse``: Optional document. A value corresponding to the expected | |||
There was a problem hiding this comment.
it seems we're treating this as a root document. should we call that out here or in Evaluating Matches?
There was a problem hiding this comment.
I added errorResponse to the non-exhaustive list in "Allowing Extra Fields in Root-level Documents" and also made a note that it should be matched as a "root-level document" here so there's no ambiguity.
FWIW, it doesn't look like we did that for any other instances. Any spec test value directly matched using the "Evaluating Matches" rule is assumed to be a root-level document. The rules for non-root-level matching only kick in when encountering an embedded document within one of those values.
There was a problem hiding this comment.
I just added some additional tests and language around an edge case I discovered with using errorResponse for basic write ops (e.g. insert, update, delete, and bulk writes). I spoke to @ShaneHarvey about this in the Python channel and decided it was worth incorporating into this spec, even if we ultimately discourage use of errorResponse.
do you think we need to try these tests/changes out in any other drivers before merge?
Write operations in PHP all throw BulkWriteExceptions, which don't provide access to raw server responses. The main limitation is mongoc_bulk_operation_execute(), which only returns a BSON reply document (the derived write result) and an error struct (message and error code). Therefore, I'll need to skip the affected CRUD tests in this PR and would request an additional driver (without that limitation) to POC.
There was a problem hiding this comment.
Cool! I can POC this in the Go driver since we conveniently, recently added the raw server response to most of our errors with GODRIVER-2325.
| rules in `Evaluating Matches`_. | ||
|
|
||
| Note that some drivers may not be able to evaluate ``errorResponse`` for write | ||
| commands (i.e. insert, update, delete) and bulk write operations. For example, |
There was a problem hiding this comment.
oh bulk is a very interesting case. I guess if the server starts adding arbitrary extra fields to write command responses we might have to do some work to figure out how to expose those via bulk results.
also, somewhat of an aside but interestingly, the scope for the new server bulk write command proposes that results are returned via a cursor (this is to handle situations where there are too many errors to fit in a single response, which can happen now sometimes for unordered bulk writes and causes an exception in the server).
if a driver did choose to add a rawResponses property or something to their BulkWriteFailure they would have to consider how to handle that upcoming case as well.
There was a problem hiding this comment.
After POCing this, there's certainly some ambiguity in which error is the errorResponse error in some situations.
Bulk write is one of them. Why are we advising against testing errorResponse with bulk writes here and then adding a test for it here? Am I misunderstanding something? I went with a last-write-error-becomes-the-errorResponse approach (which works since there's only one error), but that was arbitrary.
There's further ambiguity in that the Go driver's normal WriteException can have multiple WriteErrors with multiple raw server responses when we're batching writes. Furthermore, WriteException and BulkWriteException both contain WriteConcernErrors each with their own raw response in the Go driver.
There was a problem hiding this comment.
I guess if the server starts adding arbitrary extra fields to write command responses we might have to do some work to figure out how to expose those via bulk results.
That thought just occurred to me this morning while I was revisiting the PHPLIB POC. I noticed that DuplicateKey(11000) returns extra fields in the write error document, which may be inaccessible in languages that model WriteError and WriteConcernError. See: DRIVERS-2470.
Why are we advising against testing errorResponse with bulk writes here and then adding a test for it here? Am I misunderstanding something? I went with a last-write-error-becomes-the-errorResponse approach (which works since there's only one error), but that was arbitrary.
My intention was to provide as much test coverage as possible for drivers that can support this, and encourage incompatible drivers to skip them. I intentionally had a single operation in the bulkWrite-errorResponse test to avoid ambiguity, assuming that any exception would inherit the server-side error code from the insert command. If that seems problematic I'm happy to delete the test. Alternatively, I can add a comment explaining that things could be ambiguous with multiple operations in the bulk and note my intention for only performing a single insert.
Regarding WriteError and WriteConcernError, I don't think those come into play here since errorResponse is only asserting the top-level command response. Technically, we should be able to assert the presence of gossiped cluster time if so inclined.
There was a problem hiding this comment.
Alternatively, I can add a comment explaining that things could be ambiguous with multiple operations in the bulk and note my intention for only performing a single insert.
That sounds good to me. I can see how these tests are really just to encourage drivers to expose the raw server response wherever they can.
Regarding WriteError [assume you mean extra write errors on non-bulk operations] and WriteConcernError, I don't think those come into play here since errorResponse is only asserting the top-level command response.
Ah gotcha. Sounds good 👍
There was a problem hiding this comment.
Added a comment to the bulk and non-bulk tests in 1415116, which explains that some drivers may need to skip them. The CRUD spec doesn't go into much detail about how to construct BulkWriteException or WriteException beyond collecting write and write concern errors.
assume you mean extra write errors on non-bulk operations
I wasn't distinguishing between bulk and non-bulk writes, since the write commands all return writeErrors in an array. Rather, the fail point I'm using in these tests sets the top-level error code for the command (and forces an ok: 0 response). That is all errorResponse is asserting here.
If we wanted, we could theoretically use errorResponse to peak inside of the writeErrors array (for example), but I didn't see the need for that.
There was a problem hiding this comment.
Tests seem to pass in the Go driver (apologies for the delay I was OOO), but I'm concerned about the ambiguity of this new field when multiple errors are returned from the server.
| rules in `Evaluating Matches`_. | ||
|
|
||
| Note that some drivers may not be able to evaluate ``errorResponse`` for write | ||
| commands (i.e. insert, update, delete) and bulk write operations. For example, |
There was a problem hiding this comment.
After POCing this, there's certainly some ambiguity in which error is the errorResponse error in some situations.
Bulk write is one of them. Why are we advising against testing errorResponse with bulk writes here and then adding a test for it here? Am I misunderstanding something? I went with a last-write-error-becomes-the-errorResponse approach (which works since there's only one error), but that was arbitrary.
There's further ambiguity in that the Go driver's normal WriteException can have multiple WriteErrors with multiple raw server responses when we're batching writes. Furthermore, WriteException and BulkWriteException both contain WriteConcernErrors each with their own raw response in the Go driver.
| @@ -0,0 +1,59 @@ | |||
| description: "modifyCollection-errorResponse" | |||
There was a problem hiding this comment.
The Go driver does not have a modifyCollection helper, so I can't run this test 😢 .
There was a problem hiding this comment.
For the benefit of test coverage, have you considered implementing the operation via runCommand in your test suite? PHP has a helper for it but we don't actually model all of the possible parameters, so it should be quite trivial to just build a command document from the arguments in the test operation if you wanted.
Maybe not so relevant for this case, since the purpose of the test is to ensure a helper provides appropriate error access, but it could allow you to run some other tests that happen to use modifyCollection.
There was a problem hiding this comment.
Yeah an interesting point; I imagine users are already running collMod through our RunCommand API, so would be good to test it. Filed GODRIVER-2587.
There was a problem hiding this comment.
Waiting on the latest PHPLIB patch build https://spruce.mongodb.com/version/6348eecc850e6128847d3a21/tasks and will merge once that completes.
Adds basic tests for expectedError.errorResponse under the unified test format, which are derived from the operation-failure tests in valid-fail. Also adds tests for write operations (i.e. insert, update, delete, and bulkWrite), which may be problematic for some drivers that use special exceptions such as BulkWriteException, which may not provide direct access to a single response. Note that tests should avoid using errorResponse assertions for such operations and permit drivers to skip those tests as needed.
There was a problem hiding this comment.
Added a topology restriction to the aggregate test, which has different behavior on sharded clusters, and started a new patch build: https://spruce.mongodb.com/version/634902357742ae0d82027575/tasks
https://jira.mongodb.org/browse/DRIVERS-2385
Prototype: mongodb/mongo-php-library#989