diff mbox

[v5,06/10] REST: Add Patches to the API

Message ID 1464311566-19431-7-git-send-email-andy.doan@linaro.org
State Superseded
Headers show

Commit Message

Andy Doan May 27, 2016, 1:12 a.m. UTC 
This exposes 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)

NOTE: Patch.save was overridden incorrectly and had to be
fixed to work with DRF.

Signed-off-by: Andy Doan <andy.doan@linaro.org>
---
 patchwork/models.py              |   4 +-
 patchwork/rest_serializers.py    |  40 +++++++++-
 patchwork/tests/test_rest_api.py | 161 ++++++++++++++++++++++++++++++++++++++-
 patchwork/views/rest_api.py      |  18 ++++-
 4 files changed, 216 insertions(+), 7 deletions(-)

Comments

Stephen Finucane June 2, 2016, 1:23 p.m. UTC | #1 
On 26 May 20:12, Andy Doan wrote:
> This exposes 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)
> 
> NOTE: Patch.save was overridden incorrectly and had to be
> fixed to work with DRF.
> 
> Signed-off-by: Andy Doan <andy.doan@linaro.org>

One item. Again, fixable by me if you like.

> +    def to_representation(self, instance):
> +        data = super(PatchSerializer, self).to_representation(instance)
> +        headers = data.get('headers')
> +        if headers is not None:
> +            data['headers'] = email.parser.Parser().parsestr(headers, True)
> +            data['tags'] = [{'name': x.tag.name, 'count': x.count}
> +                            for x in instance.patchtag_set.all()]

This line needs to be unindented to ensure tags are listed on the
'patch-list' page (I think we should do this, IMO). If we don't want to
list them here, they should still be unindented and simply removed in
the PatchListSerializer code.

On another note, is there any advantage in displaying the tags per user,
rather than a cumulative count, like so:

    {
        "Reviewed-by": [
            "http://localhost:8000/api/1.0/people/28/"
        ]
    }

This could be dumb/poor-performing suggestion, so it's just that :)

Stephen
Andy Doan June 16, 2016, 8:49 p.m. UTC | #2 
On 06/02/2016 08:23 AM, Finucane, Stephen wrote:
> On another note, is there any advantage in displaying the tags per user,
> rather than a cumulative count, like so:
>
>      {
>          "Reviewed-by": [
>              "http://localhost:8000/api/1.0/people/28/"
>          ]
>      }

I'm not sure how that's possible. Tags are just:

class PatchTag(models.Model):
     patch = models.ForeignKey('Patch')
     tag = models.ForeignKey('Tag')
     count = models.IntegerField(default=1)

I don't see the way we'd show users other than parsing all the comment 
fields.

> This could be dumb/poor-performing suggestion, so it's just that:)

Probably could do in the patch-detail, and just keep the counts in the 
listing.
diff mbox

Patch

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/rest_serializers.py b/patchwork/rest_serializers.py
index 4f22870..9d9bf80 100644
--- a/patchwork/rest_serializers.py
+++ b/patchwork/rest_serializers.py
@@ -17,11 +17,14 @@ 
 # along with Patchwork; if not, write to the Free Software
 # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 
+import email.parser
+
 from django.contrib.auth.models import User
 
-from rest_framework.serializers import HyperlinkedModelSerializer
+from rest_framework.serializers import (
+    HyperlinkedModelSerializer, ListSerializer, SerializerMethodField)
 
-from patchwork.models import Person, Project
+from patchwork.models import Patch, Person, Project
 
 
 class UserSerializer(HyperlinkedModelSerializer):
@@ -44,3 +47,36 @@  class PersonSerializer(HyperlinkedModelSerializer):
 class ProjectSerializer(HyperlinkedModelSerializer):
     class Meta:
         model = Project
+
+
+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 Meta:
+        model = Patch
+        list_serializer_class = PatchListSerializer
+        read_only_fields = ('project', 'name', 'date', 'submitter', 'diff',
+                            'content', 'hash', 'msgid')
+        # there's no need to expose an entire "tags" endpoint, so we custom
+        # render this field
+        exclude = ('tags',)
+    state = SerializerMethodField()
+
+    def get_state(self, obj):
+        return obj.state.name
+
+    def to_representation(self, instance):
+        data = super(PatchSerializer, self).to_representation(instance)
+        headers = data.get('headers')
+        if headers is not None:
+            data['headers'] = email.parser.Parser().parsestr(headers, True)
+            data['tags'] = [{'name': x.tag.name, 'count': x.count}
+                            for x in instance.patchtag_set.all()]
+        return data
diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py
index 80943b4..0cbe3ca 100644
--- a/patchwork/tests/test_rest_api.py
+++ b/patchwork/tests/test_rest_api.py
@@ -25,8 +25,9 @@  from django.core.urlresolvers import reverse
 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')
@@ -216,3 +217,159 @@  class TestUserAPI(APITestCase):
 
         resp = self.client.post(self.api_url(), {'email': 'foo@f.com'})
         self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
