From 5c22f9d8fcc72deec7d8beaefc97207ea8fc646d Mon Sep 17 00:00:00 2001 From: Daniel Axtens Date: Sat, 21 Aug 2021 00:57:58 +1000 Subject: [PATCH] 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 [stephenfin: Merge test into bug fix] Signed-off-by: Stephen Finucane Tested-by: Konstantin Ryabitsev Fixes: d944f17ec059 ("REST: Use versioning for modified responses") Closes: #423 --- patchwork/api/base.py | 5 ++++- patchwork/tests/api/test_patch.py | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) 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. -- 2.47.3