Repository: falconry/falcon

Issue:
Path segment in one route's URI template masks the field expression in another route
The following test demonstrates the issue. The assertion fails since the resulting status code is 404, rather than 200. "/v2.0/thing" should route to `/{version}/thing` instead of `/v2.0`.

```py
def test_string_vs_var(self):
    self.api.add_route('/v2.0', self.resource)
    self.simulate_request('/v2.0')

    self.api.add_route('/{version}/thing', testing.TestResource())
    self.simulate_request('/v2.0/thing')
    self.assertEqual(self.srmock.status, falcon.HTTP_200)
```

```
/{version}/foo/bar
/v1.0
```

Discussion:
kgriffs: Routing tree from the test: https://github.com/falconry/falcon/pull/723
jmvrbanac: Interesting. Seems to look good to me.
kgriffs: Sorry, I had the wrong link for the routing tree. Updated.
jmvrbanac: Outside of of @fxfitz's suggestion, :+1: 

Patch:
diff --git a/falcon/routing/compiled.py b/falcon/routing/compiled.py
index 5f8f951..9177edb 100644
--- a/falcon/routing/compiled.py
+++ b/falcon/routing/compiled.py
@@ -95,7 +95,7 @@ class CompiledRouter(object):
         else:
             return None, None, None
 
-    def _compile_tree(self, nodes, indent=1, level=0):
+    def _compile_tree(self, nodes, indent=1, level=0, fast_return=True):
         """Generates Python code for a routing tree or subtree."""
 
         def line(text, indent_offset=0):
@@ -119,6 +119,18 @@ class CompiledRouter(object):
             nodes, key=lambda node: node.is_var + (node.is_var and
                                                    not node.is_complex))
 
+        # NOTE(kgriffs): Down to this branch in the tree, we can do a
+        # fast 'return None'. See if the nodes at this branch are
+        # all still simple, meaning there is only one possible path.
+        if fast_return:
+            if len(nodes) > 1:
+                # NOTE(kgriffs): There's the possibility of more than
+                # one path.
+                var_nodes = [node for node in nodes if node.is_var]
+                found_var_nodes = bool(var_nodes)
+
+                fast_return = not found_var_nodes
+
         for node in nodes:
             if node.is_var:
                 if node.is_complex:
@@ -162,10 +174,11 @@ class CompiledRouter(object):
                 resource_idx = len(self._return_values)
                 self._return_values.append(node)
 
-            self._compile_tree(node.children, indent, level + 1)
+            self._compile_tree(node.children, indent, level + 1, fast_return)
 
             if node.resource is None:
-                line('return None')
+                if fast_return:
+                    line('return None')
             else:
                 # NOTE(kgriffs): Make sure that we have consumed all of
                 # the segments for the requested route; otherwise we could
@@ -173,11 +186,12 @@ class CompiledRouter(object):
                 line('if path_len == %d:' % (level + 1))
                 line('return return_values[%d]' % resource_idx, 1)
 
-                line('return None')
+                if fast_return:
+                    line('return None')
 
             indent = level_indent
 
-        if not found_simple:
+        if not found_simple and fast_return:
             line('return None')
 
     def _compile(self):


Test patch:
diff --git a/tests/test_default_router.py b/tests/test_default_router.py
index 9dc5ecd..84af78f 100644
--- a/tests/test_default_router.py
+++ b/tests/test_default_router.py
@@ -1,5 +1,6 @@
 import ddt
 
+from falcon.routing import DefaultRouter
 import falcon.testing as testing
 
 
@@ -14,66 +15,115 @@ class ResourceWithId(object):
         resp.body = self.resource_id
 
 
