[PATCH 2/2] REST: Allow creating, updating, deleting of bundles
Stephen Finucane
stephen at that.guru
Wed Oct 2 18:25:31 AEST 2019
On Wed, 2019-10-02 at 11:50 +1000, Daniel Axtens wrote:
> Stephen Finucane <stephen at that.guru> writes:
>
> > Allow users to create a new bundle, change the name, public flag and
> > patches of an existing bundle, and delete an existing bundle.
> >
> > Some small nits with existing tests are resolved.
> >
> > Signed-off-by: Stephen Finucane <stephen at that.guru>
> > ---
> > docs/api/schemas/latest/patchwork.yaml | 170 +++++++++++++++-
> > docs/api/schemas/patchwork.j2 | 181 +++++++++++++++++-
> > docs/api/schemas/v1.0/patchwork.yaml | 5 +-
> > docs/api/schemas/v1.1/patchwork.yaml | 5 +-
> > docs/api/schemas/v1.2/patchwork.yaml | 170 +++++++++++++++-
> > patchwork/api/bundle.py | 84 +++++++-
> > patchwork/models.py | 11 ++
> > patchwork/tests/api/test_bundle.py | 118 +++++++++++-
> > patchwork/tests/api/utils.py | 16 +-
> > ...pdate-bundle-via-api-2946d8c4e730d545.yaml | 4 +
> > 10 files changed, 737 insertions(+), 27 deletions(-)
> > create mode 100644 releasenotes/notes/update-bundle-via-api-2946d8c4e730d545.yaml
> >
> > diff --git a/docs/api/schemas/latest/patchwork.yaml b/docs/api/schemas/latest/patchwork.yaml
> > index 45a61180..e6c6bb4a 100644
> > --- a/docs/api/schemas/latest/patchwork.yaml
> > +++ b/docs/api/schemas/latest/patchwork.yaml
[snip]
> > diff --git a/patchwork/api/bundle.py b/patchwork/api/bundle.py
> > index 2dec70d1..c5885aae 100644
> > --- a/patchwork/api/bundle.py
> > +++ b/patchwork/api/bundle.py
> > @@ -4,9 +4,12 @@
> > # SPDX-License-Identifier: GPL-2.0-or-later
> >
> > from django.db.models import Q
> > -from rest_framework.generics import ListAPIView
> > -from rest_framework.generics import RetrieveAPIView
> > +from rest_framework import exceptions
> > +from rest_framework.generics import ListCreateAPIView
> > +from rest_framework.generics import RetrieveUpdateDestroyAPIView
> > +from rest_framework import permissions
> > from rest_framework.serializers import SerializerMethodField
> > +from rest_framework.serializers import ValidationError
> >
> > from patchwork.api.base import BaseHyperlinkedModelSerializer
> > from patchwork.api.base import PatchworkPermission
> > @@ -14,16 +17,52 @@ from patchwork.api.filters import BundleFilterSet
> > from patchwork.api.embedded import PatchSerializer
> > from patchwork.api.embedded import ProjectSerializer
> > from patchwork.api.embedded import UserSerializer
> > +from patchwork.api import utils
> > from patchwork.models import Bundle
> >
> >
> > +class BundlePermission(permissions.BasePermission):
> > + """Ensure the API version, if configured, is >= v1.2.
> > +
> > + Bundle creation/updating was only added in API v1.2 and we don't want to
> > + change behavior in older API versions.
> > + """
> > + def has_permission(self, request, view):
> > + # read-only permission for everything
> > + if request.method in permissions.SAFE_METHODS:
> > + return True
> > +
> > + if not utils.has_version(request, '1.2'):
> > + raise exceptions.MethodNotAllowed(request.method)
> > +
> > + if request.method == 'POST' and (
> > + not request.user or not request.user.is_authenticated):
> > + return False
> > +
> > + # we have more to do but we can't do that until we have an object
> > + return True
> > +
> > + def has_object_permission(self, request, view, obj):
> > + if (request.user and
> > + request.user.is_authenticated and
> > + request.user == obj.owner):
>
> Just checking that this doesn't have the same User vs UserProfile thing
> we had with delegation? I haven't checked in great detail, I just saw a
> user comparison and thought it best to ask!
No, we're good here since Bundle.user points to the User object, not
the UserProfile object.
> > + return True
> > +
> > + if not obj.public:
> > + # if the bundle isn't public, we don't want to leak the fact that
> > + # it exists
> > + raise exceptions.NotFound
>
> A++
>
> > +
> > + return request.method in permissions.SAFE_METHODS
> > +
> > +
> > class BundleSerializer(BaseHyperlinkedModelSerializer):
> >
> > web_url = SerializerMethodField()
> > project = ProjectSerializer(read_only=True)
> > mbox = SerializerMethodField()
> > owner = UserSerializer(read_only=True)
> > - patches = PatchSerializer(many=True, read_only=True)
> > + patches = PatchSerializer(many=True)
> >
> > def get_web_url(self, instance):
> > request = self.context.get('request')
> > @@ -33,11 +72,35 @@ class BundleSerializer(BaseHyperlinkedModelSerializer):
> > request = self.context.get('request')
> > return request.build_absolute_uri(instance.get_mbox_url())
> >
> > + def create(self, validated_data):
> > + patches = validated_data.pop('patches')
> > + instance = super(BundleSerializer, self).create(validated_data)
> > + instance.overwrite_patches(patches)
> > + return instance
> > +
> > + def update(self, instance, validated_data):
> > + patches = validated_data.pop('patches')
> > + instance = super(BundleSerializer, self).update(
> > + instance, validated_data)
> > + instance.overwrite_patches(patches)
> > + return instance
> > +
> > + def validate(self, data):
> > + if not data.get('patches'):
> > + raise ValidationError('Bundles cannot be empty')
> > +
> > + if len(set([p.project.id for p in data['patches']])) > 1:
> > + raise ValidationError('Patches must belong to the same project')
> > +
> > + data['project'] = data['patches'][0].project
> > +
> > + return super(BundleSerializer, self).validate(data)
> > +
> > class Meta:
> > model = Bundle
> > fields = ('id', 'url', 'web_url', 'project', 'name', 'owner',
> > 'patches', 'public', 'mbox')
> > - read_only_fields = ('owner', 'patches', 'mbox')
> > + read_only_fields = ('project', 'owner', 'mbox')
> > versioned_fields = {
> > '1.1': ('web_url', ),
> > }
> > @@ -48,7 +111,7 @@ class BundleSerializer(BaseHyperlinkedModelSerializer):
> >
> > class BundleMixin(object):
> >
> > - permission_classes = (PatchworkPermission,)
> > + permission_classes = [PatchworkPermission & BundlePermission]
> > serializer_class = BundleSerializer
> >
> > def get_queryset(self):
> > @@ -63,16 +126,19 @@ class BundleMixin(object):
> > .select_related('owner', 'project')
> >
> >
> > -class BundleList(BundleMixin, ListAPIView):
> > - """List bundles."""
> > +class BundleList(BundleMixin, ListCreateAPIView):
> > + """List or create bundles."""
> >
> > filter_class = filterset_class = BundleFilterSet
> > search_fields = ('name',)
> > ordering_fields = ('id', 'name', 'owner')
> > ordering = 'id'
> >
> > + def perform_create(self, serializer):
> > + serializer.save(owner=self.request.user)
> > +
> Likewise and throughout the patch w/ user types.
>
> Apart from that, I think this is absolutely a Good Idea and I hope to do
> some testing of it soon.
>
> Regards,
> Daniel
>
> >
> > -class BundleDetail(BundleMixin, RetrieveAPIView):
> > - """Show a bundle."""
> > +class BundleDetail(BundleMixin, RetrieveUpdateDestroyAPIView):
> > + """Show, update or delete a bundle."""
> >
> > pass
> > diff --git a/patchwork/models.py b/patchwork/models.py
> > index 32d1b3c2..631de85d 100644
> > --- a/patchwork/models.py
> > +++ b/patchwork/models.py
> > @@ -788,6 +788,11 @@ class Bundle(models.Model):
> > patches = models.ManyToManyField(Patch, through='BundlePatch')
> > public = models.BooleanField(default=False)
> >
> > + def is_editable(self, user):
> > + if not user.is_authenticated:
> > + return False
> > + return user == self.owner
> > +
> > def ordered_patches(self):
> > return self.patches.order_by('bundlepatch__order')
> >
> > @@ -806,6 +811,12 @@ class Bundle(models.Model):
> > return BundlePatch.objects.create(bundle=self, patch=patch,
> > order=max_order + 1)
> >
> > + def overwrite_patches(self, patches):
> > + BundlePatch.objects.filter(bundle=self).delete()
> > +
> > + for patch in patches:
> > + self.append_patch(patch)
> > +
> > def get_absolute_url(self):
> > return reverse('bundle-detail', kwargs={
> > 'username': self.owner.username,
> > diff --git a/patchwork/tests/api/test_bundle.py b/patchwork/tests/api/test_bundle.py
> > index 303c500c..a3a0c113 100644
> > --- a/patchwork/tests/api/test_bundle.py
> > +++ b/patchwork/tests/api/test_bundle.py
> > @@ -8,9 +8,11 @@ import unittest
> > from django.conf import settings
> > from django.urls import reverse
> >
> > +from patchwork.models import Bundle
> > from patchwork.tests.api import utils
> > from patchwork.tests.utils import create_bundle
> > from patchwork.tests.utils import create_maintainer
> > +from patchwork.tests.utils import create_patch
> > from patchwork.tests.utils import create_project
> > from patchwork.tests.utils import create_user
> >
> > @@ -42,12 +44,15 @@ class TestBundleAPI(utils.APITestCase):
> >
> > # nested fields
> >
> > - self.assertEqual(bundle_obj.patches.count(),
> > - len(bundle_json['patches']))
> > self.assertEqual(bundle_obj.owner.id,
> > bundle_json['owner']['id'])
> > self.assertEqual(bundle_obj.project.id,
> > bundle_json['project']['id'])
> > + self.assertEqual(bundle_obj.patches.count(),
> > + len(bundle_json['patches']))
> > + for patch_obj, patch_json in zip(
> > + bundle_obj.patches.all(), bundle_json['patches']):
> > + self.assertEqual(patch_obj.id, patch_json['id'])
> >
> > def test_list_empty(self):
> > """List bundles when none are present."""
> > @@ -179,18 +184,117 @@ class TestBundleAPI(utils.APITestCase):
> > self.assertIn('url', resp.data)
> > self.assertNotIn('web_url', resp.data)
> >
> > - def test_create_update_delete(self):
> > - """Ensure creates, updates and deletes aren't allowed"""
> > + def _test_create_update(self, authenticate=True):
> > + user = create_user()
> > + project = create_project()
> > + patch_a = create_patch(project=project)
> > + patch_b = create_patch(project=project)
> > +
> > + if authenticate:
> > + self.client.force_authenticate(user=user)
> > +
> > + return user, project, patch_a, patch_b
> > +
> > + @utils.store_samples('bundle-create-error-forbidden')
> > + def test_create_anonymous(self):
> > + """Create a bundle when not signed in.
> > +
> > + Ensure creations can only be performed by signed in users.
> > + """
> > + user, project, patch_a, patch_b = self._test_create_update(
> > + authenticate=False)
> > + bundle = {
> > + 'name': 'test-bundle',
> > + 'public': True,
> > + 'patches': [patch_a.id, patch_b.id],
> > + }
> > +
> > + resp = self.client.post(self.api_url(), bundle)
> > + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> > +
> > + @utils.store_samples('bundle-create')
> > + def test_create(self):
> > + """Validate we can create a new bundle."""
> > + user, project, patch_a, patch_b = self._test_create_update()
> > + bundle = {
> > + 'name': 'test-bundle',
> > + 'public': True,
> > + 'patches': [patch_a.id, patch_b.id],
> > + }
> > +
> > + resp = self.client.post(self.api_url(), bundle)
> > + self.assertEqual(status.HTTP_201_CREATED, resp.status_code)
> > + self.assertEqual(1, Bundle.objects.all().count())
> > + self.assertSerialized(Bundle.objects.first(), resp.data)
> > +
> > + @utils.store_samples('bundle-update-not-found')
> > + def test_update_anonymous(self):
> > + """Update an existing bundle when not signed in.
> > +
> > + Ensure updates can only be performed by signed in users.
> > + """
> > + user, project, patch_a, patch_b = self._test_create_update(
> > + authenticate=False)
> > + bundle = create_bundle(owner=user, project=project)
> > +
> > + resp = self.client.patch(self.api_url(bundle.id), {
> > + 'name': 'hello-bundle', 'patches': [patch_a.id, patch_b.id]})
> > + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
> > +
> > + @utils.store_samples('bundle-update')
> > + def test_update(self):
> > + """Validate we can update an existing bundle."""
> > + user, project, patch_a, patch_b = self._test_create_update()
> > + bundle = create_bundle(owner=user, project=project)
> > +
> > + resp = self.client.patch(self.api_url(bundle.id), {
> > + 'name': 'hello-bundle', 'patches': [patch_a.id, patch_b.id]})
> > + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> > + self.assertEqual(1, Bundle.objects.all().count())
> > + self.assertEqual(len(resp.data['patches']), 2)
> > + self.assertEqual(resp.data['name'], 'hello-bundle')
> > +
> > + @utils.store_samples('bundle-delete-not-found')
> > + def test_delete_anonymous(self):
> > + """Delete a bundle when not signed in.
> > +
> > + Ensure deletions can only be performed when signed in.
> > + """
> > + user, project, patch_a, patch_b = self._test_create_update(
> > + authenticate=False)
> > + bundle = create_bundle(owner=user, project=project)
> > +
> > + resp = self.client.delete(self.api_url(bundle.id))
> > + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
> > +
> > + @utils.store_samples('bundle-delete')
> > + def test_delete(self):
> > + """Validate we can delete an existing bundle."""
> > + user = create_user()
> > + bundle = create_bundle(owner=user)
> > +
> > + self.client.force_authenticate(user=user)
> > +
> > + resp = self.client.delete(self.api_url(bundle.id))
> > + self.assertEqual(status.HTTP_204_NO_CONTENT, resp.status_code)
> > + self.assertEqual(0, Bundle.objects.all().count())
> > +
> > + def test_create_update_delete_version_1_1(self):
> > + """Ensure creates, updates and deletes aren't allowed with old API."""
> > user = create_maintainer()
> > user.is_superuser = True
> > user.save()
> > self.client.force_authenticate(user=user)
> >
> > - resp = self.client.post(self.api_url(), {'email': 'foo at f.com'})
> > + resp = self.client.post(self.api_url(version='1.1'), {'name': 'test'},
> > + validate_schema=False)
> > 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'})
> > + resp = self.client.patch(self.api_url(1, version='1.1'),
> > + {'name': 'test'},
> > + validate_schema=False)
> > self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
> >
> > - resp = self.client.delete(self.api_url(1))
> > + resp = self.client.delete(self.api_url(1, version='1.1'),
> > + validate_schema=False)
> > self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
> > diff --git a/patchwork/tests/api/utils.py b/patchwork/tests/api/utils.py
> > index 0c232d04..ce83ce2b 100644
> > --- a/patchwork/tests/api/utils.py
> > +++ b/patchwork/tests/api/utils.py
> > @@ -112,44 +112,52 @@ class APIClient(BaseAPIClient):
> > self.factory = APIRequestFactory()
> >
> > def get(self, path, data=None, follow=False, **extra):
> > + validate_schema = extra.pop('validate_schema', True)
> > request = self.factory.get(
> > path, data=data, SERVER_NAME='example.com', **extra)
> > response = super(APIClient, self).get(
> > path, data=data, follow=follow, SERVER_NAME='example.com', **extra)
> > - validator.validate_data(path, request, response)
> > + if validate_schema:
> > + validator.validate_data(path, request, response)
> > return response
> >
> > def post(self, path, data=None, format=None, content_type=None,
> > follow=False, **extra):
> > + validate_schema = extra.pop('validate_schema', True)
> > request = self.factory.post(
> > path, data=data, format='json', content_type=content_type,
> > SERVER_NAME='example.com', **extra)
> > response = super(APIClient, self).post(
> > path, data=data, format='json', content_type=content_type,
> > follow=follow, SERVER_NAME='example.com', **extra)
> > - validator.validate_data(path, request, response)
> > + if validate_schema:
> > + validator.validate_data(path, request, response)
> > return response
> >
> > def put(self, path, data=None, format=None, content_type=None,
> > follow=False, **extra):
> > + validate_schema = extra.pop('validate_schema', True)
> > request = self.factory.put(
> > path, data=data, format='json', content_type=content_type,
> > SERVER_NAME='example.com', **extra)
> > response = super(APIClient, self).put(
> > path, data=data, format='json', content_type=content_type,
> > follow=follow, SERVER_NAME='example.com', **extra)
> > - validator.validate_data(path, request, response)
> > + if validate_schema:
> > + validator.validate_data(path, request, response)
> > return response
> >
> > def patch(self, path, data=None, format=None, content_type=None,
> > follow=False, **extra):
> > + validate_schema = extra.pop('validate_schema', True)
> > request = self.factory.patch(
> > path, data=data, format='json', content_type=content_type,
> > SERVER_NAME='example.com', **extra)
> > response = super(APIClient, self).patch(
> > path, data=data, format='json', content_type=content_type,
> > follow=follow, SERVER_NAME='example.com', **extra)
> > - validator.validate_data(path, request, response)
> > + if validate_schema:
> > + validator.validate_data(path, request, response)
> > return response
> >
> >
> > diff --git a/releasenotes/notes/update-bundle-via-api-2946d8c4e730d545.yaml b/releasenotes/notes/update-bundle-via-api-2946d8c4e730d545.yaml
> > new file mode 100644
> > index 00000000..bfa1ef55
> > --- /dev/null
> > +++ b/releasenotes/notes/update-bundle-via-api-2946d8c4e730d545.yaml
> > @@ -0,0 +1,4 @@
> > +---
> > +api:
> > + - |
> > + Bundles can now be created, updated and deleted via the REST API.
> > --
> > 2.21.0
More information about the Patchwork
mailing list