Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(DefaultRouter): Handle additional edge cases #538

Merged
merged 1 commit into from
May 5, 2015
Merged

fix(DefaultRouter): Handle additional edge cases #538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 18 additions & 73 deletions falcon/routing/compiled.py
View file
Open in desktop
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
Open in desktop
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