fix bug 1229783 - Restrict resource IDs to ints

Instead of matching any string, restrict resource IDs to integers in URL
patterns. Let viewsets declare an alternative_lookup function, to be
used if an alternate lookup, like viewfeatures's slug lookup, is
allowed. This means that resource lookups will default to 404's for
non-integer IDs.

webplatformcompat/routers.py

@@ -16,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
@@ -58,10 +59,12 @@ def get_urls(self):
         - 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)
@@ -103,4 +106,33 @@ def get_urls(self):
                 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
@@ -189,6 +201,8 @@ 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."""
@@ -246,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)