Stephen Finucane <step...@that.guru> writes: > This wasn't writeable for reasons I haven't been able to figure out. > However, it's not actually needed: the 'PatchSerializer' can do the job > just fine, given enough information. This exposes a bug in DRF, which > has been reported upstream [1]. While we wait for that fix, or some > variant of it, to be merged, we must monkey patch the library. > > [1] https://github.com/encode/django-rest-framework/issues/7550 > [2] https://github.com/encode/django-rest-framework/pull/7574
ISTR there being performance reasons I wrote this, or I just couldn't bend DRF to my will. I'll double-check. But more to the point I am _extremely_ uncomfortable monkey-patching a dependency. At the absoulute very least we need to check the version of DRF before we monkey-patch it, but ideally we shouldn't be monkey-patching at all. I know, I know, I once proposed doing the same thing to get around a performance bug in Django. I was 3 years younger and dumber, but to quote your concerns at the time: > OK, so I know this is a pretty serious performance issue but I really don't > want to merge this :( It's far too...hacky, as you say above, and pretty > unmaintainable to book. Kind regards, Daniel > > Signed-off-by: Stephen Finucane <step...@that.guru> > Reported-by: Ralf Ramsauer <ralf.ramsa...@oth-regensburg.de> > Closes: #379 > Cc: Daniel Axtens <d...@axtens.net> > Cc: Rohit Sarkar <rohitsarkar5...@gmail.com> > --- > patchwork/api/__init__.py | 50 +++++++++++++++++++++++++++++++++++++++ > patchwork/api/embedded.py | 28 +--------------------- > patchwork/api/event.py | 7 +++--- > patchwork/api/patch.py | 22 ++++++++--------- > 4 files changed, 65 insertions(+), 42 deletions(-) > > diff --git patchwork/api/__init__.py patchwork/api/__init__.py > index e69de29b..efc0dd89 100644 > --- patchwork/api/__init__.py > +++ patchwork/api/__init__.py > @@ -0,0 +1,50 @@ > +# Patchwork - automated patch tracking system > +# Copyright (C) 2020, Stephen Finucane <step...@that.guru> > +# > +# SPDX-License-Identifier: GPL-2.0-or-later > + > +from rest_framework.fields import empty > +from rest_framework.fields import get_attribute > +from rest_framework.fields import SkipField > +from rest_framework.relations import ManyRelatedField > + > + > +# monkey patch django-rest-framework to work around issue #7550 [1] until > #7574 > +# [2] or some other variant lands > +# > +# [1] https://github.com/encode/django-rest-framework/issues/7550 > +# [2] https://github.com/encode/django-rest-framework/pull/7574 > + > +def _get_attribute(self, instance): > + # Can't have any relationships if not created > + if hasattr(instance, 'pk') and instance.pk is None: > + return [] > + > + try: > + relationship = get_attribute(instance, self.source_attrs) > + except (KeyError, AttributeError) as exc: > + if self.default is not empty: > + return self.get_default() > + if self.allow_null: > + return None > + if not self.required: > + raise SkipField() > + msg = ( > + 'Got {exc_type} when attempting to get a value for field ' > + '`{field}` on serializer `{serializer}`.\nThe serializer ' > + 'field might be named incorrectly and not match ' > + 'any attribute or key on the `{instance}` instance.\n' > + 'Original exception text was: {exc}.'.format( > + exc_type=type(exc).__name__, > + field=self.field_name, > + serializer=self.parent.__class__.__name__, > + instance=instance.__class__.__name__, > + exc=exc > + ) > + ) > + raise type(exc)(msg) > + > + return relationship.all() if hasattr(relationship, 'all') else > relationship > + > + > +ManyRelatedField.get_attribute = _get_attribute > diff --git patchwork/api/embedded.py patchwork/api/embedded.py > index 3f32bd42..78316979 100644 > --- patchwork/api/embedded.py > +++ patchwork/api/embedded.py > @@ -12,9 +12,8 @@ nested fields. > from collections import OrderedDict > > from rest_framework.serializers import CharField > -from rest_framework.serializers import SerializerMethodField > from rest_framework.serializers import PrimaryKeyRelatedField > -from rest_framework.serializers import ValidationError > +from rest_framework.serializers import SerializerMethodField > > from patchwork.api.base import BaseHyperlinkedModelSerializer > from patchwork.api.base import CheckHyperlinkedIdentityField > @@ -139,31 +138,6 @@ class PatchSerializer(SerializedRelatedField): > } > > > -class PatchRelationSerializer(BaseHyperlinkedModelSerializer): > - """Hide the PatchRelation model, just show the list""" > - patches = PatchSerializer(many=True, > - style={'base_template': 'input.html'}) > - > - def to_internal_value(self, data): > - if not isinstance(data, type([])): > - raise ValidationError( > - "Patch relations must be specified as a list of patch IDs" > - ) > - result = super(PatchRelationSerializer, self).to_internal_value( > - {'patches': data} > - ) > - return result > - > - def to_representation(self, instance): > - data = super(PatchRelationSerializer, > self).to_representation(instance) > - data = data['patches'] > - return data > - > - class Meta: > - model = models.PatchRelation > - fields = ('patches',) > - > - > class PersonSerializer(SerializedRelatedField): > > class _Serializer(BaseHyperlinkedModelSerializer): > diff --git patchwork/api/event.py patchwork/api/event.py > index 7ed9efb1..75bf8708 100644 > --- patchwork/api/event.py > +++ patchwork/api/event.py > @@ -13,7 +13,6 @@ from rest_framework.serializers import SlugRelatedField > from patchwork.api.embedded import CheckSerializer > from patchwork.api.embedded import CoverSerializer > from patchwork.api.embedded import PatchSerializer > -from patchwork.api.embedded import PatchRelationSerializer > from patchwork.api.embedded import ProjectSerializer > from patchwork.api.embedded import SeriesSerializer > from patchwork.api.embedded import UserSerializer > @@ -34,8 +33,10 @@ class EventSerializer(ModelSerializer): > current_delegate = UserSerializer() > created_check = SerializerMethodField() > created_check = CheckSerializer() > - previous_relation = PatchRelationSerializer(read_only=True) > - current_relation = PatchRelationSerializer(read_only=True) > + previous_relation = PatchSerializer( > + source='previous_relation.patches', many=True, default=None) > + current_relation = PatchSerializer( > + source='current_relation.patches', many=True, default=None) > > _category_map = { > Event.CATEGORY_COVER_CREATED: ['cover'], > diff --git patchwork/api/patch.py patchwork/api/patch.py > index d881504f..f6cb276d 100644 > --- patchwork/api/patch.py > +++ patchwork/api/patch.py > @@ -10,7 +10,6 @@ import email.parser > from django.core.exceptions import ValidationError > from django.utils.text import slugify > from django.utils.translation import gettext_lazy as _ > -from rest_framework import status > from rest_framework.exceptions import APIException > from rest_framework.exceptions import PermissionDenied > from rest_framework.generics import ListAPIView > @@ -18,15 +17,16 @@ from rest_framework.generics import RetrieveUpdateAPIView > from rest_framework.relations import RelatedField > from rest_framework.reverse import reverse > from rest_framework.serializers import SerializerMethodField > +from rest_framework import status > > from patchwork.api.base import BaseHyperlinkedModelSerializer > from patchwork.api.base import PatchworkPermission > -from patchwork.api.filters import PatchFilterSet > -from patchwork.api.embedded import PatchRelationSerializer > +from patchwork.api.embedded import PatchSerializer > from patchwork.api.embedded import PersonSerializer > from patchwork.api.embedded import ProjectSerializer > from patchwork.api.embedded import SeriesSerializer > from patchwork.api.embedded import UserSerializer > +from patchwork.api.filters import PatchFilterSet > from patchwork.models import Patch > from patchwork.models import PatchRelation > from patchwork.models import State > @@ -83,7 +83,8 @@ class PatchListSerializer(BaseHyperlinkedModelSerializer): > check = SerializerMethodField() > checks = SerializerMethodField() > tags = SerializerMethodField() > - related = PatchRelationSerializer() > + related = PatchSerializer( > + source='related.patches', many=True, default=[]) > > def get_web_url(self, instance): > request = self.context.get('request') > @@ -127,14 +128,11 @@ class > PatchListSerializer(BaseHyperlinkedModelSerializer): > data = super(PatchListSerializer, self).to_representation(instance) > data['series'] = [data['series']] if data['series'] else [] > > - # stop the related serializer returning this patch in the list of > - # related patches. Also make it return an empty list, not null/None > - if 'related' in data: > - if data['related']: > - data['related'] = [p for p in data['related'] > - if p['id'] != instance.id] > - else: > - data['related'] = [] > + # Remove this patch from 'related' > + if 'related' in data and data['related']: > + data['related'] = [ > + x for x in data['related'] if x['id'] != data['id'] > + ] > > return data > > -- > 2.25.4 _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
