REST: Handle regular form data requests for checks

08d1459a4a40 ("Add REST API validation using OpenAPI schema") moved
all API requests to JSON blobs rather than form data.

dc48fbce99ef ("REST: Handle JSON requests") attempted to change the
check serialiser to handle this. However, because both a JSON dict
and a QueryDict satisfy isinstance(data, dict), everything was handled
as JSON and the old style requests were broken.

Found in the process of debugging issues from the OzLabs PW & Snowpatch
crew - I'm not sure if they actually hit this one, but kudos to them
anyway as we wouldn't have found it without them.

Fixes: dc48fbce99ef ("REST: Handle JSON requests")
Signed-off-by: Daniel Axtens <dja@axtens.net>

diff --git a/patchwork/api/check.py b/patchwork/api/check.py
index 1f9fe06866a2f246820fd6d1241fa439ffda381b..4d2181d0a04b1f79445e9d9f404bd7792f5c18fe 100644 (file)
--- a/patchwork/api/check.py
+++ b/patchwork/api/check.py
@@ -4,6 +4,7 @@
 # SPDX-License-Identifier: GPL-2.0-or-later
 
 from django.http import Http404
+from django.http.request import QueryDict
 from django.shortcuts import get_object_or_404
 from rest_framework.exceptions import PermissionDenied
 from rest_framework.generics import ListCreateAPIView
@@ -39,9 +40,7 @@ class CheckSerializer(HyperlinkedModelSerializer):
             if label != data['state']:
                 continue
 
-            if isinstance(data, dict):  # json request
-                data['state'] = val
-            else:  # form-data request
+            if isinstance(data, QueryDict):  # form-data request
                 # NOTE(stephenfin): 'data' is essentially 'request.POST', which
                 # is immutable by default. However, there's no good reason for
                 # this to be this way [1], so temporarily unset that mutability
@@ -52,6 +51,8 @@ class CheckSerializer(HyperlinkedModelSerializer):
                 data._mutable = True  # noqa
                 data['state'] = val
                 data._mutable = mutable  # noqa
+            else:  # json request
+                data['state'] = val
 
             break
         return super(CheckSerializer, self).run_validation(data)

diff --git a/patchwork/tests/api/test_check.py b/patchwork/tests/api/test_check.py
index 0c10b94553d30afb3c097aa82b08bbb12af44e5e..1cfdff6e757b7f32bd55d3ca12dbd4b28c7ab503 100644 (file)
--- a/patchwork/tests/api/test_check.py
+++ b/patchwork/tests/api/test_check.py
@@ -18,6 +18,10 @@ from patchwork.tests.utils import create_user
 
 if settings.ENABLE_REST_API:
     from rest_framework import status
+    from rest_framework.test import APITestCase as BaseAPITestCase
+else:
+    # stub out APITestCase
+    from django.test import TestCase as BaseAPITestCase
 
 
 @unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
@@ -174,3 +178,66 @@ class TestCheckAPI(utils.APITestCase):
 
         resp = self.client.delete(self.api_url(check))
         self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED, resp.status_code)
+
+
+@unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
+class TestCheckAPIMultipart(BaseAPITestCase):
+    """Test a minimal subset of functionality where the data is passed as
+    multipart form data rather than as a JSON blob.
+
+    We focus on the POST path exclusively and only on state validation:
+    everything else should be handled in the JSON tests.
+
+    This is required due to the difference in handling JSON vs form-data in
+    CheckSerializer's run_validation().
+    """
+    fixtures = ['default_tags']
+
+    def setUp(self):
+        super(TestCheckAPIMultipart, self).setUp()
+        project = create_project()
+        self.user = create_maintainer(project)
+        self.patch = create_patch(project=project)
+
+    def assertSerialized(self, check_obj, check_json):
+        self.assertEqual(check_obj.id, check_json['id'])
+        self.assertEqual(check_obj.get_state_display(), check_json['state'])
+        self.assertEqual(check_obj.target_url, check_json['target_url'])
+        self.assertEqual(check_obj.context, check_json['context'])
+        self.assertEqual(check_obj.description, check_json['description'])
+        self.assertEqual(check_obj.user.id, check_json['user']['id'])
+
+    def _test_create(self, user, state='success'):
+        check = {
+            'target_url': 'http://t.co',
+            'description': 'description',
+            'context': 'context',
+        }
+        if state is not None:
+            check['state'] = state
+
+        self.client.force_authenticate(user=user)
+        return self.client.post(
+            reverse('api-check-list', args=[self.patch.id]),
+            check)
+
+    def test_creates(self):
+        """Create a set of checks.
+        """
+        resp = self._test_create(user=self.user)
+        self.assertEqual(status.HTTP_201_CREATED, resp.status_code)
+        self.assertEqual(1, Check.objects.all().count())
+        self.assertSerialized(Check.objects.last(), resp.data)
+
+        resp = self._test_create(user=self.user, state='pending')
+        self.assertEqual(status.HTTP_201_CREATED, resp.status_code)
+        self.assertEqual(2, Check.objects.all().count())
+        self.assertSerialized(Check.objects.last(), resp.data)
+
+        # you can also use the numeric ID of the state, the API explorer does
+        resp = self._test_create(user=self.user, state=2)
+        self.assertEqual(status.HTTP_201_CREATED, resp.status_code)
+        self.assertEqual(3, Check.objects.all().count())
+        # we check against the string version
+        resp.data['state'] = 'warning'
+        self.assertSerialized(Check.objects.last(), resp.data)