Fix format_suffix_patterns behavior with Django 2 path() routes

[axnsan12]

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

Closes #5672

[axnsan12]

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]

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]

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.

[tomchristie]

Perfect approach, yup.

[axnsan12]

Ignore this comment, forgot to remove from an earlier idea

[tomchristie]

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

[axnsan12]

• 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]

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.

[carltongibson]

@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! 💃🏼

[mr-skyaakash]

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.