RUST-1488 Populate inserted_ids when an insert_many fails

[kmahar]

fixes #748

The fix ended up being a little more complicated than I initially expected in order to get it work properly for multi-batch bulk writes but was still relatively straightforward; most of the diff here is just adding testing. @isabelatkinson we should consider adding automated tests around this type of thing as part of the new bulk spec

[kmahar]

I initially had just added logic to set the inserted_ids on the cumulative_failure for failed batches, but I realized that would not correctly handle cases where there are a mix of successful and unsuccessful batches -- for example, if the first batch succeeds and the second batch fails, we would put the inserted_ids from batch 1 on the cumulative_result but the inserted_ids from batch 2 on the cumulative_failure, and then only return the failure.
given this, it seemed simplest to switch to tracking the inserted_ids in one place and then just setting them on the InsertManyResult / BulkWriteFailure at the end.

[kmahar]

@isabelatkinson for the purpose of trying to write spec tests around batching behavior for bulk, we might consider requiring drivers have some way to internally override maxWriteBatchSize. otherwise we'd need massive yaml/json files to fit in enough data to generate multiple batches (the limit is either 100k writes or 16 MB, whichever is smaller).
alternatively, drivers can write prose tests like this one, but I think it would be much easier if we could artificially use bulk batches with a smaller size.
alternatively/additionally we could consider asking the server to make the parameter somehow configurable so we could temporarily decrease it for the bulk tests

[kmahar]

the evg failures don't seem related.
also, I suppose we should probably backport this to the 2.3 branch and tag 2.3.2

[abr-egn]

LGTM!

[patrickfreed]

Can we add one more case where we have a 2 batch write with a write error in both batches?

[patrickfreed]

This field is actually currently pub(crate), and the only reason the user was able to observe it was the derived Debug implementation. I did some digging and found that RUST-260 was filed to eventually expose this field properly and to stop discarding the intermediate results, and I believe the field originally was introduced for counting writes attempted rather than actually tracking inserted ids (per #377 (comment)).

Given that we've done the work for populating this field, I think we probably ought to do RUST-260 also and mark the field as pub.

Regarding the Jira logistics of this, I wonder if it makes sense to rename RUST-1488 to cover the bug of the Debug implementation leaking the unset, pub(crate) inserted_ids map, and instead have this PR be titled RUST-260. Wdyt?

[patrickfreed]

In the event that one of the batches fails with a non-BulkWriteError, we just return early without continuing it looks like. I think this is probably inconsistent with the docstring of unordered writes ("If false, when a write fails, continue with the remaining writes, if any), but besides that, it also means we discard our cumulative idea of what has been inserted thus far. I wonder if we would instead want to add a Vec<Error> to BulkWriteFailure to account for the other errors that might occur during a multi-batch write in order to preserve the inserted_ids map. We have something similar in Swift, though I think it's only a single error instead of an array of them.

This may get kind of tricky when deciding which errors we want to continue on with and which constitute a "terminal" error. For example, I imagine an auth error would not be something we'd want to continue attempting to insert after, but a network error would be (e.g. so that we could get a different mongos for the next batch). Maybe its worth deferring to another ticket? Or maybe we keep the existing behavior?

[qm3ster]

For prior art of how to decide, consider https://docs.rs/reqwest/latest/reqwest/struct.Response.html#method.error_for_status

[qm3ster]

Being as ignorant and disorganized as I am, I have reopened this in #1157 and attempted an implementation in #1158.
I am currently using that from my private fork where BulkWriteFailure.inserted_ids is pub.
If anyone would like to look over the code and give it a general go-ahead I'd be happy to write (steal from this PR) some tests.

But if this is a whole process, (I thought it may not be since it's not a breaking change) I believe it would be a lot more ergonomic to start breaking application "errors" out of the general error.
Examples would range from Result<InsertManyResult{ids: Vec<Result<Id, Err>>}, mongodb::Error>
to even Result<Result<InsertOneResult, InsertOneFailure>, mongodb::Error>

But that sounds like 4.0.

[isabelatkinson]

@qm3ster yeah that would be quite a major overhaul of our error API. 3.0 is very fresh so breaking changes will likely not happen again for a while.

Also worth noting that the new bulk write API does report inserted IDs in its error type; BulkWriteError contains a partial result which, if verbose_results was configured on the call to bulk_write, will record an InsertResult for any successful inserts that happened before the error occurred. e.g.:

let write_models = <InsertOneModel for each insert document>;
let bulk_write_result = client.bulk_write(write_models).verbose_results().await;
let insert_results = match bulk_write_result {
    Ok(result) => Some(result.insert_results),
    Err(error) => {
        if let ErrorKind::BulkWrite(bulk_write_error) = *error.kind {
            match bulk_write_error.partial_result {
                Some(PartialBulkWriteResult::Verbose(result)) => Some(result.insert_results),
                _ => None,
            }
        } else {
            None
        }
    }
}; // returns Option<HashMap<usize, InsertOneResult>>

(The major caveat here is that bulk write will not be available for use in production until MongoDB 8.0 is released, but once that happens this will be a way to inspected inserted IDs upon error without requiring changes to the driver.)