| Message ID | 20201004111002.234895-3-stephen@that.guru |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/3] tests: Rework 'create_relation' helper | expand |
Stephen Finucane <stephen@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 <stephen@that.guru> > Reported-by: Ralf Ramsauer <ralf.ramsauer@oth-regensburg.de> > Closes: #379 > Cc: Daniel Axtens <dja@axtens.net> > Cc: Rohit Sarkar <rohitsarkar5398@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 <stephen@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
On Fri, 2020-10-09 at 10:51 +1100, Daniel Axtens wrote: > Stephen Finucane <stephen@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. I've worked through a few other options today but unfortunately this is as good as I can do. The fix is sound, being based on what DRF itself does for other fields (see the PR) and I do have it proposed to DRF so we won't have to carry it forever. I could add a version check that will simply error out for newer versions of DRF, but tbh we already do that through our requirements files and anyone running with other package versions is already using an unsupported configuration. I think this should go as-is. Hopefully we can revert it in the near future once my PR gets some attention (post-COVID, probably). Cheers, Stephen > Kind regards, > Daniel
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 <stephen@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
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 Signed-off-by: Stephen Finucane <stephen@that.guru> Reported-by: Ralf Ramsauer <ralf.ramsauer@oth-regensburg.de> Closes: #379 Cc: Daniel Axtens <dja@axtens.net> Cc: Rohit Sarkar <rohitsarkar5398@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(-)