From 90280a343746f662ac2e7da4844828a61253c77d Mon Sep 17 00:00:00 2001 From: Ion Scerbatiuc Date: Thu, 19 Mar 2015 14:14:48 -0700 Subject: [PATCH 1/2] Handle reversal of non-API view_name in HyperLinkedRelatedField --- rest_framework/versioning.py | 12 +++++++++- tests/test_versioning.py | 44 +++++++++++++++++++++++++++++++++++- 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/rest_framework/versioning.py b/rest_framework/versioning.py index 51b886f38..6f7952c03 100644 --- a/rest_framework/versioning.py +++ b/rest_framework/versioning.py @@ -1,6 +1,7 @@ # coding: utf-8 from __future__ import unicode_literals from django.utils.translation import ugettext_lazy as _ +from django.core.urlresolvers import NoReverseMatch from rest_framework import exceptions from rest_framework.compat import unicode_http_header from rest_framework.reverse import _reverse @@ -122,7 +123,16 @@ class NamespaceVersioning(BaseVersioning): def reverse(self, viewname, args=None, kwargs=None, request=None, format=None, **extra): if request.version is not None: - viewname = self.get_versioned_viewname(viewname, request) + versioned_viewname = self.get_versioned_viewname(viewname, request) + try: + return super(NamespaceVersioning, self).reverse( + versioned_viewname, args, kwargs, request, format, **extra + ) + except NoReverseMatch: + # If the versioned viewname lookup fails, fallback to the + # default reversal, since it may be a non-API view + pass + return super(NamespaceVersioning, self).reverse( viewname, args, kwargs, request, format, **extra ) diff --git a/tests/test_versioning.py b/tests/test_versioning.py index 90ad8afd2..88ae56ddf 100644 --- a/tests/test_versioning.py +++ b/tests/test_versioning.py @@ -7,6 +7,7 @@ from rest_framework.response import Response from rest_framework.reverse import reverse from rest_framework.test import APIRequestFactory, APITestCase from rest_framework.versioning import NamespaceVersioning +from rest_framework.relations import PKOnlyObject import pytest @@ -234,7 +235,7 @@ class TestInvalidVersion: class TestHyperlinkedRelatedField(UsingURLPatterns, APITestCase): included = [ - url(r'^namespaced/(?P\d+)/$', dummy_view, name='namespaced'), + url(r'^namespaced/(?P\d+)/$', dummy_pk_view, name='namespaced'), ] urlpatterns = [ @@ -262,3 +263,44 @@ class TestHyperlinkedRelatedField(UsingURLPatterns, APITestCase): assert self.field.to_internal_value('/v1/namespaced/3/') == 'object 3' with pytest.raises(serializers.ValidationError): self.field.to_internal_value('/v2/namespaced/3/') + + +class TestNamespaceVersioningHyperlinkedRelatedFieldScheme(UsingURLPatterns, APITestCase): + included = [ + url(r'^namespaced/(?P\d+)/$', dummy_pk_view, name='namespaced'), + ] + + urlpatterns = [ + url(r'^v1/', include(included, namespace='v1')), + url(r'^v2/', include(included, namespace='v2')), + url(r'^non-api/(?P\d+)/$', dummy_pk_view, name='non-api-view') + ] + + def _create_field(self, view_name, version): + request = factory.get("/") + request.versioning_scheme = NamespaceVersioning() + request.version = version + + field = serializers.HyperlinkedRelatedField( + view_name=view_name, + read_only=True) + field._context = {'request': request} + return field + + def test_api_url_is_properly_reversed_with_v1(self): + field = self._create_field('namespaced', 'v1') + assert field.to_representation(PKOnlyObject(3)) == 'http://testserver/v1/namespaced/3/' + + def test_api_url_is_properly_reversed_with_v2(self): + field = self._create_field('namespaced', 'v2') + assert field.to_representation(PKOnlyObject(5)) == 'http://testserver/v2/namespaced/5/' + + def test_non_api_url_is_properly_reversed_regardless_of_the_version(self): + """ + Regression test for #2711 + """ + field = self._create_field('non-api-view', 'v1') + assert field.to_representation(PKOnlyObject(10)) == 'http://testserver/non-api/10/' + + field = self._create_field('non-api-view', 'v2') + assert field.to_representation(PKOnlyObject(10)) == 'http://testserver/non-api/10/' From fac27853418699116304ad8d77270fe9a20873dc Mon Sep 17 00:00:00 2001 From: Ion Scerbatiuc Date: Thu, 19 Mar 2015 16:12:28 -0700 Subject: [PATCH 2/2] Handling the fallback in `reverse` --- rest_framework/reverse.py | 9 ++++++++- rest_framework/versioning.py | 12 +----------- tests/test_reverse.py | 27 +++++++++++++++++++++++++++ 3 files changed, 36 insertions(+), 12 deletions(-) diff --git a/rest_framework/reverse.py b/rest_framework/reverse.py index a251d99d6..e6d3f5634 100644 --- a/rest_framework/reverse.py +++ b/rest_framework/reverse.py @@ -3,6 +3,7 @@ Provide urlresolver functions that return fully qualified URLs or view names """ from __future__ import unicode_literals from django.core.urlresolvers import reverse as django_reverse +from django.core.urlresolvers import NoReverseMatch from django.utils import six from django.utils.functional import lazy @@ -15,7 +16,13 @@ def reverse(viewname, args=None, kwargs=None, request=None, format=None, **extra """ scheme = getattr(request, 'versioning_scheme', None) if scheme is not None: - return scheme.reverse(viewname, args, kwargs, request, format, **extra) + try: + return scheme.reverse(viewname, args, kwargs, request, format, **extra) + except NoReverseMatch: + # In case the versioning scheme reversal fails, fallback to the + # default implementation + pass + return _reverse(viewname, args, kwargs, request, format, **extra) diff --git a/rest_framework/versioning.py b/rest_framework/versioning.py index 6f7952c03..51b886f38 100644 --- a/rest_framework/versioning.py +++ b/rest_framework/versioning.py @@ -1,7 +1,6 @@ # coding: utf-8 from __future__ import unicode_literals from django.utils.translation import ugettext_lazy as _ -from django.core.urlresolvers import NoReverseMatch from rest_framework import exceptions from rest_framework.compat import unicode_http_header from rest_framework.reverse import _reverse @@ -123,16 +122,7 @@ class NamespaceVersioning(BaseVersioning): def reverse(self, viewname, args=None, kwargs=None, request=None, format=None, **extra): if request.version is not None: - versioned_viewname = self.get_versioned_viewname(viewname, request) - try: - return super(NamespaceVersioning, self).reverse( - versioned_viewname, args, kwargs, request, format, **extra - ) - except NoReverseMatch: - # If the versioned viewname lookup fails, fallback to the - # default reversal, since it may be a non-API view - pass - + viewname = self.get_versioned_viewname(viewname, request) return super(NamespaceVersioning, self).reverse( viewname, args, kwargs, request, format, **extra ) diff --git a/tests/test_reverse.py b/tests/test_reverse.py index 675a9d5a0..08c270239 100644 --- a/tests/test_reverse.py +++ b/tests/test_reverse.py @@ -1,5 +1,6 @@ from __future__ import unicode_literals from django.conf.urls import patterns, url +from django.core.urlresolvers import NoReverseMatch from django.test import TestCase from rest_framework.reverse import reverse from rest_framework.test import APIRequestFactory @@ -16,6 +17,18 @@ urlpatterns = patterns( ) +class MockVersioningScheme(object): + + def __init__(self, raise_error=False): + self.raise_error = raise_error + + def reverse(self, *args, **kwargs): + if self.raise_error: + raise NoReverseMatch() + + return 'http://scheme-reversed/view' + + class ReverseTests(TestCase): """ Tests for fully qualified URLs when using `reverse`. @@ -26,3 +39,17 @@ class ReverseTests(TestCase): request = factory.get('/view') url = reverse('view', request=request) self.assertEqual(url, 'http://testserver/view') + + def test_reverse_with_versioning_scheme(self): + request = factory.get('/view') + request.versioning_scheme = MockVersioningScheme() + + url = reverse('view', request=request) + self.assertEqual(url, 'http://scheme-reversed/view') + + def test_reverse_with_versioning_scheme_fallback_to_default_on_error(self): + request = factory.get('/view') + request.versioning_scheme = MockVersioningScheme(raise_error=True) + + url = reverse('view', request=request) + self.assertEqual(url, 'http://testserver/view')