Merge pull request #98 from mdn/require_int_ids_1229783

fix bug 1229783 - require integer IDs 

r=self

webplatformcompat/routers.py

@@ -4,12 +4,10 @@
 from collections import OrderedDict
 
 from django.conf.urls import url
-from django.core.urlresolvers import RegexURLResolver
 from django.views.generic import RedirectView
 from rest_framework.response import Response
 from rest_framework.reverse import reverse
 from rest_framework.routers import DefaultRouter
-from rest_framework.urlpatterns import format_suffix_patterns
 from rest_framework.views import APIView
 
 
@@ -18,6 +16,7 @@ class GroupedRouter(DefaultRouter):
 
     view_groups = {}
     allowed_ext = ['api', 'json']
+    alt_lookup_format = '(?P<{lookup_url_kwarg}>{lookup_value})'
 
     def __init__(self, version, *args, **kwargs):
         self.version = version
@@ -55,41 +54,85 @@ def get_urls(self):
         """
         Return the URL patterns handled by this router.
 
-        Include a default root view for the API, and appending `.json` style
-        format suffixes.
+        Same as rest_framework.routers.BaseRouter.get_urls, but
+        - Adds a 'root' view for the API
+        - Asserts each viewset has mapped routes
+        - Adds redirects from URLs ending in slashes
+        - Adds format suffix ('.json') versions
+        - Adds alternate lookup views
         """
         urls = []
         assert self.include_root_view
         assert self.include_format_suffixes
+        assert not self.trailing_slash
         root_url = url(
             r'^$', self.get_api_root_view(), name=self.root_view_name)
         urls.append(root_url)
 
-        default_urls = super(DefaultRouter, self).get_urls()
-        urls.extend(default_urls)
-
-        # Add format suffix versions
-        # Include special-case of view_features allowing .html as well
-        furls = format_suffix_patterns(urls, allowed=self.allowed_ext)
-        urls = []
-        for u in furls:
-            assert not isinstance(u, RegexURLResolver)
-            match = (
-                u.name == 'viewfeatures-detail' and
-                'api|json' in u.regex.pattern)
-            if match:
-                pattern = u.regex.pattern.replace('api|json', 'api|json|html')
-                view = u._callback or u._callback_str
-                u = url(pattern, view, u.default_args, u.name)
-            urls.append(u)
-
-        # Add redirects for list views
-        assert not self.trailing_slash
-        redirect_urls = []
-        for u in default_urls:
-            if u.name.endswith('-list'):
-                pattern = u.regex.pattern.replace('$', '/$')
+        # Add URLs from registry
+        for prefix, viewset, basename in self.registry:
+            lookup = self.get_lookup_regex(viewset)
+            routes = self.get_routes(viewset)
+
+            for route in routes:
+                # Get valid routes for the viewset, or fail
+                mapping = self.get_method_map(viewset, route.mapping)
+                assert mapping, 'viewset %s has no routes.' % viewset
+
+                # Build the url pattern
+                regex = route.url.format(
+                    prefix=prefix,
+                    lookup=lookup,
+                    trailing_slash=self.trailing_slash
+                )
+                # Add standard endpoint
+                name = route.name.format(basename=basename)
+                view = viewset.as_view(mapping, **route.initkwargs)
+                urls.append(url(regex, view, name=name))
+
+                # Add redirects to remove slashes
+                no_slash_pattern = regex.replace('$', '/$')
                 view = RedirectView.as_view(
-                    pattern_name=u.name, permanent=False, query_string=True)
-                redirect_urls.append(url(pattern, view))
-        return urls + redirect_urls
+                    pattern_name=name, permanent=False, query_string=True)
+                urls.append(url(no_slash_pattern, view))
+
+                # Add endpoints that include the format as an extension
+                fmt_name = name
+                fmt_suffixes = '|'.join(
+                    getattr(viewset, 'format_suffixes', ('api', 'json')))
+                fmt_regex_suffix = (
+                    r'\.(?P<format>({}))/?$'.format(fmt_suffixes))
+                fmt_base_pattern = regex.rstrip('$')
+                fmt_regex = fmt_base_pattern + fmt_regex_suffix
+                fmt_view = viewset.as_view(mapping, **route.initkwargs)
+                urls.append(url(fmt_regex, fmt_view, name=fmt_name))
+
+                # Add optional alternate lookup endpoint
+                is_detail = name.endswith('detail')
+                if is_detail and hasattr(viewset, 'alternate_lookup'):
+                    alt_lookup_field = (
+                        getattr(viewset, 'alt_lookup_field', None) or
+                        'altkey')
+                    alt_lookup_value_regex = (
+                        getattr(viewset, 'alt_lookup_value_regex', None) or
+                        '[^/.]+')
+                    alt_lookup = self.alt_lookup_format.format(
+                        lookup_url_kwarg=alt_lookup_field,
+                        lookup_value=alt_lookup_value_regex)
+                    alt_regex = route.url.format(
+                        prefix=prefix,
+                        lookup=alt_lookup,
+                        trailing_slash='/?')
+                    alt_name = '%s-by-%s' % (basename, alt_lookup_field)
+                    alt_view = viewset.as_view(
+                        {'get': 'alternate_lookup'}, **route.initkwargs)
+                    urls.append(url(alt_regex, alt_view, name=alt_name))
+
+                    alt_base_pattern = alt_regex.rstrip('/?$')
+                    alt_fmt_regex = alt_base_pattern + fmt_regex_suffix
+                    alt_fmt_view = viewset.as_view(
+                        {'get': 'alternate_lookup'}, **route.initkwargs)
+                    urls.append(
+                        url(alt_fmt_regex, alt_fmt_view, name=alt_name))
+
+        return urls

