REST: Allow setting of values using embedded serializers

Unfortunately, the use of embedded serializers for some fields breaks
the ability to update these fields, either via the HTML interface (where
the widget is totally busted) or via a client like 'git-pw'. What we
actually want is to be able to update these fields like normal primary
key but show them using the embedded serializer. We do just this by
using a modified variant of the PrimaryKeyRelatedField and using the
serializers simply for displaying.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Closes: #216
(cherry picked from commit 1a0021a)

Author: stephenfin

patchwork/api/embedded.py

@@ -23,14 +23,54 @@
 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 patchwork.api.base import BaseHyperlinkedModelSerializer
 from patchwork.api.base import CheckHyperlinkedIdentityField
 from patchwork import models
 
 
+class SerializedRelatedField(PrimaryKeyRelatedField):
+    """
+    A read-write field that expects a primary key for writes and returns a
+    serialized version of the underlying field on reads.
+    """
+
+    def use_pk_only_optimization(self):
+        # We're using embedded serializers so we want the whole object
+        return False
+
+    def get_queryset(self):
+        return self._Serializer.Meta.model.objects.all()
+
+    def get_choices(self, cutoff=None):
+        # Override this so we don't call 'to_representation', which no longer
+        # returns a flat value
+        queryset = self.get_queryset()
+        if queryset is None:
+            # Ensure that field.choices returns something sensible
+            # even when accessed with a read-only field.
+            return {}
+
+        if cutoff is not None:
+            queryset = queryset[:cutoff]
+
+        return OrderedDict([
+            (
+                item.pk,
+                self.display_value(item)
+            )
+            for item in queryset
+        ])
+
+    def to_representation(self, data):
+        return self._Serializer(context=self.context).to_representation(data)
+
+
 class MboxMixin(BaseHyperlinkedModelSerializer):
     """Embed a link to the mbox URL.
 
@@ -55,132 +95,150 @@ def get_web_url(self, instance):
         return request.build_absolute_uri(instance.get_absolute_url())
 
 
-class BundleSerializer(MboxMixin, WebURLMixin, BaseHyperlinkedModelSerializer):
-
-    class Meta:
-        model = models.Bundle
-        fields = ('id', 'url', 'web_url', 'name', 'mbox')
-        read_only_fields = fields
-        versioned_field = {
-            '1.1': ('web_url', ),
-        }
-        extra_kwargs = {
-            'url': {'view_name': 'api-bundle-detail'},
-        }
+class BundleSerializer(SerializedRelatedField):
+
+    class _Serializer(MboxMixin, WebURLMixin, BaseHyperlinkedModelSerializer):
+
+        class Meta:
+            model = models.Bundle
+            fields = ('id', 'url', 'web_url', 'name', 'mbox')
+            read_only_fields = fields
+            versioned_field = {
+                '1.1': ('web_url', ),
+            }
+            extra_kwargs = {
+                'url': {'view_name': 'api-bundle-detail'},
+            }
+
+
+class CheckSerializer(SerializedRelatedField):
+
+    class _Serializer(BaseHyperlinkedModelSerializer):
+
+        url = CheckHyperlinkedIdentityField('api-check-detail')
+
+        def to_representation(self, instance):
+            data = super(CheckSerializer._Serializer, self).to_representation(
+                instance)
+            data['state'] = instance.get_state_display()
+            return data
+
+        class Meta:
+            model = models.Check
+            fields = ('id', 'url', 'date', 'state', 'target_url', 'context')
+            read_only_fields = fields
+            extra_kwargs = {
+                'url': {'view_name': 'api-check-detail'},
+            }
+
+
+class CoverLetterSerializer(SerializedRelatedField):
+
+    class _Serializer(MboxMixin, WebURLMixin, BaseHyperlinkedModelSerializer):
+
+        class Meta:
+            model = models.CoverLetter
+            fields = ('id', 'url', 'web_url', 'msgid', 'date', 'name', 'mbox')
+            read_only_fields = fields
+            versioned_field = {
+                '1.1': ('web_url', 'mbox', ),
+            }
+            extra_kwargs = {
+                'url': {'view_name': 'api-cover-detail'},
+            }
+
+
+class PatchSerializer(SerializedRelatedField):
+
+    class _Serializer(MboxMixin, WebURLMixin, BaseHyperlinkedModelSerializer):
+
+        class Meta:
+            model = models.Patch
+            fields = ('id', 'url', 'web_url', 'msgid', 'date', 'name', 'mbox')
+            read_only_fields = fields
+            versioned_field = {
+                '1.1': ('web_url', ),
+            }
+            extra_kwargs = {
+                'url': {'view_name': 'api-patch-detail'},
+            }
 
 
-class CheckSerializer(BaseHyperlinkedModelSerializer):
+class PersonSerializer(SerializedRelatedField):
 
-    url = CheckHyperlinkedIdentityField('api-check-detail')
+    class _Serializer(BaseHyperlinkedModelSerializer):
 
-    def to_representation(self, instance):
-        data = super(CheckSerializer, self).to_representation(instance)
-        data['state'] = instance.get_state_display()
-        return data
+        class Meta:
+            model = models.Person
+            fields = ('id', 'url', 'name', 'email')
+            read_only_fields = fields
+            extra_kwargs = {
+                'url': {'view_name': 'api-person-detail'},
+            }
 
-    class Meta:
-        model = models.Check
-        fields = ('id', 'url', 'date', 'state', 'target_url', 'context')
-        read_only_fields = fields
-        extra_kwargs = {
-            'url': {'view_name': 'api-check-detail'},
 
-        }
+class ProjectSerializer(SerializedRelatedField):
 
+    class _Serializer(BaseHyperlinkedModelSerializer):
 
-class CoverLetterSerializer(MboxMixin, WebURLMixin,
-                            BaseHyperlinkedModelSerializer):
+        link_name = CharField(max_length=255, source='linkname')
+        list_id = CharField(max_length=255, source='listid')
+        list_email = CharField(max_length=200, source='listemail')
 
-    class Meta:
-        model = models.CoverLetter
-        fields = ('id', 'url', 'web_url', 'msgid', 'date', 'name', 'mbox')
-        read_only_fields = fields
-        versioned_field = {
-            '1.1': ('web_url', 'mbox', ),
-        }
-        extra_kwargs = {
-            'url': {'view_name': 'api-cover-detail'},
-        }
+        class Meta:
+            model = models.Project
+            fields = ('id', 'url', 'name', 'link_name', 'list_id',
+                      'list_email', 'web_url', 'scm_url', 'webscm_url')
+            read_only_fields = fields
+            extra_kwargs = {
+                'url': {'view_name': 'api-project-detail'},
+            }
 
 
-class PatchSerializer(MboxMixin, WebURLMixin, BaseHyperlinkedModelSerializer):
+class SeriesSerializer(SerializedRelatedField):
 
-    class Meta:
-        model = models.Patch
-        fields = ('id', 'url', 'web_url', 'msgid', 'date', 'name', 'mbox')
-        read_only_fields = fields
-        versioned_field = {
-            '1.1': ('web_url', ),
-        }
-        extra_kwargs = {
-            'url': {'view_name': 'api-patch-detail'},
-        }
+    class _Serializer(MboxMixin, WebURLMixin, BaseHyperlinkedModelSerializer):
 
+        class Meta:
+            model = models.Series
+            fields = ('id', 'url', 'date', 'name', 'version', 'mbox')
+            read_only_fields = fields
+            versioned_field = {
+                '1.1': ('web_url', ),
+            }
+            extra_kwargs = {
+                'url': {'view_name': 'api-series-detail'},
+            }
 
-class PersonSerializer(BaseHyperlinkedModelSerializer):
 
-    class Meta:
-        model = models.Person
-        fields = ('id', 'url', 'name', 'email')
-        read_only_fields = fields
-        extra_kwargs = {
-            'url': {'view_name': 'api-person-detail'},
-        }
+class UserSerializer(SerializedRelatedField):
 
+    class _Serializer(BaseHyperlinkedModelSerializer):
 
-class ProjectSerializer(BaseHyperlinkedModelSerializer):
+        class Meta:
+            model = models.User
+            fields = ('id', 'url', 'username', 'first_name', 'last_name',
+                      'email')
+            read_only_fields = fields
+            extra_kwargs = {
+                'url': {'view_name': 'api-user-detail'},
+            }
 
-    link_name = CharField(max_length=255, source='linkname')
-    list_id = CharField(max_length=255, source='listid')
-    list_email = CharField(max_length=200, source='listemail')
 
-    class Meta:
-        model = models.Project
-        fields = ('id', 'url', 'name', 'link_name', 'list_id', 'list_email',
-                  'web_url', 'scm_url', 'webscm_url')
-        read_only_fields = fields
-        extra_kwargs = {
-            'url': {'view_name': 'api-project-detail'},
-        }
+class UserProfileSerializer(SerializedRelatedField):
 
+    class _Serializer(BaseHyperlinkedModelSerializer):
 
-class SeriesSerializer(MboxMixin, WebURLMixin,
-                       BaseHyperlinkedModelSerializer):
+        username = CharField(source='user.username')
+        first_name = CharField(source='user.first_name')
+        last_name = CharField(source='user.last_name')
+        email = CharField(source='user.email')
 
-    class Meta:
-        model = models.Series
-        fields = ('id', 'url', 'date', 'name', 'version', 'mbox')
-        read_only_fields = fields
-        versioned_field = {
-            '1.1': ('web_url', ),
-        }
-        extra_kwargs = {
-            'url': {'view_name': 'api-series-detail'},
-        }
-
-
-class UserSerializer(BaseHyperlinkedModelSerializer):
-
-    class Meta:
-        model = models.User
-        fields = ('id', 'url', 'username', 'first_name', 'last_name', 'email')
-        read_only_fields = fields
-        extra_kwargs = {
-            'url': {'view_name': 'api-user-detail'},
-        }
-
-
-class UserProfileSerializer(BaseHyperlinkedModelSerializer):
-
-    username = CharField(source='user.username')
-    first_name = CharField(source='user.first_name')
-    last_name = CharField(source='user.last_name')
-    email = CharField(source='user.email')
-
-    class Meta:
-        model = models.UserProfile
-        fields = ('id', 'url', 'username', 'first_name', 'last_name', 'email')
-        read_only_fields = fields
-        extra_kwargs = {
-            'url': {'view_name': 'api-user-detail'},
-        }
+        class Meta:
+            model = models.UserProfile
+            fields = ('id', 'url', 'username', 'first_name', 'last_name',
+                      'email')
+            read_only_fields = fields
+            extra_kwargs = {
+                'url': {'view_name': 'api-user-detail'},
+            }

patchwork/tests/api/test_patch.py

@@ -222,8 +222,7 @@ def test_update(self):
             'state': state.name, 'delegate': user.id})
         self.assertEqual(status.HTTP_200_OK, resp.status_code, resp)
         self.assertEqual(Patch.objects.get(id=patch.id).state, state)
-        # TODO(stephenfin): This is currently broken due to #216
-        # self.assertEqual(Patch.objects.get(id=patch.id).delegate, user)
+        self.assertEqual(Patch.objects.get(id=patch.id).delegate, user)
 
     def test_update_invalid(self):
         """Ensure we handle invalid Patch updates."""
@@ -243,10 +242,9 @@ def test_update_invalid(self):
         user_b = create_user()
         resp = self.client.patch(self.api_url(patch.id),
                                  {'delegate': user_b.id})
-        # TODO(stephenfin): This is currently broken due to #216
-        # self.assertEqual(status.HTTP_400_BAD_REQUEST, resp.status_code)
-        # self.assertContains(resp, "User '%s' is not a maintainer" % user_b,
-        #                     status_code=status.HTTP_400_BAD_REQUEST)
+        self.assertEqual(status.HTTP_400_BAD_REQUEST, resp.status_code)
+        self.assertContains(resp, "User '%s' is not a maintainer" % user_b,
+                            status_code=status.HTTP_400_BAD_REQUEST)
 
     def test_delete(self):
         """Ensure deletions are always rejected."""

releasenotes/notes/issue-216-d3bf9d1baa100f74.yaml

@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    An issue that prevented updating of delegates using the REST API is
+    resolved. (`#216 <https://github.com/getpatchwork/patchwork/issues/216>`__)