Skip to content

Commit

Permalink
fix(DefaultRouter): Handle additional edge cases
Browse files Browse the repository at this point in the history 
1. The order routes are added should not matter
2. Ensure entire path is consumed before returning a matched route

Closes falconry#535
  • Loading branch information
Kurt Griffiths committed May 1, 2015
1 parent 0af00af commit 8721653
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 89 deletions.
91 changes: 18 additions & 73 deletions falcon/routing/compiled.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -125,6 +57,7 @@ def insert(nodes, path_index=0):
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:
Expand Down Expand Up @@ -179,6 +112,10 @@ def line(text, indent_offset=0):
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:
Expand Down Expand Up @@ -225,7 +162,13 @@ def line(text, indent_offset=0):
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

Expand Down Expand Up @@ -326,15 +269,14 @@ def conflicts_with(self, segment):
#
# simple, simple ==> True
# simple, complex ==> True
# simple, string ==> True
# simple, string ==> False
# complex, simple ==> True
# complex, complex ==> False
# complex, string ==> False
# string, simple ==> False
# string, complex ==> False
# string, string ==> False
#

other = CompiledRouterNode(segment)

if self.is_var:
Expand All @@ -352,10 +294,13 @@ def conflicts_with(self, segment):
# /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
75 changes: 59 additions & 16 deletions tests/test_default_router.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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(
'/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(14))
{}, ResourceWithId(15))
router_interface.add_route(
'/repos/{org}/{repo}/compare/{usr0}:{branch0}',
{}, ResourceWithId(15))
{}, ResourceWithId(16))
router_interface.add_route(
'/repos/{org}/{repo}/compare/{usr0}:{branch0}/full',
{}, ResourceWithId(16))
{}, ResourceWithId(17))

router_interface.add_route(
'/gists/{id}/raw', {}, ResourceWithId(18))


@ddt.ddt
Expand All @@ -61,14 +78,23 @@ def before(self):
@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)
Expand All @@ -90,17 +116,24 @@ def test_literal_segment(self):
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(
Expand All @@ -120,6 +153,16 @@ def test_variable(self):
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')
Expand All @@ -131,7 +174,7 @@ def test_multivar(self):
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'
Expand All @@ -147,7 +190,7 @@ def test_complex(self, url_postfix, resource_id):
'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'
Expand Down

0 comments on commit 8721653

Please sign in to comment.