+
+
+@unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
+class TestPatchAPI(APITestCase):
+    fixtures = ['default_states', 'default_tags']
+
+    @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])
+
+    def test_list_simple(self):
+        """Validate we can list a patch."""
+        patches = create_patches()
+        resp = self.client.get(self.api_url())
+        self.assertEqual(status.HTTP_200_OK, resp.status_code)
+        self.assertEqual(1, len(resp.data))
+        patch = resp.data[0]
+        self.assertEqual(patches[0].name, patch['name'])
+        self.assertNotIn('content', patch)
+        self.assertNotIn('headers', patch)
+        self.assertNotIn('diff', patch)
+        self.assertNotIn('tags', patch)
+
+        # test while authenticated
+        user = create_user()
+        self.client.force_authenticate(user=user)
+        resp = self.client.get(self.api_url())
+        self.assertEqual(status.HTTP_200_OK, resp.status_code)
+        self.assertEqual(1, len(resp.data))
+        patch = resp.data[0]
+        self.assertEqual(patches[0].name, patch['name'])
+
+    def test_detail(self):
+        """Validate we can get a specific project."""
+        patches = create_patches()
+        resp = self.client.get(self.api_url(patches[0].id))
+        self.assertEqual(status.HTTP_200_OK, resp.status_code)
+        self.assertEqual(patches[0].name, resp.data['name'])
+        self.assertIn(TestProjectAPI.api_url(patches[0].project.id),
+                      resp.data['project'])
+        self.assertEqual(patches[0].msgid, resp.data['msgid'])
+        self.assertEqual(patches[0].diff, resp.data['diff'])
+        self.assertIn(TestPersonAPI.api_url(patches[0].submitter.id),
+                      resp.data['submitter'])
+        self.assertEqual(patches[0].state.name, resp.data['state'])
+
+    def test_detail_tags(self):
+        patches = create_patches()
+        patches[0].content = 'Reviewed-by: Test User <test@example.com>\n'
+        patches[0].save()
+        patches[0].refresh_tag_counts()
+        resp = self.client.get(self.api_url(patches[0].id))
+        tags = resp.data['tags']
+        self.assertEqual(1, len(tags))
+        self.assertEqual(1, tags[0]['count'])
+        self.assertEqual('Reviewed-by', tags[0]['name'])
+
+    def test_anonymous_writes(self):
+        """Ensure anonymous "write" operations are rejected."""
+        patches = create_patches()
+        patch_url = self.api_url(patches[0].id)
+        resp = self.client.get(patch_url)
+        patch = resp.data
+        patch['msgid'] = 'foo'
+        patch['name'] = 'this should fail'
+
+        # create
+        resp = self.client.post(self.api_url(), 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_anonymous_create(self):
+        """Ensure anonymous "POST" operations are rejected."""
+        patches = create_patches()
+        patch_url = self.api_url(patches[0].id)
+        resp = self.client.get(patch_url)
+        patch = resp.data
+        patch['msgid'] = 'foo'
+        patch['name'] = 'this should fail'
+
+        resp = self.client.post(self.api_url(), patch)
+        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
+
+    def test_anonymous_update(self):
+        """Ensure anonymous "PATCH" operations are rejected."""
+        patches = create_patches()
+        patch_url = self.api_url(patches[0].id)
+        resp = self.client.get(patch_url)
+        patch = resp.data
+        patch['msgid'] = 'foo'
+        patch['name'] = 'this should fail'
+
+        resp = self.client.patch(patch_url, {'name': 'foo'})
+        self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
+
+    def test_anonymous_delete(self):
+        """Ensure anonymous "DELETE" operations are rejected."""
+        patches = create_patches()
+        patch_url = self.api_url(patches[0].id)
+        resp = self.client.get(patch_url)
+
+        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(self.api_url(), 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(self.api_url(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(self.api_url(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(self.api_url(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 1cb8c88..6334ab9 100644
--- a/patchwork/views/rest_api.py
+++ b/patchwork/views/rest_api.py
@@ -20,7 +20,7 @@ 
 from django.conf import settings
 
 from patchwork.rest_serializers import (
-    ProjectSerializer, PersonSerializer, UserSerializer)
+    PatchSerializer, ProjectSerializer, PersonSerializer, UserSerializer)
 
 from rest_framework import permissions
 from rest_framework.pagination import PageNumberPagination
@@ -80,6 +80,21 @@  class PatchworkViewSet(ModelViewSet):
         return self.serializer_class.Meta.model.objects.all()
 
 
+class PatchViewSet(PatchworkViewSet):
+    permission_classes = (PatchworkPermission,)
+    serializer_class = PatchSerializer
+
+    def get_queryset(self):
+        qs = super(PatchViewSet, self).get_queryset(
+        ).prefetch_related(
+            'check_set', 'patchtag_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
+
+
 class PeopleViewSet(PatchworkViewSet):
     permission_classes = (AuthenticatedReadOnly, )
     serializer_class = PersonSerializer
@@ -100,6 +115,7 @@  class ProjectViewSet(PatchworkViewSet):
 
 
 router = DefaultRouter()
+router.register('patches', PatchViewSet, 'patch')
 router.register('people', PeopleViewSet, 'person')
 router.register('projects', ProjectViewSet, 'project')
 router.register('users', UserViewSet, 'user')