[PATCH 2/2] REST: Allow setting of values using embedded serializers
Stephen Finucane
stephen at that.guru
Sun Oct 14 02:46:15 AEDT 2018
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 at that.guru>
Closes: #216
---
I'm planning to backport this to stable/2.1 and stable/2.0. Technically
this is a change in behavior but as this was a bug, I think it's a
reasonable fix.
---
patchwork/api/embedded.py | 272 +++++++++++-------
patchwork/tests/api/test_patch.py | 10 +-
...fix-patch-delegation-74d7d90f0fb55989.yaml | 5 +
3 files changed, 174 insertions(+), 113 deletions(-)
create mode 100644 releasenotes/notes/fix-patch-delegation-74d7d90f0fb55989.yaml
diff --git a/patchwork/api/embedded.py b/patchwork/api/embedded.py
index be61cd5f..83df0e50 100644
--- a/patchwork/api/embedded.py
+++ b/patchwork/api/embedded.py
@@ -9,14 +9,54 @@ A collection of serializers. None of the serializers here should reference
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.
@@ -41,132 +81,150 @@ class WebURLMixin(BaseHyperlinkedModelSerializer):
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'},
+ }
diff --git a/patchwork/tests/api/test_patch.py b/patchwork/tests/api/test_patch.py
index 53099256..497cb2de 100644
--- a/patchwork/tests/api/test_patch.py
+++ b/patchwork/tests/api/test_patch.py
@@ -208,8 +208,7 @@ class TestPatchAPI(APITestCase):
'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."""
@@ -229,10 +228,9 @@ class TestPatchAPI(APITestCase):
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."""
diff --git a/releasenotes/notes/fix-patch-delegation-74d7d90f0fb55989.yaml b/releasenotes/notes/fix-patch-delegation-74d7d90f0fb55989.yaml
new file mode 100644
index 00000000..c3756aa0
--- /dev/null
+++ b/releasenotes/notes/fix-patch-delegation-74d7d90f0fb55989.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>`__)
--
2.17.1
More information about the Patchwork
mailing list