Skip to content

Commit

Permalink
Merge pull request #538 from kgriffs/routing
Browse files Browse the repository at this point in the history 
fix(DefaultRouter): Handle additional edge cases
  • Loading branch information
@jmvrbanac
jmvrbanac committed May 5, 2015
2 parents 6912cdc + 8721653 commit de09845
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 de09845

Please sign in to comment.