Skip to content

Ensure that the bulk write list is valid #168

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Ensure that the bulk write list is valid #168

wants to merge 1 commit into from

Conversation

rozza
Copy link
Owner

@rozza rozza commented Oct 10, 2016

And does not contain null values.

JAVA-2265

@rozza
Copy link
Owner Author

rozza commented Oct 10, 2016

I debated adding a notNullOrContainNull assertion but decided against it as we already loop the list.

I purposely didn't add checks to the legacy bulk api as its deprecated.

jyemin
jyemin requested changes Oct 10, 2016
View reviewed changes
Copy link
Collaborator

@jyemin jyemin left a comment

Choose a reason for hiding this comment

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

A couple of nits.

Also, I suspect there other places where MongoCollection takes a list and we don't check for null values in the list, e.g. insertMany and aggregate. We might as well take care of them all at once.

driver-core/src/main/com/mongodb/assertions/Assertions.java Outdated
Copy link
Collaborator

@jyemin jyemin Oct 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Unused import.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ack

driver-async/src/main/com/mongodb/async/client/MongoCollectionImpl.java Outdated
Copy link
Collaborator

@jyemin jyemin Oct 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

No real need for an extra level of conditional here. I actually think it would be clearer to leave this else as is, and move the null check to the very top before all the instance checks.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Sounds good

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done

@jyemin
Copy link
Collaborator

jyemin commented Oct 11, 2016

lgtm

@rozza rozza closed this Oct 11, 2016
@rozza rozza deleted the j2265 branch October 11, 2016 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jyemin jyemin jyemin requested changes

Assignees

@evanchooly evanchooly

@jyemin jyemin

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rozza @jyemin @evanchooly