[PATCH v3 5/5] REST: Add submission relations
Lukas Bulwahn
lukas.bulwahn at gmail.com
Sun Jan 12 03:23:01 AEDT 2020
On Sa., 11. Jan. 2020 at 12:54, Daniel Axtens <dja at axtens.net> wrote:
> Mete Polat <metepolat2000 at gmail.com> writes:
>
> > Hi Daniel,
> >
> > (sorry for the short delay)
> >
> > On 06.11.19 16:12, Daniel Axtens wrote:
> >> 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())
> >> +
> >
> > Nice, thank you. I think this should be sufficient. Just want to note
> > that get_url and get_web_url are showing the same url. I will change
> > that in the next revision.
> >
> >> + 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.
> >
> > I think you are right that there are different types of relations
> > however I am not sure how you exactly want to display them.
> >
> > When viewing a submission, how do we decide which SubmissionRelation to
> > display under 'related'? Do we list the submissions of every
> > SubmissionRelation a specific submission is part of? Do we list them
> > separately or combined? Should users be able to mark a
> > SubmissionRelation as something like "backport-relation" which can then
> > be displayed in a separate row when viewing a submission?
> >
> > I think a more complex relation model could possibly lead to some
> > confusion and unnecessary difficulties.
> >
> > Furthermore I am not sure whether a more complex model really
> > facilitates tools. As an example, PaStA, the analysis tool we are
> > currently using for finding relations and commit_refs for a specific
> > patch, does not differentiate between different types of relations.
> >
> > Nevertheless I am open to be convinced. I just don't want to complicate
> > things if there is really no need.
>
> Sorry I missed this - December was crazy with the non-patchwork parts of
> my job.
>
> My vision was to list them all, but I agree that figuring out how to do
> that is a bit tricky...
>
> >> 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.
>
> I was hoping a context field would provide space for something like
> "backports" or "PaStA-bot" so people could identify what was creating
> the relation. That would then provide headings for the list.
>
> But I agree it's complex, so let's get the simple one deployed in 2.2
> and see what people do with it. It _may_ limit people to ~1 bot per
> mailing list, but let's see what happens.
>
> Again, apologies for dropping this on the floor.
>
We share the same vision. We need a first few implementations of
scripts/heuristics that make use of this feature, though. Then, we will see
how users and projects really want to use this, and what we can implement
to support those use cases.
Lukas
> Regards,
> Daniel
>
> >>
> >>
> >>> +
> >>> +
> >>> +class SubmissionRelationList(SubmissionRelationMixin,
> ListCreateAPIView):
> >>> + ordering = 'id'
> >>> + ordering_fields = ['id', 'submission_count']
> >>
> >> What is the benefit of being able to order by submission_count?
> >
> > We can remove this. I just used it to see how the UI looks like for
> > different relation counts.
> >
> >> 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...
> >>
> >
> > I will see how manageable this is.
> >
> >>> +
> >>> + 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?
> >
> > I am not entirely sure but I will have a closer look at this.
> >
> >>
> >> Feel free to respin after you address these comments. Keep up the good
> >> work!
> >>
> >> Regards,
> >> Daniel
> >>
> >
> > Thank you for reviewing!
> >
> > Best regards,
> >
> > Mete
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/patchwork/attachments/20200111/2c5155c8/attachment-0001.htm>
More information about the Patchwork
mailing list