webplatformcompat/tests/test_viewsets.py

@@ -59,15 +59,20 @@ def test_get_by_id(self):
 
     def test_get_by_slug(self):
         feature = self.create(Feature, slug='feature')
-        ret = self.view.get_object_or_404(self.queryset, pk=feature.slug)
-        self.assertEqual(ret.pk, feature.pk)
-        with self.assertNumQueries(0):
-            self.assertIsNone(ret.parent_id)
+        url = self.api_reverse('viewfeatures-by-slug', slug=feature.slug)
+        real_url = self.api_reverse('viewfeatures-detail', pk=feature.pk)
+        request = APIRequestFactory().get(url)
+        ret = self.view.alternate_lookup(request, feature.slug)
+        self.assertEqual(ret.status_code, 302)
+        self.assertEqual(ret.url, real_url)
 
     def test_get_by_slug_not_found(self):
-        self.assertFalse(self.queryset.filter(slug='feature').exists())
+        slug = 'feature'
+        url = self.api_reverse('viewfeatures-by-slug', slug=slug)
+        self.assertFalse(self.queryset.filter(slug=slug).exists())
+        request = APIRequestFactory().get(url)
         self.assertRaises(
-            Http404, self.view.get_object_or_404, self.queryset, pk='feature')
+            Http404, self.view.alternate_lookup, request, slug)
 
     def test_feature_found_html(self):
         parent = self.create(Feature, slug='parent')
@@ -78,8 +83,19 @@ def test_feature_found_html(self):
         response = self.client.get(url)
         self.assertContains(response, '<h2>Browser compatibility</h2>')
 
+    def test_feature_found_by_slug_html(self):
+        parent = self.create(Feature, slug='parent')
+        feature = self.create(Feature, slug='feature', parent=parent)
+        self.create(Feature, slug='child', parent=feature)
+        url = self.api_reverse(
+            'viewfeatures-by-slug', slug=feature.slug, format='html')
+        real_url = self.api_reverse(
+            'viewfeatures-detail', pk=feature.pk, format='html')
+        response = self.client.get(url)
+        self.assertRedirects(response, real_url)
+
     def test_feature_not_found_html(self):
         self.assertFalse(Feature.objects.filter(id=666).exists())
-        url = self.api_reverse('viewfeatures-detail', pk='666') + '.html'
+        url = self.api_reverse('viewfeatures-by-slug', slug='666') + '.html'
         response = self.client.get(url)
         self.assertEqual(404, response.status_code)

webplatformcompat/v1/viewsets.py

