Fix parsing of Django path patterns

sentry_sdk/integrations/django/transactions.py

@@ -1,6 +1,8 @@
 """
-Copied from raven-python. Used for
-`DjangoIntegration(transaction_fron="raven_legacy")`.
+Copied from raven-python.
+
+Despite being called "legacy" in some places this resolver is very much still
+in use.
 """
 
 from __future__ import absolute_import
@@ -19,6 +21,8 @@
     from typing import Union
     from re import Pattern
 
+from django import VERSION as DJANGO_VERSION
+
 try:
     from django.urls import get_resolver
 except ImportError:
@@ -36,6 +40,9 @@ def get_regex(resolver_or_pattern):
 
 
 class RavenResolver(object):
+    _new_style_group_matcher = re.compile(
+        r"<(?:([^>:]+):)?([^>]+)>"
+    )  # https://github.com/django/django/blob/21382e2743d06efbf5623e7c9b6dccf2a325669b/django/urls/resolvers.py#L245-L247
     _optional_group_matcher = re.compile(r"\(\?\:([^\)]+)\)")
     _named_group_matcher = re.compile(r"\(\?P<(\w+)>[^\)]+\)+")
     _non_named_group_matcher = re.compile(r"\([^\)]+\)")
@@ -46,7 +53,7 @@ class RavenResolver(object):
     _cache = {}  # type: Dict[URLPattern, str]
 
     def _simplify(self, pattern):
-        # type: (str) -> str
+        # type: (Union[URLPattern, URLResolver]) -> str
         r"""
         Clean up urlpattern regexes into something readable by humans:
 
@@ -56,11 +63,23 @@ def _simplify(self, pattern):
         To:
         > "{sport_slug}/athletes/{athlete_slug}/"
         """
+        # "new-style" path patterns can be parsed directly without turning them
+        # into regexes first
+        if DJANGO_VERSION >= (2, 0):
+            from django.urls.resolvers import RoutePattern
+
+            if isinstance(pattern.pattern, RoutePattern):
+                return self._new_style_group_matcher.sub(
+                    lambda m: "{%s}" % m.group(2), pattern.pattern._route
+                )
+
+        result = get_regex(pattern).pattern
+
         # remove optional params
         # TODO(dcramer): it'd be nice to change these into [%s] but it currently
         # conflicts with the other rules because we're doing regexp matches
         # rather than parsing tokens
-        result = self._optional_group_matcher.sub(lambda m: "%s" % m.group(1), pattern)
+        result = self._optional_group_matcher.sub(lambda m: "%s" % m.group(1), result)
 
         # handle named groups first
         result = self._named_group_matcher.sub(lambda m: "{%s}" % m.group(1), result)
@@ -113,8 +132,8 @@ def _resolve(self, resolver, path, parents=None):
             except KeyError:
                 pass
 
-            prefix = "".join(self._simplify(get_regex(p).pattern) for p in parents)
-            result = prefix + self._simplify(get_regex(pattern).pattern)
+            prefix = "".join(self._simplify(p) for p in parents)
+            result = prefix + self._simplify(pattern)
             if not result.startswith("/"):
                 result = "/" + result
             self._cache[pattern] = result

tests/integrations/django/test_transactions.py

@@ -3,83 +3,121 @@
 import pytest
 import django
 
+try:
+    from unittest import mock  # python 3.3 and above
+except ImportError:
+    import mock  # python < 3.3
+
+
+# django<2.0 has only `url` with regex based patterns.
+# django>=2.0 renames `url` to `re_path`, and additionally introduces `path`
+# for new style URL patterns, e.g. <int:article_id>.
 if django.VERSION >= (2, 0):
-    # TODO: once we stop supporting django < 2, use the real name of this
-    # function (re_path)
-    from django.urls import re_path as url
+    from django.urls import path, re_path
+    from django.urls.converters import PathConverter
     from django.conf.urls import include
 else:
-    from django.conf.urls import url, include
+    from django.conf.urls import url as re_path, include
 
 if django.VERSION < (1, 9):
-    included_url_conf = (url(r"^foo/bar/(?P<param>[\w]+)", lambda x: ""),), "", ""
+    included_url_conf = (re_path(r"^foo/bar/(?P<param>[\w]+)", lambda x: ""),), "", ""
 else:
