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-3565): Error for insertMany with partially empty array is unhelpful #3221

Merged
merged 14 commits into from May 5, 2022

Conversation

kampofo
Copy link
Contributor

@kampofo kampofo commented Apr 27, 2022

Description

NODE-3565

What is changing?

  • Adding a conditional to check if invalid operation is passed to raw, then throwing a more descriptive error.
  • Adding int tests to verify the new error is being thrown at the appropriate time
Is there new documentation needed for these changes?

No

What is the motivation for this change?

The previous error being thrown was not descriptive.

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@kampofo kampofo marked this pull request as draft April 27, 2022 19:36
src/bulk/common.ts Outdated Show resolved Hide resolved
src/bulk/common.ts Outdated Show resolved Hide resolved
test/integration/crud/bulk.test.js Outdated Show resolved Hide resolved
test/integration/crud/bulk.test.js Outdated Show resolved Hide resolved
test/integration/crud/bulk.test.js Outdated Show resolved Hide resolved
test/integration/crud/bulk.test.js Outdated Show resolved Hide resolved
test/integration/crud/bulk.test.js Outdated Show resolved Hide resolved
@kampofo kampofo marked this pull request as ready for review May 2, 2022 19:05
baileympearson
baileympearson previously approved these changes May 2, 2022
@baileympearson baileympearson added the Team Review Needs review from team label May 2, 2022
@nbbeeken nbbeeken self-requested a review May 2, 2022 19:39
@nbbeeken nbbeeken changed the title chore(NODE-3565): Error for insertMany with partially empty array is unhelpful fix(NODE-3565): Error for insertMany with partially empty array is unhelpful May 2, 2022
src/bulk/common.ts Outdated Show resolved Hide resolved
test/integration/crud/bulk.test.js Outdated Show resolved Hide resolved
nbbeeken
nbbeeken previously approved these changes May 3, 2022
test/integration/crud/bulk.test.js Outdated Show resolved Hide resolved
…o check the expected error message has not been changed, fix grammar issues
@kampofo kampofo requested a review from dariakp May 4, 2022 19:46
test/integration/crud/bulk.test.js Show resolved Hide resolved
test/integration/crud/bulk.test.js Show resolved Hide resolved
if (err || res == null) {
if (err && err.message === 'Operation must be an object with an operation key') {
err = new MongoInvalidArgumentError(
'Argument "docs" is a sparse array containing element(s) that are null or undefined'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'Argument "docs" is a sparse array containing element(s) that are null or undefined'
'Collection.insertMany() cannot be called with an array that has null/undefined values'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

test/integration/crud/bulk.test.js Outdated Show resolved Hide resolved
baileympearson
baileympearson previously approved these changes May 5, 2022
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.

One non-blocking comment, otherwise LGTM

@baileympearson baileympearson merged commit 0ef2516 into main May 5, 2022
@baileympearson baileympearson deleted the NODE-3565 branch May 5, 2022 20:11
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
4 participants