diff mbox series

[3/3] Remove 'PatchRelationSerializer'

Message ID 20201004111002.234895-3-stephen@that.guru
State New
Headers show
Series [1/3] tests: Rework 'create_relation' helper | expand

Commit Message

Stephen Finucane Oct. 4, 2020, 11:10 a.m. UTC
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(-)

Comments

Daniel Axtens Oct. 8, 2020, 11:51 p.m. UTC | #1
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
diff mbox series

Patch

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