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

Return empty/zero from IN w/empty array #1076

Merged
merged 1 commit into from
Jul 8, 2015
Merged

Return empty/zero from IN w/empty array #1076

merged 1 commit into from
Jul 8, 2015

Conversation

slester
Copy link

@slester slester commented Jul 2, 2015

Calling Collection.destroy({id: []}) destroys all documents. Functionality exists in normalize that returns false if there's an empty array, but false is OR'd with {}, resulting in a {} being passed to the sails-mongo adapter, which deletes everything.

This PR just applies what is already implemented for find().

We found this out the hard way. :(

@dmarcelino
Copy link
Member

This seems reasonable to me, it makes destroy and find consistent, and protects users from deleting all their records accidentally ✅

@slester, I would love if this PR included an automated test... :)

@devinivy, what are your thoughts?

@devinivy
Copy link
Contributor

devinivy commented Jul 2, 2015

Just to make sure I understand, how would the specific call Collection.destroy() behave?

@slester
Copy link
Author

slester commented Jul 2, 2015

@devinivy Collection.destroy({id: []}) should return a callback with no error but also an empty "deleted" array. Right now, it behaves exactly like Collection.destroy({}).

@slester
Copy link
Author

slester commented Jul 2, 2015

@dmarcelino Added a unit test!

@dmarcelino
Copy link
Member

Cool! 😀

@devinivy
Copy link
Contributor

devinivy commented Jul 2, 2015

@slester what would Collection.destroy() do, exactly as typed? I know some libraries use that to clear a table, such as sails-relational-fixtures.

@slester
Copy link
Author

slester commented Jul 2, 2015

@devinivy Collection.destroy() will do the same thing as Collection.destroy({}), namely delete everything in the collection. That behavior won't be changed in our code.

@devinivy
Copy link
Contributor

devinivy commented Jul 2, 2015

Cool– sounds good :)

@dmarcelino dmarcelino added the bug label Jul 7, 2015
@slester
Copy link
Author

slester commented Jul 8, 2015

Let me know if anything is lacking for this PR!

@tjwebb
Copy link
Contributor

tjwebb commented Jul 8, 2015

+1

dmarcelino added a commit that referenced this pull request Jul 8, 2015
Return empty/zero from IN w/empty array
@dmarcelino dmarcelino merged commit f8eb70e into balderdashy:master Jul 8, 2015
@dmarcelino
Copy link
Member

Thanks @slester!

@slester slester deleted the fix-destroy-empty-in branch July 9, 2015 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants