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-5627): BulkWriteResult.insertedIds does not filter out _ids that are not actually inserted #3870

Merged
merged 5 commits into from
Sep 29, 2023

Conversation

aditi-khare-mongoDB
Copy link
Contributor

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

Description for NODE-5627

Before this change, on Node Driver Version 5.x, 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.
  • NOTE: this PR's code changes are identical to the 6.x NODE-5421 PR

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 collection.bulkwrite() or collection.insertMany() operation, 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 - 5.x

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 15, 2023
@aditi-khare-mongoDB aditi-khare-mongoDB marked this pull request as draft September 15, 2023 20:55
@aditi-khare-mongoDB aditi-khare-mongoDB changed the title fix(NODE-5421): 5.x backport - BulkWriteResult.insertedIds does not filter out _ids that are not actually inserted fix(NODE-5627): 5.x backport - BulkWriteResult.insertedIds does not filter out _ids that are not actually inserted Sep 27, 2023
@aditi-khare-mongoDB aditi-khare-mongoDB marked this pull request as ready for review September 27, 2023 19:49
@baileympearson baileympearson self-assigned this Sep 28, 2023
@baileympearson baileympearson added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Sep 28, 2023
Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

failing CI is unrelated

@baileympearson baileympearson 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 28, 2023
@baileympearson baileympearson changed the title fix(NODE-5627): 5.x backport - BulkWriteResult.insertedIds does not filter out _ids that are not actually inserted fix(NODE-5627): BulkWriteResult.insertedIds does not filter out _ids that are not actually inserted Sep 28, 2023
@baileympearson baileympearson merged commit d766ae2 into 5.x Sep 29, 2023
@baileympearson baileympearson deleted the NODE-5421/bulk-write-insert-many-error-5.x branch September 29, 2023 14:30
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.

2 participants