-def setup_routes(router_interface):
-    router_interface.add_route(
-        '/repos', {}, ResourceWithId(1))
-    router_interface.add_route(
-        '/repos/{org}', {}, ResourceWithId(2))
-    router_interface.add_route(
-        '/repos/{org}/{repo}', {}, ResourceWithId(3))
-    router_interface.add_route(
-        '/repos/{org}/{repo}/commits', {}, ResourceWithId(4))
-    router_interface.add_route(
-        '/repos/{org}/{repo}/compare/{usr0}:{branch0}...{usr1}:{branch1}',
-        {}, ResourceWithId(5))
-    router_interface.add_route(
-        '/teams/{id}', {}, ResourceWithId(6))
-    router_interface.add_route(
-        '/teams/{id}/members', {}, ResourceWithId(7))
-    router_interface.add_route(
-        '/user/memberships', {}, ResourceWithId(8))
-    router_interface.add_route(
-        '/emojis', {}, ResourceWithId(9))
-    router_interface.add_route(
-        '/repos/{org}/{repo}/compare/{usr0}:{branch0}...{usr1}:{branch1}/full',
-        {}, ResourceWithId(10))
-    router_interface.add_route(
-        '/repos/{org}/{repo}/compare/all', {}, ResourceWithId(11))
-
-    # NOTE(kgriffs): The ordering of these calls is significant; we
-    # need to test that the {id} field does not match the other routes,
-    # regardless of the order they are added.
-    router_interface.add_route(
-        '/emojis/signs/0', {}, ResourceWithId(12))
-    router_interface.add_route(
-        '/emojis/signs/{id}', {}, ResourceWithId(13))
-    router_interface.add_route(
-        '/emojis/signs/42', {}, ResourceWithId(14))
-    router_interface.add_route(
-        '/emojis/signs/42/small', {}, ResourceWithId(14.1))
-    router_interface.add_route(
-        '/emojis/signs/78/small', {}, ResourceWithId(14.1))
-
-    router_interface.add_route(
-        '/repos/{org}/{repo}/compare/{usr0}:{branch0}...{usr1}:{branch1}/part',
-        {}, ResourceWithId(15))
-    router_interface.add_route(
-        '/repos/{org}/{repo}/compare/{usr0}:{branch0}',
-        {}, ResourceWithId(16))
-    router_interface.add_route(
-        '/repos/{org}/{repo}/compare/{usr0}:{branch0}/full',
-        {}, ResourceWithId(17))
-
-    router_interface.add_route(
-        '/gists/{id}/raw', {}, ResourceWithId(18))
+class TestRegressionCases(testing.TestBase):
+    """Test specific repros reported by users of the framework."""
+
+    def before(self):
+        self.router = DefaultRouter()
+
+    def test_versioned_url(self):
+        self.router.add_route('/{version}/messages', {}, ResourceWithId(2))
+
+        resource, method_map, params = self.router.find('/v2/messages')
+        self.assertEqual(resource.resource_id, 2)
+
+        self.router.add_route('/v2', {}, ResourceWithId(1))
+
+        resource, method_map, params = self.router.find('/v2')
+        self.assertEqual(resource.resource_id, 1)
+
+        resource, method_map, params = self.router.find('/v2/messages')
+        self.assertEqual(resource.resource_id, 2)
+
+        resource, method_map, params = self.router.find('/v1/messages')
+        self.assertEqual(resource.resource_id, 2)
+
+        resource, method_map, params = self.router.find('/v1')
+        self.assertIs(resource, None)
+
+    def test_recipes(self):
+        self.router.add_route(
+            '/recipes/{activity}/{type_id}', {}, ResourceWithId(1))
+        self.router.add_route(
+            '/recipes/baking', {}, ResourceWithId(2))
+
+        resource, method_map, params = self.router.find('/recipes/baking/4242')
+        self.assertEqual(resource.resource_id, 1)
+
+        resource, method_map, params = self.router.find('/recipes/baking')
+        self.assertEqual(resource.resource_id, 2)
+
+        resource, method_map, params = self.router.find('/recipes/grilling')
+        self.assertIs(resource, None)
 
 
 @ddt.ddt
-class TestStandaloneRouter(testing.TestBase):
+class TestComplexRouting(testing.TestBase):
     def before(self):
-        from falcon.routing import DefaultRouter
         self.router = DefaultRouter()
