encode / django-rest-framework

Web APIs for Django. 🎸
https://www.django-rest-framework.org
Other
28.16k stars 6.81k forks source link

Backslashes added to paths at docs from `include_docs_urls` #5675

Closed scardine closed 6 years ago

scardine commented 6 years ago

Checklist

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 commented 6 years ago

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

carltongibson commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Good follow-up @axnsan12. Thanks.

scardine commented 6 years ago

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 commented 6 years ago

Note: Discussion on #5686 has details worth viewing.

tiltec commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

carltongibson commented 6 years ago

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 commented 6 years ago

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

carltongibson commented 6 years ago

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

If others could comment that would help too!