Don't trigger `findAll` when `Model.find()` is called with a `null` or `undefined` parameter #518

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
Contributor

raycohen commented Dec 12, 2012

Instead throw an error. This is a much easier way to understand that you're passing a bad value to Model.find

so in the new behavior:

var postId = null;
Post.find(postId); // raises
+ if ('undefined' === typeof id || id === null) {
+ throw new Ember.Error(fmt('Cannot find %@ for type %@.', [id + '', type]));
+ }
+
@wagenet

wagenet Feb 5, 2013

Owner

Should this maybe be an Ember.assert instead?

@raycohen

raycohen Feb 20, 2013

Contributor

I think I did it this way because the issue popped up in prod for us (we did not pass undefined in intentionally). The error message we got was not very useful in tracking it down.

Owner

igorT commented Apr 8, 2013

I think most of the code for these kinds of cases uses assert so I would prefer that we keep doing it that way. Would be happy to merge it once that's changed.Thanks!

Contributor

raycohen commented Apr 14, 2013

Asserts are removed in prod builds right? I think that won't be particularly helpful. Developers aren't intentionally passing undefined to find. What happened for us was that in prod we unexpectedly had undefined show up in a couple circumstances. The actual error we got was totally removed from what the problem is, and a pain to track down.

I honestly wonder if having find ever do a findAll is a good api choice. The need for this PR (whether assert or raise) suggests maybe not.

I paired with @stefanpenner on this.

Owner

tomdale commented May 10, 2013

Agreed that this should be an assert. If you want extra debugging in production, running rake dist produces a minified build of Ember.js that does not strip asserts.

@raycohen raycohen closed this May 10, 2013

Owner

stefanpenner commented May 10, 2013

@tomdale i am unsure, @raycohen raises a good point, undefined's and null's entering a system unexpectedly is always hazardous.

This is totally the type of error you would hope your bugsense/hoptoad would be notified with.

Member

lukemelia commented May 12, 2013

I don't like overloading find in the first place, but if we are sticking with that, I strongly agree that a call with arguments > 0 should never be able to trigger the path for arguments == 0. That is just asking for trouble.

Owner

stefanpenner commented May 13, 2013

@tomdale @raycohen @lukemelia

thoughts for discussion.

ray's addition is legit, this is an actual runtime error.

  • null or undefined unexpectedly entering a system is never desired.
  • undefined behaviour is essentially a bug, further execution down that path should be halted, and reported.
  • this undefined condition is easily encountered in day to day usage and as such this should be a runtime exception not an development assertion.

Proposal:
overloading find is dubious:

  • find should do one thing and doit well
  • if your intent is to findAll, you should invoke findAll. If you want to find something find is your guy, and same should be true with findQuery.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment