Remove 'PatchRelationSerializer'

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] encode/django-rest-framework#7550
[2] encode/django-rest-framework#7574

Signed-off-by: Stephen Finucane <[email protected]>
Reported-by: Ralf Ramsauer <[email protected]>
Closes: #379
Cc: Daniel Axtens <[email protected]>
Cc: Rohit Sarkar <[email protected]>

Author: stephenfin

patchwork/api/__init__.py

@@ -0,0 +1,50 @@
+# Patchwork - automated patch tracking system
+# Copyright (C) 2020, Stephen Finucane <[email protected]>
+#
+# 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

patchwork/api/embedded.py

@@ -12,9 +12,8 @@
 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 Meta:
             }
 
 
-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):

patchwork/api/event.py

@@ -13,7 +13,6 @@
 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'],

patchwork/api/patch.py

@@ -10,23 +10,23 @@
 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
 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 @@ def to_representation(self, instance):
         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