Skip to content

Add SimplePathRouter #6789

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jan 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion docs/api-guide/routers.md
Original file line number Diff line number Diff line change
Expand Up @@ -167,12 +167,23 @@ This behavior can be modified by setting the `trailing_slash` argument to `False

Trailing slashes are conventional in Django, but are not used by default in some other frameworks such as Rails. Which style you choose to use is largely a matter of preference, although some javascript frameworks may expect a particular routing style.

The router will match lookup values containing any characters except slashes and period characters. For a more restrictive (or lenient) lookup pattern, set the `lookup_value_regex` attribute on the viewset. For example, you can limit the lookup to valid UUIDs:
By default the URLs created by `SimpleRouter` use regular expressions. This behavior can be modified by setting the `use_regex_path` argument to `False` when instantiating the router, in this case [path converters][path-converters-topic-reference] are used. For example:

router = SimpleRouter(use_regex_path=False)

**Note**: `use_regex_path=False` only works with Django 2.x or above, since this feature was introduced in 2.0.0. See [release note][simplified-routing-release-note]


The router will match lookup values containing any characters except slashes and period characters. For a more restrictive (or lenient) lookup pattern, set the `lookup_value_regex` attribute on the viewset or `lookup_value_converter` if using path converters. For example, you can limit the lookup to valid UUIDs:

class MyModelViewSet(mixins.RetrieveModelMixin, viewsets.GenericViewSet):
lookup_field = 'my_model_id'
lookup_value_regex = '[0-9a-f]{32}'

class MyPathModelViewSet(mixins.RetrieveModelMixin, viewsets.GenericViewSet):
lookup_field = 'my_model_uuid'
lookup_value_converter = 'uuid'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use better name for lookup_value_converter?any better suited name instead of converter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used "converter" since django calls these elements path converters, maybe we can use the full expanded name to have lookup_value_pathconverter (or lookup_value_path_converter).

Any suggestion is welcome.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would inclined to @tomchristie for a better insight here as I'm bit confused about naming

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about just lookup_value_path? this alighned with the name lookup_value_regex

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it would be better to use the same naming of django.

In this case it was named "converter" thus an attribute which should define one should be named after that. In this way it is easier to find references to what this element does.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I am going to merge this, we can improve it if needed


## DefaultRouter

This router is similar to `SimpleRouter` as above, but additionally includes a default API root view, that returns a response containing hyperlinks to all the list views. It also generates routes for optional `.json` style format suffixes.
Expand Down Expand Up @@ -340,3 +351,5 @@ The [`DRF-extensions` package][drf-extensions] provides [routers][drf-extensions
[drf-extensions-customizable-endpoint-names]: https://chibisov.github.io/drf-extensions/docs/#controller-endpoint-name
[url-namespace-docs]: https://docs.djangoproject.com/en/4.0/topics/http/urls/#url-namespaces
[include-api-reference]: https://docs.djangoproject.com/en/4.0/ref/urls/#include
[simplified-routing-release-note]: https://docs.djangoproject.com/en/2.0/releases/2.0/#simplified-url-routing-syntax
[path-converters-topic-reference]: https://docs.djangoproject.com/en/2.0/topics/http/urls/#path-converters
48 changes: 39 additions & 9 deletions rest_framework/routers.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from collections import OrderedDict, namedtuple

from django.core.exceptions import ImproperlyConfigured
from django.urls import NoReverseMatch, re_path
from django.urls import NoReverseMatch, path, re_path

from rest_framework import views
from rest_framework.response import Response
Expand Down Expand Up @@ -123,8 +123,29 @@ class SimpleRouter(BaseRouter):
),
]

def __init__(self, trailing_slash=True):
def __init__(self, trailing_slash=True, use_regex_path=True):
self.trailing_slash = '/' if trailing_slash else ''
self._use_regex = use_regex_path
if use_regex_path:
self._base_pattern = '(?P<{lookup_prefix}{lookup_url_kwarg}>{lookup_value})'
self._default_value_pattern = '[^/.]+'
self._url_conf = re_path
else:
self._base_pattern = '<{lookup_value}:{lookup_prefix}{lookup_url_kwarg}>'
self._default_value_pattern = 'path'
self._url_conf = path
# remove regex characters from routes
_routes = []
for route in self.routes:
url_param = route.url
if url_param[0] == '^':
url_param = url_param[1:]
if url_param[-1] == '$':
url_param = url_param[:-1]

_routes.append(route._replace(url=url_param))
self.routes = _routes

super().__init__()

def get_default_basename(self, viewset):
Expand Down Expand Up @@ -213,13 +234,18 @@ def get_lookup_regex(self, viewset, lookup_prefix=''):

https://github.com/alanjds/drf-nested-routers
"""
base_regex = '(?P<{lookup_prefix}{lookup_url_kwarg}>{lookup_value})'
# Use `pk` as default field, unset set. Default regex should not
# consume `.json` style suffixes and should break at '/' boundaries.
lookup_field = getattr(viewset, 'lookup_field', 'pk')
lookup_url_kwarg = getattr(viewset, 'lookup_url_kwarg', None) or lookup_field
lookup_value = getattr(viewset, 'lookup_value_regex', '[^/.]+')
return base_regex.format(
lookup_value = None
if not self._use_regex:
# try to get a more appropriate attribute when not using regex
lookup_value = getattr(viewset, 'lookup_value_converter', None)
if lookup_value is None:
# fallback to legacy
lookup_value = getattr(viewset, 'lookup_value_regex', self._default_value_pattern)
return self._base_pattern.format(
lookup_prefix=lookup_prefix,
lookup_url_kwarg=lookup_url_kwarg,
lookup_value=lookup_value
Expand Down Expand Up @@ -253,8 +279,12 @@ def get_urls(self):
# controlled by project's urls.py and the router is in an app,
# so a slash in the beginning will (A) cause Django to give
# warnings and (B) generate URLS that will require using '//'.
if not prefix and regex[:2] == '^/':
regex = '^' + regex[2:]
if not prefix:
if self._url_conf is path:
if regex[0] == '/':
regex = regex[1:]
elif regex[:2] == '^/':
regex = '^' + regex[2:]

initkwargs = route.initkwargs.copy()
initkwargs.update({
Expand All @@ -264,7 +294,7 @@ def get_urls(self):

view = viewset.as_view(mapping, **initkwargs)
name = route.name.format(basename=basename)
ret.append(re_path(regex, view, name=name))
ret.append(self._url_conf(regex, view, name=name))

return ret

Expand Down Expand Up @@ -339,7 +369,7 @@ def get_urls(self):

if self.include_root_view:
view = self.get_api_root_view(api_urls=urls)
root_url = re_path(r'^$', view, name=self.root_view_name)
root_url = path('', view, name=self.root_view_name)
urls.append(root_url)

if self.include_format_suffixes:
Expand Down
101 changes: 100 additions & 1 deletion tests/test_routers.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
from rest_framework.decorators import action
from rest_framework.response import Response
from rest_framework.routers import DefaultRouter, SimpleRouter
from rest_framework.test import APIRequestFactory, URLPatternsTestCase
from rest_framework.test import (
APIClient, APIRequestFactory, URLPatternsTestCase
)
from rest_framework.utils import json

factory = APIRequestFactory()
Expand Down Expand Up @@ -75,9 +77,28 @@ def regex_url_path_detail(self, request, *args, **kwargs):
return Response({'pk': pk, 'kwarg': kwarg})


class UrlPathViewSet(viewsets.ViewSet):
@action(detail=False, url_path='list/<int:kwarg>')
def url_path_list(self, request, *args, **kwargs):
kwarg = self.kwargs.get('kwarg', '')
return Response({'kwarg': kwarg})

@action(detail=True, url_path='detail/<int:kwarg>')
def url_path_detail(self, request, *args, **kwargs):
pk = self.kwargs.get('pk', '')
kwarg = self.kwargs.get('kwarg', '')
return Response({'pk': pk, 'kwarg': kwarg})


notes_router = SimpleRouter()
notes_router.register(r'notes', NoteViewSet)

notes_path_router = SimpleRouter(use_regex_path=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add another test case which use DeafultRouter please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should not be a problem to replicate that test also with the other router.

notes_path_router.register('notes', NoteViewSet)

notes_path_default_router = DefaultRouter(use_regex_path=False)
notes_path_default_router.register('notes', NoteViewSet)

kwarged_notes_router = SimpleRouter()
kwarged_notes_router.register(r'notes', KWargedNoteViewSet)

Expand All @@ -90,6 +111,9 @@ def regex_url_path_detail(self, request, *args, **kwargs):
regex_url_path_router = SimpleRouter()
regex_url_path_router.register(r'', RegexUrlPathViewSet, basename='regex')

url_path_router = SimpleRouter(use_regex_path=False)
url_path_router.register('', UrlPathViewSet, basename='path')


class BasicViewSet(viewsets.ViewSet):
def list(self, request, *args, **kwargs):
Expand Down Expand Up @@ -459,6 +483,81 @@ def test_regex_url_path_detail(self):
assert json.loads(response.content.decode()) == {'pk': pk, 'kwarg': kwarg}


class TestUrlPath(URLPatternsTestCase, TestCase):
client_class = APIClient
urlpatterns = [
path('path/', include(url_path_router.urls)),
path('default/', include(notes_path_default_router.urls)),
path('example/', include(notes_path_router.urls)),
]

def setUp(self):
RouterTestModel.objects.create(uuid='123', text='foo bar')
RouterTestModel.objects.create(uuid='a b', text='baz qux')

def test_create(self):
new_note = {
'uuid': 'foo',
'text': 'example'
}
response = self.client.post('/example/notes/', data=new_note)
assert response.status_code == 201
assert response['location'] == 'http://testserver/example/notes/foo/'
assert response.data == {"url": "http://testserver/example/notes/foo/", "uuid": "foo", "text": "example"}
assert RouterTestModel.objects.filter(uuid='foo').exists()

def test_retrieve(self):
for url in ('/example/notes/123/', '/default/notes/123/'):
with self.subTest(url=url):
response = self.client.get(url)
assert response.status_code == 200
# only gets example path since was the last to be registered
assert response.data == {"url": "http://testserver/example/notes/123/", "uuid": "123", "text": "foo bar"}

def test_list(self):
for url in ('/example/notes/', '/default/notes/'):
with self.subTest(url=url):
response = self.client.get(url)
assert response.status_code == 200
# only gets example path since was the last to be registered
assert response.data == [
{"url": "http://testserver/example/notes/123/", "uuid": "123", "text": "foo bar"},
{"url": "http://testserver/example/notes/a%20b/", "uuid": "a b", "text": "baz qux"},
]

def test_update(self):
updated_note = {
'text': 'foo bar example'
}
response = self.client.patch('/example/notes/123/', data=updated_note)
assert response.status_code == 200
assert response.data == {"url": "http://testserver/example/notes/123/", "uuid": "123", "text": "foo bar example"}

def test_delete(self):
response = self.client.delete('/example/notes/123/')
assert response.status_code == 204
assert not RouterTestModel.objects.filter(uuid='123').exists()

def test_list_extra_action(self):
kwarg = 1234
response = self.client.get('/path/list/{}/'.format(kwarg))
assert response.status_code == 200
assert json.loads(response.content.decode()) == {'kwarg': kwarg}

def test_detail_extra_action(self):
pk = '1'
kwarg = 1234
response = self.client.get('/path/{}/detail/{}/'.format(pk, kwarg))
assert response.status_code == 200
assert json.loads(response.content.decode()) == {'pk': pk, 'kwarg': kwarg}

def test_defaultrouter_root(self):
response = self.client.get('/default/')
assert response.status_code == 200
# only gets example path since was the last to be registered
assert response.data == {"notes": "http://testserver/example/notes/"}


class TestViewInitkwargs(URLPatternsTestCase, TestCase):
urlpatterns = [
path('example/', include(notes_router.urls)),
Expand Down