From f6ca88632451fb22346c1c91436c384fd1a50ed8 Mon Sep 17 00:00:00 2001 From: Kurt Griffiths Date: Tue, 27 Jun 2017 21:25:15 -0600 Subject: [PATCH] feat(API): Foundational support for URI template field converters (#1067) Provides an initial converter (int) along with relevant plumbing and tests. Additional converters and docs to come in future PRs. --- docs/api/api.rst | 2 + docs/api/routing.rst | 3 +- falcon/api.py | 9 + falcon/routing/__init__.py | 2 +- falcon/routing/compiled.py | 449 +++++++++++++++++++++++++++++------ falcon/routing/converters.py | 64 +++++ tests/conftest.py | 1 - tests/test_default_router.py | 170 ++++++++++++- tests/test_uri_converters.py | 54 +++++ tests/test_uri_templates.py | 48 ++++ 10 files changed, 718 insertions(+), 84 deletions(-) create mode 100644 falcon/routing/converters.py create mode 100644 tests/test_uri_converters.py diff --git a/docs/api/api.rst b/docs/api/api.rst index c278872..0d2e4c2 100644 --- a/docs/api/api.rst +++ b/docs/api/api.rst @@ -21,3 +21,5 @@ standard-compliant WSGI server. .. autoclass:: falcon.ResponseOptions :members: +.. autoclass:: falcon.routing.CompiledRouterOptions + :noindex: diff --git a/docs/api/routing.rst b/docs/api/routing.rst index c01cb2e..57096fa 100644 --- a/docs/api/routing.rst +++ b/docs/api/routing.rst @@ -63,4 +63,5 @@ A custom routing engine may be specified when instantiating api = API(router=fancy) .. automodule:: falcon.routing - :members: create_http_method_map, compile_uri_template, CompiledRouter + :members: create_http_method_map, compile_uri_template, + CompiledRouter, CompiledRouterOptions diff --git a/falcon/api.py b/falcon/api.py index 837e0c7..7a58991 100644 --- a/falcon/api.py +++ b/falcon/api.py @@ -113,6 +113,11 @@ class API(object): requests. See also: :py:class:`~.RequestOptions` resp_options: A set of behavioral options related to outgoing responses. See also: :py:class:`~.ResponseOptions` + router_options: Configuration options for the router. If a + custom router is in use, and it does not expose any + configurable options, referencing this attribute will raise + an instance of ``AttributeError``. See also: + :py:class:`falcon.routing.CompiledRouterOptions`. """ # PERF(kgriffs): Reference via self since that is faster than @@ -282,6 +287,10 @@ class API(object): start_response(resp_status, headers) return body + @property + def router_options(self): + return self._router.options + def add_route(self, uri_template, resource, *args, **kwargs): """Associates a templatized URI path with a resource. diff --git a/falcon/routing/__init__.py b/falcon/routing/__init__.py index 228ce76..24e442f 100644 --- a/falcon/routing/__init__.py +++ b/falcon/routing/__init__.py @@ -19,7 +19,7 @@ includes utility functions to aid in the implementation of custom routers. """ -from falcon.routing.compiled import CompiledRouter +from falcon.routing.compiled import CompiledRouter, CompiledRouterOptions # NOQA from falcon.routing.util import create_http_method_map # NOQA from falcon.routing.util import compile_uri_template # NOQA diff --git a/falcon/routing/compiled.py b/falcon/routing/compiled.py index 2c6a53f..f11c057 100644 --- a/falcon/routing/compiled.py +++ b/falcon/routing/compiled.py @@ -16,10 +16,18 @@ import keyword import re +import textwrap + +from six.moves import UserDict + +from falcon.routing import converters -_FIELD_PATTERN = re.compile('{([^}]*)}') _TAB_STR = ' ' * 4 +_FIELD_PATTERN = re.compile( + '{((?P[^}:]*)((?P:(?P[^}\(]*))(\((?P.*)\))?)?)}' +) +_IDENTIFIER_PATTERN = re.compile('[A-Za-z_][A-Za-z0-9_]*$') class CompiledRouter(object): @@ -37,19 +45,38 @@ class CompiledRouter(object): __slots__ = ( '_ast', + '_converter_map', + '_converters', '_find', '_finder_src', + '_options', '_patterns', '_return_values', '_roots', ) def __init__(self): - self._roots = [] - self._find = self._compile() + self._ast = None + self._converters = None self._finder_src = None + + self._options = CompiledRouterOptions() + + # PERF(kgriffs): This is usually an anti-pattern, but we do it + # here to reduce lookup time. + self._converter_map = self._options.converters.data + self._patterns = None self._return_values = None + self._roots = [] + + # NOTE(kgriffs): Call _compile() last since it depends on + # the variables above. + self._find = self._compile() + + @property + def options(self): + return self._options @property def finder_src(self): @@ -66,34 +93,17 @@ class CompiledRouter(object): the URI template. """ - if re.search('\s', uri_template): + # NOTE(kgriffs): Fields may have whitespace in them, so sub + # those before checking the rest of the URI template. + if re.search('\s', _FIELD_PATTERN.sub('{FIELD}', uri_template)): raise ValueError('URI templates may not include whitespace.') - # NOTE(kgriffs): Ensure fields are valid Python identifiers, - # since they will be passed as kwargs to responders. Also - # ensure there are no duplicate names, since that causes the - # following problems: - # - # 1. For simple nodes, values from deeper nodes overwrite - # values from more shallow nodes. - # 2. For complex nodes, re.compile() raises a nasty error - # - fields = _FIELD_PATTERN.findall(uri_template) - used_names = set() - for name in fields: - is_identifier = re.match('[A-Za-z_][A-Za-z0-9_]*$', name) - if not is_identifier or name in keyword.kwlist: - raise ValueError('Field names must be valid identifiers ' - "('{}' is not valid).".format(name)) - - if name in used_names: - raise ValueError('Field names may not be duplicated ' - "('{}' was used more than once)".format(name)) - - used_names.add(name) - path = uri_template.strip('/').split('/') + used_names = set() + for segment in path: + self._validate_template_segment(segment, used_names) + def insert(nodes, path_index=0): for node in nodes: segment = path[path_index] @@ -110,16 +120,13 @@ class CompiledRouter(object): return if node.conflicts_with(segment): - msg = ( - 'The URI template for this route conflicts with another' - "route's template. This is usually caused by using " - 'different field names at the same level in the path. ' - 'For example, given the route paths ' - "'/parents/{id}' and '/parents/{parent_id}/children', " - 'the conflict can be resolved by renaming one of the ' - 'fields to match the other, i.e.: ' - "'/parents/{parent_id}' and '/parents/{parent_id}/children'." - ) + msg = textwrap.dedent(""" + The URI template for this route is inconsistent or conflicts with another + route's template. This is usually caused by configuring a field converter + differently for the same field in two different routes, or by using + different field names at the same level in the path (e.g., + '/parents/{id}' and '/parents/{parent_id}/children') + """).strip().replace('\n', ' ') raise ValueError(msg) # NOTE(richardolsson): If we got this far, the node doesn't already @@ -157,7 +164,8 @@ class CompiledRouter(object): path = uri.lstrip('/').split('/') params = {} - node = self._find(path, self._return_values, self._patterns, params) + node = self._find(path, self._return_values, self._patterns, + self._converters, params) if node is not None: return node.resource, node.method_map, params, node.uri_template @@ -168,6 +176,48 @@ class CompiledRouter(object): # Private # ----------------------------------------------------------------- + def _validate_template_segment(self, segment, used_names): + """Validates a single path segment of a URI template. + + 1. Ensure field names are valid Python identifiers, since they + will be passed as kwargs to responders. + 2. Check that there are no duplicate names, since that causes + (at least) the following problems: + + a. For simple nodes, values from deeper nodes overwrite + values from more shallow nodes. + b. For complex nodes, re.compile() raises a nasty error + 3. Check that when the converter syntax is used, the named + converter exists. + """ + + for field in _FIELD_PATTERN.finditer(segment): + name = field.group('fname') + + is_identifier = _IDENTIFIER_PATTERN.match(name) + if not is_identifier or name in keyword.kwlist: + msg_template = ('Field names must be valid identifiers ' + '("{0}" is not valid)') + msg = msg_template.format(name) + raise ValueError(msg) + + if name in used_names: + msg_template = ('Field names may not be duplicated ' + '("{0}" was used more than once)') + msg = msg_template.format(name) + raise ValueError(msg) + + used_names.add(name) + + if field.group('cname_sep') == ':': + msg = 'Missing converter for field "{0}"'.format(name) + raise ValueError(msg) + + name = field.group('cname') + if name and name not in self._converter_map: + msg = 'Unknown converter: "{0}"'.format(name) + raise ValueError(msg) + def _generate_ast(self, nodes, parent, return_values, patterns, level=0, fast_return=True): """Generates a coarse AST for the router.""" @@ -176,7 +226,7 @@ class CompiledRouter(object): return outer_parent = _CxIfPathLength('>', level) - parent.append(outer_parent) + parent.append_child(outer_parent) parent = outer_parent found_simple = False @@ -212,13 +262,45 @@ class CompiledRouter(object): construct = _CxIfPathSegmentPattern(level, pattern_idx, node.var_pattern.pattern) - parent.append(construct) + parent.append_child(construct) parent = construct + if node.var_converter_map: + parent.append_child(_CxPrefetchGroupsFromPatternMatch()) + parent = self._generate_conversion_ast(parent, node) + + else: + parent.append_child(_CxSetParamsFromPatternMatch()) + else: # NOTE(kgriffs): Simple nodes just capture the entire path # segment as the value for the param. - parent.append(_CxSetParam(node.var_name, level)) + + if node.var_converter_map: + assert len(node.var_converter_map) == 1 + + parent.append_child(_CxSetFragmentFromPath(level)) + + field_name = node.var_name + __, converter_name, converter_argstr = node.var_converter_map[0] + converter_class = self._converter_map[converter_name] + + converter_obj = self._instantiate_converter( + converter_class, + converter_argstr + ) + converter_idx = len(self._converters) + self._converters.append(converter_obj) + + construct = _CxIfConverterField( + field_name, + converter_idx, + ) + + parent.append_child(construct) + parent = construct + else: + parent.append_child(_CxSetParam(node.var_name, level)) # NOTE(kgriffs): We don't allow multiple simple var nodes # to exist at the same level, e.g.: @@ -233,7 +315,7 @@ class CompiledRouter(object): else: # NOTE(kgriffs): Not a param, so must match exactly construct = _CxIfPathSegmentLiteral(level, node.raw_segment) - parent.append(construct) + parent.append_child(construct) parent = construct if node.resource is not None: @@ -253,22 +335,52 @@ class CompiledRouter(object): if node.resource is None: if fast_return: - parent.append(_CxReturnNone()) + parent.append_child(_CxReturnNone()) else: # 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}". construct = _CxIfPathLength('==', level + 1) - construct.append(_CxReturnValue(resource_idx)) - parent.append(construct) + construct.append_child(_CxReturnValue(resource_idx)) + parent.append_child(construct) if fast_return: - parent.append(_CxReturnNone()) + parent.append_child(_CxReturnNone()) parent = outer_parent if not found_simple and fast_return: - parent.append(_CxReturnNone()) + parent.append_child(_CxReturnNone()) + + def _generate_conversion_ast(self, parent, node): + # NOTE(kgriffs): Unroll the converter loop into + # a series of nested "if" constructs. + for field_name, converter_name, converter_argstr in node.var_converter_map: + converter_class = self._converter_map[converter_name] + + converter_obj = self._instantiate_converter( + converter_class, + converter_argstr + ) + converter_idx = len(self._converters) + self._converters.append(converter_obj) + + parent.append_child(_CxSetFragmentFromField(field_name)) + + construct = _CxIfConverterField( + field_name, + converter_idx, + ) + + parent.append_child(construct) + parent = construct + + # NOTE(kgriffs): Add remaining fields that were not + # converted, if any. + if node.num_fields > len(node.var_converter_map): + parent.append_child(_CxSetParamsFromPatternMatchPrefetched()) + + return parent def _compile(self): """Generates Python code for the entire routing tree. @@ -278,12 +390,13 @@ class CompiledRouter(object): """ src_lines = [ - 'def find(path, return_values, patterns, params):', + 'def find(path, return_values, patterns, converters, params):', _TAB_STR + 'path_len = len(path)', ] self._return_values = [] self._patterns = [] + self._converters = [] self._ast = _CxParent() self._generate_ast( @@ -307,6 +420,14 @@ class CompiledRouter(object): return scope['find'] + def _instantiate_converter(self, klass, argstr=None): + if argstr is None: + return klass() + + # NOTE(kgriffs): Don't try this at home. ;) + src = '{0}({1})'.format(klass.__name__, argstr) + return eval(src, {klass.__name__: klass}) + class CompiledRouterNode(object): """Represents a single URI segment in a URI.""" @@ -322,8 +443,13 @@ class CompiledRouterNode(object): self.is_var = False self.is_complex = False + self.num_fields = 0 + + # TODO(kgriffs): Rename these since the docs talk about "fields" + # or "field expressions", not "vars" or "variables". self.var_name = None self.var_pattern = None + self.var_converter_map = [] # NOTE(kgriffs): CompiledRouter.add_route validates field names, # so here we can just assume they are OK and use the simple @@ -334,14 +460,34 @@ class CompiledRouterNode(object): self.is_var = False else: self.is_var = True + self.num_fields = len(matches) - if len(matches) == 1 and matches[0].span() == (0, len(raw_segment)): - # NOTE(richardolsson): if there is a single variable and - # it spans the entire segment, the segment is not - # complex and the variable name is simply the string - # contained within curly braces. + for field in matches: + # NOTE(kgriffs): We already validated the field + # expression to disallow blank converter names, or names + # that don't match a known converter, so if a name is + # given, we can just go ahead and use it. + if field.group('cname'): + self.var_converter_map.append( + ( + field.group('fname'), + field.group('cname'), + field.group('argstr'), + ) + ) + + if matches[0].span() == (0, len(raw_segment)): + # NOTE(kgriffs): Single field, spans entire segment + assert len(matches) == 1 + + # TODO(kgriffs): It is not "complex" because it only + # contains a single field. Rename this variable to make + # it more descriptive. self.is_complex = False - self.var_name = raw_segment[1:-1] + + field = matches[0] + self.var_name = field.group('fname') + else: # NOTE(richardolsson): Complex segments need to be # converted into regular expressions in order to match @@ -364,12 +510,15 @@ class CompiledRouterNode(object): # trick the parser into doing the right thing. escaped_segment = re.sub(r'[\.\(\)\[\]\?\$\*\+\^\|]', r'\\\g<0>', raw_segment) - pattern_text = _FIELD_PATTERN.sub(r'(?P<\1>.+)', escaped_segment) + pattern_text = _FIELD_PATTERN.sub(r'(?P<\2>.+)', escaped_segment) pattern_text = '^' + pattern_text + '$' self.is_complex = True self.var_pattern = re.compile(pattern_text) + if self.is_complex: + assert self.is_var + def matches(self, segment): """Returns True if this node matches the supplied template segment.""" @@ -431,6 +580,111 @@ class CompiledRouterNode(object): return False +class ConverterDict(UserDict): + """A dict-like class for storing field converters.""" + + def update(self, other): + try: + # NOTE(kgriffs): If it is a mapping type, it should + # implement keys(). + names = other.keys() + except AttributeError: + # NOTE(kgriffs): Not a mapping type, so assume it is an + # iterable of 2-item iterables. But we need to make it + # re-iterable if it is a generator, for when we pass + # it on to the parent's update(). + other = list(other) + names = [n for n, __ in other] + + for n in names: + self._validate(n) + + UserDict.update(self, other) + + def __setitem__(self, name, converter): + self._validate(name) + UserDict.__setitem__(self, name, converter) + + def _validate(self, name): + if not _IDENTIFIER_PATTERN.match(name): + raise ValueError( + 'Invalid converter name. Names may not be blank, and may ' + 'only use ASCII letters, digits, and underscores. Names' + 'must begin with a letter or underscore.' + ) + + +class CompiledRouterOptions(object): + """Defines a set of configurable router options. + + An instance of this class is exposed via :any:`API.router_options` + for configuring certain :py:class:`~.CompiledRouter` behaviors. + + Attributes: + converters: Represents the collection of named + converters that may be referenced in URI template field + expressions. Adding additional converters is simply a + matter of mapping a name to a converter class:: + + api.router_options.converters['myconverter'] = MyConverter + + Note: + + Converter names may only contain ASCII letters, digits, + and underscores, and must start with either a letter or + an underscore. + + A converter is any class that implements the following + method:: + + def convert(self, fragment): + # TODO: Convert the matched URI path fragment and + # return the result, or None to reject the fragment + # if it is not in the expected format or otherwise + # can not be converted. + pass + + Converters are instantiated with the argument specification + given in the field expression. These specifications follow + the standard Python syntax for passing arguments. For + example, the comments in the following code show how a + converter would be instantiated given different + argument specifications in the URI template:: + + # MyConverter() + api.add_route( + '/a/{some_field:myconverter}', + some_resource + ) + + # MyConverter(True) + api.add_route( + '/b/{some_field:myconverter(True)}', + some_resource + ) + + # MyConverter(True, some_kwarg=10) + api.add_route( + '/c/{some_field:myconverter(True, some_kwarg=10)}', + some_resource + ) + + Warning: + + Converter instances are shared between requests. + Therefore, in threaded deployments, care must be taken + to implement custom converters in a thread-safe + manner. + """ + + __slots__ = ('converters',) + + def __init__(self): + self.converters = ConverterDict( + (name, converter) for name, converter in converters.BUILTIN + ) + + # -------------------------------------------------------------------- # AST Constructs # @@ -448,7 +702,7 @@ class _CxParent(object): def __init__(self): self._children = [] - def append(self, construct): + def append_child(self, construct): self._children.append(construct) def src(self, indentation): @@ -503,27 +757,86 @@ class _CxIfPathSegmentPattern(_CxParent): self._pattern_text = pattern_text def src(self, indentation): - lines = [] - - lines.append( + lines = [ '{0}match = patterns[{1}].match(path[{2}]) # {3}'.format( _TAB_STR * indentation, self._pattern_idx, self._segment_idx, self._pattern_text, - ) - ) - - lines.append('{0}if match is not None:'.format(_TAB_STR * indentation)) - lines.append('{0}params.update(match.groupdict())'.format( - _TAB_STR * (indentation + 1) - )) - - lines.append(self._children_src(indentation + 1)) + ), + '{0}if match is not None:'.format(_TAB_STR * indentation), + self._children_src(indentation + 1), + ] return '\n'.join(lines) +class _CxIfConverterField(_CxParent): + def __init__(self, field_name, converter_idx): + super(_CxIfConverterField, self).__init__() + self._field_name = field_name + self._converter_idx = converter_idx + + def src(self, indentation): + lines = [ + '{0}field_value = converters[{1}].convert(fragment)'.format( + _TAB_STR * indentation, + self._converter_idx, + ), + '{0}if field_value is not None:'.format(_TAB_STR * indentation), + "{0}params['{1}'] = field_value".format( + _TAB_STR * (indentation + 1), + self._field_name, + ), + self._children_src(indentation + 1), + ] + + return '\n'.join(lines) + + +class _CxSetFragmentFromField(object): + def __init__(self, field_name): + self._field_name = field_name + + def src(self, indentation): + return "{0}fragment = groups.pop('{1}')".format( + _TAB_STR * indentation, + self._field_name, + ) + + +class _CxSetFragmentFromPath(object): + def __init__(self, segment_idx): + self._segment_idx = segment_idx + + def src(self, indentation): + return '{0}fragment = path[{1}]'.format( + _TAB_STR * indentation, + self._segment_idx, + ) + + +class _CxSetParamsFromPatternMatch(object): + def src(self, indentation): + return '{0}params.update(match.groupdict())'.format( + _TAB_STR * indentation + ) + + +class _CxSetParamsFromPatternMatchPrefetched(object): + def src(self, indentation): + return '{0}params.update(groups)'.format( + _TAB_STR * indentation + ) + + +class _CxPrefetchGroupsFromPatternMatch(object): + def src(self, indentation): + return '{0}groups = match.groupdict()'.format( + _TAB_STR * indentation + ) + + class _CxReturnNone(object): def src(self, indentation): return '{0}return None'.format(_TAB_STR * indentation) diff --git a/falcon/routing/converters.py b/falcon/routing/converters.py new file mode 100644 index 0000000..51c6b27 --- /dev/null +++ b/falcon/routing/converters.py @@ -0,0 +1,64 @@ +# Copyright 2017 by Rackspace Hosting, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + + +class IntConverter(object): + """Converts a field value to an int. + + Keyword Args: + num_digits (int): Require the value to have the given + number of digits. + min (int): Reject the value if it is less than this value. + max (int): Reject the value if it is greater than this value. + """ + + __slots__ = ('_num_digits', '_min', '_max') + + def __init__(self, num_digits=None, min=None, max=None): + if num_digits is not None and num_digits < 1: + raise ValueError('num_digits must be at least 1') + + self._num_digits = num_digits + self._min = min + self._max = max + + def convert(self, fragment): + if self._num_digits is not None and len(fragment) != self._num_digits: + return None + + # NOTE(kgriffs): int() will accept numbers with preceding or + # trailing whitespace, so we need to do our own check. Using + # strip() is faster than either a regex or a series of or'd + # membership checks via "in", esp. as the length of contiguous + # numbers in the fragment grows. + if fragment.strip() != fragment: + return None + + try: + value = int(fragment) + except ValueError: + return None + + if self._min is not None and value < self._min: + return None + + if self._max is not None and value > self._max: + return None + + return value + + +BUILTIN = ( + ('int', IntConverter), +) diff --git a/tests/conftest.py b/tests/conftest.py index b6b17f2..b7d6774 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2,7 +2,6 @@ import platform import pytest - import falcon diff --git a/tests/test_default_router.py b/tests/test_default_router.py index e62ca79..062e4e8 100644 --- a/tests/test_default_router.py +++ b/tests/test_default_router.py @@ -88,6 +88,29 @@ def router(): router.add_route('/item/{q}', {}, ResourceWithId(28)) + # ---------------------------------------------------------------- + # Routes with field converters + # ---------------------------------------------------------------- + + router.add_route( + '/cvt/teams/{id:int(min=7)}', {}, ResourceWithId(29)) + router.add_route( + '/cvt/teams/{id:int(min=7)}/members', {}, ResourceWithId(30)) + router.add_route( + '/cvt/teams/default', {}, ResourceWithId(31)) + router.add_route( + '/cvt/teams/default/members/{id:int}-{tenure:int}', {}, ResourceWithId(32)) + + router.add_route( + '/cvt/repos/{org}/{repo}/compare/{usr0}:{branch0:int}...{usr1}:{branch1:int}/part', + {}, ResourceWithId(33)) + router.add_route( + '/cvt/repos/{org}/{repo}/compare/{usr0}:{branch0:int}', + {}, ResourceWithId(34)) + router.add_route( + '/cvt/repos/{org}/{repo}/compare/{usr0}:{branch0:int}/full', + {}, ResourceWithId(35)) + return router @@ -102,6 +125,19 @@ class ResourceWithId(object): resp.body = self.resource_id +class SpamConverter(object): + def __init__(self, times, eggs=False): + self._times = times + self._eggs = eggs + + def convert(self, fragment): + item = fragment + if self._eggs: + item += '&eggs' + + return ', '.join(item for i in range(self._times)) + + # ===================================================================== # Regression tests for use cases reported by users # ===================================================================== @@ -233,7 +269,7 @@ def test_root_path(): assert resource.resource_id == 42 expected_src = textwrap.dedent(""" - def find(path, return_values, patterns, params): + def find(path, return_values, patterns, converters, params): path_len = len(path) if path_len > 0: if path[0] == '': @@ -275,11 +311,12 @@ def test_match_entire_path(uri_template, path): @pytest.mark.parametrize('uri_template', [ - '/teams/{collision}', # simple vs simple + '/teams/{conflict}', # simple vs simple '/emojis/signs/{id_too}', # another simple vs simple - '/repos/{org}/{repo}/compare/{complex}:{vs}...{complex2}:{collision}', + '/repos/{org}/{repo}/compare/{complex}:{vs}...{complex2}:{conflict}', + '/teams/{id:int}/settings', # converted vs. non-converted ]) -def test_collision(router, uri_template): +def test_conflict(router, uri_template): with pytest.raises(ValueError): router.add_route(uri_template, {}, ResourceWithId(-1)) @@ -289,7 +326,7 @@ def test_collision(router, uri_template): '/repos/{complex}.{vs}.{simple}', '/repos/{org}/{repo}/compare/{complex}:{vs}...{complex2}/full', ]) -def test_non_collision(router, uri_template): +def test_non_conflict(router, uri_template): router.add_route(uri_template, {}, ResourceWithId(-1)) @@ -365,23 +402,23 @@ def test_literal_segment(router): assert route is None -@pytest.mark.parametrize('uri_template', [ +@pytest.mark.parametrize('path', [ '/teams', '/emojis/signs', '/gists', '/gists/42', ]) -def test_dead_segment(router, uri_template): - route = router.find(uri_template) +def test_dead_segment(router, path): + route = router.find(path) assert route is None -@pytest.mark.parametrize('uri_template', [ +@pytest.mark.parametrize('path', [ '/repos/racker/falcon/compare/foo', '/repos/racker/falcon/compare/foo/full', ]) -def test_malformed_pattern(router, uri_template): - route = router.find(uri_template) +def test_malformed_pattern(router, path): + route = router.find(path) assert route is None @@ -390,6 +427,58 @@ def test_literal(router): assert resource.resource_id == 8 +@pytest.mark.parametrize('path,expected_params', [ + ('/cvt/teams/007', {'id': 7}), + ('/cvt/teams/1234/members', {'id': 1234}), + ('/cvt/teams/default/members/700-5', {'id': 700, 'tenure': 5}), + ( + '/cvt/repos/org/repo/compare/xkcd:353', + {'org': 'org', 'repo': 'repo', 'usr0': 'xkcd', 'branch0': 353}, + ), + ( + '/cvt/repos/org/repo/compare/gunmachan:1234...kumamon:5678/part', + { + 'org': 'org', + 'repo': 'repo', + 'usr0': 'gunmachan', + 'branch0': 1234, + 'usr1': 'kumamon', + 'branch1': 5678, + } + ), + ( + '/cvt/repos/xkcd/353/compare/susan:0001/full', + {'org': 'xkcd', 'repo': '353', 'usr0': 'susan', 'branch0': 1}, + ) +]) +def test_converters(router, path, expected_params): + __, __, params, __ = router.find(path) + assert params == expected_params + + +@pytest.mark.parametrize('uri_template', [ + '/foo/{bar:int(0)}', + '/foo/{bar:int(num_digits=0)}', + '/foo/{bar:int(-1)}/baz', + '/foo/{bar:int(num_digits=-1)}/baz', +]) +def test_converters_with_invalid_options(router, uri_template): + # NOTE(kgriffs): Sanity-check that errors are properly bubbled up + # when calling add_route(). Additional checks can be found + # in test_uri_converters.py + with pytest.raises(ValueError): + router.add_route(uri_template, {}, ResourceWithId(1)) + + +@pytest.mark.parametrize('uri_template', [ + '/foo/{bar:}', + '/foo/{bar:unknown}/baz', +]) +def test_converters_malformed_specification(router, uri_template): + with pytest.raises(ValueError): + router.add_route(uri_template, {}, ResourceWithId(1)) + + def test_variable(router): resource, __, params, __ = router.find('/teams/42') assert resource.resource_id == 6 @@ -413,8 +502,10 @@ def test_single_character_field_name(router): @pytest.mark.parametrize('path,expected_id', [ ('/teams/default', 19), ('/teams/default/members', 7), - ('/teams/foo', 6), - ('/teams/foo/members', 7), + ('/cvt/teams/default', 31), + ('/cvt/teams/default/members/1234-10', 32), + ('/teams/1234', 6), + ('/teams/1234/members', 7), ('/gists/first', 20), ('/gists/first/raw', 18), ('/gists/first/pdf', 21), @@ -446,6 +537,11 @@ def test_literal_vs_variable(router, path, expected_id): '/teams/default/undefined', '/teams/default/undefined/segments', + # Literal vs. variable (converters) + '/cvt/teams/default/members', # 'default' can't be converted to an int + '/cvt/teams/NaN', + '/cvt/teams/default/members/NaN', + # Literal vs variable (emojis) '/emojis/signs', '/emojis/signs/0/small', @@ -512,3 +608,51 @@ def test_complex_alt(router, url_postfix, resource_id, expected_template): 'branch0': 'master', }) assert uri_template == expected_template + + +def test_options_converters_set(router): + router.options.converters['spam'] = SpamConverter + + router.add_route('/{food:spam(3, eggs=True)}', {}, ResourceWithId(1)) + resource, __, params, __ = router.find('/spam') + + assert params == {'food': 'spam&eggs, spam&eggs, spam&eggs'} + + +@pytest.mark.parametrize('converter_name', [ + 'spam', + 'spam_2' +]) +def test_options_converters_update(router, converter_name): + router.options.converters.update({ + 'spam': SpamConverter, + 'spam_2': SpamConverter, + }) + + template = '/{food:' + converter_name + '(3, eggs=True)}' + router.add_route(template, {}, ResourceWithId(1)) + resource, __, params, __ = router.find('/spam') + + assert params == {'food': 'spam&eggs, spam&eggs, spam&eggs'} + + +@pytest.mark.parametrize('name', [ + 'has whitespace', + 'whitespace ', + ' whitespace ', + ' whitespace', + 'funky$character', + '42istheanswer', + 'with-hyphen', +]) +def test_options_converters_invalid_name(router, name): + with pytest.raises(ValueError): + router.options.converters[name] = object + + +def test_options_converters_invalid_name_on_update(router): + with pytest.raises(ValueError): + router.options.converters.update({ + 'valid_name': SpamConverter, + '7eleven': SpamConverter, + }) diff --git a/tests/test_uri_converters.py b/tests/test_uri_converters.py new file mode 100644 index 0000000..66a6f71 --- /dev/null +++ b/tests/test_uri_converters.py @@ -0,0 +1,54 @@ +import string + +import pytest + +from falcon.routing import converters + + +@pytest.mark.parametrize('segment,num_digits,min,max,expected', [ + ('123', None, None, None, 123), + ('01', None, None, None, 1), + ('001', None, None, None, 1), + ('0', None, None, None, 0), + ('00', None, None, None, 00), + + ('1', 1, None, None, 1), + ('12', 1, None, None, None), + ('12', 2, None, None, 12), + + ('1', 1, 1, 1, 1), + ('1', 1, 1, None, 1), + ('1', 1, 1, 2, 1), + ('1', 1, 2, None, None), + ('1', 1, 2, 1, None), + ('2', 1, 1, 2, 2), + ('2', 1, 2, 2, 2), + ('3', 1, 1, 2, None), + + ('12', 1, None, None, None), + ('12', 1, 1, 12, None), + ('12', 2, None, None, 12), + ('12', 2, 1, 12, 12), + ('12', 2, 12, 12, 12), + ('12', 2, 13, 12, None), + ('12', 2, 13, 13, None), +]) +def test_int_filter(segment, num_digits, min, max, expected): + c = converters.IntConverter(num_digits, min, max) + assert c.convert(segment) == expected + + +@pytest.mark.parametrize('segment', ( + ['0x0F', 'something', '', ' '] + + ['123' + w for w in string.whitespace] + + [w + '123' for w in string.whitespace] +)) +def test_int_filter_malformed(segment): + c = converters.IntConverter() + assert c.convert(segment) is None + + +@pytest.mark.parametrize('num_digits', [0, -1, -10]) +def test_int_filter_invalid_config(num_digits): + with pytest.raises(ValueError): + converters.IntConverter(num_digits) diff --git a/tests/test_uri_templates.py b/tests/test_uri_templates.py index 484f296..f18c229 100644 --- a/tests/test_uri_templates.py +++ b/tests/test_uri_templates.py @@ -131,6 +131,54 @@ def test_single(client, resource, field_name): assert resource.captured_kwargs[field_name] == '123' +@pytest.mark.parametrize('uri_template,', [ + '/{id:int}', + '/{id:int(3)}', + '/{id:int(min=123)}', + '/{id:int(min=123, max=123)}', +]) +def test_converter(client, uri_template): + resource1 = IDResource() + client.app.add_route(uri_template, resource1) + + result = client.simulate_get('/123') + + assert result.status_code == 200 + assert resource1.called + assert resource1.id == 123 + assert resource1.req.path == '/123' + + +@pytest.mark.parametrize('uri_template,', [ + '/{id:int(2)}', + '/{id:int(min=124)}', + '/{id:int(num_digits=3, max=100)}', +]) +def test_converter_rejections(client, uri_template): + resource1 = IDResource() + client.app.add_route(uri_template, resource1) + + result = client.simulate_get('/123') + + assert result.status_code == 404 + assert not resource1.called + + +def test_converter_custom(client, resource): + class SpamConverter(object): + def convert(self, fragment): + return 'spam!' + + client.app.router_options.converters['spam'] = SpamConverter + client.app.add_route('/{food:spam}', resource) + + result = client.simulate_get('/something') + + assert result.status_code == 200 + assert resource.called + assert resource.captured_kwargs['food'] == 'spam!' + + def test_single_trailing_slash(client): resource1 = IDResource() client.app.add_route('/1/{id}/', resource1)