[RFC 06/11] REST: Add Patches to the API
Finucane, Stephen
stephen.finucane at intel.com
Mon May 9 23:45:26 AEST 2016
On 15 Apr 13:24, Andy Doan wrote:
> This exports patches via the REST API.
>
> Security Constraints:
> * Anyone (logged in or not) can read all objects.
> * No one can create/delete objects.
> * Project maintainers are allowed to update (ie "patch"
> attributes)
Love these summaries :)
> NOTE: Patch.save was overridden incorrectly and had to be
> fixed to work with DRF.
Good catch. No serious concerns here, that I can tell.
Stephen
> Signed-off-by: Andy Doan <andy.doan at linaro.org>
> ---
> patchwork/models.py | 4 +-
> patchwork/tests/test_rest_api.py | 98 +++++++++++++++++++++++++++++++++++++++-
> patchwork/views/rest_api.py | 13 +++++-
> 3 files changed, 109 insertions(+), 6 deletions(-)
>
> diff --git a/patchwork/models.py b/patchwork/models.py
> index 4d454c7..6324273 100644
> --- a/patchwork/models.py
> +++ b/patchwork/models.py
> @@ -359,14 +359,14 @@ class Patch(Submission):
> for tag in tags:
> self._set_tag(tag, counter[tag])
>
> - def save(self):
> + def save(self, **kwargs):
> if not hasattr(self, 'state') or not self.state:
> self.state = get_default_initial_patch_state()
>
> if self.hash is None and self.diff is not None:
> self.hash = hash_patch(self.diff).hexdigest()
>
> - super(Patch, self).save()
> + super(Patch, self).save(**kwargs)
>
> self.refresh_tag_counts()
>
> diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py
> index 76f0c12..c2a092b 100644
> --- a/patchwork/tests/test_rest_api.py
> +++ b/patchwork/tests/test_rest_api.py
> @@ -24,8 +24,9 @@ from django.conf import settings
> from rest_framework import status
> from rest_framework.test import APITestCase
>
> -from patchwork.models import Project
> -from patchwork.tests.utils import defaults, create_maintainer, create_user
> +from patchwork.models import Patch, Project
> +from patchwork.tests.utils import (
> + defaults, create_maintainer, create_user, create_patches, make_msgid)
>
>
> @unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> @@ -142,3 +143,96 @@ class TestPersonAPI(APITestCase):
>
> resp = self.client.post('/api/1.0/people/', {'email': 'foo at f.com'})
> self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +
> +
> + at unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> +class TestPatchAPI(APITestCase):
> + fixtures = ['default_states']
> +
> + def test_list_simple(self):
> + """Validate we can list a patch."""
> + patches = create_patches()
> + resp = self.client.get('/api/1.0/patches/')
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertEqual(1, resp.data['count'])
> + patch = resp.data['results'][0]
> + self.assertEqual(patches[0].name, patch['name'])
Might be worth testing an authenticated user also - corner cases can
be tricky.
> + def test_get(self):
> + """Validate we can get a specific project."""
> + patches = create_patches()
> + resp = self.client.get('/api/1.0/patches/%d/' % patches[0].id)
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertEqual(patches[0].name, resp.data['name'])
> + self.assertEqual(patches[0].project.id, resp.data['project'])
> + self.assertEqual(patches[0].msgid, resp.data['msgid'])
> + self.assertEqual(patches[0].diff, resp.data['diff'])
> + self.assertEqual(patches[0].submitter.id, resp.data['submitter'])
> + self.assertEqual(patches[0].state.id, resp.data['state'])
> +
> + def test_anonymous_writes(self):
> + """Ensure anonymous "write" operations are rejected."""
> + patches = create_patches()
> + patch_url = '/api/1.0/patches/%d/' % patches[0].id
As mentioned previously, 'reverse' seems like a natural fit here...
> + resp = self.client.get(patch_url)
> + patch = resp.data
> + patch['msgid'] = 'foo'
> + patch['name'] = 'this will should fail'
"will should"?
> +
> + # create
> + resp = self.client.post('/api/1.0/patches/', patch)
> + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> + # update
> + resp = self.client.patch(patch_url, {'name': 'foo'})
> + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> + # delete
> + resp = self.client.delete(patch_url)
> + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +
> + def test_create(self):
> + """Ensure creations are rejected."""
> + create_patches()
> + patch = {
> + 'project': defaults.project.id,
> + 'submitter': defaults.patch_author_person.id,
> + 'msgid': make_msgid(),
> + 'name': 'test-create-patch',
> + 'diff': 'patch diff',
> + }
> +
> + user = create_maintainer(defaults.project)
> + user.is_superuser = True
> + user.save()
> + self.client.force_authenticate(user=user)
> + resp = self.client.post('/api/1.0/patches/', patch)
> + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +
> + def test_update(self):
> + """Ensure updates can be performed maintainers."""
> + patches = create_patches()
> +
> + # A maintainer can update
> + user = create_maintainer(defaults.project)
> + self.client.force_authenticate(user=user)
> + resp = self.client.patch(
> + '/api/1.0/patches/%d/' % patches[0].id, {'state': 2})
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> +
> + # A normal user can't
> + user = create_user()
> + self.client.force_authenticate(user=user)
> + resp = self.client.patch(
> + '/api/1.0/patches/%d/' % patches[0].id, {'state': 2})
> + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +
> + def test_delete(self):
> + """Ensure deletions are rejected."""
> + patches = create_patches()
> +
> + user = create_maintainer(defaults.project)
> + user.is_superuser = True
> + user.save()
> + self.client.force_authenticate(user=user)
> + 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())
> diff --git a/patchwork/views/rest_api.py b/patchwork/views/rest_api.py
> index ab576cc..c3b5cec 100644
> --- a/patchwork/views/rest_api.py
> +++ b/patchwork/views/rest_api.py
> @@ -19,7 +19,7 @@
>
> from django.conf.urls import url, include
>
> -from patchwork.models import Person, Project
> +from patchwork.models import Patch, Person, Project
>
> from rest_framework import permissions
> from rest_framework.pagination import PageNumberPagination
> @@ -61,13 +61,21 @@ class PatchworkViewSet(ModelViewSet):
> return self.serializer_class.Meta.model.objects.all()
>
>
> -def create_model_serializer(model_class):
> +def create_model_serializer(model_class, read_only=None):
> class PatchworkSerializer(ModelSerializer):
> class Meta:
> model = model_class
> + read_only_fields = read_only
Ah, here we go. I suspect excluded fields will come up soon. In any
case, surely DRF has this kind of factory functionality built in?
> return PatchworkSerializer
>
>
> +class PatchViewSet(PatchworkViewSet):
> + permission_classes = (PatchworkPermission,)
> + serializer_class = create_model_serializer(
> + Patch, ('project', 'name', 'date', 'submitter', 'diff', 'content',
> + 'hash', 'msgid'))
> +
> +
> class PeopleViewSet(PatchworkViewSet):
> permission_classes = (AuthenticatedReadOnly,)
> serializer_class = create_model_serializer(Person)
> @@ -79,6 +87,7 @@ class ProjectViewSet(PatchworkViewSet):
>
>
> router = DefaultRouter()
> +router.register('patches', PatchViewSet, 'patch')
> router.register('people', PeopleViewSet, 'person')
> router.register('projects', ProjectViewSet, 'project')
>
> --
> 2.7.4
>
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
More information about the Patchwork
mailing list