From 2cbe3c61beef30158b86210ec72ba0e5eb6dff9c Mon Sep 17 00:00:00 2001 From: David Graves Date: Wed, 30 Mar 2022 23:31:10 -0500 Subject: [PATCH 1/5] raise ImproperlyConfigured if basename already exists --- rest_framework/routers.py | 17 ++++++++++++++++- tests/test_routers.py | 14 ++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/rest_framework/routers.py b/rest_framework/routers.py index e0ae24b95c..3a0eb87e78 100644 --- a/rest_framework/routers.py +++ b/rest_framework/routers.py @@ -52,12 +52,27 @@ def __init__(self): def register(self, prefix, viewset, basename=None): if basename is None: basename = self.get_default_basename(viewset) - self.registry.append((prefix, viewset, basename)) + + if not self.basename_already_registered(basename): + self.registry.append((prefix, viewset, basename)) # invalidate the urls cache if hasattr(self, '_urls'): del self._urls + def basename_already_registered(self, new_basename): + """ + If `basename` is already registered, raise an exception + """ + for route in self.registry: + prefix, viewset, basename = route + if new_basename == basename: + msg = (f'Route with basename "{new_basename}" is already registered. ' + f'Please provide a unique basename for viewset "{viewset}"') + raise ImproperlyConfigured(msg) + + return False + 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..2ef2bd5413 100644 --- a/tests/test_routers.py +++ b/tests/test_routers.py @@ -481,3 +481,17 @@ def test_basename(self): initkwargs = match.func.initkwargs assert initkwargs['basename'] == 'routertestmodel' + + +class TestDuplicateBasename(URLPatternsTestCase, TestCase): + def test_exception_for_duplicate_basename(self): + class NoteViewSet(viewsets.ModelViewSet): + queryset = RouterTestModel.objects.all() + + self.router = SimpleRouter(trailing_slash=False) + self.router.register(r'notes', NoteViewSet) + + with pytest.raises(ImproperlyConfigured): + self.router.register(r'notes_duplicate', NoteViewSet) + + self.router.register(r'notes_duplicate_2', NoteViewSet, basename='note_duplicate') \ No newline at end of file From badd1c1a6f76852fd6ae7ece1de75690f0df4aa1 Mon Sep 17 00:00:00 2001 From: David Graves Date: Wed, 30 Nov 2022 21:22:50 -0600 Subject: [PATCH 2/5] rename already_registered function; return True/False --- rest_framework/routers.py | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/rest_framework/routers.py b/rest_framework/routers.py index 3a0eb87e78..d96ea8cc04 100644 --- a/rest_framework/routers.py +++ b/rest_framework/routers.py @@ -53,25 +53,22 @@ def register(self, prefix, viewset, basename=None): if basename is None: basename = self.get_default_basename(viewset) - if not self.basename_already_registered(basename): - self.registry.append((prefix, viewset, basename)) + if self.is_already_registered(basename): + msg = (f'Route 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 basename_already_registered(self, new_basename): + def is_already_registered(self, new_basename): """ - If `basename` is already registered, raise an exception + Check if `basename` is already registered """ - for route in self.registry: - prefix, viewset, basename = route - if new_basename == basename: - msg = (f'Route with basename "{new_basename}" is already registered. ' - f'Please provide a unique basename for viewset "{viewset}"') - raise ImproperlyConfigured(msg) - - return False + return any(basename == new_basename for _prefix, _viewset, basename in self.registry) def get_default_basename(self, viewset): """ From 91a74068b388c16c87e1700dbf27484922e747cf Mon Sep 17 00:00:00 2001 From: David Graves Date: Wed, 30 Nov 2022 22:14:55 -0600 Subject: [PATCH 3/5] additional basename tests --- tests/test_routers.py | 102 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 95 insertions(+), 7 deletions(-) diff --git a/tests/test_routers.py b/tests/test_routers.py index 2ef2bd5413..aa7d4ca30a 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 @@ -483,15 +493,93 @@ def test_basename(self): assert initkwargs['basename'] == 'routertestmodel' -class TestDuplicateBasename(URLPatternsTestCase, TestCase): - def test_exception_for_duplicate_basename(self): - class NoteViewSet(viewsets.ModelViewSet): - queryset = RouterTestModel.objects.all() - +class TestDuplicateBasename(TestCase): + def test_conflicting_autogenerated_basenames(self): + """ + Ensure 2 routers with the same model, and no basename specified + throws an ImproperlyConfigured exception + """ self.router = SimpleRouter(trailing_slash=False) 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) - self.router.register(r'notes_duplicate_2', NoteViewSet, basename='note_duplicate') \ No newline at end of file + 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 = SimpleRouter(trailing_slash=False) + 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 = SimpleRouter(trailing_slash=False) + 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 = SimpleRouter(trailing_slash=False) + 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 = SimpleRouter(trailing_slash=False) + 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 = SimpleRouter(trailing_slash=False) + 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 = SimpleRouter(trailing_slash=False) + 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 = SimpleRouter(trailing_slash=False) + self.router.register(r'notes', NoteViewSet) + self.router.register(r'notes_basename', BasenameViewSet) From d3954efc9cc15dae5c847f483e0c3bd1585dfb2d Mon Sep 17 00:00:00 2001 From: David Graves Date: Thu, 1 Dec 2022 09:18:19 -0600 Subject: [PATCH 4/5] additional basename tests --- tests/test_routers.py | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/tests/test_routers.py b/tests/test_routers.py index aa7d4ca30a..6b006242ae 100644 --- a/tests/test_routers.py +++ b/tests/test_routers.py @@ -493,13 +493,12 @@ def test_basename(self): assert initkwargs['basename'] == 'routertestmodel' -class TestDuplicateBasename(TestCase): +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 = SimpleRouter(trailing_slash=False) self.router.register(r'notes', NoteViewSet) with pytest.raises(ImproperlyConfigured): @@ -513,7 +512,6 @@ 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 = SimpleRouter(trailing_slash=False) self.router.register(r'notes', NoteViewSet) with pytest.raises(ImproperlyConfigured): @@ -527,7 +525,6 @@ 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 = SimpleRouter(trailing_slash=False) 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') @@ -537,7 +534,6 @@ 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 = SimpleRouter(trailing_slash=False) self.router.register(r'notes', NoteViewSet, basename='notes') with pytest.raises(ImproperlyConfigured): @@ -551,7 +547,6 @@ 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 = SimpleRouter(trailing_slash=False) 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') @@ -561,7 +556,6 @@ 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 = SimpleRouter(trailing_slash=False) self.router.register(r'notes', NoteViewSet, basename='notes') self.router.register(r'notes_basename', BasenameViewSet, basename='notes_basename') @@ -570,7 +564,6 @@ def test_conflicting_specified_basename_different_models(self): Ensure 2 routers with different models, and a conflicting basename specified throws an exception """ - self.router = SimpleRouter(trailing_slash=False) self.router.register(r'notes', NoteViewSet) with pytest.raises(ImproperlyConfigured): self.router.register(r'notes_basename', BasenameViewSet, basename='routertestmodel') @@ -580,6 +573,21 @@ 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 = SimpleRouter(trailing_slash=False) 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' From 2cec7fee208df9ef0095c2bb5aff39ebf37f86df Mon Sep 17 00:00:00 2001 From: Asif Saif Uddin Date: Sat, 10 Dec 2022 22:43:19 +0600 Subject: [PATCH 5/5] Update rest_framework/routers.py --- rest_framework/routers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rest_framework/routers.py b/rest_framework/routers.py index d96ea8cc04..10df22aa31 100644 --- a/rest_framework/routers.py +++ b/rest_framework/routers.py @@ -54,7 +54,7 @@ def register(self, prefix, viewset, basename=None): basename = self.get_default_basename(viewset) if self.is_already_registered(basename): - msg = (f'Route with basename "{basename}" is already registered. ' + msg = (f'Router with basename "{basename}" is already registered. ' f'Please provide a unique basename for viewset "{viewset}"') raise ImproperlyConfigured(msg)