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.
> 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?
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
