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

Backslashes added to paths at docs from include_docs_urls #5675

Closed
4 of 6 tasks
scardine opened this issue Dec 17, 2017 · 14 comments
Closed
4 of 6 tasks

Backslashes added to paths at docs from include_docs_urls #5675

scardine opened this issue Dec 17, 2017 · 14 comments
Labels

Comments

@scardine
Copy link
Contributor

scardine commented Dec 17, 2017

Checklist

  • I have verified that that issue exists against the master branch of Django REST framework.
  • I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
  • This is not a usage question. (Those should be directed to the discussion group instead.)
  • This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third party libraries where possible.)
  • I have reduced the issue to the simplest possible case.
  • I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.)

Steps to reproduce

Using DRF installed from github, Django 2.0.

Included the following at urls.py:

urlpatterns = [
    ...
    path('^api/token-auth/', authtoken_views.obtain_auth_token)
    path('^docs/', include_docs_urls(title='API Docs', authentication_classes=[], permission_classes=[], public=True)),
    ...
]

Expected behavior

Urls in docs should api/endpoint-name/

Actual behavior

Urls are api\/endpoint\-name/, when using the "interact" button you get a 404 (request URL is /api%5C/profiles/)

captura de tela 2017-12-16 as 23 49 05

@scardine scardine reopened this Dec 17, 2017
@scardine scardine changed the title include_docs_urls Backslashes added to paths at docs from include_docs_urls Dec 17, 2017
@scardine
Copy link
Contributor Author

scardine commented Dec 17, 2017

Downgrading to Django 1.11 fixed the problem. Would tests/test_renderers.py be the appropriate file to add a test for this issue?

axnsan12 added a commit to axnsan12/drf-yasg that referenced this issue Dec 18, 2017
In Django 2, routes defines via urls.path are aggresively escaped when converted into regex.

This is a naive fix which unescapes all characters outside capture groups, but in the context of OpenAPI is okay because regular expressions inside paths are not supported anyway.

This issue affects django-rest-framework as well, as outlined in encode/django-rest-framework#5672, encode/django-rest-framework#5675.
@carltongibson
Copy link
Collaborator

Hi @scardine.

I need to look into this but I'm not sure it is a renderer issue. See @axnsan12's commit referencing this issue and #5672.

#5609 (although distinct) is in a similar ballpark here.

@axnsan12
Copy link
Contributor

axnsan12 commented Dec 18, 2017

More specifically, I think this should be addressed by Django in simplify_regex, which should, quote,

    r"""
    Clean up urlpattern regexes into something more readable by humans. For
    example, turn "^(?P<sport_slug>\w+)/athletes/(?P<athlete_slug>\w+)/$"
    into "/<sport_slug>/athletes/<athlete_slug>/".
    """

get_path_from_regex in drf makes use of that function.

I filed a bug in their tracker - https://code.djangoproject.com/ticket/28936

@carltongibson
Copy link
Collaborator

Good follow-up @axnsan12. Thanks.

@scardine
Copy link
Contributor Author

Thanks, if it is not fixed by next week I will try to submit a patch for simplify_regex on my holiday's recess.

@carltongibson
Copy link
Collaborator

Note: Discussion on #5686 has details worth viewing.

@tiltec
Copy link
Contributor

tiltec commented Dec 19, 2017

I think one should do str(pattern) before passing it to simplify_regex. In fact, you don't need to pass a RoutePattern into simplify_regex at all, as it's not a regex (but you may for code simplicity reasons).

The bigger problem here seems not the escaping (can be dealt with calling str(pattern)), but that the result may contain type coercion tags.

Before in Django 1.x, the output of simplify_regex would always look like: /example/<id>/.
Whereas now, it can also be: /example/<int:id>/. Which may be unexpected for some follow-up code.

@scardine
Copy link
Contributor Author

@tiltec I +1 your assessment.

Calling str(pattern) in Python 2.7 may trigger UnicodeError. BTW, are there any plans to do the same in DRF now that Django 2.0 dropped support for 2.7? Life is easier when you don't have to think about 2.7 compatibility.

Django 1.11 is the last version supporting 2.7 but it is an LTS version and support ends only in April 2020.

@tiltec
Copy link
Contributor

tiltec commented Dec 19, 2017

Seems only tangentially related, but I wonder if there's a bug in Django 2.0 when using str(pattern) together with translatable URL patterns.
It clearly returns the gettext_lazy function instead of the translated string. Will set up a more comprehensive testcase for it...

(EDIT) Upstream bug filed: https://code.djangoproject.com/ticket/28947

@tiltec
Copy link
Contributor

tiltec commented Dec 19, 2017

In Django 1.x, the output of simplify_regex would always look like: /example/<id>/.
Whereas now, it can also be: /example/<int:id>/. Which may be unexpected for some follow-up code.

I encountered a follow-up bug.

DRF uses uritemplate to parse the URL variables, but unfortunately the colon means something else there. RFC 6570 says that id:5 specifies a parameter with max length of 5.

Doesn't really fit int:id, where int is the converter and id the parameter name. Therefore I get ValueError: invalid literal for int() with base 10: 'id' from uritemplate.

Maybe it's easiest to strip the converter part out for SchemaGenerator purposes?

@axnsan12
Copy link
Contributor

axnsan12 commented Dec 19, 2017

I think the converter part could be useful for the typing information it could provide about the path parameter. Perhaps the most complete solution would be to try and parse it just like django does?

EDIT: or strip it and use the already parsed converters on the RoutePattern

tiltec added a commit to tiltec/django-rest-framework that referenced this issue Dec 19, 2017
- Call `str(pattern)` to get non-escaped route
- Strip converters from path to comply with uritemplate format

Fixes encode#5675
tiltec added a commit to tiltec/django-rest-framework that referenced this issue Dec 19, 2017
- Call `str(pattern)` to get non-escaped route
- Strip converters from path to comply with uritemplate format

Fixes encode#5675
tiltec added a commit to tiltec/django-rest-framework that referenced this issue Dec 19, 2017
- Call `str(pattern)` to get non-escaped route
- Strip converters from path to comply with uritemplate format

Fixes encode#5675
@carltongibson
Copy link
Collaborator

3.7.4 will go tomorrow. I don’t know if a PR could be incoming by then, but it would make a great addition for the holidays if it could. 🙂

@tiltec
Copy link
Contributor

tiltec commented Dec 19, 2017

@carltongibson PR is ready, I could work on it a bit more today if needed 😄

@carltongibson
Copy link
Collaborator

Ah, good work @tiltec! I will review tomorrow AM.

If others could comment that would help too!

carltongibson pushed a commit that referenced this issue Dec 20, 2017
* Fix url parsing in schema generation

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

Fixes #5675
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this issue 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
Projects
None yet
Development

No branches or pull requests

4 participants