Stephen Finucane <step...@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@def/comments/ Exception Value: invalid literal for int() with base 10: 'abc@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 <step...@that.guru> > Cc: Daniel Axtens <d...@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 _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork