[v2,06/13] REST: Stop using ViewSets

These are hacked a lot of to get them to work as we want. Use a more
verbose, but ultimately easier to parse, version. This lets us remove
the dependency of drf-nested-routers, bringing us back down to two main
dependencies. It also lets us remove the AuthenticedReadOnly permission
class, as the generic views enforce this for us.

The main user facing change is the use of 405 (Method Not Allowed)
errors when trying to use an invalid method, such as POST on the
project endpoint.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Cc: Andy Doan <andy.doan@linaro.org>
---
 patchwork/api/__init__.py        | 27 +++++-------
 patchwork/api/check.py           | 76 +++++++++++++++++++--------------
 patchwork/api/index.py           | 36 ++++++++++++++++
 patchwork/api/patch.py           | 90 +++++++++++++++++++++++-----------------
 patchwork/api/person.py          | 30 ++++++++++----
 patchwork/api/project.py         | 53 ++++++++++++++---------
 patchwork/api/user.py            | 28 ++++++++++---
 patchwork/settings/base.py       |  5 ++-
 patchwork/tests/test_rest_api.py | 80 ++++++++++++++++++-----------------
 patchwork/urls.py                | 63 +++++++++++++++++++---------
 10 files changed, 315 insertions(+), 173 deletions(-)
 create mode 100644 patchwork/api/index.py

Hi Stephen,

> These are hacked a lot of to get them to work as we want. Use a more
> verbose, but ultimately easier to parse, version. This lets us remove
> the dependency of drf-nested-routers, bringing us back down to two main
> dependencies. It also lets us remove the AuthenticedReadOnly permission
> class, as the generic views enforce this for us.
>
> The main user facing change is the use of 405 (Method Not Allowed)
> errors when trying to use an invalid method, such as POST on the
> project endpoint.

I'm not 100% clear the meaning of this commit message. What was the
behaviour before and what is the behaviour now? I think you're saying a
user will now see a 405?

Beyond that, I'm really not qualified to comment, and given that we're
spinning a new API version, if Andrew and Russell are happy with it, I'm
happy!

If you really want a reviewed-by I can come to grips with DRF but it
will take a while.

Regards,
Daniel