@@ -125,3 +125,4 @@ class ViewFeaturesViewSet(ViewFeaturesBaseViewSet):
     filter_backends = (UnorderedDjangoFilterBackend,)
     filter_fields = ('slug',)
     template_name = 'webplatformcompat/feature-basic-v1.html'
+    namespace = 'v1'

webplatformcompat/v2/viewsets.py

@@ -396,3 +396,4 @@ class ViewFeaturesViewSet(ViewFeaturesBaseViewSet):
         JsonApiV10Renderer, BrowsableAPIRenderer,
         JsonApiV10TemplateHTMLRenderer)
     template_name = 'webplatformcompat/feature-basic-v2.html'
+    namespace = 'v2'

webplatformcompat/viewsets.py

@@ -2,6 +2,8 @@
 """API endpoints for CRUD operations."""
 
 from django.contrib.auth.models import User
+from django.core.urlresolvers import reverse
+from django.shortcuts import redirect
 from django.utils.functional import cached_property
 from django.http import Http404
 from rest_framework.mixins import UpdateModelMixin
@@ -73,18 +75,28 @@ def get_fields_extra(self):
         return serializer_cls.get_fields_extra()
 
 
+class GroupRouterMixin(object):
+    """Extra parameters used by the GroupedRouter."""
+
+    lookup_value_regex = r'\d+'
+    alt_lookup_field = None
+    alt_lookup_value_regex = None
+
+
 class ModelViewSet(
-        PartialPutMixin, CachedViewMixin, FieldsExtraMixin, BaseModelViewSet):
+        PartialPutMixin, CachedViewMixin, FieldsExtraMixin, GroupRouterMixin,
+        BaseModelViewSet):
     """Base class for ViewSets supporting CRUD operations on models."""
 
 
-class ReadOnlyModelViewSet(FieldsExtraMixin, BaseROModelViewSet):
+class ReadOnlyModelViewSet(
+        FieldsExtraMixin, GroupRouterMixin, BaseROModelViewSet):
     """Base class for ViewSets supporting read operations on models."""
 
 
 class ReadUpdateModelViewSet(
         PartialPutMixin, CachedViewMixin, FieldsExtraMixin, UpdateModelMixin,
-        BaseROModelViewSet):
+        GroupRouterMixin, BaseROModelViewSet):
     """Base class for ViewSets supporting read and update operations."""
 
     pass
@@ -188,6 +200,9 @@ class HistoricalVersionBaseViewSet(ReadOnlyModelViewSet):
 
 class ViewFeaturesBaseViewSet(ReadUpdateModelViewSet):
     queryset = Feature.objects.order_by('id')
+    format_suffixes = ('api', 'json', 'html')
+    alt_lookup_field = 'slug'
+    alt_lookup_value_regex = r'[-a-zA-Z0-9_]+'
 
     def get_serializer_class(self):
         """Return the serializer to use based on action and query."""
@@ -245,19 +260,13 @@ def include_child_pages(self):
         falsy = ('0', 'false', 'no')
         return bool(child_pages.lower() not in falsy)
 
-    def get_object_or_404(self, queryset, *filter_args, **filter_kwargs):
-        """The feature can be accessed by primary key or by feature slug."""
-        pk_or_slug = filter_kwargs['pk']
+    def alternate_lookup(self, request, slug, **extra_kwargs):
+        """Lookup features by slug."""
         try:
-            pk = int(pk_or_slug)
-        except ValueError:
-            try:
-                # parent_id is needed by the MPTT model loader,
-                # including it saves a query later.
-                pk = Feature.objects.only('pk', 'parent_id').get(
-                    slug=pk_or_slug).pk
-            except queryset.model.DoesNotExist:
-                raise Http404(
-                    'No %s matches the given query.' % queryset.model)
-        return super(ViewFeaturesBaseViewSet, self).get_object_or_404(
-            queryset, pk=pk)
+            pk = Feature.objects.only('pk').get(slug=slug).pk
+        except Feature.DoesNotExist:
+            raise Http404('No feature has the requested slug.')
+        kwargs = {'pk': pk}
+        kwargs.update(extra_kwargs)
+        url = reverse('%s:viewfeatures-detail' % self.namespace, kwargs=kwargs)
+        return redirect(url)