Skip to content
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

Fix URL pattern parsing in schema generation #5689

Merged
merged 2 commits into from
Dec 20, 2017
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
10 changes: 9 additions & 1 deletion rest_framework/compat.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
def get_regex_pattern(urlpattern):
if hasattr(urlpattern, 'pattern'):
# Django 2.0
return urlpattern.pattern.regex.pattern
return str(urlpattern.pattern)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this has broken #5672 even further - instead of getting the expected regex it now gets a path/regex hybrid like 'convtest/<int:pk>\\.(?P<format>[a-z0-9]+)/?$' when trying to add suffix patterns.

Copy link
Contributor Author

@tiltec tiltec Dec 20, 2017

Choose a reason for hiding this comment

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

Indeed, str(urlpattern.pattern) is not always a regex pattern anymore. Maybe we need to undo this change and have an if-else in EndpointEnumerator, depending if urlpattern is a RoutePattern or a RegexPattern?

Copy link
Contributor

Choose a reason for hiding this comment

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

That would still leave #5672 broken, just in a different way.

Perhaps renaming the method to something like get_original_route would help avoid confusion, but a separate fix would still be needed for the other issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No problem adding a similar, not quite duplicate, function to compat. (We can always abstract again later.)

else:
# Django < 2.0
return urlpattern.regex.pattern
Expand Down Expand Up @@ -255,6 +255,14 @@ def md_filter_add_syntax_highlight(md):
except ImportError:
InvalidTimeError = Exception

# Django 1.x url routing syntax. Remove when dropping Django 1.11 support.
try:
from django.urls import include, path, re_path # noqa
except ImportError:
from django.conf.urls import include, url # noqa
path = None
re_path = url


# `separators` argument to `json.dumps()` differs between 2.x and 3.x
# See: http://bugs.python.org/issue22767
Expand Down
10 changes: 9 additions & 1 deletion rest_framework/schemas/generators.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

See schemas.__init__.py for package overview.
"""
import re
import warnings
from collections import Counter, OrderedDict
from importlib import import_module
Expand Down Expand Up @@ -135,6 +136,11 @@ def endpoint_ordering(endpoint):
return (path, method_priority)


_PATH_PARAMETER_COMPONENT_RE = re.compile(
r'<(?:(?P<converter>[^>:]+):)?(?P<parameter>\w+)>'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the full regex copied from Django 2. I didn't dare to import it, since it had an underscore in front of it.

)


class EndpointEnumerator(object):
"""
A class to determine the available API endpoints that a project exposes.
Expand Down Expand Up @@ -189,7 +195,9 @@ def get_path_from_regex(self, path_regex):
Given a URL conf regex, return a URI template string.
"""
path = simplify_regex(path_regex)
path = path.replace('<', '{').replace('>', '}')

# Strip Django 2.0 convertors as they are incompatible with uritemplate format
path = re.sub(_PATH_PARAMETER_COMPONENT_RE, r'{\g<parameter>}', path)
return path

def should_include_endpoint(self, path, callback):
Expand Down
55 changes: 54 additions & 1 deletion tests/test_schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from rest_framework import (
filters, generics, pagination, permissions, serializers
)
from rest_framework.compat import coreapi, coreschema, get_regex_pattern
from rest_framework.compat import coreapi, coreschema, get_regex_pattern, path
from rest_framework.decorators import (
api_view, detail_route, list_route, schema
)
Expand Down Expand Up @@ -361,6 +361,59 @@ def test_schema_for_regular_views(self):
assert schema == expected


@unittest.skipUnless(coreapi, 'coreapi is not installed')
@unittest.skipUnless(path, 'needs Django 2')
class TestSchemaGeneratorDjango2(TestCase):
def setUp(self):
self.patterns = [
path('example/', ExampleListView.as_view()),
path('example/<int:pk>/', ExampleDetailView.as_view()),
path('example/<int:pk>/sub/', ExampleDetailView.as_view()),
]

def test_schema_for_regular_views(self):
"""
Ensure that schema generation works for APIView classes.
"""
generator = SchemaGenerator(title='Example API', patterns=self.patterns)
schema = generator.get_schema()
expected = coreapi.Document(
url='',
title='Example API',
content={
'example': {
'create': coreapi.Link(
url='/example/',
action='post',
fields=[]
),
'list': coreapi.Link(
url='/example/',
action='get',
fields=[]
),
'read': coreapi.Link(
url='/example/{id}/',
action='get',
fields=[
coreapi.Field('id', required=True, location='path', schema=coreschema.String())
]
),
'sub': {
'list': coreapi.Link(
url='/example/{id}/sub/',
action='get',
fields=[
coreapi.Field('id', required=True, location='path', schema=coreschema.String())
]
)
}
}
}
)
assert schema == expected


@unittest.skipUnless(coreapi, 'coreapi is not installed')
class TestSchemaGeneratorNotAtRoot(TestCase):
def setUp(self):
Expand Down