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

NestedViewSetMixin returns ValueError if the wrong pk type is given in URL #86

Open
frwickst opened this issue May 15, 2015 · 5 comments

Comments

@frwickst
Copy link
Contributor

Lets say you have the following nested url:
/books/23/pages, which works fine, and then change the url to
/books/NOT_AN_INTEGER/pages
this will cause the server to throw:

ValueError at /books/NOT_AN_INTEGER/pages
invalid literal for int() with base 10: 'NOT_AN_INTEGER'

...
/rest_framework_extensions/mixins.py in get_queryset
        60.  super(NestedViewSetMixin, self).get_queryset() 

/rest_framework_extensions/mixins.py in filter_queryset_by_parents_lookups
        67.  return queryset.filter(**parents_query_dict) 
...

The route is of course wrong, but it should return 404 and not a 500 error

I am running:
Django 1.7
djangorestframework 3.1.1
drf-extensions 1.5.2

@frwickst
Copy link
Contributor Author

A suggestion for a fix here is to have a try-except around queryset.filter() like so:

from rest_framework.exceptions import NotFound
...
    def filter_queryset_by_parents_lookups(self, queryset):
        parents_query_dict = self.get_parents_query_dict()
        if parents_query_dict:
            try:
                return queryset.filter(**parents_query_dict)
            except ValueError:
                raise NotFound
        else:
            return queryset

This would cause the server to respond with 404 Not Found instead.
I don't know if this is something you want to do or of you have a better solution in mind. I'll create a pull request if you think this is enough :)

I noticed that Django did something similar here:
django/django@e2ee02c

@maryokhin
Copy link
Contributor

This is because the url kwarg lookup allows non-integer regex. A proper solution would be to allow the user to set the lookup_value_regex on the viewset (failed attempt #63). This is issue just shows that without a solution this can actually be considered a bug.

frwickst pushed a commit to frwickst/drf-extensions that referenced this issue May 18, 2015
If a route such as /users/NOT_AN_INTEGER/settings would be
requested and the /users endpoint expects an integer as the second
part of the url then this request would throw a internal server error.

This is a temp fix and a proper solution should be found. Discussion
about this can be found on Github:
chibisov#86
chibisov#63
chibisov#50
frwickst pushed a commit to frwickst/drf-extensions that referenced this issue May 18, 2015
If a route such as /users/NOT_AN_INTEGER/settings would be
requested and the /users endpoint expects an integer as the second
part of the url then this request would throw a internal server error.

This is a temp fix and a proper solution should be found. Discussion
about this can be found on Github:
chibisov#86
chibisov#63
chibisov#50
@frwickst
Copy link
Contributor Author

Sorry, but I don't see that as a proper solution. That would put the responsibility on the developer using drf-extensions, to see to it that a 500 error is thrown on a default setup. That is not OK in my book at least.

The problem here is not that the router does not handle custom regex values, but that it tries filtering a query with non-proper values.

I have created a pull request with the fix I thought was temporary but seems to be the proper one after creating #87.

frwickst pushed a commit to frwickst/drf-extensions that referenced this issue Jun 2, 2015
If a route such as /users/NOT_AN_INTEGER/settings would be
requested and the /users endpoint expects an integer as the second
part of the url then this request would throw a internal server error.

This is a temp fix and a proper solution should be found. Discussion
about this can be found on Github:
chibisov#86
chibisov#63
chibisov#50
@auvipy
Copy link
Collaborator

auvipy commented Dec 1, 2015

PR?

@auvipy
Copy link
Collaborator

auvipy commented Dec 6, 2015

@frwickst this issue should be open to be fixed with workaround?

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

No branches or pull requests

3 participants