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 format_suffix_patterns behavior with Django 2 path() routes #5691

Merged
merged 5 commits into from Dec 20, 2017

Conversation

axnsan12
Copy link
Contributor

@axnsan12 axnsan12 commented Dec 20, 2017

Broken urls are generated by format_suffix_patterns when using urlpatterns specified with the new path() routes added in Django 2.

Closes #5672

@axnsan12
Copy link
Contributor Author

axnsan12 commented Dec 20, 2017

I think this is a bit hard to fix properly. As it is now, format_suffix_patterns takes an "allowed" argument which is a list of accepted formats that is converted to a regx like (?P<format>fmt1|fmt2|fmt3).

def format_suffix_patterns(urlpatterns, suffix_required=False, allowed=None):
    ...

As far as I can tell, the only way to enforce that in Django 2 path would be to register a custom path converter for all combinations of allowed that are needed, and use that format specifier in a path of the form blabla.<registered_format_name:format>. This could get a bit clumsy, since each registered converted would have to have a dinamically generated name, maybe something like 'drf_format_suffix_' + '_'.join(allowed).

Note: generating regex patterns from route patterns is not an option, because the new converters actually transform their captured value, i.e. a view registered at path('api/<int:id>/', view) would expect to receive a value of type int, unlike a function registered at url('api/(?P<id>[0-9]+)', view) which would expect a string containing only digits.

@tomchristie
Copy link
Member

tomchristie commented Dec 20, 2017

would be to register a custom path converter for all combinations of allowed that are needed

Declaring the converter class within a function, rather than at the top-level of a module would be perfectly reasonable. We could just call the class SuffixConverter in all cases, and just vary the regex within the class definition.

@tomchristie
Copy link
Member

tomchristie commented Dec 20, 2017

A reasonable approach to fixing this would be to start with the simpler, but not-quite-there case of just capturing suffixes against the [^.]+ pattern. (And not worry about the fact that it would also capture unknown suffixes)

If you wanted to tackle getting the PR to that point, I'd be very happy to then look at taking it the rest of the way over the line.

return value.strip('./')

def to_url(self, value):
return '.' + value + '/'
Copy link
Member

@tomchristie tomchristie Dec 20, 2017

Choose a reason for hiding this comment

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

Perfect approach, yup.

ret.append(url(regex, view, kwargs, name))

# we create a new RegexPattern url; if the original pattern
# was a RoutePattern we need to preserve its converters
Copy link
Contributor Author

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

Ignore this comment, forgot to remove from an earlier idea

@tomchristie
Copy link
Member

tomchristie commented Dec 20, 2017

@axnsan12 Looking good! Last commit is "WIP" - what's left to do here?

@axnsan12
Copy link
Contributor Author

axnsan12 commented Dec 20, 2017

  • needs more tests
  • maybe needs some refactoring

I am writing some tests now, found a bug related to the trailing slash one. I'm trying to figure out how it's implemented for regex, because for the path case, my implementation generates a path which matches the first, not the second.

        (URLTestPath('/test/.api', (), {'format': 'api'}), False),
        (URLTestPath('/test.api/', (), {'format': 'api'}), True),

EDIT: found it

@axnsan12
Copy link
Contributor Author

axnsan12 commented Dec 20, 2017

I think it should be about good to go

EDIT: however, since #5672 did not provide any concrete details about the errors encountered, I have no idea if they were fixed; this definitely did fix the path converter issue I outlined in the first comment, though.

Copy link
Collaborator

@carltongibson carltongibson left a comment

@axnsan12 This looks great. Thanks for the quick input.

(If there are edge-cases left we'll take them as reported.)

Nice to get this in the release! 💃🏼

@carltongibson carltongibson merged commit 6de12e5 into encode:master Dec 20, 2017
1 check passed
@mr-skyaakash
Copy link

mr-skyaakash commented Dec 20, 2017

Thanks for such detailed analysis @axnsan12. Being a beginnner with django-rest-framework didnt let me dive into the depth of such an issue. Always good to know new stuff.

carltongibson added a commit to carltongibson/django-rest-framework that referenced this pull request Dec 20, 2017
carltongibson added a commit that referenced this pull request Dec 20, 2017
* Update version for 3.7.4 Release

* Add release notes to 01587b9

* Django 2.0 is now final.

* Add trove classifer for Django 2.0

* Finalise release notes for v3.7.4

* Set release date: December 20, 2017

* Update Transifex

* Add release note for #5691

* Move Issue links to bottom
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
…de#5691)

* Add failing test for encode#5672

* Add get_original_route to complement get_regex_pattern

* [WIP] Fix path handling

* needs more tests
* maybe needs some refactoring

* Add django 2 variant for all tests and fix trailing slash bug

* Add more combinations to mixed path test
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
* Update version for 3.7.4 Release

* Add release notes to 01587b9

* Django 2.0 is now final.

* Add trove classifer for Django 2.0

* Finalise release notes for v3.7.4

* Set release date: December 20, 2017

* Update Transifex

* Add release note for encode#5691

* Move Issue links to bottom
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants