fix(NODE-5628): bulkWriteResult.insertedIds does not filter out _ids that are not actually inserted

[aditi-khare-mongoDB]

Description for NODE-5628

Before this change, BulkWriteResult.insertedIds (1) would return an Object containing all attempted insertedIds, regardless of if any of the inserts failed. This affects BulkWrite and InsertMany operations. After this PR is merged, BulkWriteResult.insertedIds will reflect which _ids were actually inserted.

Other Notes for Reviewers

• The JIRA ticket only mentions the bug on the InsertMany operation; however, I was able to reproduce it generally for any insert operation that returns BulkWriteResult namely BulkWrite.
• BulkResult.insertedIds will still contain unsuccessful inserted _ids. The BulkResult class is not user facing.
• In this PR, unsuccessful inserts are identified through BulkResult.writeErrors list property, rather than the actual direct insert response from the server, since the batch result does not contain _id information.

Is there new documentation needed for these changes?

Yes, it needs to be communicated to users that when an insert does not succeed through a bulkwrite() or collection.insertMany(), it will no longer appear in the error's .result.insertedIds

What is the motivation for this change?

See release highlights.

Release Highlight

Inserted ids in bulk write only contain successful insertions

Prior to this fix the bulk write error's insertedIds field contained ids of all attempted inserts in a bulk operation.

When a bulkwrite() or an insertMany() operation rejects one or more inserts, throwing an error, the error's BulkWriteResult property will be filtered to only contain successfully inserted ids (ex: error.result.insertedIds).

Bug Description

Previous Behavior

The error's .result.insertedIds does includes all attempted inserts, regardless of if they failed or not.

New Behavior

The error's .result.insertedIds does not include unsuccessful inserts.
Namely,

• On an ordered bulk insert operation, after the first error, all future inserts fail, and therefore should not be included in insertedIds. (This is because the order would be invalid once one insert has failed).
• On an unordered bulk insert operation run on a transaction, after the first error, all future inserts fail, and therefore should not be included in insertedIds. (See here for more information.)
• On an unordered bulk insert operation that is not run on a transaction, all inserts that individually fail should be removed.

Checklist

• Ran npm run check:lint script
• Self-review completed using the steps outlined here
• PR title follows the correct format: type(NODE-xxxx)[!]: description
  • Example: feat(NODE-1234)!: rewriting everything in coffeescript
• Changes are covered by tests
• New TODOs have a related JIRA ticket

[durran]

The AC on the ticket has a requirement of perf tests covering the changes. We do have perf tests covering these 2 scenarios so I don't think we need to add anything but at least a run to report what the impact is of this change would be helpful. (The script is npm run check:bench)

[aditi-khare-mongoDB]

@durran Here's a link to perf tests on new branch.

[durran]

Still missing the requested test structure changes and updated wording.

[W-A-James]

Just a couple quick test changes.

[nbbeeken]

The section of your PR body between "RELEASE_HIGHLIGHT_START" and "RELEASE_HIGHLIGHT_END" is what ends up being put in the release notes for this change.

I have a few suggestions:

• current title: BulkWriteResult.insertedIds Bug Fix

  • Let's make is a bit more descriptive as a first touch point to what is changing here. "Inserted ids in bulk write results filtered for successful insertion" (or something like that, I don't think my phrasing is ideal here 🤔)

• current body: When invalid id(s) is/are attempted to be inserted through a BulkWrite or InsertMany, they will no longer appear BulkWriteResult.insertedIds .

  • It's not only invalid id(s) right? I believe there are other possible write errors that can indicate an id was not actually inserted. If so, let's generalize the language.
  • When referencing APIs it helps to match up the monospace with what users expect to see in their usages, so instead of InsertMany we should refer to .insertMany() or collection.insertMany() (same for .bulkWrite()).
    • So, in the same area of concern, BulkWriteResult.insertedIds should reference precisely how users would access the insertedIds prop in the scenario we are changing. This probably calls for a code block where you show try/catch insertMany inside the catch you access the insertedIds however they are meant to be accessed: error.result.x.y.z.insertedIds

[nbbeeken]

code changes lgtm, just looking for the release highlights updates now, TIA