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

Exact URL Matching. Allow Syntactically Correct Patterns. Better Tests #25

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

Exact URL Matching. Allow Syntactically Correct Patterns. Better Tests #25

Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
c9b3e26
Add a Handler for Scripts + more
da3dsoul Mar 7, 2019
445f146
Stupid Whitespace Fix. Sort import better. Add a comment
da3dsoul Mar 7, 2019
02f531a
super().__init__ is not compatible with Py2
da3dsoul Mar 8, 2019
91c94b1
dispatch uses copied code...Fix it for exact matches. Fix test for pa…
da3dsoul Mar 11, 2019
045d5d8
Fix argv for scripts (handles don't exist). Bump Version
da3dsoul Mar 11, 2019
d875572
Fix a minor bug in routing. Fix Unsorted Files Playback
da3dsoul Mar 20, 2019
a4496bb
Fix a Minor Error in Converting Values
da3dsoul Mar 21, 2019
2cd8a88
Bump Version
da3dsoul Mar 21, 2019
a391633
Sanitize args as well as kwargs. Perform a full route test on paths.
da3dsoul Mar 22, 2019
c425491
Clean up a debug variable
da3dsoul Mar 22, 2019
00dacef
Bump Version
da3dsoul Mar 22, 2019
869dfc1
Fix tests to use assert_called_with(args).
da3dsoul Mar 23, 2019
8c82184
Change kwargs convert_args to use a prettier one-liner.
da3dsoul Mar 23, 2019
9dc4627
Add comment
da3dsoul Mar 23, 2019
6c86b57
Update path_args test to check query args as well.
da3dsoul Mar 23, 2019
dc7ab99
Exact Matching, Support i1 pattern names, More Comprehensive Testing
da3dsoul Sep 10, 2019
5e1bb33
Merge remote-tracking branch 'origin/master'
da3dsoul Sep 10, 2019
c92945d
Fix a stupid PyLint complaint
da3dsoul Sep 10, 2019
be8c654
Change b2 to var_with_num_underscore2. Support Underscores
da3dsoul Sep 10, 2019
3b17bfc
Double quote and unquote variable values
da3dsoul Oct 27, 2019
cfcb323
Fix double unquote on query. Fix Relevant Tests.
da3dsoul Jan 10, 2020
2e91ce2
Fix some pylint complaints about line length
da3dsoul Jan 10, 2020
fe210f3
One more stupid pylint
da3dsoul Jan 10, 2020
c0b4552
Remove the for loop in tests.py
da3dsoul Jan 12, 2020
8115f32
Remove the quoting stuff. I don't know what I was thinking....
da3dsoul Jan 12, 2020
078ac25
Remove irrelevant comment
da3dsoul Jan 12, 2020
2c7be81
More consistency in the argument checking in tests.py
da3dsoul Jan 12, 2020
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
69 changes: 51 additions & 18 deletions lib/routing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@
except ImportError:
from urllib.parse import urlsplit, parse_qs
try:
from urllib import urlencode
from urllib import urlencode, quote as q, unquote as uq
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally prefer the original names as it improves code readability.
Only when there are namespace problems or for backward compatibility renaming is warranted.

For example, q is a well-known debugging tool: https://pypi.org/project/q/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I guess. It might just be my preference, but I hate long lines with repeated function names. By your standards, would you prefer this?

quoted_string = double_quote(s)

def double_quote(s):
    return quote(quote(s))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't even determined when or why this double quoting is needed.

except ImportError:
from urllib.parse import urlencode
from urllib.parse import urlencode, quote as q, unquote as uq

try:
import xbmc
Expand Down Expand Up @@ -81,7 +81,17 @@ def route_for(self, path):
if path.startswith(self.base_url):
path = path.split(self.base_url, 1)[1]

for view_fun, rules in iter(list(self._rules.items())):
# only list convert once
list_rules = list(self._rules.items())

# first, search for exact matches
for view_fun, rules in iter(list_rules):
for rule in rules:
if rule.exact_match(path):
return view_fun

# then, search for regex matches
for view_fun, rules in iter(list_rules):
for rule in rules:
if rule.match(path) is not None:
return view_fun
Expand All @@ -96,8 +106,8 @@ def url_for(self, func, *args, **kwargs):
path = rule.make_path(*args, **kwargs)
if path is not None:
return self.url_for_path(path)
raise RoutingError("No known paths to '{0}' with args {1} and "
"kwargs {2}".format(func.__name__, args, kwargs))
raise RoutingError("No known paths to '{0}' with args {1} "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a good reason for this change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PyLint was complaining about line length and marking my PR with a red X

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I have a PR open to raise this to 160. But in that case I understand.

"and kwargs {2}".format(func.__name__, args, kwargs))

def url_for_path(self, path):
"""
Expand Down Expand Up @@ -127,33 +137,48 @@ def run(self, argv=None):
self.path = self.path.rstrip('/')
if len(argv) > 2:
self.args = parse_qs(argv[2].lstrip('?'))
self.args = dict((k, list(uq(uq(v2)) for v2 in v)) for k, v in self.args.items())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the double quoting and unquoting?
We need to document the reason for added complexity.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put it in the test for some reason....idk it was late.

# we wanted double quote for the +, %, and any others that might be in the string

It won't touch ASCII, but double unquoting doesn't hurt if there's a chance of invalid parsing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why would there be any invalid parsing? I would like to see concrete examples where the current implementation failed.

self._dispatch(self.path)

def redirect(self, path):
self._dispatch(path)

def _dispatch(self, path):
for view_func, rules in iter(list(self._rules.items())):
list_rules = list(self._rules.items())
for view_func, rules in iter(list_rules):
for rule in rules:
if not rule.exact_match(path):
continue
log("Dispatching to '%s', exact match" % view_func.__name__)
view_func()
return

# then, search for regex matches
for view_func, rules in iter(list_rules):
for rule in rules:
kwargs = rule.match(path)
if kwargs is not None:
log("Dispatching to '%s', args: %s" % (view_func.__name__, kwargs))
view_func(**kwargs)
return
if kwargs is None:
continue
log("Dispatching to '%s', args: %s" % (view_func.__name__, kwargs))
view_func(**kwargs)
return
raise RoutingError('No route to path "%s"' % path)


class UrlRule:

def __init__(self, pattern):
pattern = pattern.rstrip('/')
kw_pattern = r'<(?:[^:]+:)?([A-z]+)>'
arg_regex = re.compile('<([A-z_][A-z0-9_]*)>')
self._has_args = bool(arg_regex.search(pattern))

kw_pattern = r'<(?:[^:]+:)?([A-z_][A-z0-9_]*)>'
self._pattern = re.sub(kw_pattern, '{\\1}', pattern)
self._keywords = re.findall(kw_pattern, pattern)

p = re.sub('<([A-z]+)>', '<string:\\1>', pattern)
p = re.sub('<string:([A-z]+)>', '(?P<\\1>[^/]+?)', p)
p = re.sub('<path:([A-z]+)>', '(?P<\\1>.*)', p)
p = re.sub('<([A-z_][A-z0-9_]*)>', '<string:\\1>', pattern)
p = re.sub('<string:([A-z_][A-z0-9_]*)>', '(?P<\\1>[^/]+?)', p)
p = re.sub('<path:([A-z_][A-z0-9_]*)>', '(?P<\\1>.*)', p)
self._compiled_pattern = p
self._regex = re.compile('^' + p + '$')

Expand All @@ -164,7 +189,12 @@ def match(self, path):
"""
# match = self._regex.search(urlsplit(path).path)
match = self._regex.search(path)
return match.groupdict() if match else None
if match is None:
return None
return dict((k, uq(uq(v))) for k, v in match.groupdict().items())

def exact_match(self, path):
return not self._has_args and self._pattern == path

def make_path(self, *args, **kwargs):
"""Construct a path from arguments."""
Expand All @@ -173,14 +203,17 @@ def make_path(self, *args, **kwargs):
if args:
# Replace the named groups %s and format
try:
return re.sub(r'{[A-z]+}', r'%s', self._pattern) % args
args = tuple(q(q(str(x), ''), '') for x in args)
return re.sub(r'{[A-z_][A-z0-9_]*}', r'%s', self._pattern) % args
except TypeError:
return None

# We need to find the keys from kwargs that occur in our pattern.
# Unknown keys are pushed to the query string.
url_kwargs = dict(((k, v) for k, v in list(kwargs.items()) if k in self._keywords))
qs_kwargs = dict(((k, v) for k, v in list(kwargs.items()) if k not in self._keywords))
url_kwargs = dict(((k, q(q(str(v), ''), '')) for k, v in list(kwargs.items())
if k in self._keywords))
qs_kwargs = dict(((k, q(q(str(v), ''), '')) for k, v in list(kwargs.items())
if k not in self._keywords))

query = '?' + urlencode(qs_kwargs) if qs_kwargs else ''
try:
Expand Down
44 changes: 30 additions & 14 deletions lib/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,15 @@ def test_make_path():
assert rule.make_path(1) is None


def test_make_path_should_urlencode_args():
rule = UrlRule("/foo")
assert rule.make_path(bar="b a&r") == "/foo?bar=b+a%26r"
def test_make_path_should_urlencode_args(plugin):
f = mock.create_autospec(lambda: None)
plugin.route('/foo')(f)
# we wanted double quote for the +, %, and any others that might be in the string
assert plugin.url_for(f, bar='b a&r+c') == \
plugin.base_url + '/foo?bar=b%252520a%252526r%25252Bc'
plugin.run(['plugin://py.test/foo', '0', '?bar=b%252520a%252526r%25252Bc'])
f.assert_called_with()
assert plugin.args['bar'] == ['b a&r+c']


def test_url_for_path():
Expand All @@ -56,15 +62,15 @@ def test_url_for(plugin):


def test_url_for_kwargs(plugin):
f = lambda a, b: None
plugin.route("/foo/<a>/<b>")(f)
assert plugin.url_for(f, a=1, b=2) == plugin.base_url + "/foo/1/2"
f = lambda a, var_with_num_underscore2: None
plugin.route("/foo/<a>/<var_with_num_underscore2>")(f)
assert plugin.url_for(f, a=1, var_with_num_underscore2=2) == plugin.base_url + "/foo/1/2"


def test_url_for_args(plugin):
f = lambda a, b: None
plugin.route("/<a>/<b>")(f)
assert plugin.url_for(f, 1, 2) == plugin.base_url + "/1/2"
f = lambda a, var_with_num_underscore2, c, d: None
plugin.route("/<a>/<var_with_num_underscore2>/<c>/<d>")(f)
assert plugin.url_for(f, 1, 2.6, True, 'baz') == plugin.base_url + "/1/2.6/True/baz"


def test_route_for(plugin):
Expand All @@ -74,16 +80,23 @@ def test_route_for(plugin):


def test_route_for_args(plugin):
f = lambda: None
plugin.route("/foo/<a>/<b>")(f)
assert plugin.route_for(plugin.base_url + "/foo/1/2") is f
f = lambda a, var_with_num_underscore2: None
g = lambda: (None, None) # just to make sure that they are easily different
plugin.route("/foo/<a>/<var_with_num_underscore2>")(f)
plugin.route("/foo/a/b")(g)

# due to the unpredictable sorting of dict, just do it 100 times to see if it fails
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt this is needed, dict-ordering is not guaranteed (in Python 3.5 or older) but it is deterministic. It is not random.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not in Py3, but it was an issue before this PR, so it's there to prove that it's fixed with the PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think this is a problem on Python 2 either. Again, the ordering is not guaranteed, but it is deterministic. So every single run for the 100 runs will be identical.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would think, right? I expected that, since every other language I've worked with is like that. I can tell you that, in my attempts just to find this issue, I proved the opposite on Python 2.7. When it says that it can't guarantee order there, for the first time I've seen, it really means it.

Just for the sake of it, I'll write up a test case and run it. If it yields consistent results, then I'll concede.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

PS C:\Users\da3ds> & "C:/Program Files/Python37/python.exe" c:/Users/da3ds/Documents/loop_test.py
apples
PS C:\Users\da3ds> & "C:/Program Files/Python37/python.exe" c:/Users/da3ds/Documents/loop_test.py
apples
PS C:\Users\da3ds> & "C:/Program Files/Python37/python.exe" c:/Users/da3ds/Documents/loop_test.py
apples
PS C:\Users\da3ds> & "C:/Program Files/Python27/python.exe" c:/Users/da3ds/Documents/loop_test.py
beetroot
PS C:\Users\da3ds> & "C:/Program Files/Python27/python.exe" c:/Users/da3ds/Documents/loop_test.py
beetroot
PS C:\Users\da3ds> & "C:/Program Files/Python27/python.exe" c:/Users/da3ds/Documents/loop_test.py
beetroot

You win. I don't know what was different before that was giving unreliable results, but it definitely was. That was a long time ago, a different installation of Windows, etc.

for _ in range(0, 100):
assert plugin.route_for(plugin.base_url + "/foo/1/2") is f
assert plugin.route_for(plugin.base_url + "/foo/a/b") is g


def test_dispatch(plugin):
f = mock.create_autospec(lambda: None)
plugin.route("/foo")(f)
plugin.run(['plugin://py.test/foo', '0', '?bar=baz'])
f.assert_called_with()
assert plugin.args['bar'] == ['baz']


def test_path(plugin):
Expand Down Expand Up @@ -111,8 +124,9 @@ def test_no_route(plugin):
def test_arg_parsing(plugin):
f = mock.create_autospec(lambda: None)
plugin.route("/foo")(f)
plugin.run(['plugin://py.test/foo', '0', '?bar=baz'])
assert plugin.args['bar'][0] == 'baz'
plugin.run(['plugin://py.test/foo', '0', '?bar=baz&bar2=baz2'])
assert plugin.args['bar'][0] == 'baz' and plugin.args['bar2'][0] == 'baz2'


def test_trailing_slash_in_route_definition(plugin):
""" Should call registered route with trailing slash. """
Expand All @@ -121,13 +135,15 @@ def test_trailing_slash_in_route_definition(plugin):
plugin.run(['plugin://py.test/foo', '0'])
assert f.call_count == 1


def test_trailing_slashes_in_run(plugin):
""" Should call registered route without trailing slash. """
f = mock.create_autospec(lambda: None)
plugin.route("/foo")(f)
plugin.run(['plugin://py.test/foo/', '0'])
assert f.call_count == 1


def test_trailing_slash_handling_for_root(plugin):
f = mock.create_autospec(lambda: None)
plugin.route("/<a>")(lambda: None)
Expand Down