-        setup_routes(self.router)
+
+        self.router.add_route(
+            '/repos', {}, ResourceWithId(1))
+        self.router.add_route(
+            '/repos/{org}', {}, ResourceWithId(2))
+        self.router.add_route(
+            '/repos/{org}/{repo}', {}, ResourceWithId(3))
+        self.router.add_route(
+            '/repos/{org}/{repo}/commits', {}, ResourceWithId(4))
+        self.router.add_route(
+            '/repos/{org}/{repo}/compare/{usr0}:{branch0}...{usr1}:{branch1}',
+            {}, ResourceWithId(5))
+
+        self.router.add_route(
+            '/teams/{id}', {}, ResourceWithId(6))
+        self.router.add_route(
+            '/teams/{id}/members', {}, ResourceWithId(7))
+
+        self.router.add_route(
+            '/teams/default', {}, ResourceWithId(19))
+        self.router.add_route(
+            '/teams/default/members/thing', {}, ResourceWithId(19))
+
+        self.router.add_route(
+            '/user/memberships', {}, ResourceWithId(8))
+        self.router.add_route(
+            '/emojis', {}, ResourceWithId(9))
+        self.router.add_route(
+            '/repos/{org}/{repo}/compare/{usr0}:{branch0}...{usr1}:{branch1}/full',
+            {}, ResourceWithId(10))
+        self.router.add_route(
+            '/repos/{org}/{repo}/compare/all', {}, ResourceWithId(11))
+
+        # NOTE(kgriffs): The ordering of these calls is significant; we
+        # need to test that the {id} field does not match the other routes,
+        # regardless of the order they are added.
+        self.router.add_route(
+            '/emojis/signs/0', {}, ResourceWithId(12))
+        self.router.add_route(
+            '/emojis/signs/{id}', {}, ResourceWithId(13))
+        self.router.add_route(
+            '/emojis/signs/42', {}, ResourceWithId(14))
+        self.router.add_route(
+            '/emojis/signs/42/small', {}, ResourceWithId(14.1))
+        self.router.add_route(
+            '/emojis/signs/78/small', {}, ResourceWithId(22))
+
+        self.router.add_route(
+            '/repos/{org}/{repo}/compare/{usr0}:{branch0}...{usr1}:{branch1}/part',
+            {}, ResourceWithId(15))
+        self.router.add_route(
+            '/repos/{org}/{repo}/compare/{usr0}:{branch0}',
+            {}, ResourceWithId(16))
+        self.router.add_route(
+            '/repos/{org}/{repo}/compare/{usr0}:{branch0}/full',
+            {}, ResourceWithId(17))
+
+        self.router.add_route(
+            '/gists/{id}/{representation}', {}, ResourceWithId(21))
+        self.router.add_route(
+            '/gists/{id}/raw', {}, ResourceWithId(18))
+        self.router.add_route(
+            '/gists/first', {}, ResourceWithId(20))
 
     @ddt.data(
         '/teams/{collision}',  # simple vs simple
@@ -103,20 +153,6 @@ class TestStandaloneRouter(testing.TestBase):
         resource, method_map, params = self.router.find('/emojis/signs/0')
         self.assertEqual(resource.resource_id, -1)
 
-    def test_missing(self):
-        resource, method_map, params = self.router.find('/this/does/not/exist')
-        self.assertIs(resource, None)
-
-        resource, method_map, params = self.router.find('/user/bogus')
-        self.assertIs(resource, None)
-
-        resource, method_map, params = self.router.find('/teams/1234/bogus')
-        self.assertIs(resource, None)
-
-        resource, method_map, params = self.router.find(
-            '/repos/racker/falcon/compare/johndoe:master...janedoe:dev/bogus')
-        self.assertIs(resource, None)
-
     def test_literal_segment(self):
         resource, method_map, params = self.router.find('/emojis/signs/0')
         self.assertEqual(resource.resource_id, 12)
@@ -167,6 +203,54 @@ class TestStandaloneRouter(testing.TestBase):
         resource, method_map, params = self.router.find('/gists/42/raw')
         self.assertEqual(params, {'id': '42'})
 
+    @ddt.data(
+        ('/teams/default', 19),
+        ('/teams/default/members', 7),
+        ('/teams/foo', 6),
+        ('/teams/foo/members', 7),
+        ('/gists/first', 20),
+        ('/gists/first/raw', 18),
+        ('/gists/first/pdf', 21),
+        ('/gists/1776/pdf', 21),
+        ('/emojis/signs/78', 13),
+        ('/emojis/signs/78/small', 22),
+    )
+    @ddt.unpack
+    def test_literal_vs_variable(self, path, expected_id):
+        resource, method_map, params = self.router.find(path)
+        self.assertEqual(resource.resource_id, expected_id)
+
+    @ddt.data(
+        # Misc.
+        '/this/does/not/exist',
+        '/user/bogus',
+        '/repos/racker/falcon/compare/johndoe:master...janedoe:dev/bogus',
+
+        # Literal vs variable (teams)
+        '/teams',
+        '/teams/42/members/undefined',
+        '/teams/42/undefined',
+        '/teams/42/undefined/segments',
+        '/teams/default/members/undefined',
+        '/teams/default/members/thing/undefined',
+        '/teams/default/members/thing/undefined/segments',
+        '/teams/default/undefined',
+        '/teams/default/undefined/segments',
+
+        # Literal vs variable (emojis)
+        '/emojis/signs',
+        '/emojis/signs/0/small',
+        '/emojis/signs/0/undefined',
+        '/emojis/signs/0/undefined/segments',
+        '/emojis/signs/20/small',
+        '/emojis/signs/20/undefined',
+        '/emojis/signs/42/undefined',
+        '/emojis/signs/78/undefined',
+    )
+    def test_not_found(self, path):
+        resource, method_map, params = self.router.find(path)
+        self.assertIs(resource, None)
+
     def test_subsegment_not_found(self):
         resource, method_map, params = self.router.find('/emojis/signs/0/x')
         self.assertIs(resource, None)
@@ -195,7 +279,7 @@ class TestStandaloneRouter(testing.TestBase):
             'usr0': 'johndoe',
             'branch0': 'master',
             'usr1': 'janedoe',
-            'branch1': 'dev'
+            'branch1': 'dev',
         })
 
     @ddt.data(('', 16), ('/full', 17))