[RFC 07/11] REST: Add Patch Checks to the API
Finucane, Stephen
stephen.finucane at intel.com
Mon May 9 23:58:13 AEST 2016
On 15 Apr 13:24, Andy Doan wrote:
> This exports patch checks via the REST API.
>
> The drf-nested-routers package is used to handle the fact Checks are
> nested under a Patch.
>
> Security Constraints:
> * Anyone (logged in or not) can read all objects.
> * No one can update/delete objects.
> * Project maintainers and patch owners may create objects.
>
> Signed-off-by: Andy Doan <andy.doan at linaro.org>
Some work needed here, mostly owing to the additional requirement.
Stephen
> ---
> patchwork/tests/test_rest_api.py | 82 +++++++++++++++++++++++++++++++++++++++-
> patchwork/views/rest_api.py | 67 ++++++++++++++++++++++++++++++--
> requirements-test.txt | 1 +
> 3 files changed, 146 insertions(+), 4 deletions(-)
>
> diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py
> index c2a092b..f98ad75 100644
> --- a/patchwork/tests/test_rest_api.py
> +++ b/patchwork/tests/test_rest_api.py
> @@ -24,7 +24,7 @@ from django.conf import settings
> from rest_framework import status
> from rest_framework.test import APITestCase
>
> -from patchwork.models import Patch, Project
> +from patchwork.models import Check, Patch, Project
> from patchwork.tests.utils import (
> defaults, create_maintainer, create_user, create_patches, make_msgid)
>
> @@ -236,3 +236,83 @@ class TestPatchAPI(APITestCase):
> resp = self.client.delete('/api/1.0/patches/%d/' % patches[0].id)
> self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> self.assertEqual(1, Patch.objects.all().count())
> +
> +
> + at unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> +class TestCheckAPI(APITestCase):
> + fixtures = ['default_states']
> +
> + def setUp(self):
> + super(TestCheckAPI, self).setUp()
> + self.patch = create_patches()[0]
> + self.urlbase = '/api/1.0/patches/%d/checks/' % self.patch.id
> + defaults.project.save()
> + self.user = create_maintainer(defaults.project)
> +
> + def create_check(self):
> + return Check.objects.create(patch=self.patch, user=self.user,
> + state=Check.STATE_WARNING, target_url='t',
> + description='d', context='c')
> +
> + def test_list_simple(self):
> + """Validate we can list checks on a patch."""
> + resp = self.client.get(self.urlbase)
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertEqual(0, resp.data['count'])
> +
> + c = self.create_check()
> + resp = self.client.get(self.urlbase)
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertEqual(1, resp.data['count'])
> + check = resp.data['results'][0]
> + self.assertEqual(c.state, check['state'])
> + self.assertEqual(c.target_url, check['target_url'])
> + self.assertEqual(c.context, check['context'])
> + self.assertEqual(c.description, check['description'])
> +
> + def test_get(self):
> + """Validate we can get a specific check."""
> + c = self.create_check()
> + resp = self.client.get(self.urlbase + str(c.id) + '/')
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertEqual(c.target_url, resp.data['target_url'])
> +
> + # and we can get the combined check status
> + resp = self.client.get('/api/1.0/patches/%d/check/' % self.patch.id)
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertEqual(c.state, resp.data['state'])
> +
> + def test_update_delete(self):
> + """Ensure updates and deletes aren't allowed"""
> + c = self.create_check()
> +
> + self.user.is_superuser = True
> + self.user.save()
> + self.client.force_authenticate(user=self.user)
> +
> + # update
> + resp = self.client.patch(
> + self.urlbase + str(c.id) + '/', {'target_url': 'fail'})
> + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> + # delete
> + resp = self.client.delete(self.urlbase + str(c.id) + '/')
> + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +
> + def test_create(self):
> + """Ensure creations can be performed by user of patch."""
> + check = {
> + 'state': Check.STATE_SUCCESS,
I think we should expose the string value here, rather than an integer
that people have to go an convert. I'm pretty sure this is what happens
with the XML-RPC API, and if not the predecessor to this patch [1]
could be some help (I'll resend shortly).
[1] https://patchwork.ozlabs.org/patch/602073/
> + 'target_url': 'http://t.co',
> + 'description': 'description',
> + 'context': 'context',
> + }
> +
> + self.client.force_authenticate(user=self.user)
> + resp = self.client.post(self.urlbase, 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)
> + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> diff --git a/patchwork/views/rest_api.py b/patchwork/views/rest_api.py
> index c3b5cec..3918a7a 100644
> --- a/patchwork/views/rest_api.py
> +++ b/patchwork/views/rest_api.py
> @@ -19,13 +19,18 @@
>
> from django.conf.urls import url, include
>
> -from patchwork.models import Patch, Person, Project
> +from patchwork.models import Check, Patch, Person, Project
>
> from rest_framework import permissions
> +from rest_framework.exceptions import PermissionDenied
> from rest_framework.pagination import PageNumberPagination
> +from rest_framework.response import Response
> from rest_framework.routers import DefaultRouter
> -from rest_framework.serializers import ModelSerializer
> -from rest_framework.viewsets import ModelViewSet
> +from rest_framework.serializers import (
> + CurrentUserDefault, HiddenField, ModelSerializer, PrimaryKeyRelatedField)
> +from rest_framework.viewsets import GenericViewSet, ModelViewSet
> +
> +from rest_framework_nested.routers import NestedSimpleRouter
How widely available is this dependency? Is it available in the Ubuntu
LTS repos? Also, this is another dependency that you need to check for
as part of 'settings.py' (where you raise the RuntimeException if
missing).
>
>
> class PageSizePagination(PageNumberPagination):
> @@ -86,11 +91,67 @@ class ProjectViewSet(PatchworkViewSet):
> serializer_class = create_model_serializer(Project)
>
>
> +class CurrentPatchDefault(object):
> + def set_context(self, serializer_field):
> + self.patch = serializer_field.context['request'].patch
> +
> + def __call__(self):
> + return self.patch
> +
> +
> +class ChecksSerializer(ModelSerializer):
> + class Meta:
> + model = Check
> + user = PrimaryKeyRelatedField(read_only=True, default=CurrentUserDefault())
> + patch = HiddenField(default=CurrentPatchDefault())
> +
> +
> +class ChecksViewSet(PatchworkViewSet):
> + serializer_class = ChecksSerializer
> +
> + def not_allowed(self, request, **kwargs):
> + raise PermissionDenied()
> +
> + update = not_allowed
> + partial_update = not_allowed
> + destroy = not_allowed
Ooh - haven't seen this done in a while. Reasons why I <3 Python.
> + def create(self, request, patch_pk):
> + p = Patch.objects.get(id=patch_pk)
> + if not p.is_editable(request.user):
> + raise PermissionDenied()
> + request.patch = p
> + return super(ChecksViewSet, 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)
> +
> + serializer = self.get_serializer(queryset, many=True)
> + return Response(serializer.data)
> +
> +
> +class CheckViewSet(GenericViewSet):
> + def list(self, request, patch_pk):
> + state = Patch.objects.get(id=patch_pk).combined_check_state
> + return Response({'state': state})
> +
> +
> router = DefaultRouter()
> router.register('patches', PatchViewSet, 'patch')
> router.register('people', PeopleViewSet, 'person')
> router.register('projects', ProjectViewSet, 'project')
>
> +patches_router = NestedSimpleRouter(router, r'patches', lookup='patch')
> +patches_router.register(r'checks', ChecksViewSet, base_name='patch-checks')
> +patches_router.register(r'check', CheckViewSet, base_name='patch-check')
I think that 'check' would be more valuable as part of the 'patch'
endpoint, rather than as a separate endpoint. The whole idea of 'check'
is to provide a quick overview of the status, before delving into more
detailed results via 'checks'.
> +
> urlpatterns = [
> url(r'^api/1.0/', include(router.urls)),
> + url(r'^api/1.0/', include(patches_router.urls)),
> ]
> diff --git a/requirements-test.txt b/requirements-test.txt
> index fb6a8c8..0b5b69e 100644
> --- a/requirements-test.txt
> +++ b/requirements-test.txt
> @@ -3,3 +3,4 @@ django-debug-toolbar==1.4
> python-dateutil>2.0,<3.0
> selenium>2.0,<3.0
> djangorestframework==3.3.3
> +drf-nested-routers==0.11.1
Again - can we avoid being so specific in our pinning requirements?
> --
> 2.7.4
>
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
More information about the Patchwork
mailing list