-    included_url_conf = ((url(r"^foo/bar/(?P<param>[\w]+)", lambda x: ""),), "")
+    included_url_conf = ((re_path(r"^foo/bar/(?P<param>[\w]+)", lambda x: ""),), "")
 
 from sentry_sdk.integrations.django.transactions import RavenResolver
 
 
 example_url_conf = (
-    url(r"^api/(?P<project_id>[\w_-]+)/store/$", lambda x: ""),
-    url(r"^api/(?P<version>(v1|v2))/author/$", lambda x: ""),
-    url(
+    re_path(r"^api/(?P<project_id>[\w_-]+)/store/$", lambda x: ""),
+    re_path(r"^api/(?P<version>(v1|v2))/author/$", lambda x: ""),
+    re_path(
         r"^api/(?P<project_id>[^\/]+)/product/(?P<pid>(?:\d+|[A-Fa-f0-9-]{32,36}))/$",
         lambda x: "",
     ),
-    url(r"^report/", lambda x: ""),
-    url(r"^example/", include(included_url_conf)),
+    re_path(r"^report/", lambda x: ""),
+    re_path(r"^example/", include(included_url_conf)),
 )
 
 
-def test_legacy_resolver_no_match():
+def test_resolver_no_match():
     resolver = RavenResolver()
     result = resolver.resolve("/foo/bar", example_url_conf)
     assert result is None
 
 
-def test_legacy_resolver_complex_match():
+def test_resolver_re_path_complex_match():
     resolver = RavenResolver()
     result = resolver.resolve("/api/1234/store/", example_url_conf)
     assert result == "/api/{project_id}/store/"
 
 
-def test_legacy_resolver_complex_either_match():
+def test_resolver_re_path_complex_either_match():
     resolver = RavenResolver()
     result = resolver.resolve("/api/v1/author/", example_url_conf)
     assert result == "/api/{version}/author/"
     result = resolver.resolve("/api/v2/author/", example_url_conf)
     assert result == "/api/{version}/author/"
 
 
-def test_legacy_resolver_included_match():
+def test_resolver_re_path_included_match():
     resolver = RavenResolver()
     result = resolver.resolve("/example/foo/bar/baz", example_url_conf)
     assert result == "/example/foo/bar/{param}"
 
 
-def test_capture_multiple_named_groups():
+def test_resolver_re_path_multiple_groups():
     resolver = RavenResolver()
     result = resolver.resolve(
         "/api/myproject/product/cb4ef1caf3554c34ae134f3c1b3d605f/", example_url_conf
     )
     assert result == "/api/{project_id}/product/{pid}/"
 
 
-@pytest.mark.skipif(django.VERSION < (2, 0), reason="Requires Django > 2.0")
-def test_legacy_resolver_newstyle_django20_urlconf():
-    from django.urls import path
-
+@pytest.mark.skipif(
+    django.VERSION < (2, 0),
+    reason="Django>=2.0 required for <converter:parameter> patterns",
+)
+def test_resolver_path_group():
     url_conf = (path("api/v2/<int:project_id>/store/", lambda x: ""),)
     resolver = RavenResolver()
     result = resolver.resolve("/api/v2/1234/store/", url_conf)
     assert result == "/api/v2/{project_id}/store/"
 
 
-@pytest.mark.skipif(django.VERSION < (2, 0), reason="Requires Django > 2.0")
-def test_legacy_resolver_newstyle_django20_urlconf_multiple_groups():
-    from django.urls import path
-
-    url_conf = (path("api/v2/<int:project_id>/product/<int:pid>", lambda x: ""),)
+@pytest.mark.skipif(
+    django.VERSION < (2, 0),
+    reason="Django>=2.0 required for <converter:parameter> patterns",
+)
+def test_resolver_path_multiple_groups():
+    url_conf = (path("api/v2/<str:project_id>/product/<int:pid>", lambda x: ""),)
     resolver = RavenResolver()
-    result = resolver.resolve("/api/v2/1234/product/5689", url_conf)
+    result = resolver.resolve("/api/v2/myproject/product/5689", url_conf)
     assert result == "/api/v2/{project_id}/product/{pid}"
+
+
+@pytest.mark.skipif(
+    django.VERSION < (2, 0),
+    reason="Django>=2.0 required for <converter:parameter> patterns",
+)
+def test_resolver_path_complex_path():
+    class CustomPathConverter(PathConverter):
+        regex = r"[^/]+(/[^/]+){0,2}"
+
+    with mock.patch(
+        "django.urls.resolvers.get_converter", return_value=CustomPathConverter
+    ):
+        url_conf = (path("api/v3/<custom_path:my_path>", lambda x: ""),)
+        resolver = RavenResolver()
+        result = resolver.resolve("/api/v3/abc/def/ghi", url_conf)
+        assert result == "/api/v3/{my_path}"
+
+
+@pytest.mark.skipif(
+    django.VERSION < (2, 0),
+    reason="Django>=2.0 required for <converter:parameter> patterns",
+)
+def test_resolver_path_no_converter():
+    url_conf = (path("api/v4/<project_id>", lambda x: ""),)
+    resolver = RavenResolver()
+    result = resolver.resolve("/api/v4/myproject", url_conf)
+    assert result == "/api/v4/{project_id}"