Fix format_suffix_patterns behavior with Django 2 path() routes (#5691)

* Add failing test for #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

Author: axnsan12

rest_framework/compat.py

@@ -29,7 +29,11 @@
     )
 
 
-def get_regex_pattern(urlpattern):
+def get_original_route(urlpattern):
+    """
+    Get the original route/regex that was typed in by the user into the path(), re_path() or url() directive. This
+    is in contrast with get_regex_pattern below, which for RoutePattern returns the raw regex generated from the path().
+    """
     if hasattr(urlpattern, 'pattern'):
         # Django 2.0
         return str(urlpattern.pattern)
@@ -38,6 +42,29 @@ def get_regex_pattern(urlpattern):
         return urlpattern.regex.pattern
 
 
+def get_regex_pattern(urlpattern):
+    """
+    Get the raw regex out of the urlpattern's RegexPattern or RoutePattern. This is always a regular expression,
+    unlike get_original_route above.
+    """
+    if hasattr(urlpattern, 'pattern'):
+        # Django 2.0
+        return urlpattern.pattern.regex.pattern
+    else:
+        # Django < 2.0
+        return urlpattern.regex.pattern
+
+
+def is_route_pattern(urlpattern):
+    if hasattr(urlpattern, 'pattern'):
+        # Django 2.0
+        from django.urls.resolvers import RoutePattern
+        return isinstance(urlpattern.pattern, RoutePattern)
+    else:
+        # Django < 2.0
+        return False
+
+
 def make_url_resolver(regex, urlpatterns):
     try:
         # Django 2.0
@@ -257,10 +284,11 @@ def md_filter_add_syntax_highlight(md):
 
 # Django 1.x url routing syntax. Remove when dropping Django 1.11 support.
 try:
-    from django.urls import include, path, re_path # noqa
+    from django.urls import include, path, re_path, register_converter  # noqa
 except ImportError:
     from django.conf.urls import include, url # noqa
     path = None
+    register_converter = None
     re_path = url
 
 

rest_framework/schemas/generators.py

@@ -16,7 +16,7 @@
 
 from rest_framework import exceptions
 from rest_framework.compat import (
-    URLPattern, URLResolver, coreapi, coreschema, get_regex_pattern
+    URLPattern, URLResolver, coreapi, coreschema, get_original_route
 )
 from rest_framework.request import clone_request
 from rest_framework.settings import api_settings
@@ -170,7 +170,7 @@ def get_api_endpoints(self, patterns=None, prefix=''):
         api_endpoints = []
 
         for pattern in patterns:
-            path_regex = prefix + get_regex_pattern(pattern)
+            path_regex = prefix + get_original_route(pattern)
             if isinstance(pattern, URLPattern):
                 path = self.get_path_from_regex(path_regex)
                 callback = pattern.callback

rest_framework/urlpatterns.py

@@ -2,11 +2,39 @@
 
 from django.conf.urls import include, url
 
-from rest_framework.compat import URLResolver, get_regex_pattern
+from rest_framework.compat import (
+    URLResolver, get_regex_pattern, is_route_pattern, path, register_converter
+)
 from rest_framework.settings import api_settings
 
 
-def apply_suffix_patterns(urlpatterns, suffix_pattern, suffix_required):
+def _get_format_path_converter(suffix_kwarg, allowed):
+    if allowed:
+        if len(allowed) == 1:
+            allowed_pattern = allowed[0]
+        else:
+            allowed_pattern = '(?:%s)' % '|'.join(allowed)
+        suffix_pattern = r"\.%s/?" % allowed_pattern
+    else:
+        suffix_pattern = r"\.[a-z0-9]+/?"
+
+    class FormatSuffixConverter:
+        regex = suffix_pattern
+
+        def to_python(self, value):
+            return value.strip('./')
+
+        def to_url(self, value):
+            return '.' + value + '/'
+
+    converter_name = 'drf_format_suffix'
+    if allowed:
+        converter_name += '_' + '_'.join(allowed)
+
+    return converter_name, FormatSuffixConverter
+
+
+def apply_suffix_patterns(urlpatterns, suffix_pattern, suffix_required, suffix_route=None):
     ret = []
     for urlpattern in urlpatterns:
         if isinstance(urlpattern, URLResolver):
@@ -18,8 +46,18 @@ def apply_suffix_patterns(urlpatterns, suffix_pattern, suffix_required):
             # Add in the included patterns, after applying the suffixes
             patterns = apply_suffix_patterns(urlpattern.url_patterns,
                                              suffix_pattern,
-                                             suffix_required)
-            ret.append(url(regex, include((patterns, app_name), namespace), kwargs))
+                                             suffix_required,
+                                             suffix_route)
+
+            # if the original pattern was a RoutePattern we need to preserve it
+            if is_route_pattern(urlpattern):
+                assert path is not None
+                route = str(urlpattern.pattern)
+                new_pattern = path(route, include((patterns, app_name), namespace), kwargs)
+            else:
+                new_pattern = url(regex, include((patterns, app_name), namespace), kwargs)
+
+            ret.append(new_pattern)
         else:
             # Regular URL pattern
             regex = get_regex_pattern(urlpattern).rstrip('$').rstrip('/') + suffix_pattern
@@ -29,7 +67,17 @@ def apply_suffix_patterns(urlpatterns, suffix_pattern, suffix_required):
             # Add in both the existing and the new urlpattern
             if not suffix_required:
                 ret.append(urlpattern)
-            ret.append(url(regex, view, kwargs, name))
+
+            # if the original pattern was a RoutePattern we need to preserve it
+            if is_route_pattern(urlpattern):
+                assert path is not None
+                assert suffix_route is not None
+                route = str(urlpattern.pattern).rstrip('$').rstrip('/') + suffix_route
+                new_pattern = path(route, view, kwargs, name)
+            else:
+                new_pattern = url(regex, view, kwargs, name)
+
+            ret.append(new_pattern)
 
     return ret
 
@@ -60,4 +108,12 @@ def format_suffix_patterns(urlpatterns, suffix_required=False, allowed=None):
     else:
         suffix_pattern = r'\.(?P<%s>[a-z0-9]+)/?$' % suffix_kwarg
 
-    return apply_suffix_patterns(urlpatterns, suffix_pattern, suffix_required)
+    if path and register_converter:
+        converter_name, suffix_converter = _get_format_path_converter(suffix_kwarg, allowed)
+        register_converter(suffix_converter, converter_name)
+
+        suffix_route = '<%s:%s>' % (converter_name, suffix_kwarg)
+    else:
+        suffix_route = None
+
+    return apply_suffix_patterns(urlpatterns, suffix_pattern, suffix_required, suffix_route)