diff options
| -rw-r--r-- | rest_framework/generics.py | 13 | ||||
| -rw-r--r-- | rest_framework/relations.py | 35 | ||||
| -rw-r--r-- | rest_framework/tests/generics.py | 10 |
3 files changed, 40 insertions, 18 deletions
diff --git a/rest_framework/generics.py b/rest_framework/generics.py index afcb8a9f..9ccc7898 100644 --- a/rest_framework/generics.py +++ b/rest_framework/generics.py @@ -6,7 +6,7 @@ from __future__ import unicode_literals from django.core.exceptions import ImproperlyConfigured, PermissionDenied from django.core.paginator import Paginator, InvalidPage from django.http import Http404 -from django.shortcuts import get_object_or_404 +from django.shortcuts import get_object_or_404 as _get_object_or_404 from django.utils.translation import ugettext as _ from rest_framework import views, mixins, exceptions from rest_framework.request import clone_request @@ -14,6 +14,17 @@ from rest_framework.settings import api_settings import warnings +def get_object_or_404(queryset, **filter_kwargs): + """ + Same as Django's standard shortcut, but make sure to raise 404 + if the filter_kwargs don't match the required types. + """ + try: + return _get_object_or_404(queryset, **filter_kwargs) + except (TypeError, ValueError): + raise Http404 + + class GenericAPIView(views.APIView): """ Base class for all other generic views. diff --git a/rest_framework/relations.py b/rest_framework/relations.py index c4271e33..41707efc 100644 --- a/rest_framework/relations.py +++ b/rest_framework/relations.py @@ -518,8 +518,6 @@ class HyperlinkedIdentityField(Field): request = self.context.get('request', None) format = self.context.get('format', None) view_name = self.view_name or self.parent.opts.view_name - lookup_field = getattr(obj, self.lookup_field) - kwargs = {self.lookup_field: lookup_field} if request is None: warnings.warn("Using `HyperlinkedIdentityField` without including the " @@ -539,27 +537,30 @@ class HyperlinkedIdentityField(Field): if format and self.format and self.format != format: format = self.format + lookup_field = getattr(obj, self.lookup_field) + kwargs = {self.lookup_field: lookup_field} try: return reverse(view_name, kwargs=kwargs, request=request, format=format) except NoReverseMatch: pass - slug = getattr(obj, self.slug_field, None) - - if not slug: - raise Exception('Could not resolve URL for field using view name "%s"' % view_name) - - kwargs = {self.slug_url_kwarg: slug} - try: - return reverse(view_name, kwargs=kwargs, request=request, format=format) - except NoReverseMatch: - pass + if self.pk_url_kwarg != 'pk': + # Only try pk lookup if it has been explicitly set. + # Otherwise, the default `lookup_field = 'pk'` has us covered. + kwargs = {self.pk_url_kwarg: obj.pk} + try: + return reverse(view_name, kwargs=kwargs, request=request, format=format) + except NoReverseMatch: + pass - kwargs = {self.pk_url_kwarg: obj.pk, self.slug_url_kwarg: slug} - try: - return reverse(view_name, kwargs=kwargs, request=request, format=format) - except NoReverseMatch: - pass + slug = getattr(obj, self.slug_field, None) + if slug: + # Only use slug lookup if a slug field exists on the model + kwargs = {self.slug_url_kwarg: slug} + try: + return reverse(view_name, kwargs=kwargs, request=request, format=format) + except NoReverseMatch: + pass raise Exception('Could not resolve URL for field using view name "%s"' % view_name) diff --git a/rest_framework/tests/generics.py b/rest_framework/tests/generics.py index f091d0db..37734195 100644 --- a/rest_framework/tests/generics.py +++ b/rest_framework/tests/generics.py @@ -279,6 +279,16 @@ class TestInstanceView(TestCase): self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(response.data, expected) + def test_get_instance_view_incorrect_arg(self): + """ + GET requests with an incorrect pk type, should raise 404, not 500. + Regression test for #890. + """ + request = factory.get('/a') + with self.assertNumQueries(0): + response = self.view(request, pk='a').render() + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + def test_put_cannot_set_id(self): """ PUT requests to create a new object should not be able to set the id. |