>
> Signed-off-by: Stephen Finucane <stephen@that.guru>
> Cc: Andy Doan <andy.doan@linaro.org>
> ---
>  patchwork/api/__init__.py        | 27 +++++-------
>  patchwork/api/check.py           | 76 +++++++++++++++++++--------------
>  patchwork/api/index.py           | 36 ++++++++++++++++
>  patchwork/api/patch.py           | 90 +++++++++++++++++++++++-----------------
>  patchwork/api/person.py          | 30 ++++++++++----
>  patchwork/api/project.py         | 53 ++++++++++++++---------
>  patchwork/api/user.py            | 28 ++++++++++---
>  patchwork/settings/base.py       |  5 ++-
>  patchwork/tests/test_rest_api.py | 80 ++++++++++++++++++-----------------
>  patchwork/urls.py                | 63 +++++++++++++++++++---------
>  10 files changed, 315 insertions(+), 173 deletions(-)
>  create mode 100644 patchwork/api/index.py
>
> diff --git a/patchwork/api/__init__.py b/patchwork/api/__init__.py
> index e91a282..dbd8148 100644
> --- a/patchwork/api/__init__.py
> +++ b/patchwork/api/__init__.py
> @@ -18,11 +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):
> @@ -54,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:
> @@ -66,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 d24b0c6..1ff9992 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 import PatchworkViewSet
> +from patchwork.api 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', )
> +        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 bc4cb5c..737ada1 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 import PatchworkPermission
> -from patchwork.api 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
> @@ -55,39 +47,61 @@ class PatchSerializer(HyperlinkedModelSerializer):
>          return {x.name: getattr(instance, x.attr_name)
>                  for x in instance.project.tags}
>  
> -    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."""
> +
> +    queryset = Patch.objects.all().with_tag_counts().prefetch_related(
> +        'check_set').select_related(
> +        'state', 'submitter', 'delegate').defer(
> +        'content', 'diff', 'headers')
> +    permission_classes = (PatchworkPermission,)
> +    serializer_class = PatchListSerializer
> +
>  
> +class PatchDetail(RetrieveUpdateAPIView):
> +    """Show a patch."""
>  
> -class PatchViewSet(PatchworkViewSet):
> +    queryset = Patch.objects.all().with_tag_counts().prefetch_related(
> +        'check_set').select_related(
> +        'state', 'submitter', 'delegate')
>      permission_classes = (PatchworkPermission,)
> -    serializer_class = PatchSerializer
> -
> -    def get_queryset(self):
> -        qs = super(PatchViewSet, self).get_queryset().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
> +    serializer_class = PatchDetailSerializer
> diff --git a/patchwork/api/person.py b/patchwork/api/person.py
> index 7507fe5..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 import AuthenticatedReadOnly
> -from patchwork.api 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 9e5d2aa..3319339 100644
> --- a/patchwork/api/project.py
> +++ b/patchwork/api/project.py
> @@ -17,11 +17,12 @@
>  # along with Patchwork; if not, write to the Free Software
>  # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>  
> +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 import PatchworkPermission
> -from patchwork.api import PatchworkViewSet
>  from patchwork.models import Project
>  
>  
> @@ -35,26 +36,40 @@ 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):
> +
>      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()
> -        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)
> +    def get_queryset(self):
> +        query = Project.objects.all()
> +
> +        if 'pk' in self.kwargs:
> +            try:
> +                query.get(id=int(self.kwargs['pk']))
> +            except (ValueError, Project.DoesNotExist):
> +                query.get(linkname=self.kwargs['pk'])
> +
> +            # NOTE(stephenfin): We must do this to make sure the 'url'
> +            # field is populated correctly
> +            self.kwargs['pk'] = query[0].id
> +
> +        return query
> +
> +
> +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 1da90ac..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 import AuthenticatedReadOnly
> -from patchwork.api 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..35ac96a 100644
> --- a/patchwork/settings/base.py
> +++ b/patchwork/settings/base.py
> @@ -140,7 +140,10 @@ except ImportError:
>  
>  
>  REST_FRAMEWORK = {
> -    'DEFAULT_VERSIONING_CLASS': 'rest_framework.versioning.NamespaceVersioning'
> +    'DEFAULT_VERSIONING_CLASS': (
> +        'rest_framework.versioning.NamespaceVersioning'
> +    ),
> +    'DEFAULT_PAGINATION_CLASS': 'patchwork.api.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@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@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@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@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 7644da9..7c29319 100644
> --- a/patchwork/urls.py
> +++ b/patchwork/urls.py
> @@ -147,29 +147,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 += [
> -- 
> 2.7.4
>
> _______________________________________________
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork

On 20/11/16 03:51, Stephen Finucane wrote:
> +class ProjectMixin(object):
> +
>      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()
> -        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)
> +    def get_queryset(self):
> +        query = Project.objects.all()
> +
> +        if 'pk' in self.kwargs:
> +            try:
> +                query.get(id=int(self.kwargs['pk']))
> +            except (ValueError, Project.DoesNotExist):
> +                query.get(linkname=self.kwargs['pk'])
> +

This should be:

+        if 'pk' in self.kwargs:
+            try:
+                query = query.filter(id=int(self.kwargs['pk']))
+            except (ValueError, Project.DoesNotExist):
+                query = query.filter(linkname=self.kwargs['pk'])

On Thu, 2016-11-24 at 14:03 +1100, Andrew Donnellan wrote:
> On 20/11/16 03:51, Stephen Finucane wrote:
> > 
> > +class ProjectMixin(object):
> > +
> >      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()
> > -        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)
> > +    def get_queryset(self):
> > +        query = Project.objects.all()
> > +
> > +        if 'pk' in self.kwargs:
> > +            try:
> > +                query.get(id=int(self.kwargs['pk']))
> > +            except (ValueError, Project.DoesNotExist):
> > +                query.get(linkname=self.kwargs['pk'])
> > +
> 
> This should be:
> 
> +        if 'pk' in self.kwargs:
> +            try:
> +                query = query.filter(id=int(self.kwargs['pk']))
> +            except (ValueError, Project.DoesNotExist):
> +                query = query.filter(linkname=self.kwargs['pk'])
> 

Good spot. I was actually overriding the wrong function altogether - it
should have been 'get_object' and not 'get_queryset'. Fixed in v3.

Stephen

On Mon, 2016-11-21 at 14:27 +1100, Daniel Axtens wrote:
> Hi Stephen,
> 
> > 
> > These are hacked a lot of to get them to work as we want. Use a
> > more
> > verbose, but ultimately easier to parse, version. This lets us
> > remove
> > the dependency of drf-nested-routers, bringing us back down to two
> > main
> > dependencies. It also lets us remove the AuthenticedReadOnly
> > permission
> > class, as the generic views enforce this for us.
> > 
> > The main user facing change is the use of 405 (Method Not Allowed)
> > errors when trying to use an invalid method, such as POST on the
> > project endpoint.
> 
> I'm not 100% clear the meaning of this commit message. What was the
> behaviour before and what is the behaviour now? I think you're saying
> a
> user will now see a 405?

I've clarified this in the next revision.

> Beyond that, I'm really not qualified to comment, and given that
> we're
> spinning a new API version, if Andrew and Russell are happy with it,
> I'm
> happy!
> 
> If you really want a reviewed-by I can come to grips with DRF but it
> will take a while.

To be honest, a quick perusal of the web interface for the API and
running of the tests should show that things are as expected. Outside
of that, let's wait for the other folks as you suggest.

Stephen