[04/17] REST: Ensure submission exists for comment listing

Message ID	20181030111916.7342-5-stephen@that.guru
State	Accepted
Headers	show Return-Path: <patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org> From: Stephen Finucane <stephen@that.guru> To: patchwork@lists.ozlabs.org Subject: [PATCH 04/17] REST: Ensure submission exists for comment listing Date: Tue, 30 Oct 2018 11:19:03 +0000 Message-Id: <20181030111916.7342-5-stephen@that.guru> In-Reply-To: <20181030111916.7342-1-stephen@that.guru> References: <20181030111916.7342-1-stephen@that.guru> Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "Patchwork" <patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org>
Series	Add OpenAPI 3.0.0 REST API spec \| expand [00/17] Add OpenAPI 3.0.0 REST API spec [01/17] REST: Add additional documentation [02/17] REST: Show 'web_url' in embedded series responses [03/17] REST: Ensure patch exists for check creation [04/17] REST: Ensure submission exists for comment listing [05/17] docs: Start documenting API using OpenAPI [06/17] docs: Document the '/users' resource [07/17] docs: Document the '/people' resource [08/17] docs: Document the '/projects' resource [09/17] docs: Document the '/bundles' resource [10/17] docs: Document the '/covers' resource [11/17] docs: Document the '/patches' resource [12/17] docs: Document the '/series' resource [13/17] docs: Document the '/covers/{coverID}/comments' resource [14/17] docs: Document the '/patch/{patchID}/comments' resource [15/17] docs: Document the '/patches/{patchID}/checks' resource [16/17] docs: Document the '/events' resource [17/17] docs: Make API document versioned

Message ID

20181030111916.7342-5-stephen@that.guru

State

Accepted

Headers

From: Stephen Finucane <stephen@that.guru>
To: patchwork@lists.ozlabs.org
Subject: [PATCH 04/17] REST: Ensure submission exists for comment listing
Date: Tue, 30 Oct 2018 11:19:03 +0000
Message-Id: <20181030111916.7342-5-stephen@that.guru>
In-Reply-To: <20181030111916.7342-1-stephen@that.guru>
References: <20181030111916.7342-1-stephen@that.guru>
Precedence: list
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org
Sender: "Patchwork"
	<patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org>

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: #225
---
 patchwork/api/comment.py                           |  5 +++++
 patchwork/tests/api/test_comment.py                | 13 +++++++++++++
 releasenotes/notes/issue-225-94215600c1b23f6e.yaml |  6 ++++++
 3 files changed, 24 insertions(+)
 create mode 100644 releasenotes/notes/issue-225-94215600c1b23f6e.yaml

Comments

Daniel Axtens Nov. 8, 2018, 1:19 p.m. UTC | #1

LGTM.

Regards,
Daniel

Stephen Finucane <stephen@that.guru> writes:

> Signed-off-by: Stephen Finucane <stephen@that.guru>
> Closes: #225
> ---
>  patchwork/api/comment.py                           |  5 +++++
>  patchwork/tests/api/test_comment.py                | 13 +++++++++++++
>  releasenotes/notes/issue-225-94215600c1b23f6e.yaml |  6 ++++++
>  3 files changed, 24 insertions(+)
>  create mode 100644 releasenotes/notes/issue-225-94215600c1b23f6e.yaml
>
> diff --git a/patchwork/api/comment.py b/patchwork/api/comment.py
> index 214a9438..57b37111 100644
> --- a/patchwork/api/comment.py
> +++ b/patchwork/api/comment.py
> @@ -5,6 +5,7 @@
>  
>  import email.parser
>  
> +from django.http import Http404
>  from rest_framework.generics import ListAPIView
>  from rest_framework.serializers import SerializerMethodField
>  
> @@ -12,6 +13,7 @@ from patchwork.api.base import BaseHyperlinkedModelSerializer
>  from patchwork.api.base import PatchworkPermission
>  from patchwork.api.embedded import PersonSerializer
>  from patchwork.models import Comment
> +from patchwork.models import Submission
>  
>  
>  class CommentListSerializer(BaseHyperlinkedModelSerializer):
> @@ -64,6 +66,9 @@ class CommentList(ListAPIView):
>      lookup_url_kwarg = 'pk'
>  
>      def get_queryset(self):
> +        if not Submission.objects.filter(pk=self.kwargs['pk']).exists():
> +            raise Http404
> +
>          return Comment.objects.filter(
>              submission=self.kwargs['pk']
>          ).select_related('submitter')
> diff --git a/patchwork/tests/api/test_comment.py b/patchwork/tests/api/test_comment.py
> index a0aec594..06fe2de3 100644
> --- a/patchwork/tests/api/test_comment.py
> +++ b/patchwork/tests/api/test_comment.py
> @@ -5,6 +5,7 @@
>  
>  import unittest
>  
> +from django.http import Http404
>  from django.conf import settings
>  from django.urls import NoReverseMatch
>  from django.urls import reverse
> @@ -61,6 +62,12 @@ class TestCoverComments(APITestCase):
>          with self.assertRaises(NoReverseMatch):
>              self.client.get(self.api_url(cover_obj, version='1.0'))
>  
> +    def test_list_invalid_cover(self):
> +        """Ensure we get a 404 for a non-existent cover letter."""
> +        resp = self.client.get(
> +            reverse('api-cover-comment-list', kwargs={'pk': '99999'}))
> +        self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
> +
>  
>  @unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
>  class TestPatchComments(APITestCase):
> @@ -99,3 +106,9 @@ class TestPatchComments(APITestCase):
>          # check we can't access comments using the old version of the API
>          with self.assertRaises(NoReverseMatch):
>              self.client.get(self.api_url(patch_obj, version='1.0'))
> +
> +    def test_list_invalid_patch(self):
> +        """Ensure we get a 404 for a non-existent patch."""
> +        resp = self.client.get(
> +            reverse('api-patch-comment-list', kwargs={'pk': '99999'}))
> +        self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
> diff --git a/releasenotes/notes/issue-225-94215600c1b23f6e.yaml b/releasenotes/notes/issue-225-94215600c1b23f6e.yaml
> new file mode 100644
> index 00000000..035e38d8
> --- /dev/null
> +++ b/releasenotes/notes/issue-225-94215600c1b23f6e.yaml
> @@ -0,0 +1,6 @@
> +---
> +fixes:
> +  - |
> +    Showing comments for a non-existant patch or cover letter was returning an
> +    empty response instead of a HTTP 404. This issue is resolved for both
> +    resources.
> -- 
> 2.17.2
>
> _______________________________________________
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork

