[PATCH v2 1/5] api: add addressed field and detail endpoint for patch comments
Stephen Finucane
stephen at that.guru
Thu Aug 12 21:38:00 AEST 2021
On Wed, 2021-08-11 at 09:20 +1000, Daniel Axtens wrote:
> Raxel Gutierrez <raxel at google.com> writes:
>
> > Add addressed boolean field to PatchComment to be able to distinguish
> > between them in the patch-detail page. Change PatchComment to have same
> > is_editable from the related patch with the addition of being editable
> > by the user who submitted the comment.
> >
> > Add endpoint for patch comments at api/.../comments/<comment_id>.
> > The endpoint will make it possible to use the REST API to update the new
> > `addressed` field for patch comments with JavaScript on the client side.
> > In the process of these changes, clean up use of CurrentPatchDefault so
> > that it exists in base.py and can be used throughout the API.
> >
> > Add the OpenAPI definition of the new endpoint and upgrade API version
> > to v1.3 to reflect the minor change. In order for tests to pass, clean
> > up test_comment.py to reflect the change from the <pk> to <patch_id>
> > parameter for the `api-patch-comment-list` endpoint.
> >
> > Signed-off-by: Raxel Gutierrez <raxel at google.com>
> > ---
> > docs/api/schemas/generate-schemas.py | 4 +-
> > docs/api/schemas/latest/patchwork.yaml | 144 +-
> > docs/api/schemas/patchwork.j2 | 148 +-
> > docs/api/schemas/v1.0/patchwork.yaml | 35 +-
> > docs/api/schemas/v1.1/patchwork.yaml | 35 +-
> > docs/api/schemas/v1.2/patchwork.yaml | 35 +-
> > docs/api/schemas/v1.3/patchwork.yaml | 2749 +++++++++++++++++
> > patchwork/api/base.py | 24 +-
> > patchwork/api/check.py | 21 +-
> > patchwork/api/comment.py | 76 +-
> > patchwork/api/patch.py | 2 +-
> > .../migrations/0045_patchcomment_addressed.py | 18 +
> > patchwork/models.py | 5 +-
> > patchwork/tests/api/test_comment.py | 4 +-
> > patchwork/urls.py | 17 +-
> > 15 files changed, 3256 insertions(+), 61 deletions(-)
> > create mode 100644 docs/api/schemas/v1.3/patchwork.yaml
> > create mode 100644 patchwork/migrations/0045_patchcomment_addressed.py
> >
> > diff --git a/docs/api/schemas/generate-schemas.py b/docs/api/schemas/generate-schemas.py
> > index a0c1e45..3a436a1 100755
> > --- a/docs/api/schemas/generate-schemas.py
> > +++ b/docs/api/schemas/generate-schemas.py
> > @@ -14,8 +14,8 @@ except ImportError:
> > yaml = None
> >
> > ROOT_DIR = os.path.dirname(os.path.realpath(__file__))
> > -VERSIONS = [(1, 0), (1, 1), (1, 2), None]
> > -LATEST_VERSION = (1, 2)
> > +VERSIONS = [(1, 0), (1, 1), (1, 2), (1, 3), None]
> > +LATEST_VERSION = (1, 3)
> >
> >
> > def generate_schemas():
>
> [generated schema snipped]
>
> > diff --git a/docs/api/schemas/patchwork.j2 b/docs/api/schemas/patchwork.j2
> > index af20743..3600139 100644
> > --- a/docs/api/schemas/patchwork.j2
> > +++ b/docs/api/schemas/patchwork.j2
> > @@ -619,14 +619,14 @@ paths:
> > {% endif %}
> > tags:
> > - patches
> > - /api/{{ version_url }}patches/{id}/comments/:
> > + /api/{{ version_url }}patches/{patch_id}/comments/:
> > parameters:
> > - in: path
> > - name: id
> > + name: patch_id
>
> As Emily suggested, this is probably not something we can do in old
> schema versions. I tried running the OpenAPI generator
> (https://openapi-generator.tech/) over the new and old v1.2 schema,
> generating a Go client for the v1.2 API, and observed different method
> signatures and struct field names. I don't know if people use named
> parameters in Go so maybe things would keep working by accident but
> hopefully you get what I'm going for: if you had written some program
> that interacts with the API using code generated from the API definition
> file, things might break.
>
> There shouldn't generally be any meaningful changes to older API version
> descriptions at this stage.
tbh, I doubt it makes much of a difference. Ultimately the API behaves the same
way - all we've changed is the token name. If we break something, I'm sure
someone will yell at us.
I would rather this change happened separately in a precursor patch though.
There's already enough going on here without muddying the water further.
>
> > description: A unique integer value identifying the parent patch.
> > required: true
> > schema:
> > - title: ID
> > + title: Patch ID
> > type: integer
> > get:
> > description: List comments
> > @@ -656,6 +656,112 @@ paths:
> > $ref: '#/components/schemas/Error'
> > tags:
> > - comments
>
> > + /api/{{ version_url }}patches/{patch_id}/comments/{comment_id}/:
> I think the {% if ... needs to start above this line - otherwise all the
> API versions see this endpoint, just without any methods. I think you've
> correctly hidden it from older versions with the api_1_3_patterns part
> in urls.py, so we don't want them to see it in the definitions file
> either.
Correct.
>
> > + parameters:
> > + - in: path
> > + name: patch_id
> > + description: A unique integer value identifying the parent patch.
> > + required: true
> > + schema:
> > + title: Patch ID
> > + type: integer
> > + - in: path
> > + name: comment_id
> > + description: A unique integer value identifying this comment.
> > + required: true
> > + schema:
> > + title: Comment ID
> > + type: integer
> > +{% if version >= (1, 3) %}
> > + get:
> > + description: Show a patch comment.
> > + operationId: patch_comments_read
> > + responses:
> > + '200':
> > + description: ''
> > + content:
> > + application/json:
> > + schema:
> > + $ref: '#/components/schemas/CommentDetail'
> > + '404':
> > + description: Not found
> > + content:
> > + application/json:
> > + schema:
> > + $ref: '#/components/schemas/Error'
> > + tags:
> > + - comments
> > + patch:
> > + description: Update a patch comment (partial).
> > + operationId: patch_comments_partial_update
> > +# security:
> > +# - basicAuth: []
> > +# - apiKeyAuth: []
>
> I can see you've copy-pasted this from the other definitions, which is
> exactly what I said to do. It feels silly to keep it but also seems
> silly to break consistency. Idk. Unless Stephen weighs in feel free to
> just do whatever you feel most comfortable with.
> [See commit 2047107468a1 ("docs: Resolve issues with 'patches'")]
Yeah, this sucks. Feel free to either copy-paste as you've done or drop all of
these commented out blobs. I've tried to fix this a few times and been
frustrated every single time.
>
> > + requestBody:
> > + $ref: '#/components/requestBodies/Comment'
> > + responses:
> > + '200':
> > + description: ''
> > + content:
> > + application/json:
> > + schema:
> > + $ref: '#/components/schemas/CommentDetail'
> > + '400':
> > + description: Invalid Request
> > + content:
> > + application/json:
> > + schema:
> > + $ref: '#/components/schemas/ErrorCommentUpdate'
> > + '403':
> > + description: Forbidden
> > + content:
> > + application/json:
> > + schema:
> > + $ref: '#/components/schemas/Error'
> > + '404':
> > + description: Not found
> > + content:
> > + application/json:
> > + schema:
> > + $ref: '#/components/schemas/Error'
> > + tags:
> > + - comments
> > + put:
> > + description: Update a patch comment.
> > + operationId: patch_comments_update
> > +# security:
> > +# - basicAuth: []
> > +# - apiKeyAuth: []
> > + requestBody:
> > + $ref: '#/components/requestBodies/Comment'
> > + responses:
> > + '200':
> > + description: ''
> > + content:
> > + application/json:
> > + schema:
> > + $ref: '#/components/schemas/CommentDetail'
> > + '400':
> > + description: Invalid Request
> > + content:
> > + application/json:
> > + schema:
> > + $ref: '#/components/schemas/ErrorCommentUpdate'
> > + '403':
> > + description: Forbidden
> > + content:
> > + application/json:
> > + schema:
> > + $ref: '#/components/schemas/Error'
> > + '404':
> > + description: Not found
> > + content:
> > + application/json:
> > + schema:
> > + $ref: '#/components/schemas/Error'
> > + tags:
> > + - comments
>
> Hmm. Do we want a PUT method?
>
> We have a pretty eclectic collection of PUTable objects atm: User,
> Bundle, Patch and Project.
>
> If I understand the HTTP methods correctly, I don't think that we're
> supposed to allow the PUT method unless you can create the object or
> replace its contents via the request. So I think it makes sense to allow
> us to PUT a Bundle, but I think we should only allow Users, Patches,
> Projects and Comments to be PATCHed: they should all be created in other
> ways, and usually we don't allow their contents to be replaced.
>
> I think you only need a PATCH method here, unless Stephen has any other ideas.
If we're documenting PUT here then that means we support it at the API layer
too, assuming tests have been fully implemented. I've no issues with not
supporting it but we should remove support at the code level also. IIRC, this
might have implications for the web UI autogenerated by django-rest-framework
but you'd need to investigate this yourself.
>
> > +{% endif %}
> > /api/{{ version_url }}patches/{patch_id}/checks/:
> > parameters:
> > - in: path
> > @@ -1277,6 +1383,12 @@ components:
> > application/x-www-form-urlencoded:
> > schema:
> > $ref: '#/components/schemas/CheckCreate'
> > + Comment:
> > + required: true
> > + content:
> > + application/json:
> > + schema:
> > + $ref: '#/components/schemas/CommentUpdate'
> > Patch:
> > required: true
> > content:
> > @@ -1586,6 +1698,25 @@ components:
> > additionalProperties:
> > type: string
> > readOnly: true
> > + CommentDetail:
> > + allOf:
> > + - $ref: '#/components/schemas/Comment'
> > +{% if version >= (1, 3) %}
This is in the wrong place also. The entire resource should not be included if
version < 1.3. You need to move this above 'CommentDetail'.
> > + - properties:
> > + patch:
> > + $ref: '#/components/schemas/PatchEmbedded'
> > + addressed:
> > + title: Addressed
> > + type: boolean
> > +{% endif %}
> > + CommentUpdate:
> > + type: object
> > +{% if version >= (1, 3) %}
Ditto.
> > + properties:
> > + addressed:
> > + title: Addressed
> > + type: boolean
> > +{% endif %}
> > CoverList:
> > type: object
> > properties:
> > @@ -2659,6 +2790,17 @@ components:
> > items:
> > type: string
> > readOnly: true
> > + ErrorCommentUpdate:
> > + type: object
> > +{% if version >= (1, 3) %}
Ditto.
> > + properties:
> > + addressed:
> > + title: Addressed
> > + type: array
> > + items:
> > + type: string
> > + readOnly: true
> > +{% endif %}
>
> I think we should move the if condition for these objects too. I think
> if the types are only used in v1.3 they should only be defined in v1.3 -
> I can't think of a reason we would need to have objects with no
> properties in older API versions?
+1
> > ErrorPatchUpdate:
> > type: object
> > properties:
> > diff --git a/docs/api/schemas/v1.0/patchwork.yaml b/docs/api/schemas/v1.0/patchwork.yaml
> > index 23e8930..3f90b3e 100644
>
> [snip generated versions]
I wonder if we want to send the auto-generated version in a separate patch? Long
term we should probably remove those auto-generated versions from tree in favour
of building them via the test suite and a Sphinx extension.
>
> > diff --git a/patchwork/api/base.py b/patchwork/api/base.py
> > index 89a4311..856fbd3 100644
> > --- a/patchwork/api/base.py
> > +++ b/patchwork/api/base.py
> > @@ -3,6 +3,7 @@
> > #
> > # SPDX-License-Identifier: GPL-2.0-or-later
> >
> > +import rest_framework
> >
> > from django.conf import settings
> > from django.shortcuts import get_object_or_404
> > @@ -15,6 +16,24 @@ from rest_framework.serializers import HyperlinkedModelSerializer
> > from patchwork.api import utils
> >
> >
> > +DRF_VERSION = tuple(int(x) for x in rest_framework.__version__.split('.'))
> > +
> > +
> > +if DRF_VERSION > (3, 11):
> > + class CurrentPatchDefault(object):
> > + requires_context = True
> > +
> > + def __call__(self, serializer_field):
> > + return serializer_field.context['request'].patch
> > +else:
> > + class CurrentPatchDefault(object):
> > + def set_context(self, serializer_field):
> > + self.patch = serializer_field.context['request'].patch
> > +
> > + def __call__(self):
> > + return self.patch
>
> Hmm. I was going to comment on this but then I realised you just moved
> this code. I think we might be able to drop the DRF version condition
> entirely but you don't need to be the person to do that (and this series
> isn't necessarily the right place).
>
> No action required here.
An I was going to say you should leave a comment here explaining why you're
doing this. However, this is code moved from 'patchwork/api/check.py' which I
authored so never mind.
@Daniel We can't remove it until we formally drop support for DRF < 3.12, which
we haven't done. I need to evaluate the package situation on Debian before I do
this.
> > +
> > +
> > class LinkHeaderPagination(PageNumberPagination):
> > """Provide pagination based on rfc5988.
> >
> > @@ -44,7 +63,10 @@ class LinkHeaderPagination(PageNumberPagination):
> >
> >
> > class PatchworkPermission(permissions.BasePermission):
> > - """This permission works for Project and Patch model objects"""
> > + """
> > + This permission works for Project, Patch, and PatchComment
> > + model objects
> > + """
> > def has_object_permission(self, request, view, obj):
> > # read only for everyone
> > if request.method in permissions.SAFE_METHODS:
> > diff --git a/patchwork/api/check.py b/patchwork/api/check.py
> > index a6bf5f8..fde6b61 100644
> > --- a/patchwork/api/check.py
> > +++ b/patchwork/api/check.py
> > @@ -6,7 +6,7 @@
> > from django.http import Http404
> > from django.http.request import QueryDict
> > from django.shortcuts import get_object_or_404
> > -import rest_framework
> > +
nit: don't add a newline here. Imports should be grouped like so:
stdlib imports
third-party package imports
patchwork imports
> > from rest_framework.exceptions import PermissionDenied
> > from rest_framework.generics import ListCreateAPIView
> > from rest_framework.generics import RetrieveAPIView
> > @@ -17,30 +17,13 @@ from rest_framework.serializers import ValidationError
> >
> > from patchwork.api.base import CheckHyperlinkedIdentityField
> > from patchwork.api.base import MultipleFieldLookupMixin
> > +from patchwork.api.base import CurrentPatchDefault
> > from patchwork.api.embedded import UserSerializer
> > from patchwork.api.filters import CheckFilterSet
> > from patchwork.models import Check
> > from patchwork.models import Patch
> >
> >
> > -DRF_VERSION = tuple(int(x) for x in rest_framework.__version__.split('.'))
> > -
> > -
> > -if DRF_VERSION > (3, 11):
> > - class CurrentPatchDefault(object):
> > - requires_context = True
> > -
> > - def __call__(self, serializer_field):
> > - return serializer_field.context['request'].patch
> > -else:
> > - class CurrentPatchDefault(object):
> > - def set_context(self, serializer_field):
> > - self.patch = serializer_field.context['request'].patch
> > -
> > - def __call__(self):
> > - return self.patch
> > -
> > -
> > class CheckSerializer(HyperlinkedModelSerializer):
> >
> > url = CheckHyperlinkedIdentityField('api-check-detail')
> > diff --git a/patchwork/api/comment.py b/patchwork/api/comment.py
> > index 43b26c6..50d70d1 100644
> > --- a/patchwork/api/comment.py
> > +++ b/patchwork/api/comment.py
> > @@ -5,12 +5,17 @@
> >
> > import email.parser
> >
> > +from django.shortcuts import get_object_or_404
> > from django.http import Http404
> > from rest_framework.generics import ListAPIView
> > +from rest_framework.generics import RetrieveUpdateAPIView
> > +from rest_framework.serializers import HiddenField
> > from rest_framework.serializers import SerializerMethodField
> >
> > from patchwork.api.base import BaseHyperlinkedModelSerializer
> > +from patchwork.api.base import MultipleFieldLookupMixin
> > from patchwork.api.base import PatchworkPermission
> > +from patchwork.api.base import CurrentPatchDefault
> > from patchwork.api.embedded import PersonSerializer
> > from patchwork.models import Cover
> > from patchwork.models import CoverComment
> > @@ -66,15 +71,50 @@ class CoverCommentListSerializer(BaseCommentListSerializer):
> > versioned_fields = BaseCommentListSerializer.Meta.versioned_fields
> >
> >
> > -class PatchCommentListSerializer(BaseCommentListSerializer):
> > +class PatchCommentSerializer(BaseCommentListSerializer):
> > +
> > + patch = HiddenField(default=CurrentPatchDefault())
> >
> > class Meta:
> > model = PatchComment
> > - fields = BaseCommentListSerializer.Meta.fields
> > - read_only_fields = fields
> > + fields = BaseCommentListSerializer.Meta.fields + (
> > + 'patch', 'addressed')
> > + read_only_fields = fields[:-1] # able to write to addressed field
>
> Please make this explicit, that is
> BaseCommentListSerializer.Meta.fields + ('patch', )
+1
>
> > + versioned_fields = {
> > + '1.3': ('patch', 'addressed'),
> > + }
> > + extra_kwargs = {
> > + 'url': {'view_name': 'api-patch-comment-detail'}
> > + }
> > versioned_fields = BaseCommentListSerializer.Meta.versioned_fields
> >
> >
> > +class PatchCommentMixin(object):
> > +
> > + permission_classes = (PatchworkPermission,)
> > + serializer_class = PatchCommentSerializer
> > +
> > + def get_object(self):
> > + queryset = self.filter_queryset(self.get_queryset())
> > + comment_id = self.kwargs['comment_id']
> > + try:
> > + obj = queryset.get(id=int(comment_id))
> > + except (ValueError, PatchComment.DoesNotExist):
> > + obj = get_object_or_404(queryset, linkname=comment_id)
> > + self.kwargs['comment_id'] = obj.id
> > + self.check_object_permissions(self.request, obj)
> > + return obj
> > +
> > + def get_queryset(self):
> > + patch_id = self.kwargs['patch_id']
> > + if not Patch.objects.filter(id=patch_id).exists():
> > + raise Http404
> > +
> > + return PatchComment.objects.filter(
> > + patch=patch_id
> > + ).select_related('submitter')
> > +
> > +
> > class CoverCommentList(ListAPIView):
> > """List cover comments"""
> >
> > @@ -94,20 +134,24 @@ class CoverCommentList(ListAPIView):
> > ).select_related('submitter')
> >
> >
> > -class PatchCommentList(ListAPIView):
> > - """List comments"""
> > +class PatchCommentList(PatchCommentMixin, ListAPIView):
> > + """List patch comments"""
> >
> > - permission_classes = (PatchworkPermission,)
> > - serializer_class = PatchCommentListSerializer
> > search_fields = ('subject',)
> > ordering_fields = ('id', 'subject', 'date', 'submitter')
> > ordering = 'id'
> > - lookup_url_kwarg = 'pk'
> > -
> > - def get_queryset(self):
> > - if not Patch.objects.filter(pk=self.kwargs['pk']).exists():
> > - raise Http404
> > -
> > - return PatchComment.objects.filter(
> > - patch=self.kwargs['pk']
> > - ).select_related('submitter')
> > + lookup_url_kwarg = 'patch_id'
> > +
> > +
> > +class PatchCommentDetail(PatchCommentMixin, MultipleFieldLookupMixin,
> > + RetrieveUpdateAPIView):
> > + """
> > + get:
> > + Show a patch comment.
> > + patch:
> > + Update a patch comment.
> > + put:
> > + Update a patch comment.
> > + """
> > + lookup_url_kwargs = ('patch_id', 'comment_id')
> > + lookup_fields = ('patch_id', 'id')
> > diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
> > index 9d22275..a97a882 100644
> > --- a/patchwork/api/patch.py
> > +++ b/patchwork/api/patch.py
> > @@ -97,7 +97,7 @@ class PatchListSerializer(BaseHyperlinkedModelSerializer):
> >
> > def get_comments(self, patch):
> > return self.context.get('request').build_absolute_uri(
> > - reverse('api-patch-comment-list', kwargs={'pk': patch.id}))
> > + reverse('api-patch-comment-list', kwargs={'patch_id': patch.id}))
> >
> > def get_check(self, instance):
> > return instance.combined_check_state
> > diff --git a/patchwork/migrations/0045_patchcomment_addressed.py b/patchwork/migrations/0045_patchcomment_addressed.py
> > new file mode 100644
> > index 0000000..92e6c4e
> > --- /dev/null
> > +++ b/patchwork/migrations/0045_patchcomment_addressed.py
> > @@ -0,0 +1,18 @@
> > +# Generated by Django 3.1.12 on 2021-07-16 04:12
> > +
> > +from django.db import migrations, models
> > +
> > +
> > +class Migration(migrations.Migration):
> > +
> > + dependencies = [
> > + ('patchwork', '0044_add_project_linkname_validation'),
> > + ]
> > +
> > + operations = [
> > + migrations.AddField(
> > + model_name='patchcomment',
> > + name='addressed',
> > + field=models.BooleanField(default=False),
Thinking out loud: I suspect this will be an expensive migration since it will
add a new non-nullable field to all patchcomment rows. Maybe that's something we
can live with, but making this nullable might make more sense, particularly if
we want to make this feature enableable/disableable on a project-by-project
basis (I don't know if we do).
> > + ),
> > + ]
> > diff --git a/patchwork/models.py b/patchwork/models.py
> > index 00273da..ef52f2c 100644
> > --- a/patchwork/models.py
> > +++ b/patchwork/models.py
> > @@ -693,6 +693,7 @@ class PatchComment(EmailMixin, models.Model):
> > related_query_name='comment',
> > on_delete=models.CASCADE,
> > )
> > + addressed = models.BooleanField(default=False)
> >
> > @property
> > def list_archive_url(self):
> > @@ -718,7 +719,9 @@ class PatchComment(EmailMixin, models.Model):
> > self.patch.refresh_tag_counts()
> >
> > def is_editable(self, user):
> > - return False
> > + if user == self.submitter.user:
> > + return True
> > + return self.patch.is_editable(user)
> >
> > class Meta:
> > ordering = ['date']
> > diff --git a/patchwork/tests/api/test_comment.py b/patchwork/tests/api/test_comment.py
> > index 5bbebf2..59450d8 100644
> > --- a/patchwork/tests/api/test_comment.py
> > +++ b/patchwork/tests/api/test_comment.py
> > @@ -90,7 +90,7 @@ class TestPatchComments(utils.APITestCase):
> > kwargs = {}
> > if version:
> > kwargs['version'] = version
> > - kwargs['pk'] = patch.id
> > + kwargs['patch_id'] = patch.id
>
> I think we might be able to get away with renaming the parameter
> internally even if we can't rename it in old versions of the API, but
> please split the 'pk'->'patch_id' change into a different patch.
>
> Kind regards,
> Daniel
Cheers,
Stephen
More information about the Patchwork
mailing list