From 8721653f9abf0ec075fdd9e65fba5bdbfdb02574 Mon Sep 17 00:00:00 2001 From: Kurt Griffiths Date: Fri, 1 May 2015 18:33:11 -0500 Subject: [PATCH] fix(DefaultRouter): Handle additional edge cases 1. The order routes are added should not matter 2. Ensure entire path is consumed before returning a matched route Closes #535 --- falcon/routing/compiled.py | 91 +++++++----------------------------- tests/test_default_router.py | 77 +++++++++++++++++++++++------- 2 files changed, 78 insertions(+), 90 deletions(-) diff --git a/falcon/routing/compiled.py b/falcon/routing/compiled.py index 1a2f903..79a7bac 100644 --- a/falcon/routing/compiled.py +++ b/falcon/routing/compiled.py @@ -29,74 +29,6 @@ class CompiledRouter(object): tree for each look-up, it generates inlined, bespoke Python code to perform the search, then compiles that code. This makes the route processing quite fast. - - The generated code looks something like this:: - - def find(path, return_values, expressions, params): - path_len = len(path) - if path_len > 0: - if path[0] == "repos": - if path_len > 1: - params["org"] = path[1] - if path_len > 2: - params["repo"] = path[2] - if path_len > 3: - if path[3] == "commits": - return return_values[3] - if path[3] == "compare": - if path_len > 4: - match = expressions[0].match(path[4]) - if match is not None: - params.update(match.groupdict()) - if path_len > 5: - if path[5] == "full": - return return_values[5] - if path[5] == "part": - return return_values[6] - return None - return return_values[4] - if path[4] == "all": - return return_values[7] - match = expressions[1].match(path[4]) - if match is not None: - params.update(match.groupdict()) - if path_len > 5: - if path[5] == "full": - return return_values[9] - return None - return return_values[8] - return None - return None - return None - return return_values[2] - return return_values[1] - return return_values[0] - if path[0] == "teams": - if path_len > 1: - params["id"] = path[1] - if path_len > 2: - if path[2] == "members": - return return_values[11] - return None - return return_values[10] - return None - if path[0] == "user": - if path_len > 1: - if path[1] == "memberships": - return return_values[12] - return None - return None - if path[0] == "emojis": - if path_len > 1: - if path[1] == "signs": - if path_len > 2: - params["id"] = path[2] - return return_values[14] - return None - return None - return return_values[13] - return None - return None """ def __init__(self): @@ -125,6 +57,7 @@ class CompiledRouter(object): if node.matches(segment): path_index += 1 if path_index == len(path): + # NOTE(kgriffs): Override previous node node.method_map = method_map node.resource = resource else: @@ -179,6 +112,10 @@ class CompiledRouter(object): level_indent = indent found_simple = False + # NOTE(kgriffs): Sort static nodes before var nodes so that + # none of them get masked. False sorts before True. + nodes = sorted(nodes, key=lambda node: node.is_var) + for node in nodes: if node.is_var: if node.is_complex: @@ -225,7 +162,13 @@ class CompiledRouter(object): if node.resource is None: line('return None') else: - line('return return_values[%d]' % resource_idx) + # NOTE(kgriffs): Make sure that we have consumed all of + # the segments for the requested route; otherwise we could + # mistakenly match "/foo/23/bar" against "/foo/{id}". + line('if path_len == %d:' % (level + 1)) + line('return return_values[%d]' % resource_idx, 1) + + line('return None') indent = level_indent @@ -326,7 +269,7 @@ class CompiledRouterNode(object): # # simple, simple ==> True # simple, complex ==> True - # simple, string ==> True + # simple, string ==> False # complex, simple ==> True # complex, complex ==> False # complex, string ==> False @@ -334,7 +277,6 @@ class CompiledRouterNode(object): # string, complex ==> False # string, string ==> False # - other = CompiledRouterNode(segment) if self.is_var: @@ -352,10 +294,13 @@ class CompiledRouterNode(object): # /foo/{thing1} # /foo/{thing2} # - # or + # On the other hand, this is OK: # # /foo/{thing1} # /foo/all - return True + # + return other.is_var + # NOTE(kgriffs): If self is a static string match, then all the cases + # for other are False, so no need to check. return False diff --git a/tests/test_default_router.py b/tests/test_default_router.py index cb14489..8e3b518 100644 --- a/tests/test_default_router.py +++ b/tests/test_default_router.py @@ -7,6 +7,9 @@ class ResourceWithId(object): def __init__(self, resource_id): self.resource_id = resource_id + def __repr__(self): + return 'ResourceWithId({0})'.format(self.resource_id) + def on_get(self, req, resp): resp.body = self.resource_id @@ -36,19 +39,33 @@ def setup_routes(router_interface): {}, 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( - '/repos/{org}/{repo}/compare/{usr0}:{branch0}...{usr1}:{branch1}/part', - {}, ResourceWithId(14)) + '/emojis/signs/42', {}, ResourceWithId(14)) router_interface.add_route( - '/repos/{org}/{repo}/compare/{usr0}:{branch0}', + '/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}/full', + '/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)) @ddt.ddt @@ -61,14 +78,23 @@ class TestStandaloneRouter(testing.TestBase): @ddt.data( '/teams/{collision}', '/repos/{org}/{repo}/compare/{simple-collision}', - '/emojis/signs/1', + '/emojis/signs/{id_too}', ) def test_collision(self, template): self.assertRaises( ValueError, - self.router.add_route, template, {}, ResourceWithId(6) + self.router.add_route, template, {}, ResourceWithId(-1) ) + def test_dump(self): + print(self.router._src) + + def test_override(self): + self.router.add_route('/emojis/signs/0', {}, ResourceWithId(-1)) + + 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) @@ -90,17 +116,24 @@ class TestStandaloneRouter(testing.TestBase): resource, method_map, params = self.router.find('/emojis/signs/1') self.assertEqual(resource.resource_id, 13) - def test_dead_segment(self): - resource, method_map, params = self.router.find('/teams') - self.assertIs(resource, None) + resource, method_map, params = self.router.find('/emojis/signs/42') + self.assertEqual(resource.resource_id, 14) - resource, method_map, params = self.router.find('/emojis/signs') - self.assertIs(resource, None) + resource, method_map, params = self.router.find('/emojis/signs/42/small') + self.assertEqual(resource.resource_id, 14.1) - resource, method_map, params = self.router.find('/emojis/signs/stop') - self.assertEqual(params, { - 'id': 'stop', - }) + resource, method_map, params = self.router.find('/emojis/signs/1/small') + self.assertEqual(resource, None) + + @ddt.data( + '/teams', + '/emojis/signs', + '/gists', + '/gists/42', + ) + def test_dead_segment(self, template): + resource, method_map, params = self.router.find(template) + self.assertIs(resource, None) def test_malformed_pattern(self): resource, method_map, params = self.router.find( @@ -120,6 +153,16 @@ class TestStandaloneRouter(testing.TestBase): self.assertEqual(resource.resource_id, 6) self.assertEqual(params, {'id': '42'}) + resource, method_map, params = self.router.find('/emojis/signs/stop') + self.assertEqual(params, {'id': 'stop'}) + + resource, method_map, params = self.router.find('/gists/42/raw') + self.assertEqual(params, {'id': '42'}) + + def test_subsegment_not_found(self): + resource, method_map, params = self.router.find('/emojis/signs/0/x') + self.assertIs(resource, None) + def test_multivar(self): resource, method_map, params = self.router.find( '/repos/racker/falcon/commits') @@ -131,7 +174,7 @@ class TestStandaloneRouter(testing.TestBase): self.assertEqual(resource.resource_id, 11) self.assertEqual(params, {'org': 'racker', 'repo': 'falcon'}) - @ddt.data(('', 5), ('/full', 10), ('/part', 14)) + @ddt.data(('', 5), ('/full', 10), ('/part', 15)) @ddt.unpack def test_complex(self, url_postfix, resource_id): uri = '/repos/racker/falcon/compare/johndoe:master...janedoe:dev' @@ -147,7 +190,7 @@ class TestStandaloneRouter(testing.TestBase): 'branch1': 'dev' }) - @ddt.data(('', 15), ('/full', 16)) + @ddt.data(('', 16), ('/full', 17)) @ddt.unpack def test_complex_alt(self, url_postfix, resource_id): uri = '/repos/falconry/falcon/compare/johndoe:master'