diff --git a/patchwork/api/comment.py b/patchwork/api/comment.py
index 214a9438..57b37111 100644
--- a/patchwork/api/comment.py
+++ b/patchwork/api/comment.py
@@ -5,6 +5,7 @@ 
 
 import email.parser
 
+from django.http import Http404
 from rest_framework.generics import ListAPIView
 from rest_framework.serializers import SerializerMethodField
 
@@ -12,6 +13,7 @@  from patchwork.api.base import BaseHyperlinkedModelSerializer
 from patchwork.api.base import PatchworkPermission
 from patchwork.api.embedded import PersonSerializer
 from patchwork.models import Comment
+from patchwork.models import Submission
 
 
 class CommentListSerializer(BaseHyperlinkedModelSerializer):
@@ -64,6 +66,9 @@  class CommentList(ListAPIView):
     lookup_url_kwarg = 'pk'
 
     def get_queryset(self):
+        if not Submission.objects.filter(pk=self.kwargs['pk']).exists():
+            raise Http404
+
         return Comment.objects.filter(
             submission=self.kwargs['pk']
         ).select_related('submitter')
diff --git a/patchwork/tests/api/test_comment.py b/patchwork/tests/api/test_comment.py
index a0aec594..06fe2de3 100644
--- a/patchwork/tests/api/test_comment.py
+++ b/patchwork/tests/api/test_comment.py
@@ -5,6 +5,7 @@ 
 
 import unittest
 
+from django.http import Http404
 from django.conf import settings
 from django.urls import NoReverseMatch
 from django.urls import reverse
@@ -61,6 +62,12 @@  class TestCoverComments(APITestCase):
         with self.assertRaises(NoReverseMatch):
             self.client.get(self.api_url(cover_obj, version='1.0'))
 
+    def test_list_invalid_cover(self):
+        """Ensure we get a 404 for a non-existent cover letter."""
+        resp = self.client.get(
+            reverse('api-cover-comment-list', kwargs={'pk': '99999'}))
+        self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
+
 
 @unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
 class TestPatchComments(APITestCase):
@@ -99,3 +106,9 @@  class TestPatchComments(APITestCase):
         # check we can't access comments using the old version of the API
         with self.assertRaises(NoReverseMatch):
             self.client.get(self.api_url(patch_obj, version='1.0'))
+
+    def test_list_invalid_patch(self):
+        """Ensure we get a 404 for a non-existent patch."""
+        resp = self.client.get(
+            reverse('api-patch-comment-list', kwargs={'pk': '99999'}))
+        self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
diff --git a/releasenotes/notes/issue-225-94215600c1b23f6e.yaml b/releasenotes/notes/issue-225-94215600c1b23f6e.yaml
new file mode 100644
index 00000000..035e38d8
--- /dev/null
+++ b/releasenotes/notes/issue-225-94215600c1b23f6e.yaml
@@ -0,0 +1,6 @@ 
+---
+fixes:
+  - |
+    Showing comments for a non-existant patch or cover letter was returning an
+    empty response instead of a HTTP 404. This issue is resolved for both
+    resources.