[PATCH v3 5/5] REST: Add submission relations
Daniel Axtens
dja at axtens.net
Thu Nov 7 02:12:46 AEDT 2019
Mete Polat <metepolat2000 at gmail.com> writes:
> View relations or add/update/delete them as a maintainer. Maintainers
> can only create relations of submissions (patches/cover letters) which
> are part of a project they maintain.
>
> New REST API urls:
> api/relations/
> api/relations/<relation_id>/
>
> Signed-off-by: Mete Polat <metepolat2000 at gmail.com>
> ---
> Previously it was possible to use the PatchSerializer. As we expanded the
> support to submissions in general, it isn't a simple task anymore showing
> hyperlinked submissions (as Patch and CoverLetter are not flattened into
> one model yet). Right now only the submission ids are shown.
Hmm, that's unfortunate. I didn't intend to lead you in to this problem,
sorry! Having said that, it'd be good to supply some common information.
How about this, which is super-gross but which I think is probably the
best we can do unless Stephen can chime in with something better...
diff --git a/patchwork/api/embedded.py b/patchwork/api/embedded.py
index de4f31165ee7..8d44592b51f1 100644
--- a/patchwork/api/embedded.py
+++ b/patchwork/api/embedded.py
@@ -102,6 +102,45 @@ class CheckSerializer(SerializedRelatedField):
}
+def _upgrade_instance(instance):
+ if hasattr(instance, 'patch'):
+ return instance.patch
+ else:
+ return instance.coverletter
+
+
+class SubmissionSerializer(SerializedRelatedField):
+
+ class _Serializer(BaseHyperlinkedModelSerializer):
+ """We need to 'upgrade' or specialise the submission to the relevant
+ subclass, so we can't use the mixins. This is gross but can go away
+ once we flatten the models."""
+ url = SerializerMethodField()
+ web_url = SerializerMethodField()
+ mbox = SerializerMethodField()
+
+ def get_url(self, instance):
+ instance = _upgrade_instance(instance)
+ request = self.context.get('request')
+ return request.build_absolute_uri(instance.get_absolute_url())
+
+ def get_web_url(self, instance):
+ instance = _upgrade_instance(instance)
+ request = self.context.get('request')
+ return request.build_absolute_uri(instance.get_absolute_url())
+
+ def get_mbox(self, instance):
+ instance = _upgrade_instance(instance)
+ request = self.context.get('request')
+ return request.build_absolute_uri(instance.get_mbox_url())
+
+ class Meta:
+ model = models.Submission
+ fields = ('id', 'url', 'web_url', 'msgid', 'list_archive_url',
+ 'date', 'name', 'mbox')
+ read_only_fields = fields
+
+
class CoverLetterSerializer(SerializedRelatedField):
class _Serializer(MboxMixin, WebURLMixin, BaseHyperlinkedModelSerializer):
diff --git a/patchwork/api/relation.py b/patchwork/api/relation.py
index e7d002b9375a..c02a6fe67e2c 100644
--- a/patchwork/api/relation.py
+++ b/patchwork/api/relation.py
@@ -9,6 +9,7 @@ from rest_framework.generics import ListCreateAPIView
from rest_framework.generics import RetrieveUpdateDestroyAPIView
from rest_framework.serializers import ModelSerializer
+from patchwork.api.embedded import SubmissionSerializer
from patchwork.models import SubmissionRelation
@@ -34,6 +35,8 @@ class MaintainerPermission(permissions.BasePermission):
class SubmissionRelationSerializer(ModelSerializer):
+ submissions = SubmissionSerializer(many=True)
+
class Meta:
model = SubmissionRelation
fields = ('id', 'url', 'submissions',)
>
> docs/api/schemas/latest/patchwork.yaml | 218 +++++++++++++++++
> docs/api/schemas/patchwork.j2 | 230 ++++++++++++++++++
> docs/api/schemas/v1.2/patchwork.yaml | 218 +++++++++++++++++
> patchwork/api/index.py | 1 +
> patchwork/api/relation.py | 73 ++++++
> patchwork/tests/api/test_relation.py | 194 +++++++++++++++
> patchwork/tests/utils.py | 11 +
> patchwork/urls.py | 11 +
> ...submission-relations-c96bb6c567b416d8.yaml | 10 +
> 9 files changed, 966 insertions(+)
> diff --git a/patchwork/api/relation.py b/patchwork/api/relation.py
> new file mode 100644
> index 0000000..e7d002b
> --- /dev/null
> +++ b/patchwork/api/relation.py
> @@ -0,0 +1,73 @@
> +# Patchwork - automated patch tracking system
> +# Copyright (C) 2019, Bayerische Motoren Werke Aktiengesellschaft (BMW AG)
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +from django.db.models import Count
> +from rest_framework import permissions
> +from rest_framework.generics import ListCreateAPIView
> +from rest_framework.generics import RetrieveUpdateDestroyAPIView
> +from rest_framework.serializers import ModelSerializer
> +
> +from patchwork.models import SubmissionRelation
> +
> +
> +class MaintainerPermission(permissions.BasePermission):
> +
> + def has_object_permission(self, request, view, submissions):
> + if request.method in permissions.SAFE_METHODS:
> + return True
> +
> + user = request.user
> + if not user.is_authenticated:
> + return False
> +
> + if isinstance(submissions, SubmissionRelation):
> + submissions = list(submissions.submissions.all())
> + maintaining = user.profile.maintainer_projects.all()
> + return all(s.project in maintaining for s in submissions)
If I understand this correctly, you are saying that to have permissions,
for all patches, you must be a maintainer of that project.
That's correct, I think, but a comment spelling it out would be helpful:
perhaps it's because I don't do enough functional programming but it
took me a while to understand what you'd written.
Partially due to my lack of expertise with DRF, it wasn't clear to me
that this prevents the _creation_ (as opposed to modification) of a
relation where you're not the maintainer of all the involved projects,
but upon testing it would appear that it does, so that's good.
> +
> + def has_permission(self, request, view):
> + return request.method in permissions.SAFE_METHODS or \
> + (request.user.is_authenticated and
> + request.user.profile.maintainer_projects.count() > 0)
> +
> +
> +class SubmissionRelationSerializer(ModelSerializer):
> + class Meta:
> + model = SubmissionRelation
> + fields = ('id', 'url', 'submissions',)
> + read_only_fields = ('url',)
> + extra_kwargs = {
> + 'url': {'view_name': 'api-relation-detail'},
> + }
> +
> +
> +class SubmissionRelationMixin:
> + serializer_class = SubmissionRelationSerializer
> + permission_classes = (MaintainerPermission,)
> +
> + def get_queryset(self):
> + return SubmissionRelation.objects.all() \
> + .prefetch_related('submissions')
So prefetch_related always makes me perk up my ears.
Here, we end up doing an individual database query for every submission
that is in a relation (you can observe this with the Django Debug
Toolbar). That's probably not ideal: you could get potentially a large
number of relations per page with a large number of patches per
relation.
More interestingly, looking into it I think the way you've implemented
the model in patch 3 means that a patch can only have at most one
relation? Indeed, testing it shows that to be the case.
That's deeply counter-intuitive to me - shouldn't a patch be able to be
involved in multiple relations? Currently it can only be in one set of
relations (belong to one SubmissionRelation), it seems to me that it
ought to be able to be in multiple sets of relations.
I'm trying to think of a good example of where that makes sense, and I
think one is versions vs backports. So I say that v1 of patch A is
related to v2 and v3 of patch A, and it makes sense for that to be one
SubmissionRelation which v1, v2 and v3 all belong to. Then say that v3
is accepted, and then there's a stable backport of that to some old
trees. I would then say that v3 was related to the backport, but I'm not
sure I'd say that v1 was related to the backport in quite the same way.
So I would have thought there would be a many-to-many relationship
between submissions and relations. This would also facilitate relations
set by multiple tools where we want to be able to maintain some
separation.
Perhaps there's a good rationale for the current one-to-many
relationship, and I'm open to hearing it. If that's the case, then at the
very least, you shouldn't be able to silently _remove_ a patch from a
relation by adding it to another. You should be forced to first remove
the patch from the original relationship explictly, leaving it with
related=NULL, and then you can set another relationship.
Further to that, please could you add a field to SubmissionRelation with
some sort of comment/context/tool identification/etc so we can track
who's doing what. See e.g. the context field in the Check model.
> +
> +
> +class SubmissionRelationList(SubmissionRelationMixin, ListCreateAPIView):
> + ordering = 'id'
> + ordering_fields = ['id', 'submission_count']
What is the benefit of being able to order by submission_count?
It would be really nice to be able to filter by project, depending on
how feasible that is - it requires jumping though a JOIN which is
unpleasant...
> +
> + def create(self, request, *args, **kwargs):
> + serializer = self.get_serializer(data=request.data)
> + serializer.is_valid(raise_exception=True)
> + submissions = serializer.validated_data['submissions']
> + self.check_object_permissions(request, submissions)
> + return super().create(request, *args, **kwargs)
> +
> + def get_queryset(self):
> + return super().get_queryset() \
> + .annotate(submission_count=Count('submission'))
> +
> +
> +class SubmissionRelationDetail(SubmissionRelationMixin,
> + RetrieveUpdateDestroyAPIView):
> + pass
> diff --git a/patchwork/tests/api/test_relation.py b/patchwork/tests/api/test_relation.py
> new file mode 100644
> index 0000000..296926d
> --- /dev/null
> +++ b/patchwork/tests/api/test_relation.py
> @@ -0,0 +1,194 @@
> +# Patchwork - automated patch tracking system
> +# Copyright (C) 2019, Bayerische Motoren Werke Aktiengesellschaft (BMW AG)
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +import unittest
> +from enum import Enum
> +from enum import auto
> +
> +import six
> +from django.conf import settings
> +from django.urls import reverse
> +
> +from patchwork.models import SubmissionRelation
> +from patchwork.tests.api import utils
> +from patchwork.tests.utils import create_maintainer
> +from patchwork.tests.utils import create_patches
> +from patchwork.tests.utils import create_project
> +from patchwork.tests.utils import create_relation
> +from patchwork.tests.utils import create_user
> +
> +if settings.ENABLE_REST_API:
> + from rest_framework import status
> +
> +
> +class UserType(Enum):
> + ANONYMOUS = auto()
> + NON_MAINTAINER = auto()
> + MAINTAINER = auto()
> +
> + at unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> +class TestRelationAPI(utils.APITestCase):
> + fixtures = ['default_tags']
> +
> + @staticmethod
> + def api_url(item=None):
> + kwargs = {}
> + if item is None:
> + return reverse('api-relation-list', kwargs=kwargs)
> + kwargs['pk'] = item
> + return reverse('api-relation-detail', kwargs=kwargs)
> +
> + def request_restricted(self, method, user_type: UserType):
> + # setup
> +
> + project = create_project()
> +
> + if user_type == UserType.ANONYMOUS:
> + expected_status = status.HTTP_403_FORBIDDEN
> + elif user_type == UserType.NON_MAINTAINER:
> + expected_status = status.HTTP_403_FORBIDDEN
> + self.client.force_authenticate(user=create_user())
> + elif user_type == UserType.MAINTAINER:
> + if method == 'post':
> + expected_status = status.HTTP_201_CREATED
> + elif method == 'delete':
> + expected_status = status.HTTP_204_NO_CONTENT
> + else:
> + expected_status = status.HTTP_200_OK
> + user = create_maintainer(project)
> + self.client.force_authenticate(user=user)
> + else:
> + raise ValueError
> +
> + resource_id = None
> + send = None
> +
> + if method == 'delete':
> + resource_id = create_relation(project=project).id
> + elif method == 'post':
> + patch_ids = [p.id for p in create_patches(2, project=project)]
> + send = {'submissions': patch_ids}
> + elif method == 'patch':
> + resource_id = create_relation(project=project).id
> + patch_ids = [p.id for p in create_patches(2, project=project)]
> + send = {'submissions': patch_ids}
> + else:
> + raise ValueError
> +
> + # request
> +
> + resp = getattr(self.client, method)(self.api_url(resource_id), send)
> +
> + # check
> +
> + self.assertEqual(expected_status, resp.status_code)
> +
> + if resp.status_code not in range(200, 202):
> + return
> +
> + if resource_id:
> + self.assertEqual(resource_id, resp.data['id'])
> +
> + send_ids = send['submissions']
> + resp_ids = resp.data['submissions']
> + six.assertCountEqual(self, resp_ids, send_ids)
> +
> + def assertSerialized(self, obj: SubmissionRelation, resp: dict):
> + self.assertEqual(obj.id, resp['id'])
> + obj = [s.id for s in obj.submissions.all()]
> + six.assertCountEqual(self, obj, resp['submissions'])
> +
> + def test_list_empty(self):
> + """List relation when none are present."""
> + resp = self.client.get(self.api_url())
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertEqual(0, len(resp.data))
> +
> + @utils.store_samples('relation-list')
> + def test_list(self):
> + """List relations."""
> + relation = create_relation()
> +
> + resp = self.client.get(self.api_url())
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertEqual(1, len(resp.data))
> + self.assertSerialized(relation, resp.data[0])
> +
> + def test_detail(self):
> + """Show relation."""
> + relation = create_relation()
> +
> + resp = self.client.get(self.api_url(relation.id))
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertSerialized(relation, resp.data)
> +
> + @utils.store_samples('relation-update-error-forbidden')
> + def test_update_anonymous(self):
> + """Update relation as anonymous user.
> +
> + Ensure updates can be performed by maintainers.
> + """
> + self.request_restricted('patch', UserType.ANONYMOUS)
> +
> + def test_update_non_maintainer(self):
> + """Update relation as non-maintainer.
> +
> + Ensure updates can be performed by maintainers.
> + """
> + self.request_restricted('patch', UserType.NON_MAINTAINER)
> +
> + @utils.store_samples('relation-update')
> + def test_update_maintainer(self):
> + """Update relation as maintainer.
> +
> + Ensure updates can be performed by maintainers.
> + """
> + self.request_restricted('patch', UserType.MAINTAINER)
> +
> + @utils.store_samples('relation-delete-error-forbidden')
> + def test_delete_anonymous(self):
> + """Delete relation as anonymous user.
> +
> + Ensure deletes can be performed by maintainers.
> + """
> + self.request_restricted('delete', UserType.ANONYMOUS)
> +
> + def test_delete_non_maintainer(self):
> + """Delete relation as non-maintainer.
> +
> + Ensure deletes can be performed by maintainers.
> + """
> + self.request_restricted('delete', UserType.NON_MAINTAINER)
> +
> + @utils.store_samples('relation-update')
> + def test_delete_maintainer(self):
> + """Delete relation as maintainer.
> +
> + Ensure deletes can be performed by maintainers.
> + """
> + self.request_restricted('delete', UserType.MAINTAINER)
> +
> + @utils.store_samples('relation-create-error-forbidden')
> + def test_create_anonymous(self):
> + """Create relation as anonymous user.
> +
> + Ensure creates can be performed by maintainers.
> + """
> + self.request_restricted('post', UserType.ANONYMOUS)
> +
> + def test_create_non_maintainer(self):
> + """Create relation as non-maintainer.
> +
> + Ensure creates can be performed by maintainers.
> + """
> + self.request_restricted('post', UserType.NON_MAINTAINER)
> +
> + @utils.store_samples('relation-create')
> + def test_create_maintainer(self):
> + """Create relation as maintainer.
> +
> + Ensure creates can be performed by maintainers.
> + """
> + self.request_restricted('post', UserType.MAINTAINER)
> diff --git a/patchwork/tests/utils.py b/patchwork/tests/utils.py
> index 577183d..47149de 100644
> --- a/patchwork/tests/utils.py
> +++ b/patchwork/tests/utils.py
> @@ -16,6 +16,7 @@ from patchwork.models import Check
> from patchwork.models import Comment
> from patchwork.models import CoverLetter
> from patchwork.models import Patch
> +from patchwork.models import SubmissionRelation
> from patchwork.models import Person
> from patchwork.models import Project
> from patchwork.models import Series
> @@ -347,3 +348,13 @@ def create_covers(count=1, **kwargs):
> kwargs (dict): Overrides for various cover letter fields
> """
> return _create_submissions(create_cover, count, **kwargs)
> +
> +
> +def create_relation(count_patches=2, **kwargs):
> + relation = SubmissionRelation.objects.create()
> + values = {
> + 'related': relation
> + }
> + values.update(kwargs)
> + create_patches(count_patches, **values)
> + return relation
I haven't looked at the tests in great detail, but they look very
comprehensive. You can get coverage data out of tox with -e coverage, so
just check that you've covered a reasonably amount of the new code
you're adding. (It doesn't necessarily have to be 100%.)
Lastly, you should probably also add a field on Patch and CoverLetter
that links to the related submissions if they exist. Otherwise I think
you have to enumerate all the relations in the API to determine if there
is one for the patch/cover letter that you're interested in?
Feel free to respin after you address these comments. Keep up the good
work!
Regards,
Daniel
More information about the Patchwork
mailing list