[PATCH] urls: Add missing path converters for REST APIs
Daniel Axtens
dja at axtens.net
Mon Aug 23 14:16:30 AEST 2021
Stephen Finucane <stephen at that.guru> writes:
> Almost all of the API endpoints expect numerical resource IDs, with
> '/projects' being the sole exception. However, we were not actually
> enforcing this anywhere. Instead, we were relying on the custom
> 'get_object_or_404' implementation used by 'GenericAPIView.retrieve' via
> 'GenericAPIView.get_object'. Unfortunately we weren't using this
> everywhere, most notably in our handler for 'GET /patches/{id}/checks'.
> The end result was a HTTP 500 due to a TypeError.
A ValueError, not a TypeError:
Exception Type: ValueError at /api/patches/abc at def/comments/
Exception Value: invalid literal for int() with base 10: 'abc at def'
> Resolve this by adding the path converters for all REST API paths in
> 'patchwork.urls', along with tests to prevent regressions going forward.
> We also switch to the DRF variant of 'get_object_or_404' in some places
> to provide additional protection.
LGTM, applied with a tweaked commit message and the change mentioned below:
>
> Signed-off-by: Stephen Finucane <stephen at that.guru>
> Cc: Daniel Axtens <dja at axtens.net>
> ---
> patchwork/api/base.py | 3 ++-
> patchwork/api/check.py | 7 +++----
> patchwork/tests/api/test_bundle.py | 11 +++++++++++
> patchwork/tests/api/test_comment.py | 19 +++++++++++++++++--
> patchwork/tests/api/test_cover.py | 11 +++++++++++
> patchwork/tests/api/test_patch.py | 11 +++++++++++
> patchwork/tests/api/test_person.py | 14 ++++++++++++++
> patchwork/tests/api/test_project.py | 8 ++++++++
> patchwork/tests/api/test_series.py | 11 +++++++++++
> patchwork/tests/api/test_user.py | 14 ++++++++++++++
> patchwork/urls.py | 20 ++++++++++----------
> 11 files changed, 112 insertions(+), 17 deletions(-)
>
> diff --git patchwork/api/base.py patchwork/api/base.py
> index 6cb703b1..5368c0cb 100644
> --- patchwork/api/base.py
> +++ patchwork/api/base.py
> @@ -59,7 +59,8 @@ class MultipleFieldLookupMixin(object):
> queryset = self.filter_queryset(self.get_queryset())
> filter_kwargs = {}
> for field_name, field in zip(
> - self.lookup_fields, self.lookup_url_kwargs):
> + self.lookup_fields, self.lookup_url_kwargs,
> + ):
AFAICT this is purely a cosmetic change in an otherwise-unmodified file,
so I have dropped it.
> if self.kwargs[field]:
> filter_kwargs[field_name] = self.kwargs[field]
>
The rest of the patch looks good, and the full suite of tox tests pass.*
Kind regards,
Daniel
* no they don't, the docs fail, but that's not because of this series.
> diff --git patchwork/api/check.py patchwork/api/check.py
> index a6bf5f8c..5137c1b9 100644
> --- patchwork/api/check.py
> +++ patchwork/api/check.py
> @@ -3,11 +3,10 @@
> #
> # 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
> import rest_framework
> from rest_framework.exceptions import PermissionDenied
> +from rest_framework.generics import get_object_or_404
> from rest_framework.generics import ListCreateAPIView
> from rest_framework.generics import RetrieveAPIView
> from rest_framework.serializers import CurrentUserDefault
> @@ -95,8 +94,8 @@ 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
> + # ensure the patch exists
> + get_object_or_404(Patch, id=self.kwargs['patch_id'])
>
> return Check.objects.prefetch_related('user').filter(patch=patch_id)
>
> diff --git patchwork/tests/api/test_bundle.py patchwork/tests/api/test_bundle.py
> index 79924486..1ada79c3 100644
> --- patchwork/tests/api/test_bundle.py
> +++ patchwork/tests/api/test_bundle.py
> @@ -6,6 +6,7 @@
> import unittest
>
> from django.conf import settings
> +from django.urls import NoReverseMatch
> from django.urls import reverse
>
> from patchwork.models import Bundle
> @@ -184,6 +185,16 @@ class TestBundleAPI(utils.APITestCase):
> self.assertIn('url', resp.data)
> self.assertNotIn('web_url', resp.data)
>
> + def test_detail_non_existent(self):
> + """Ensure we get a 404 for a non-existent bundle."""
> + resp = self.client.get(self.api_url('999999'))
> + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
> +
> + def test_detail_invalid(self):
> + """Ensure we get a 404 for an invalid bundle ID."""
> + with self.assertRaises(NoReverseMatch):
> + self.client.get(self.api_url('foo'))
> +
> def _test_create_update(self, authenticate=True):
> user = create_user()
> project = create_project()
> diff --git patchwork/tests/api/test_comment.py patchwork/tests/api/test_comment.py
> index 59450d8a..22638d2f 100644
> --- patchwork/tests/api/test_comment.py
> +++ patchwork/tests/api/test_comment.py
> @@ -22,6 +22,7 @@ if settings.ENABLE_REST_API:
>
> @unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> class TestCoverComments(utils.APITestCase):
> +
(I nearly dropped this hunk too, but you're also making other changes in
this file, so I relented and kept it.)
> @staticmethod
> def api_url(cover, version=None):
> kwargs = {}
> @@ -76,12 +77,19 @@ class TestCoverComments(utils.APITestCase):
> with self.assertRaises(NoReverseMatch):
> self.client.get(self.api_url(cover, version='1.0'))
>
> - def test_list_invalid_cover(self):
> + def test_list_non_existent_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)
>
> + def test_list_invalid_cover(self):
> + """Ensure we get a 404 for an invalid cover letter ID."""
> + with self.assertRaises(NoReverseMatch):
> + self.client.get(
> + reverse('api-cover-comment-list', kwargs={'pk': 'foo'}),
> + )
> +
>
> @unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> class TestPatchComments(utils.APITestCase):
> @@ -139,8 +147,15 @@ class TestPatchComments(utils.APITestCase):
> with self.assertRaises(NoReverseMatch):
> self.client.get(self.api_url(patch, version='1.0'))
>
> - def test_list_invalid_patch(self):
> + def test_list_non_existent_patch(self):
> """Ensure we get a 404 for a non-existent patch."""
> resp = self.client.get(
> reverse('api-patch-comment-list', kwargs={'patch_id': '99999'}))
> self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
> +
> + def test_list_invalid_patch(self):
> + """Ensure we get a 404 for an invalid patch ID."""
> + with self.assertRaises(NoReverseMatch):
> + self.client.get(
> + reverse('api-patch-comment-list', kwargs={'patch_id': 'foo'}),
> + )
> diff --git patchwork/tests/api/test_cover.py patchwork/tests/api/test_cover.py
> index 806ee6a4..1c232ddb 100644
> --- patchwork/tests/api/test_cover.py
> +++ patchwork/tests/api/test_cover.py
> @@ -7,6 +7,7 @@ import email.parser
> import unittest
>
> from django.conf import settings
> +from django.urls import NoReverseMatch
> from django.urls import reverse
>
> from patchwork.tests.api import utils
> @@ -169,6 +170,16 @@ class TestCoverAPI(utils.APITestCase):
> self.assertNotIn('web_url', resp.data)
> self.assertNotIn('comments', resp.data)
>
> + def test_detail_non_existent(self):
> + """Ensure we get a 404 for a non-existent cover."""
> + resp = self.client.get(self.api_url('999999'))
> + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
> +
> + def test_detail_invalid(self):
> + """Ensure we get a 404 for an invalid cover ID."""
> + with self.assertRaises(NoReverseMatch):
> + self.client.get(self.api_url('foo'))
> +
> def test_create_update_delete(self):
> user = create_maintainer()
> user.is_superuser = True
> diff --git patchwork/tests/api/test_patch.py patchwork/tests/api/test_patch.py
> index b94ad229..1c616409 100644
> --- patchwork/tests/api/test_patch.py
> +++ patchwork/tests/api/test_patch.py
> @@ -8,6 +8,7 @@ from email.utils import make_msgid
> import unittest
>
> from django.conf import settings
> +from django.urls import NoReverseMatch
> from django.urls import reverse
>
> from patchwork.models import Patch
> @@ -261,6 +262,16 @@ class TestPatchAPI(utils.APITestCase):
> self.assertNotIn('web_url', resp.data)
> self.assertNotIn('comments', resp.data)
>
> + def test_detail_non_existent(self):
> + """Ensure we get a 404 for a non-existent patch."""
> + resp = self.client.get(self.api_url('999999'))
> + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
> +
> + def test_detail_invalid(self):
> + """Ensure we get a 404 for an invalid patch ID."""
> + with self.assertRaises(NoReverseMatch):
> + self.client.get(self.api_url('foo'))
> +
> def test_create(self):
> """Ensure creations are rejected."""
> project = create_project()
> diff --git patchwork/tests/api/test_person.py patchwork/tests/api/test_person.py
> index 2139574b..e9070007 100644
> --- patchwork/tests/api/test_person.py
> +++ patchwork/tests/api/test_person.py
> @@ -6,6 +6,7 @@
> import unittest
>
> from django.conf import settings
> +from django.urls import NoReverseMatch
> from django.urls import reverse
>
> from patchwork.tests.api import utils
> @@ -99,6 +100,19 @@ class TestPersonAPI(utils.APITestCase):
> self.assertEqual(status.HTTP_200_OK, resp.status_code)
> self.assertSerialized(person, resp.data, has_user=True)
>
> + def test_detail_non_existent(self):
> + """Ensure we get a 404 for a non-existent person."""
> + user = create_user(link_person=True)
> + self.client.force_authenticate(user=user)
> +
> + resp = self.client.get(self.api_url('999999'))
> + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
> +
> + def test_detail_invalid(self):
> + """Ensure we get a 404 for an invalid person ID."""
> + with self.assertRaises(NoReverseMatch):
> + self.client.get(self.api_url('foo'))
> +
> def test_create_update_delete(self):
> """Ensure creates, updates and deletes aren't allowed"""
> user = create_maintainer()
> diff --git patchwork/tests/api/test_project.py patchwork/tests/api/test_project.py
> index 5c2fbe12..f2110af9 100644
> --- patchwork/tests/api/test_project.py
> +++ patchwork/tests/api/test_project.py
> @@ -172,6 +172,14 @@ class TestProjectAPI(utils.APITestCase):
> self.assertIn('name', resp.data)
> self.assertNotIn('subject_match', resp.data)
>
> + def test_detail_non_existent(self):
> + """Ensure we get a 404 for a non-existent project."""
> + resp = self.client.get(self.api_url('999999'))
> + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
> +
> + resp = self.client.get(self.api_url('foo'))
> + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
> +
> def test_create(self):
> """Ensure creations are rejected."""
> project = create_project()
> diff --git patchwork/tests/api/test_series.py patchwork/tests/api/test_series.py
> index 491dd616..ef661773 100644
> --- patchwork/tests/api/test_series.py
> +++ patchwork/tests/api/test_series.py
> @@ -6,6 +6,7 @@
> import unittest
>
> from django.conf import settings
> +from django.urls import NoReverseMatch
> from django.urls import reverse
>
> from patchwork.tests.api import utils
> @@ -173,6 +174,16 @@ class TestSeriesAPI(utils.APITestCase):
> self.assertNotIn('mbox', resp.data['cover_letter'])
> self.assertNotIn('web_url', resp.data['patches'][0])
>
> + def test_detail_non_existent(self):
> + """Ensure we get a 404 for a non-existent series."""
> + resp = self.client.get(self.api_url('999999'))
> + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
> +
> + def test_detail_invalid(self):
> + """Ensure we get a 404 for an invalid series ID."""
> + with self.assertRaises(NoReverseMatch):
> + self.client.get(self.api_url('foo'))
> +
> def test_create_update_delete(self):
> """Ensure creates, updates and deletes aren't allowed"""
> user = create_maintainer()
> diff --git patchwork/tests/api/test_user.py patchwork/tests/api/test_user.py
> index af340dfe..9e9008b8 100644
> --- patchwork/tests/api/test_user.py
> +++ patchwork/tests/api/test_user.py
> @@ -6,6 +6,7 @@
> import unittest
>
> from django.conf import settings
> +from django.urls import NoReverseMatch
> from django.urls import reverse
>
> from patchwork.tests.api import utils
> @@ -98,6 +99,19 @@ class TestUserAPI(utils.APITestCase):
> self.assertEqual(status.HTTP_200_OK, resp.status_code)
> self.assertSerialized(user, resp.data, has_settings=True)
>
> + def test_detail_non_existent(self):
> + """Ensure we get a 404 for a non-existent user."""
> + user = create_user()
> +
> + self.client.force_authenticate(user=user)
> + resp = self.client.get(self.api_url('999999'))
> + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
> +
> + def test_detail_invalid(self):
> + """Ensure we get a 404 for an invalid user ID."""
> + with self.assertRaises(NoReverseMatch):
> + self.client.get(self.api_url('foo'))
> +
> @utils.store_samples('users-update-error-forbidden')
> def test_update_anonymous(self):
> """Update user as anonymous user."""
> diff --git patchwork/urls.py patchwork/urls.py
> index 1e6c12ab..34b07e2a 100644
> --- patchwork/urls.py
> +++ patchwork/urls.py
> @@ -249,7 +249,7 @@ if settings.ENABLE_REST_API:
> name='api-user-list',
> ),
> path(
> - 'users/<pk>/',
> + 'users/<int:pk>/',
> api_user_views.UserDetail.as_view(),
> name='api-user-detail',
> ),
> @@ -259,7 +259,7 @@ if settings.ENABLE_REST_API:
> name='api-person-list',
> ),
> path(
> - 'people/<pk>/',
> + 'people/<int:pk>/',
> api_person_views.PersonDetail.as_view(),
> name='api-person-detail',
> ),
> @@ -269,7 +269,7 @@ if settings.ENABLE_REST_API:
> name='api-cover-list',
> ),
> path(
> - 'covers/<pk>/',
> + 'covers/<int:pk>/',
> api_cover_views.CoverDetail.as_view(),
> name='api-cover-detail',
> ),
> @@ -279,17 +279,17 @@ if settings.ENABLE_REST_API:
> name='api-patch-list',
> ),
> path(
> - 'patches/<pk>/',
> + 'patches/<int:pk>/',
> api_patch_views.PatchDetail.as_view(),
> name='api-patch-detail',
> ),
> path(
> - 'patches/<patch_id>/checks/',
> + 'patches/<int:patch_id>/checks/',
> api_check_views.CheckListCreate.as_view(),
> name='api-check-list',
> ),
> path(
> - 'patches/<patch_id>/checks/<check_id>/',
> + 'patches/<int:patch_id>/checks/<int:check_id>/',
> api_check_views.CheckDetail.as_view(),
> name='api-check-detail',
> ),
> @@ -299,7 +299,7 @@ if settings.ENABLE_REST_API:
> name='api-series-list',
> ),
> path(
> - 'series/<pk>/',
> + 'series/<int:pk>/',
> api_series_views.SeriesDetail.as_view(),
> name='api-series-detail',
> ),
> @@ -309,7 +309,7 @@ if settings.ENABLE_REST_API:
> name='api-bundle-list',
> ),
> path(
> - 'bundles/<pk>/',
> + 'bundles/<int:pk>/',
> api_bundle_views.BundleDetail.as_view(),
> name='api-bundle-detail',
> ),
> @@ -332,12 +332,12 @@ if settings.ENABLE_REST_API:
>
> api_1_1_patterns = [
> path(
> - 'patches/<patch_id>/comments/',
> + 'patches/<int:patch_id>/comments/',
> api_comment_views.PatchCommentList.as_view(),
> name='api-patch-comment-list',
> ),
> path(
> - 'covers/<pk>/comments/',
> + 'covers/<int:pk>/comments/',
> api_comment_views.CoverCommentList.as_view(),
> name='api-cover-comment-list',
> ),
> --
> 2.31.1
More information about the Patchwork
mailing list