openstack/deb-python-falcon
Code Issues Proposed changes

fix(DefaultRouter): Handle additional edge cases

Browse Source 
1. The order routes are added should not matter
2. Ensure entire path is consumed before returning a matched route

Closes #535
This commit is contained in:
Kurt Griffiths
2015-05-01 18:33:11 -05:00
parent 0af00afd67
commit 8721653f9a
2 changed files with 78 additions and 90 deletions
Download Patch File Download Diff File Expand all files Collapse all files

91
falcon/routing/compiled.py
View File

@@ -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

77
tests/test_default_router.py
View File

@@ -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'