Mete Polat <metepolat2...@gmail.com> writes:
> Hi Daniel,
>
> (sorry for the short delay)
>
> On 06.11.19 16:12, Daniel Axtens wrote:
>> Mete Polat <metepolat2...@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 <metepolat2...@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.
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()
>>> +
>>> +@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
_______________________________________________
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
