[PATCH v3 08/16] REST: Use generic views instead of ViewSets

Daniel Axtens dja at axtens.net
Wed Nov 30 09:14:16 AEDT 2016


Stephen Finucane <stephen at that.guru> writes:

> ViewSet provide an easy way to define an API, but they don't offer much
> flexibility. To get them to work as expected, a lot of hacks were
> required. Generic views provide a more verbose, but ultimately easier to
> understand, version. Using generic views lets us remove the dependency
> of drf-nested-routers, bringing us back down to two main dependencies.
> It also lets us remove the AuthenticatedReadOnly permission class, as
> the DRF provides a similar permission class that can be used with
> generic views.
>
> The main user facing change is that invalid methods, such as POST on an
> endpoint that doesn't allow object creation, will now return a HTTP 405
> (Method Not Allowed) error code rather than the HTTP 403 (Forbidden)
> error code previously returned. This is the semantically correct option
> and should have been used all along.
>

Heaps better, thank you!

Regards,
Daniel

> Signed-off-by: Stephen Finucane <stephen at that.guru>
> ---
> v3:
> - Override 'get_queryset' for '/patch' endpoints, resolving an issue
>   under Python 3.4
> - Correctly raise 404 for invalid project id and linkname (Andrew
>   Donnellan)
> - Remove drf-nested-routers dependency (Andrew Donnellan)
> ---
>  patchwork/api/base.py            | 26 +++++-------
>  patchwork/api/check.py           | 74 ++++++++++++++++++++--------------
>  patchwork/api/index.py           | 36 +++++++++++++++++
>  patchwork/api/patch.py           | 86 ++++++++++++++++++++++++----------------
>  patchwork/api/person.py          | 30 ++++++++++----
>  patchwork/api/project.py         | 56 +++++++++++++++++---------
>  patchwork/api/user.py            | 28 ++++++++++---
>  patchwork/settings/base.py       |  4 +-
>  patchwork/tests/test_rest_api.py | 80 ++++++++++++++++++++-----------------
>  patchwork/urls.py                | 63 ++++++++++++++++++++---------
>  requirements-dev.txt             |  1 -
>  requirements-prod.txt            |  1 -
>  tox.ini                          |  1 -
>  13 files changed, 317 insertions(+), 169 deletions(-)
>  create mode 100644 patchwork/api/index.py
>
> diff --git a/patchwork/api/base.py b/patchwork/api/base.py
> index 3639ebe..dbd8148 100644
> --- a/patchwork/api/base.py
> +++ b/patchwork/api/base.py
> @@ -18,10 +18,10 @@
>  # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>  
>  from django.conf import settings
> +from django.shortcuts import get_object_or_404
>  from rest_framework import permissions
>  from rest_framework.pagination import PageNumberPagination
>  from rest_framework.response import Response
> -from rest_framework.viewsets import ModelViewSet
>  
>  
>  class LinkHeaderPagination(PageNumberPagination):
> @@ -53,11 +53,6 @@ class LinkHeaderPagination(PageNumberPagination):
>  
>  class PatchworkPermission(permissions.BasePermission):
>      """This permission works for Project and Patch model objects"""
> -    def has_permission(self, request, view):
> -        if request.method in ('POST', 'DELETE'):
> -            return False
> -        return super(PatchworkPermission, self).has_permission(request, view)
> -
>      def has_object_permission(self, request, view, obj):
>          # read only for everyone
>          if request.method in permissions.SAFE_METHODS:
> @@ -65,14 +60,15 @@ class PatchworkPermission(permissions.BasePermission):
>          return obj.is_editable(request.user)
>  
>  
> -class AuthenticatedReadOnly(permissions.BasePermission):
> -    def has_permission(self, request, view):
> -        authenticated = request.user.is_authenticated()
> -        return authenticated and request.method in permissions.SAFE_METHODS
> -
> +class MultipleFieldLookupMixin(object):
> +    """Enable multiple lookups fields."""
>  
> -class PatchworkViewSet(ModelViewSet):
> -    pagination_class = LinkHeaderPagination
> +    def get_object(self):
> +        queryset = self.filter_queryset(self.get_queryset())
> +        filter_kwargs = {}
> +        for field_name, field in zip(
> +                self.lookup_fields, self.lookup_url_kwargs):
> +            if self.kwargs[field]:
> +                filter_kwargs[field_name] = self.kwargs[field]
>  
> -    def get_queryset(self):
> -        return self.serializer_class.Meta.model.objects.all()
> +        return get_object_or_404(queryset, **filter_kwargs)
> diff --git a/patchwork/api/check.py b/patchwork/api/check.py
> index 2fd681a..78c2141 100644
> --- a/patchwork/api/check.py
> +++ b/patchwork/api/check.py
> @@ -17,15 +17,16 @@
>  # along with Patchwork; if not, write to the Free Software
>  # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>  
> -from django.core.urlresolvers import reverse
>  from rest_framework.exceptions import PermissionDenied
> +from rest_framework.generics import ListCreateAPIView
> +from rest_framework.generics import RetrieveAPIView
>  from rest_framework.relations import HyperlinkedRelatedField
> -from rest_framework.response import Response
>  from rest_framework.serializers import CurrentUserDefault
>  from rest_framework.serializers import HiddenField
> -from rest_framework.serializers import ModelSerializer
> +from rest_framework.serializers import HyperlinkedModelSerializer
> +from rest_framework.serializers import HyperlinkedIdentityField
>  
> -from patchwork.api.base import PatchworkViewSet
> +from patchwork.api.base import MultipleFieldLookupMixin
>  from patchwork.models import Check
>  from patchwork.models import Patch
>  
> @@ -38,10 +39,29 @@ class CurrentPatchDefault(object):
>          return self.patch
>  
>  
> -class CheckSerializer(ModelSerializer):
> +class CheckHyperlinkedIdentityField(HyperlinkedIdentityField):
> +
> +    def get_url(self, obj, view_name, request, format):
> +        # Unsaved objects will not yet have a valid URL.
> +        if obj.pk is None:
> +            return None
> +
> +        return self.reverse(
> +            view_name,
> +            kwargs={
> +                'patch_id': obj.patch.id,
> +                'check_id': obj.id,
> +            },
> +            request=request,
> +            format=format,
> +        )
> +
> +
> +class CheckSerializer(HyperlinkedModelSerializer):
>      user = HyperlinkedRelatedField(
> -        'user-detail', read_only=True, default=CurrentUserDefault())
> +        'api-user-detail', read_only=True, default=CurrentUserDefault())
>      patch = HiddenField(default=CurrentPatchDefault())
> +    url = CheckHyperlinkedIdentityField('api-check-detail')
>  
>      def run_validation(self, data):
>          for val, label in Check.STATE_CHOICES:
> @@ -53,45 +73,39 @@ class CheckSerializer(ModelSerializer):
>      def to_representation(self, instance):
>          data = super(CheckSerializer, self).to_representation(instance)
>          data['state'] = instance.get_state_display()
> -        # drf-nested doesn't handle HyperlinkedModelSerializers properly,
> -        # so we have to put the url in by hand here.
> -        url = self.context['request'].build_absolute_uri(reverse(
> -            'api_1.0:patch-detail', args=[instance.patch.id]))
> -        data['url'] = url + 'checks/%s/' % instance.id
>          return data
>  
>      class Meta:
>          model = Check
> -        fields = ('patch', 'user', 'date', 'state', 'target_url',
> +        fields = ('url', 'patch', 'user', 'date', 'state', 'target_url',
>                    'context', 'description')
>          read_only_fields = ('date',)
> +        extra_kwargs = {
> +            'url': {'view_name': 'api-check-detail'},
> +        }
> +
>  
> +class CheckMixin(object):
>  
> -class CheckViewSet(PatchworkViewSet):
> +    queryset = Check.objects.prefetch_related('patch', 'user')
>      serializer_class = CheckSerializer
>  
> -    def not_allowed(self, request, **kwargs):
> -        raise PermissionDenied()
>  
> -    update = not_allowed
> -    partial_update = not_allowed
> -    destroy = not_allowed
> +class CheckListCreate(CheckMixin, ListCreateAPIView):
> +    """List or create checks."""
> +
> +    lookup_url_kwarg = 'patch_id'
>  
> -    def create(self, request, patch_pk):
> -        p = Patch.objects.get(id=patch_pk)
> +    def create(self, request, patch_id):
> +        p = Patch.objects.get(id=patch_id)
>          if not p.is_editable(request.user):
>              raise PermissionDenied()
>          request.patch = p
> -        return super(CheckViewSet, self).create(request)
> +        return super(CheckListCreate, self).create(request)
>  
> -    def list(self, request, patch_pk):
> -        queryset = self.filter_queryset(self.get_queryset())
> -        queryset = queryset.filter(patch=patch_pk)
>  
> -        page = self.paginate_queryset(queryset)
> -        if page is not None:
> -            serializer = self.get_serializer(page, many=True)
> -            return self.get_paginated_response(serializer.data)
> +class CheckDetail(CheckMixin, MultipleFieldLookupMixin, RetrieveAPIView):
> +    """Show a check."""
>  
> -        serializer = self.get_serializer(queryset, many=True)
> -        return Response(serializer.data)
> +    lookup_url_kwargs = ('patch_id', 'check_id')
> +    lookup_fields = ('patch_id', 'id')
> diff --git a/patchwork/api/index.py b/patchwork/api/index.py
> new file mode 100644
> index 0000000..58aeb87
> --- /dev/null
> +++ b/patchwork/api/index.py
> @@ -0,0 +1,36 @@
> +# Patchwork - automated patch tracking system
> +# Copyright (C) 2016 Linaro Corporation
> +#
> +# This file is part of the Patchwork package.
> +#
> +# Patchwork is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# Patchwork is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with Patchwork; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> +
> +from django.core.urlresolvers import reverse
> +from rest_framework.response import Response
> +from rest_framework.views import APIView
> +
> +
> +class IndexView(APIView):
> +
> +    def get(self, request, format=None):
> +        return Response({
> +            'projects': request.build_absolute_uri(
> +                reverse('api-project-list')),
> +            'users': request.build_absolute_uri(reverse('api-user-list')),
> +            'people': request.build_absolute_uri(reverse('api-person-list')),
> +            'patches': request.build_absolute_uri(reverse('api-patch-list')),
> +            'covers': request.build_absolute_uri(reverse('api-cover-list')),
> +            'series': request.build_absolute_uri(reverse('api-series-list')),
> +        })
> diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
> index 811ba1e..8427450 100644
> --- a/patchwork/api/patch.py
> +++ b/patchwork/api/patch.py
> @@ -19,30 +19,22 @@
>  
>  import email.parser
>  
> +from django.core.urlresolvers import reverse
>  from rest_framework.serializers import HyperlinkedModelSerializer
> -from rest_framework.serializers import ListSerializer
> +from rest_framework.generics import ListAPIView
> +from rest_framework.generics import RetrieveUpdateAPIView
>  from rest_framework.serializers import SerializerMethodField
>  
>  from patchwork.api.base import PatchworkPermission
> -from patchwork.api.base import PatchworkViewSet
>  from patchwork.models import Patch
>  
>  
> -class PatchListSerializer(ListSerializer):
> -    """Semi hack to make the list of patches more efficient"""
> -    def to_representation(self, data):
> -        del self.child.fields['content']
> -        del self.child.fields['headers']
> -        del self.child.fields['diff']
> -        return super(PatchListSerializer, self).to_representation(data)
> -
> -
> -class PatchSerializer(HyperlinkedModelSerializer):
> +class PatchListSerializer(HyperlinkedModelSerializer):
>      mbox = SerializerMethodField()
>      state = SerializerMethodField()
>      tags = SerializerMethodField()
> -    headers = SerializerMethodField()
>      check = SerializerMethodField()
> +    checks = SerializerMethodField()
>  
>      def get_state(self, instance):
>          return instance.state.name
> @@ -58,39 +50,65 @@ class PatchSerializer(HyperlinkedModelSerializer):
>          else:
>              return None
>  
> -    def get_headers(self, instance):
> -        if instance.headers:
> -            return
> -        email.parser.Parser().parsestr(instance.headers, True)
> -
>      def get_check(self, instance):
>          return instance.combined_check_state
>  
> -    def to_representation(self, instance):
> -        data = super(PatchSerializer, self).to_representation(instance)
> -        data['checks'] = data['url'] + 'checks/'
> -        return data
> +    def get_checks(self, instance):
> +        return self.context.get('request').build_absolute_uri(
> +            reverse('api-check-list', kwargs={'patch_id': instance.id}))
>  
>      class Meta:
>          model = Patch
> -        list_serializer_class = PatchListSerializer
>          fields = ('url', 'project', 'msgid', 'date', 'name', 'commit_ref',
>                    'pull_url', 'state', 'archived', 'hash', 'submitter',
> -                  'delegate', 'mbox', 'check', 'checks', 'tags', 'headers',
> -                  'content', 'diff')
> -        read_only_fields = ('project', 'name', 'date', 'submitter', 'diff',
> -                            'content', 'hash', 'msgid')
> +                  'delegate', 'mbox', 'check', 'checks', 'tags')
> +        read_only_fields = ('project', 'msgid', 'date', 'name', 'hash',
> +                            'submitter', 'mbox', 'mbox', 'series', 'check',
> +                            'checks', 'tags')
> +        extra_kwargs = {
> +            'url': {'view_name': 'api-patch-detail'},
> +            'project': {'view_name': 'api-project-detail'},
> +            'submitter': {'view_name': 'api-person-detail'},
> +            'delegate': {'view_name': 'api-user-detail'},
> +        }
> +
> +
> +class PatchDetailSerializer(PatchListSerializer):
> +    headers = SerializerMethodField()
> +
> +    def get_headers(self, patch):
> +        if patch.headers:
> +            return email.parser.Parser().parsestr(patch.headers, True)
> +
> +    class Meta:
> +        model = Patch
> +        fields = PatchListSerializer.Meta.fields + (
> +            'headers', 'content', 'diff')
> +        read_only_fields = PatchListSerializer.Meta.read_only_fields + (
> +            'headers', 'content', 'diff')
> +        extra_kwargs = PatchListSerializer.Meta.extra_kwargs
> +
> +
> +class PatchList(ListAPIView):
> +    """List patches."""
> +
> +    permission_classes = (PatchworkPermission,)
> +    serializer_class = PatchListSerializer
> +
> +    def get_queryset(self):
> +        return Patch.objects.all().with_tag_counts()\
> +            .prefetch_related('check_set')\
> +            .select_related('state', 'submitter', 'delegate')\
> +            .defer('content', 'diff', 'headers')
> +
>  
> +class PatchDetail(RetrieveUpdateAPIView):
> +    """Show a patch."""
>  
> -class PatchViewSet(PatchworkViewSet):
>      permission_classes = (PatchworkPermission,)
> -    serializer_class = PatchSerializer
> +    serializer_class = PatchDetailSerializer
>  
>      def get_queryset(self):
> -        qs = super(PatchViewSet, self).get_queryset().with_tag_counts()\
> +        return Patch.objects.all().with_tag_counts()\
>              .prefetch_related('check_set')\
>              .select_related('state', 'submitter', 'delegate')
> -        if 'pk' not in self.kwargs:
> -            # we are doing a listing, we don't need these fields
> -            qs = qs.defer('content', 'diff', 'headers')
> -        return qs
> diff --git a/patchwork/api/person.py b/patchwork/api/person.py
> index fe1e3b7..c84cff5 100644
> --- a/patchwork/api/person.py
> +++ b/patchwork/api/person.py
> @@ -18,9 +18,10 @@
>  # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>  
>  from rest_framework.serializers import HyperlinkedModelSerializer
> +from rest_framework.generics import ListAPIView
> +from rest_framework.generics import RetrieveAPIView
> +from rest_framework.permissions import IsAuthenticated
>  
> -from patchwork.api.base import AuthenticatedReadOnly
> -from patchwork.api.base import PatchworkViewSet
>  from patchwork.models import Person
>  
>  
> @@ -28,12 +29,27 @@ class PersonSerializer(HyperlinkedModelSerializer):
>      class Meta:
>          model = Person
>          fields = ('url', 'name', 'email', 'user')
> +        read_only_fields = fields
> +        extra_kwargs = {
> +            'url': {'view_name': 'api-person-detail'},
> +            'user': {'view_name': 'api-user-detail'},
> +        }
>  
>  
> -class PeopleViewSet(PatchworkViewSet):
> -    permission_classes = (AuthenticatedReadOnly,)
> +class PersonMixin(object):
> +
> +    queryset = Person.objects.prefetch_related('user')
> +    permission_classes = (IsAuthenticated,)
>      serializer_class = PersonSerializer
>  
> -    def get_queryset(self):
> -        qs = super(PeopleViewSet, self).get_queryset()
> -        return qs.prefetch_related('user')
> +
> +class PersonList(PersonMixin, ListAPIView):
> +    """List users."""
> +
> +    pass
> +
> +
> +class PersonDetail(PersonMixin, RetrieveAPIView):
> +    """Show a user."""
> +
> +    pass
> diff --git a/patchwork/api/project.py b/patchwork/api/project.py
> index af2aea0..14bc73d 100644
> --- a/patchwork/api/project.py
> +++ b/patchwork/api/project.py
> @@ -17,11 +17,13 @@
>  # along with Patchwork; if not, write to the Free Software
>  # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>  
> +from django.shortcuts import get_object_or_404
> +from rest_framework.generics import ListAPIView
> +from rest_framework.generics import RetrieveUpdateAPIView
>  from rest_framework.serializers import CharField
>  from rest_framework.serializers import HyperlinkedModelSerializer
>  
>  from patchwork.api.base import PatchworkPermission
> -from patchwork.api.base import PatchworkViewSet
>  from patchwork.models import Project
>  
>  
> @@ -34,26 +36,44 @@ class ProjectSerializer(HyperlinkedModelSerializer):
>          model = Project
>          fields = ('url', 'name', 'link_name', 'list_id', 'list_email',
>                    'web_url', 'scm_url', 'webscm_url')
> +        read_only_fields = ('name', 'maintainers')
> +        extra_kwargs = {
> +            'url': {'view_name': 'api-project-detail'},
> +        }
>  
>  
> -class ProjectViewSet(PatchworkViewSet):
> +class ProjectMixin(object):
> +
> +    queryset = Project.objects.all()
>      permission_classes = (PatchworkPermission,)
>      serializer_class = ProjectSerializer
>  
> -    def _handle_linkname(self, pk):
> -        '''Make it easy for users to list by project-id or linkname'''
> -        qs = self.get_queryset()
> +    def get_object(self):
> +        queryset = self.filter_queryset(self.get_queryset())
> +
> +        assert 'pk' in self.kwargs
> +
>          try:
> -            qs.get(id=pk)
> -        except (self.serializer_class.Meta.model.DoesNotExist, ValueError):
> -            # probably a non-numeric value which means we are going by linkname
> -            self.kwargs = {'linkname': pk}  # try and lookup by linkname
> -            self.lookup_field = 'linkname'
> -
> -    def retrieve(self, request, pk=None):
> -        self._handle_linkname(pk)
> -        return super(ProjectViewSet, self).retrieve(request, pk)
> -
> -    def partial_update(self, request, pk=None):
> -        self._handle_linkname(pk)
> -        return super(ProjectViewSet, self).partial_update(request, pk)
> +            obj = queryset.get(id=int(self.kwargs['pk']))
> +        except (ValueError, Project.DoesNotExist):
> +            obj = get_object_or_404(queryset, linkname=self.kwargs['pk'])
> +
> +        # NOTE(stephenfin): We must do this to make sure the 'url'
> +        # field is populated correctly
> +        self.kwargs['pk'] = obj.id
> +
> +        self.check_object_permissions(self.request, obj)
> +
> +        return obj
> +
> +
> +class ProjectList(ProjectMixin, ListAPIView):
> +    """List projects."""
> +
> +    pass
> +
> +
> +class ProjectDetail(ProjectMixin, RetrieveUpdateAPIView):
> +    """Show a project."""
> +
> +    pass
> diff --git a/patchwork/api/user.py b/patchwork/api/user.py
> index 3a4ae1a..8fe3e74 100644
> --- a/patchwork/api/user.py
> +++ b/patchwork/api/user.py
> @@ -18,18 +18,36 @@
>  # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>  
>  from django.contrib.auth.models import User
> +from rest_framework.generics import ListAPIView
> +from rest_framework.generics import RetrieveAPIView
> +from rest_framework.permissions import IsAuthenticated
>  from rest_framework.serializers import HyperlinkedModelSerializer
>  
> -from patchwork.api.base import AuthenticatedReadOnly
> -from patchwork.api.base import PatchworkViewSet
> -
>  
>  class UserSerializer(HyperlinkedModelSerializer):
> +
>      class Meta:
>          model = User
>          fields = ('url', 'username', 'first_name', 'last_name', 'email')
> +        extra_kwargs = {
> +            'url': {'view_name': 'api-user-detail'},
> +        }
> +
>  
> +class UserMixin(object):
>  
> -class UserViewSet(PatchworkViewSet):
> -    permission_classes = (AuthenticatedReadOnly,)
> +    queryset = User.objects.all()
> +    permission_classes = (IsAuthenticated,)
>      serializer_class = UserSerializer
> +
> +
> +class UserList(UserMixin, ListAPIView):
> +    """List users."""
> +
> +    pass
> +
> +
> +class UserDetail(UserMixin, RetrieveAPIView):
> +    """Show a user."""
> +
> +    pass
> diff --git a/patchwork/settings/base.py b/patchwork/settings/base.py
> index a32710f..b7b10c3 100644
> --- a/patchwork/settings/base.py
> +++ b/patchwork/settings/base.py
> @@ -140,7 +140,9 @@ except ImportError:
>  
>  
>  REST_FRAMEWORK = {
> -    'DEFAULT_VERSIONING_CLASS': 'rest_framework.versioning.NamespaceVersioning'
> +    'DEFAULT_VERSIONING_CLASS':
> +        'rest_framework.versioning.NamespaceVersioning',
> +    'DEFAULT_PAGINATION_CLASS': 'patchwork.api.base.LinkHeaderPagination',
>  }
>  
>  #
> diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py
> index ddce4d7..469fd26 100644
> --- a/patchwork/tests/test_rest_api.py
> +++ b/patchwork/tests/test_rest_api.py
> @@ -48,8 +48,8 @@ class TestProjectAPI(APITestCase):
>      @staticmethod
>      def api_url(item=None):
>          if item is None:
> -            return reverse('api_1.0:project-list')
> -        return reverse('api_1.0:project-detail', args=[item])
> +            return reverse('api-project-list')
> +        return reverse('api-project-detail', args=[item])
>  
>      def test_list_simple(self):
>          """Validate we can list the default test project."""
> @@ -89,7 +89,7 @@ class TestProjectAPI(APITestCase):
>          resp = self.client.post(
>              self.api_url(),
>              {'linkname': 'l', 'name': 'n', 'listid': 'l', 'listemail': 'e'})
> -        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
>  
>      def test_anonymous_update(self):
>          """Ensure anonymous "PATCH" operations are rejected."""
> @@ -104,7 +104,7 @@ class TestProjectAPI(APITestCase):
>          project = create_project()
>  
>          resp = self.client.delete(self.api_url(project.id))
> -        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
>  
>      def test_create(self):
>          """Ensure creations are rejected."""
> @@ -117,7 +117,7 @@ class TestProjectAPI(APITestCase):
>          resp = self.client.post(
>              self.api_url(),
>              {'linkname': 'l', 'name': 'n', 'listid': 'l', 'listemail': 'e'})
> -        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
>  
>      def test_update(self):
>          """Ensure updates can be performed maintainers."""
> @@ -147,7 +147,7 @@ class TestProjectAPI(APITestCase):
>          user.save()
>          self.client.force_authenticate(user=user)
>          resp = self.client.delete(self.api_url(project.id))
> -        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
>          self.assertEqual(1, Project.objects.all().count())
>  
>  
> @@ -157,8 +157,8 @@ class TestPersonAPI(APITestCase):
>      @staticmethod
>      def api_url(item=None):
>          if item is None:
> -            return reverse('api_1.0:person-list')
> -        return reverse('api_1.0:person-detail', args=[item])
> +            return reverse('api-person-list')
> +        return reverse('api-person-detail', args=[item])
>  
>      def test_anonymous_list(self):
>          """The API should reject anonymous users."""
> @@ -196,14 +196,14 @@ class TestPersonAPI(APITestCase):
>          self.client.force_authenticate(user=user)
>  
>          resp = self.client.delete(self.api_url(user.id))
> -        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
>  
>          resp = self.client.patch(self.api_url(user.id),
>                                   {'email': 'foo at f.com'})
> -        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
>  
>          resp = self.client.post(self.api_url(), {'email': 'foo at f.com'})
> -        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
>  
>  
>  @unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> @@ -212,8 +212,8 @@ class TestUserAPI(APITestCase):
>      @staticmethod
>      def api_url(item=None):
>          if item is None:
> -            return reverse('api_1.0:user-list')
> -        return reverse('api_1.0:user-detail', args=[item])
> +            return reverse('api-user-list')
> +        return reverse('api-user-detail', args=[item])
>  
>      def test_anonymous_list(self):
>          """The API should reject anonymous users."""
> @@ -239,13 +239,13 @@ class TestUserAPI(APITestCase):
>          self.client.force_authenticate(user=user)
>  
>          resp = self.client.delete(self.api_url(1))
> -        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
>  
>          resp = self.client.patch(self.api_url(1), {'email': 'foo at f.com'})
> -        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
>  
>          resp = self.client.post(self.api_url(), {'email': 'foo at f.com'})
> -        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
>  
>  
>  @unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> @@ -255,8 +255,8 @@ class TestPatchAPI(APITestCase):
>      @staticmethod
>      def api_url(item=None):
>          if item is None:
> -            return reverse('api_1.0:patch-list')
> -        return reverse('api_1.0:patch-detail', args=[item])
> +            return reverse('api-patch-list')
> +        return reverse('api-patch-detail', args=[item])
>  
>      def test_list_simple(self):
>          """Validate we can list a patch."""
> @@ -265,6 +265,8 @@ class TestPatchAPI(APITestCase):
>          self.assertEqual(0, len(resp.data))
>  
>          patch_obj = create_patch()
> +
> +        # anonymous user
>          resp = self.client.get(self.api_url())
>          self.assertEqual(status.HTTP_200_OK, resp.status_code)
>          self.assertEqual(1, len(resp.data))
> @@ -274,7 +276,7 @@ class TestPatchAPI(APITestCase):
>          self.assertNotIn('headers', patch_rsp)
>          self.assertNotIn('diff', patch_rsp)
>  
> -        # test while authenticated
> +        # authenticated user
>          user = create_user()
>          self.client.force_authenticate(user=user)
>          resp = self.client.get(self.api_url())
> @@ -284,7 +286,7 @@ class TestPatchAPI(APITestCase):
>          self.assertEqual(patch_obj.name, patch_rsp['name'])
>  
>      def test_detail(self):
> -        """Validate we can get a specific project."""
> +        """Validate we can get a specific patch."""
>          patch = create_patch()
>  
>          resp = self.client.get(self.api_url(patch.id))
> @@ -318,7 +320,7 @@ class TestPatchAPI(APITestCase):
>          }
>  
>          resp = self.client.post(self.api_url(), patch)
> -        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
>  
>      def test_anonymous_update(self):
>          """Ensure anonymous "PATCH" operations are rejected."""
> @@ -339,7 +341,7 @@ class TestPatchAPI(APITestCase):
>          patch_url = self.api_url(patch.id)
>  
>          resp = self.client.delete(patch_url)
> -        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
>  
>      def test_create(self):
>          """Ensure creations are rejected."""
> @@ -359,7 +361,7 @@ class TestPatchAPI(APITestCase):
>          }
>  
>          resp = self.client.post(self.api_url(), patch)
> -        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
>  
>      def test_update(self):
>          """Ensure updates can be performed by maintainers."""
> @@ -391,7 +393,7 @@ class TestPatchAPI(APITestCase):
>          self.client.force_authenticate(user=user)
>  
>          resp = self.client.delete(self.api_url(patch.id))
> -        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
>          self.assertEqual(1, Patch.objects.all().count())
>  
>  
> @@ -399,13 +401,17 @@ class TestPatchAPI(APITestCase):
>  class TestCheckAPI(APITestCase):
>      fixtures = ['default_tags']
>  
> +    def api_url(self, item=None):
> +        if item is None:
> +            return reverse('api-check-list', args=[self.patch.id])
> +        return reverse('api-check-detail', kwargs={
> +            'patch_id': self.patch.id, 'check_id': item.id})
> +
>      def setUp(self):
>          super(TestCheckAPI, self).setUp()
>          project = create_project()
>          self.user = create_maintainer(project)
>          self.patch = create_patch(project=project)
> -        self.urlbase = reverse('api_1.0:patch-detail', args=[self.patch.id])
> -        self.urlbase += 'checks/'
>  
>      def _create_check(self):
>          values = {
> @@ -416,13 +422,13 @@ class TestCheckAPI(APITestCase):
>  
>      def test_list_simple(self):
>          """Validate we can list checks on a patch."""
> -        resp = self.client.get(self.urlbase)
> +        resp = self.client.get(self.api_url())
>          self.assertEqual(status.HTTP_200_OK, resp.status_code)
>          self.assertEqual(0, len(resp.data))
>  
>          check_obj = self._create_check()
>  
> -        resp = self.client.get(self.urlbase)
> +        resp = self.client.get(self.api_url())
>          self.assertEqual(status.HTTP_200_OK, resp.status_code)
>          self.assertEqual(1, len(resp.data))
>          check_rsp = resp.data[0]
> @@ -434,7 +440,7 @@ class TestCheckAPI(APITestCase):
>      def test_detail(self):
>          """Validate we can get a specific check."""
>          check = self._create_check()
> -        resp = self.client.get(self.urlbase + str(check.id) + '/')
> +        resp = self.client.get(self.api_url(check))
>          self.assertEqual(status.HTTP_200_OK, resp.status_code)
>          self.assertEqual(check.target_url, resp.data['target_url'])
>  
> @@ -446,13 +452,13 @@ class TestCheckAPI(APITestCase):
>          self.client.force_authenticate(user=self.user)
>  
>          # update
> -        resp = self.client.patch(
> -            self.urlbase + str(check.id) + '/', {'target_url': 'fail'})
> -        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        resp = self.client.patch(self.api_url(check),
> +                                 {'target_url': 'fail'})
> +        self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
>  
>          # delete
> -        resp = self.client.delete(self.urlbase + str(check.id) + '/')
> -        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +        resp = self.client.delete(self.api_url(check))
> +        self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
>  
>      def test_create(self):
>          """Ensure creations can be performed by user of patch."""
> @@ -464,13 +470,13 @@ class TestCheckAPI(APITestCase):
>          }
>  
>          self.client.force_authenticate(user=self.user)
> -        resp = self.client.post(self.urlbase, check)
> +        resp = self.client.post(self.api_url(), check)
>          self.assertEqual(status.HTTP_201_CREATED, resp.status_code)
>          self.assertEqual(1, Check.objects.all().count())
>  
>          user = create_user()
>          self.client.force_authenticate(user=user)
> -        resp = self.client.post(self.urlbase, check)
> +        resp = self.client.post(self.api_url(), check)
>          self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
>  
>      def test_create_invalid(self):
> @@ -483,6 +489,6 @@ class TestCheckAPI(APITestCase):
>          }
>  
>          self.client.force_authenticate(user=self.user)
> -        resp = self.client.post(self.urlbase, check)
> +        resp = self.client.post(self.api_url(), check)
>          self.assertEqual(status.HTTP_400_BAD_REQUEST, resp.status_code)
>          self.assertEqual(0, Check.objects.all().count())
> diff --git a/patchwork/urls.py b/patchwork/urls.py
> index 6671837..56db24c 100644
> --- a/patchwork/urls.py
> +++ b/patchwork/urls.py
> @@ -152,29 +152,54 @@ if settings.ENABLE_REST_API:
>          raise RuntimeError(
>              'djangorestframework must be installed to enable the REST API.')
>  
> -    from rest_framework.routers import DefaultRouter
> -    from rest_framework_nested.routers import NestedSimpleRouter
> -
> -    from patchwork.api.check import CheckViewSet
> -    from patchwork.api.patch import PatchViewSet
> -    from patchwork.api.person import PeopleViewSet
> -    from patchwork.api.project import ProjectViewSet
> -    from patchwork.api.user import UserViewSet
> -
> -    router = DefaultRouter()
> -    router.register('patches', PatchViewSet, 'patch')
> -    router.register('people', PeopleViewSet, 'person')
> -    router.register('projects', ProjectViewSet, 'project')
> -    router.register('users', UserViewSet, 'user')
> -
> -    patches_router = NestedSimpleRouter(router, r'patches', lookup='patch')
> -    patches_router.register(r'checks', CheckViewSet, base_name='patch-checks')
> +    from patchwork.api import check as api_check_views
> +    from patchwork.api import index as api_index_views
> +    from patchwork.api import patch as api_patch_views
> +    from patchwork.api import person as api_person_views
> +    from patchwork.api import project as api_project_views
> +    from patchwork.api import user as api_user_views
> +
> +    api_patterns = [
> +        url(r'^$',
> +            api_index_views.IndexView.as_view(),
> +            name='api-index'),
> +        url(r'^users/$',
> +            api_user_views.UserList.as_view(),
> +            name='api-user-list'),
> +        url(r'^users/(?P<pk>[^/]+)/$',
> +            api_user_views.UserDetail.as_view(),
> +            name='api-user-detail'),
> +        url(r'^people/$',
> +            api_person_views.PersonList.as_view(),
> +            name='api-person-list'),
> +        url(r'^people/(?P<pk>[^/]+)/$',
> +            api_person_views.PersonDetail.as_view(),
> +            name='api-person-detail'),
> +        url(r'^patches/$',
> +            api_patch_views.PatchList.as_view(),
> +            name='api-patch-list'),
> +        url(r'^patches/(?P<pk>[^/]+)/$',
> +            api_patch_views.PatchDetail.as_view(),
> +            name='api-patch-detail'),
> +        url(r'^patches/(?P<patch_id>[^/]+)/checks/$',
> +            api_check_views.CheckListCreate.as_view(),
> +            name='api-check-list'),
> +        url(r'^patches/(?P<patch_id>[^/]+)/checks/(?P<check_id>[^/]+)/$',
> +            api_check_views.CheckDetail.as_view(),
> +            name='api-check-detail'),
> +        url(r'^projects/$',
> +            api_project_views.ProjectList.as_view(),
> +            name='api-project-list'),
> +        url(r'^projects/(?P<pk>[^/]+)/$',
> +            api_project_views.ProjectDetail.as_view(),
> +            name='api-project-detail'),
> +    ]
>  
>      urlpatterns += [
> -        url(r'^api/1.0/', include(router.urls, namespace='api_1.0')),
> -        url(r'^api/1.0/', include(patches_router.urls, namespace='api_1.0')),
> +        url(r'^api/1.0/', include(api_patterns)),
>      ]
>  
> +
>  # redirect from old urls
>  if settings.COMPAT_REDIR:
>      urlpatterns += [
> diff --git a/requirements-dev.txt b/requirements-dev.txt
> index fdfe00c..0f8f08b 100644
> --- a/requirements-dev.txt
> +++ b/requirements-dev.txt
> @@ -1,4 +1,3 @@
>  Django>=1.8,<1.11
>  djangorestframework>=3.5,<3.6
> -drf-nested-routers>=0.11.1,<0.12
>  -r requirements-test.txt
> diff --git a/requirements-prod.txt b/requirements-prod.txt
> index 57e6fce..64d4339 100644
> --- a/requirements-prod.txt
> +++ b/requirements-prod.txt
> @@ -1,5 +1,4 @@
>  Django>=1.8,<1.11
>  djangorestframework>=3.5,<3.6
> -drf-nested-routers>=0.11.1,<0.12
>  psycopg2>2.6,<2.7
>  sqlparse
> diff --git a/tox.ini b/tox.ini
> index d68588a..c5a9349 100644
> --- a/tox.ini
> +++ b/tox.ini
> @@ -14,7 +14,6 @@ deps =
>      django19: django>=1.9,<1.10
>      django110: django>=1.10,<1.11
>      django{18,19,110}: djangorestframework>=3.5,<3.6
> -    drf-nested-routers>=0.11.1,<0.12
>  setenv =
>      DJANGO_SETTINGS_MODULE = patchwork.settings.dev
>      PYTHONDONTWRITEBYTECODE = 1
> -- 
> 2.7.4
>
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork


More information about the Patchwork mailing list