diff mbox series

[03/17] REST: Ensure patch exists for check creation

Message ID 20181030111916.7342-4-stephen@that.guru
State Accepted
Headers show
Series Add OpenAPI 3.0.0 REST API spec | expand

Commit Message

Stephen Finucane Oct. 30, 2018, 11:19 a.m. UTC 
Signed-off-by: Stephen Finucane <stephen@that.guru>
Closes: #226
---
 patchwork/api/check.py                        |  8 ++++-
 patchwork/tests/api/test_check.py             | 31 ++++++++++++++++++-
 .../notes/issue-226-27ea72266d3ee9ac.yaml     |  7 +++++
 3 files changed, 44 insertions(+), 2 deletions(-)
 create mode 100644 releasenotes/notes/issue-226-27ea72266d3ee9ac.yaml
diff mbox series

Patch

diff --git a/patchwork/api/check.py b/patchwork/api/check.py
index 4771455f..0e35bd45 100644
--- 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 783a6154..06255957 100644
--- 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={'pk': '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={'pk': '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
index 00000000..8f891e04
--- /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.