REST: Ensure patch exists for check creation

Signed-off-by: Stephen Finucane <stephen@that.guru>
Closes: #226

diff --git a/patchwork/api/check.py b/patchwork/api/check.py
index 4771455fd808d65eefc76b0ae2c7cb96548ab874..0e35bd455f6314fcbd393f7366f0f6b3b9fe3f8a 100644 (file)
--- a/patchwork/api/check.py
+++ b/patchwork/api/check.py
@@ -3,6 +3,8 @@
 #
 # SPDX-License-Identifier: GPL-2.0-or-later
 
+from django.http import Http404
+from django.shortcuts import get_object_or_404
 from rest_framework.exceptions import PermissionDenied
 from rest_framework.generics import ListCreateAPIView
 from rest_framework.generics import RetrieveAPIView
@@ -70,6 +72,10 @@ class CheckMixin(object):
 
     def get_queryset(self):
         patch_id = self.kwargs['patch_id']
+
+        if not Patch.objects.filter(pk=self.kwargs['patch_id']).exists():
+            raise Http404
+
         return Check.objects.prefetch_related('user').filter(patch=patch_id)
 
 
@@ -86,7 +92,7 @@ class CheckListCreate(CheckMixin, ListCreateAPIView):
     ordering = 'id'
 
     def create(self, request, patch_id, *args, **kwargs):
-        p = Patch.objects.get(id=patch_id)
+        p = get_object_or_404(Patch, id=patch_id)
         if not p.is_editable(request.user):
             raise PermissionDenied()
         request.patch = p

diff --git a/patchwork/tests/api/test_check.py b/patchwork/tests/api/test_check.py
index 783a6154427c115472e384318a24a011b20201aa..25a2a2c4704157334881717681908f0c41948bca 100644 (file)
--- a/patchwork/tests/api/test_check.py
+++ b/patchwork/tests/api/test_check.py
@@ -77,6 +77,12 @@ class TestCheckAPI(APITestCase):
         resp = self.client.get(self.api_url(), {'user': 'otheruser'})
         self.assertEqual(0, len(resp.data))
 
+    def test_list_invalid_patch(self):
+        """Ensure we get a 404 for a non-existent patch."""
+        resp = self.client.get(
+            reverse('api-check-list', kwargs={'patch_id': '99999'}))
+        self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
+
     def test_detail(self):
         """Validate we can get a specific check."""
         check = self._create_check()
@@ -99,12 +105,21 @@ class TestCheckAPI(APITestCase):
         self.assertEqual(1, Check.objects.all().count())
         self.assertSerialized(Check.objects.first(), resp.data)
 
+    def test_create_no_permissions(self):
+        """Ensure creations are rejected by standard users."""
+        check = {
+            'state': 'success',
+            'target_url': 'http://t.co',
+            'description': 'description',
+            'context': 'context',
+        }
+
         user = create_user()
         self.client.force_authenticate(user=user)
         resp = self.client.post(self.api_url(), check)
         self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
 
-    def test_create_invalid(self):
+    def test_create_invalid_state(self):
         """Ensure we handle invalid check states."""
         check = {
             'state': 'this-is-not-a-valid-state',
@@ -118,6 +133,20 @@ class TestCheckAPI(APITestCase):
         self.assertEqual(status.HTTP_400_BAD_REQUEST, resp.status_code)
         self.assertEqual(0, Check.objects.all().count())
 
+    def test_create_invalid_patch(self):
+        """Ensure we handle non-existent patches."""
+        check = {
+            'state': 'success',
+            'target_url': 'http://t.co',
+            'description': 'description',
+            'context': 'context',
+        }
+
+        self.client.force_authenticate(user=self.user)
+        resp = self.client.post(
+            reverse('api-check-list', kwargs={'patch_id': '99999'}), check)
+        self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
+
     def test_update_delete(self):
         """Ensure updates and deletes aren't allowed"""
         check = self._create_check()

diff --git a/releasenotes/notes/issue-226-27ea72266d3ee9ac.yaml b/releasenotes/notes/issue-226-27ea72266d3ee9ac.yaml
new file mode 100644 (file)
index 0000000..8f891e0
--- /dev/null
+++ b/releasenotes/notes/issue-226-27ea72266d3ee9ac.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Showing checks for a non-existant patch was returning an empty response
+    instead of a HTTP 404. Similarly, attempting to create a new check against
+    this patch would result in a HTTP 5xx error instead of a HTTP 404. Both
+    issues are now resolved.