From 48a21aa0eb3a95d32456c2a927eff9552a04231e Mon Sep 17 00:00:00 2001 From: David Graves <68711926+dgravitate@users.noreply.github.com> Date: Sat, 10 Dec 2022 10:50:41 -0600 Subject: [PATCH] raise ImproperlyConfigured exception if `basename` is not unique (#8438) * raise ImproperlyConfigured if basename already exists * rename already_registered function; return True/False * additional basename tests * additional basename tests * Update rest_framework/routers.py Co-authored-by: David Graves Co-authored-by: Asif Saif Uddin --- rest_framework/routers.py | 12 ++++ tests/test_routers.py | 112 +++++++++++++++++++++++++++++++++++++- 2 files changed, 123 insertions(+), 1 deletion(-) diff --git a/rest_framework/routers.py b/rest_framework/routers.py index e0ae24b95c..10df22aa31 100644 --- a/rest_framework/routers.py +++ b/rest_framework/routers.py @@ -52,12 +52,24 @@ def __init__(self): def register(self, prefix, viewset, basename=None): if basename is None: basename = self.get_default_basename(viewset) + + if self.is_already_registered(basename): + msg = (f'Router with basename "{basename}" is already registered. ' + f'Please provide a unique basename for viewset "{viewset}"') + raise ImproperlyConfigured(msg) + self.registry.append((prefix, viewset, basename)) # invalidate the urls cache if hasattr(self, '_urls'): del self._urls + def is_already_registered(self, new_basename): + """ + Check if `basename` is already registered + """ + return any(basename == new_basename for _prefix, _viewset, basename in self.registry) + def get_default_basename(self, viewset): """ If `basename` is not specified, attempt to automatically determine diff --git a/tests/test_routers.py b/tests/test_routers.py index f767a843d9..6b006242ae 100644 --- a/tests/test_routers.py +++ b/tests/test_routers.py @@ -21,6 +21,11 @@ class RouterTestModel(models.Model): text = models.CharField(max_length=200) +class BasenameTestModel(models.Model): + uuid = models.CharField(max_length=20) + text = models.CharField(max_length=200) + + class NoteSerializer(serializers.HyperlinkedModelSerializer): url = serializers.HyperlinkedIdentityField(view_name='routertestmodel-detail', lookup_field='uuid') @@ -42,6 +47,11 @@ class KWargedNoteViewSet(viewsets.ModelViewSet): lookup_url_kwarg = 'text' +class BasenameViewSet(viewsets.ModelViewSet): + queryset = BasenameTestModel.objects.all() + serializer_class = None + + class MockViewSet(viewsets.ModelViewSet): queryset = None serializer_class = None @@ -156,7 +166,7 @@ def test_multiple_action_handlers(self): def test_register_after_accessing_urls(self): self.router.register(r'notes', NoteViewSet) assert len(self.router.urls) == 2 # list and detail - self.router.register(r'notes_bis', NoteViewSet) + self.router.register(r'notes_bis', NoteViewSet, basename='notes_bis') assert len(self.router.urls) == 4 @@ -481,3 +491,103 @@ def test_basename(self): initkwargs = match.func.initkwargs assert initkwargs['basename'] == 'routertestmodel' + + +class BasenameTestCase: + def test_conflicting_autogenerated_basenames(self): + """ + Ensure 2 routers with the same model, and no basename specified + throws an ImproperlyConfigured exception + """ + self.router.register(r'notes', NoteViewSet) + + with pytest.raises(ImproperlyConfigured): + self.router.register(r'notes_kwduplicate', KWargedNoteViewSet) + + with pytest.raises(ImproperlyConfigured): + self.router.register(r'notes_duplicate', NoteViewSet) + + def test_conflicting_mixed_basenames(self): + """ + Ensure 2 routers with the same model, and no basename specified on 1 + throws an ImproperlyConfigured exception + """ + self.router.register(r'notes', NoteViewSet) + + with pytest.raises(ImproperlyConfigured): + self.router.register(r'notes_kwduplicate', KWargedNoteViewSet, basename='routertestmodel') + + with pytest.raises(ImproperlyConfigured): + self.router.register(r'notes_duplicate', NoteViewSet, basename='routertestmodel') + + def test_nonconflicting_mixed_basenames(self): + """ + Ensure 2 routers with the same model, and a distinct basename + specified on the second router does not fail + """ + self.router.register(r'notes', NoteViewSet) + self.router.register(r'notes_kwduplicate', KWargedNoteViewSet, basename='routertestmodel_kwduplicate') + self.router.register(r'notes_duplicate', NoteViewSet, basename='routertestmodel_duplicate') + + def test_conflicting_specified_basename(self): + """ + Ensure 2 routers with the same model, and the same basename specified + on both throws an ImproperlyConfigured exception + """ + self.router.register(r'notes', NoteViewSet, basename='notes') + + with pytest.raises(ImproperlyConfigured): + self.router.register(r'notes_kwduplicate', KWargedNoteViewSet, basename='notes') + + with pytest.raises(ImproperlyConfigured): + self.router.register(r'notes_duplicate', KWargedNoteViewSet, basename='notes') + + def test_nonconflicting_specified_basename(self): + """ + Ensure 2 routers with the same model, and a distinct basename specified + on each does not throw an exception + """ + self.router.register(r'notes', NoteViewSet, basename='notes') + self.router.register(r'notes_kwduplicate', KWargedNoteViewSet, basename='notes_kwduplicate') + self.router.register(r'notes_duplicate', NoteViewSet, basename='notes_duplicate') + + def test_nonconflicting_specified_basename_different_models(self): + """ + Ensure 2 routers with different models, and a distinct basename specified + on each does not throw an exception + """ + self.router.register(r'notes', NoteViewSet, basename='notes') + self.router.register(r'notes_basename', BasenameViewSet, basename='notes_basename') + + def test_conflicting_specified_basename_different_models(self): + """ + Ensure 2 routers with different models, and a conflicting basename specified + throws an exception + """ + self.router.register(r'notes', NoteViewSet) + with pytest.raises(ImproperlyConfigured): + self.router.register(r'notes_basename', BasenameViewSet, basename='routertestmodel') + + def test_nonconflicting_autogenerated_basename_different_models(self): + """ + Ensure 2 routers with different models, and a distinct basename specified + on each does not throw an exception + """ + self.router.register(r'notes', NoteViewSet) + self.router.register(r'notes_basename', BasenameViewSet) + + +class TestDuplicateBasenameSimpleRouter(BasenameTestCase, TestCase): + def setUp(self): + self.router = SimpleRouter(trailing_slash=False) + + +class TestDuplicateBasenameDefaultRouter(BasenameTestCase, TestCase): + def setUp(self): + self.router = DefaultRouter() + + +class TestDuplicateBasenameDefaultRouterRootViewName(BasenameTestCase, TestCase): + def setUp(self): + self.router = DefaultRouter() + self.router.root_view_name = 'nameable-root'