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

Conversation

tiltec
Copy link
Contributor

@tiltec tiltec commented Dec 19, 2017

Closes #5675

- Call `str(pattern)` to get non-escaped route
- Strip converters from path to comply with uritemplate format

Fixes encode#5675
@carltongibson
Copy link
Collaborator

carltongibson commented Dec 19, 2017

@encode/django-rest-framework-core-team 3.7.4 is due to go tomorrow. I can review in the morning but @tiltec has capacity to continue work today. Thus if any of you have the capacity to review nowish that would help greatly.

(My initial thought was Looks Good, but it’s all new and I haven’t had time to explore properly.)

@carltongibson
Copy link
Collaborator

carltongibson commented Dec 19, 2017

@tiltec That failure will be the isort change. There’s a weirdness such that isort reports different results for Python 2.7. Revert it and it will pass.

We need to work out why this is. We also need to stop using Python 2.7 to run the lint build. 🙂

@tiltec
Copy link
Contributor Author

tiltec commented Dec 19, 2017

Oh, a lint failure for Py2.7. Let's see where it is.

@carltongibson
I also have time tomorrow, but the earlier, the better!

@@ -4,6 +4,7 @@
import re
from collections import MutableMapping, OrderedDict

import coreapi
Copy link
Collaborator

@carltongibson carltongibson Dec 19, 2017

Choose a reason for hiding this comment

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

This change will be the one causing the failure. Not sure what’s causing the isort inconsistency.

Copy link
Collaborator

@carltongibson carltongibson Dec 19, 2017

Choose a reason for hiding this comment

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

https://travis-ci.org/encode/django-rest-framework/jobs/318806673#L499

It’s reporting for test_schemas too so you’lol need to confirm that, but the test_renderers one is a glitch with isort and Python 2.7 (and possibly our configuration, I’m not sure...🙂)

@carltongibson carltongibson added this to the v3.7.4 milestone Dec 19, 2017
@@ -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

@tiltec tiltec Dec 19, 2017

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.

@tiltec
Copy link
Contributor Author

tiltec commented Dec 19, 2017

lint passed!

Copy link
Collaborator

@carltongibson carltongibson left a comment

OK. This looks great. Lets have it.

@tiltec Thanks for the timely input! 💃🏼

@carltongibson carltongibson merged commit d38b94f into encode:master Dec 20, 2017
1 check passed
Copy link
Contributor

@axnsan12 axnsan12 left a comment

I'm trying to find a fix just now

@@ -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

@axnsan12 axnsan12 Dec 20, 2017

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

@axnsan12 axnsan12 Dec 20, 2017

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

@carltongibson carltongibson Dec 20, 2017

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.)

@carltongibson
Copy link
Collaborator

carltongibson commented Dec 20, 2017

@axnsan12 I'm rolling the release as we speak. I'll hit pause. Be very happy to take extra input here. (Just own this or helping #5672 too!)

I saw you committed a test. Fancy begging a PR with that? (It makes it public, which always helps.)

@carltongibson
Copy link
Collaborator

carltongibson commented Dec 20, 2017

Well done! #5691 🙂

@axnsan12
Copy link
Contributor

axnsan12 commented Dec 20, 2017

@carltongibson just a test for now; I need to think a bit about possible ways to fix it and more tests/edge cases

@carltongibson
Copy link
Collaborator

carltongibson commented Dec 20, 2017

@axnsan12 All good! 😉

pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
* Fix url parsing in schema generation

- Call `str(pattern)` to get non-escaped route
- Strip converters from path to comply with uritemplate format. 
   Background: encode#5675 (comment)

Fixes encode#5675
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants