From: Daniel Axtens <dja@axtens.net>
Date: Fri, 20 Aug 2021 14:57:58 +0000 (+1000)
Subject: REST: Don't error if a versioned field we would remove is absent
X-Git-Tag: v3.1.0~78
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5c22f9d8fcc72deec7d8beaefc97207ea8fc646d;p=thirdparty%2Fpatchwork.git

REST: Don't error if a versioned field we would remove is absent

We remove fields that shouldn't be seen on old versions of the API.
This was done with `pop(field name)`, which will throw an exception
if the named field is absent from the data. However, sometimes if
a patch request is via an old API version, we hit this line without
ever having the field present.

This is odd, but not harmful and we definitely shouldn't 500.

Signed-off-by: Daniel Axtens <dja@axtens.net>
[stephenfin: Merge test into bug fix]
Signed-off-by: Stephen Finucane <stephen@that.guru>
Tested-by: Konstantin Ryabitsev <konstantin@linuxfoundation.org>
Fixes: d944f17ec059 ("REST: Use versioning for modified responses")
Closes: #423
---

diff --git a/patchwork/api/base.py b/patchwork/api/base.py
index 89a43114..6cb703b1 100644
--- a/patchwork/api/base.py
+++ b/patchwork/api/base.py
@@ -96,6 +96,9 @@ class BaseHyperlinkedModelSerializer(HyperlinkedModelSerializer):
             # field was added, we drop it
             if not utils.has_version(request, version):
                 for field in self.Meta.versioned_fields[version]:
-                    data.pop(field)
+                    # After a PATCH with an older API version, we may not see
+                    # these fields. If they don't exist, don't panic, return
+                    # (and then discard) None.
+                    data.pop(field, None)
 
         return data
diff --git a/patchwork/tests/api/test_patch.py b/patchwork/tests/api/test_patch.py
index da2dd6e9..b94ad229 100644
--- a/patchwork/tests/api/test_patch.py
+++ b/patchwork/tests/api/test_patch.py
@@ -334,6 +334,20 @@ class TestPatchAPI(utils.APITestCase):
         self.assertEqual(status.HTTP_200_OK, resp.status_code, resp)
         self.assertIsNone(Patch.objects.get(id=patch.id).delegate)
 
+    def test_update_maintainer_version_1_0(self):
+        """Update patch as maintainer on v1.1."""
+        project = create_project()
+        patch = create_patch(project=project)
+        state = create_state()
+        user = create_maintainer(project)
+
+        self.client.force_authenticate(user=user)
+        resp = self.client.patch(self.api_url(patch.id, version="1.1"),
+                                 {'state': state.slug, 'delegate': user.id})
+        self.assertEqual(status.HTTP_200_OK, resp.status_code, resp)
+        self.assertEqual(Patch.objects.get(id=patch.id).state, state)
+        self.assertEqual(Patch.objects.get(id=patch.id).delegate, user)
+
     @utils.store_samples('patch-update-error-bad-request')
     def test_update_invalid_state(self):
         """Update patch with invalid fields.