[PATCH 3/3] Remove 'PatchRelationSerializer'
Daniel Axtens
dja at axtens.net
Fri Oct 9 10:51:28 AEDT 2020
Stephen Finucane <stephen at 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 at that.guru>
> Reported-by: Ralf Ramsauer <ralf.ramsauer at oth-regensburg.de>
> Closes: #379
> Cc: Daniel Axtens <dja at axtens.net>
> Cc: Rohit Sarkar <rohitsarkar5398 at 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 at 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
More information about the Patchwork
mailing list