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

store.find('model', ''); shouldn't query 'api/models/' #2348

Closed
calvinmetcalf opened this issue Oct 6, 2014 · 7 comments · Fixed by #3795
Closed

store.find('model', ''); shouldn't query 'api/models/' #2348

calvinmetcalf opened this issue Oct 6, 2014 · 7 comments · Fixed by #3795

Comments

@calvinmetcalf
Copy link
Contributor

basically if you do find with an empty string then it does a search for api/models/ with no id which leads to it downloading all of the models, coerceID seems like it should be coercing empty strings to null.

@stefanpenner
Copy link
Member

is this still an issue on master? I seem to remember @raycohen fixing this ages ago. Maybe a regression?

@calvinmetcalf
Copy link
Contributor Author

canary definitely not sure about master

@stefanpenner
Copy link
Member

@calvinmetcalf
Copy link
Contributor Author

at no place in that path does it bother to check if it is an empty string

@igorT
Copy link
Member

igorT commented May 25, 2015

Sorry for the late followup, but I am not sure what is the use case for passing an empty string? Would you expect it to immediately reject?

@raycohen
Copy link
Contributor

The (unmerged) PR @stefanpenner recalls is #518

I agree with @lukemelia's comment found in the discussion.

@igorT
Copy link
Member

igorT commented May 25, 2015

My bad. We deprecated the overloaded find but should still add the assert
On May 24, 2015 8:02 PM, "Ray Cohen" notifications@github.com wrote:

The (unmerged) PR @stefanpenner https://github.com/stefanpenner recalls
is #518 #518

I agree with @lukemelia https://github.com/lukemelia's comment found in
the discussion.


Reply to this email directly or view it on GitHub
#2348 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants