Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 13 commits into from
Sep 27, 2023

Conversation

aditi-khare-mongoDB
Copy link
Contributor

@aditi-khare-mongoDB aditi-khare-mongoDB commented Sep 14, 2023

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

@aditi-khare-mongoDB aditi-khare-mongoDB self-assigned this Sep 14, 2023
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title (WIP) feat(NODE-5421): bulkWriteResult.insertedIds does not filter out _ids that are not actually inserted (WIP) fix(NODE-5421): bulkWriteResult.insertedIds does not filter out _ids that are not actually inserted Sep 14, 2023
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title (WIP) fix(NODE-5421): bulkWriteResult.insertedIds does not filter out _ids that are not actually inserted fix(NODE-5421): bulkWriteResult.insertedIds does not filter out _ids that are not actually inserted Sep 15, 2023
@durran durran self-assigned this Sep 18, 2023
@durran durran added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Sep 18, 2023
@dariakp dariakp changed the title fix(NODE-5421): bulkWriteResult.insertedIds does not filter out _ids that are not actually inserted fix(NODE-5428): bulkWriteResult.insertedIds does not filter out _ids that are not actually inserted Sep 18, 2023
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title fix(NODE-5428): bulkWriteResult.insertedIds does not filter out _ids that are not actually inserted fix(NODE-5628): bulkWriteResult.insertedIds does not filter out _ids that are not actually inserted Sep 18, 2023
src/bulk/common.ts Outdated Show resolved Hide resolved
Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

test/integration/crud/bulk.test.ts Outdated Show resolved Hide resolved
test/integration/crud/bulk.test.ts Outdated Show resolved Hide resolved
test/integration/crud/bulk.test.ts Outdated Show resolved Hide resolved
test/integration/crud/bulk.test.ts Outdated Show resolved Hide resolved
@aditi-khare-mongoDB
Copy link
Contributor Author

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

test/integration/crud/bulk.test.ts Outdated Show resolved Hide resolved
test/integration/crud/bulk.test.ts Outdated Show resolved Hide resolved
test/integration/crud/bulk.test.ts Outdated Show resolved Hide resolved
test/integration/crud/bulk.test.ts Outdated Show resolved Hide resolved
test/integration/crud/bulk.test.ts Outdated Show resolved Hide resolved
test/integration/crud/bulk.test.ts Outdated Show resolved Hide resolved
Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still missing the requested test structure changes and updated wording.

@durran durran added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Sep 21, 2023
durran
durran previously approved these changes Sep 21, 2023
src/bulk/common.ts Outdated Show resolved Hide resolved
src/bulk/common.ts Show resolved Hide resolved
Copy link
Contributor

@W-A-James W-A-James left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple quick test changes.

test/integration/crud/bulk.test.ts Outdated Show resolved Hide resolved
test/integration/crud/bulk.test.ts Outdated Show resolved Hide resolved
src/bulk/common.ts Outdated Show resolved Hide resolved
test/integration/crud/bulk.test.ts Show resolved Hide resolved
test/integration/crud/bulk.test.ts Outdated Show resolved Hide resolved
test/integration/crud/bulk.test.ts Outdated Show resolved Hide resolved
test/integration/crud/bulk.test.ts Show resolved Hide resolved
test/integration/crud/bulk.test.ts Show resolved Hide resolved
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/bulk/common.ts Outdated Show resolved Hide resolved
test/integration/crud/bulk.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants