CDRIVER-4425 Provide access to raw result if server error occurs

[galon1]

Summary

These changes provide access to the raw result document from the server if an error has occurred. For read operations the cursor will already return the original result from the server, as the result is stored in cursor->error_doc. For write operations a new field in write_result_t was required (raw_error_response), which is only populated if a server error occurs. New unified tests were added according to this specification (original new tests, but the latest edit is here).

Notes

1. The raw responses from the server for write operations are wrapped in an errorReplies field. This is to avoid test failures that parse out expected fields in the response, since write operations process write results. For example, the index for bulkwrite operations are increased by 1 when the raw server result is processed. However, since the goal of this ticket was to return the raw result from the server with no processing (in case of future changes), I left the raw response in from the server, and I left the processed result, since users are expecting the processed result.
2. Tests did not include a name field when running createIndex, but a name field is required for createIndexes which is how the test runner executes createIndex commands. So if no name field was set, I just created a name field so the command can be executed.

[kkloberdanz]

Since error is not being modified, and is therefore not an out parameter, should this instead be const bson_error_t *error?

[kkloberdanz]

This isn't something that should block merging, but it looks like this function has grown to be pretty long. I wonder if we should start thinking of breaking it up into smaller pieces?

[kkloberdanz]

Looks good! Just a couple of minor questions, neither of which should block merging. Although, I'm still getting up to speed on the codebase, so someone else might want to double check. Good job! 👍

[kevinAlbs]

Great work. I suggest storing an array of error replies to give callers access to all server errors on a bulk write.

[jmikola]

Offhand, would the first { ok: 0 } command response (post-retry) completely interrupt a bulk write operation regardless of the ordered option (note: ordered: false is essentially continue-on-error).

Our specs are ambiguous about this (see: DRIVERS-2090, DRIVERS-2159).

I noted in the unified test runner code below, the assertion is only done on errorReplies.0, which assumes that three will only ever be on error reply. That's fine, as I intentionally wrote the tests with the above ambiguity in mind (see: mongodb/specifications@33a03f2.

But here, I'm more curious about whether multiple errorReplies are even possible in libmongoc. If not, are we just using an errorReplies array on the result for flexibility?

[galon1]

It is possible for libmongoc to return multiple errorReplies. Here is a test displaying this behavior. Since ordered: false continues on error when the failPoint is hit twice, two errorReplies are returned by libmongoc, as well as other errors that occurred if they did (in the test a writeError also was raised).

[jmikola]

Thanks for indulging my question. Looks like an oversight in the test runner syntax I introduced in DRIVERS-2385. I've made a note about this in DRIVERS-1547 as that's the most likely ticket we'd use to add spec test for multiple command failures in a bulk write.

[kevinAlbs]

LGTM with leak fixed. Great work.

[jmikola]

Continuing the conversation in #1144 (comment)

Sorry, but I misread bson_lookup_doc_null_ok and thought it gracefully allowed for missing values. I don't think there's any reason to use that, as it could still abort if the array is empty.

I could have sworn there was a function that allowed full field paths and gracefully handled missing keys. Perhaps I was thinking of bson_iter_find_descendant, which bson_lookup uses internally before it aborts on a failed resolution.

Perhaps it'd be best to use assert_match_bson() to check that "errorReplies.0" is a document. That function will emit a test error on failure, unlike ASSERT_MATCH(), which aborts. I believe you can use the $$type, although I don't know offhand if the function supports field paths (e.g. "errorReplies.0"), or if you might need to do something like:

{ "errorReplies": { "0": { "$$type": "object" } } }

/cc @kevinAlbs

[galon1]

I just pushed changes that check if errorReplies.0 is a document. I ended up using BSON_ITER_HOLDS_DOCUMENT and BSON_ITER_HOLDS_ARRAY to check that errorReplies was the expected type. I thought to use these because that is how the reply is also checked right above my code (line 496 where error_labels_contain is checked).

[jmikola]

Optional suggestion, but LGTM regardless.