Repository: falconry/falcon

Issue:
Parameters unexpectedly captured from field expressions in multiple routes
Based on a report by @tim-clipchamp on Gitter.

Consider the following example
```python
import falcon
import falcon.testing


class Resource:
    def __init__(self, name):
        self.name = name

    def on_get(self, req, res, **kw):
        print(self.name, kw)


api = falcon.API()
api.add_route("/upload/{service}/auth/token", Resource('service'))
api.add_route("/upload/youtube/{project_id}/share", Resource('project'))

client = falcon.testing.TestClient(api)
client.simulate_get('/upload/youtube/auth/token')
```
which produces the following output (tested with Falcon 1.4.1, 2.0 and 3.0.0a2; the 3.0 series also emit the expected `API` deprecation warning):
```
service {'project_id': 'auth', 'service': 'youtube'}
```

The expectation is that only field expressions from the matched route should be used.
Instead, the observed result here is that the `{project_id}` field expression from a non-matching route is also present in the evaluated responder parameters.

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/falconry/falcon/pull/1780?src=pr&el=h1) Report
> Merging [#1780](https://codecov.io/gh/falconry/falcon/pull/1780?src=pr&el=desc) into [master](https://codecov.io/gh/falconry/falcon/commit/820c51b5f0af8d1c6e9fb8cccb560b6366f97332?el=desc) will **decrease** coverage by `31.77%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/falconry/falcon/pull/1780/graphs/tree.svg?width=650&height=150&src=pr&token=zOtkfNQ3h6)](https://codecov.io/gh/falconry/falcon/pull/1780?src=pr&el=tree)

```diff
@@             Coverage Diff              @@
##            master    #1780       +/-   ##
============================================
- Coverage   100.00%   68.22%   -31.78%     
============================================
  Files           53       53               
  Lines         4445     4481       +36     
  Branches       701      702        +1     
============================================
- Hits          4445     3057     -1388     
- Misses           0     1414     +1414     
- Partials         0       10       +10     
```


| [Impacted Files](https://codecov.io/gh/falconry/falcon/pull/1780?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [falcon/routing/compiled.py](https://codecov.io/gh/falconry/falcon/pull/1780/diff?src=pr&el=tree#diff-ZmFsY29uL3JvdXRpbmcvY29tcGlsZWQucHk=) | `95.68% <100.00%> (-4.32%)` | :arrow_down: |
| [falcon/asgi/app.py](https://codecov.io/gh/falconry/falcon/pull/1780/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvYXBwLnB5) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/reader.py](https://codecov.io/gh/falconry/falcon/pull/1780/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvcmVhZGVyLnB5) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/stream.py](https://codecov.io/gh/falconry/falcon/pull/1780/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvc3RyZWFtLnB5) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/request.py](https://codecov.io/gh/falconry/falcon/pull/1780/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvcmVxdWVzdC5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/response.py](https://codecov.io/gh/falconry/falcon/pull/1780/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvcmVzcG9uc2UucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/multipart.py](https://codecov.io/gh/falconry/falcon/pull/1780/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvbXVsdGlwYXJ0LnB5) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/structures.py](https://codecov.io/gh/falconry/falcon/pull/1780/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvc3RydWN0dXJlcy5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/asgi/\_request\_helpers.py](https://codecov.io/gh/falconry/falcon/pull/1780/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvX3JlcXVlc3RfaGVscGVycy5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [falcon/testing/helpers.py](https://codecov.io/gh/falconry/falcon/pull/1780/diff?src=pr&el=tree#diff-ZmFsY29uL3Rlc3RpbmcvaGVscGVycy5weQ==) | `47.58% <0.00%> (-52.42%)` | :arrow_down: |
| ... and [24 more](https://codecov.io/gh/falconry/falcon/pull/1780/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/falconry/falcon/pull/1780?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/falconry/falcon/pull/1780?src=pr&el=footer). Last update [820c51b...c097341](https://codecov.io/gh/falconry/falcon/pull/1780?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

CaselIT: > I have not reviewed the actual compilation logic, because it is not easy to follow.
> I'll try to test it instead.

Checking the generated code is quite useful. See the [gist](https://gist.github.com/CaselIT/8bc22a248b6c71c89940676d2e86b344) as an example

Patch:
diff --git a/docs/_newsfragments/1779.bugfix.rst b/docs/_newsfragments/1779.bugfix.rst
new file mode 100644
index 0000000..beeb601
--- /dev/null
+++ b/docs/_newsfragments/1779.bugfix.rst
@@ -0,0 +1,2 @@
+The :class:`falcon.routing.CompiledRouter` no longer mistakenly sets route parameters
+while exploring non matching routes.
\ No newline at end of file
diff --git a/falcon/routing/compiled.py b/falcon/routing/compiled.py
index 6a06858..cdc6cd1 100644
--- a/falcon/routing/compiled.py
+++ b/falcon/routing/compiled.py
@@ -26,6 +26,8 @@ from falcon.routing.util import map_http_methods, set_default_responders
 from falcon.util.misc import is_python_func
 from falcon.util.sync import _should_wrap_non_coroutines, wrap_sync_to_async
 
+if False:  # TODO: switch to TYPE_CHECKING once support for py3.5 is dropped
+    from typing import Any
 
 _TAB_STR = ' ' * 4
 _FIELD_PATTERN = re.compile(
@@ -368,8 +370,21 @@ class CompiledRouter:
                     msg = 'Cannot instantiate converter "{}"'.format(name)
                     raise ValueError(msg) from e
 
-    def _generate_ast(self, nodes, parent, return_values, patterns, level=0, fast_return=True):
+    def _generate_ast(
+        self,
+        nodes: list,
+        parent,
+        return_values: list,
+        patterns: list,
+        params_stack: list,
+        level=0,
+        fast_return=True
+    ):
         """Generate a coarse AST for the router."""
+        # NOTE(caselit): setting of the parameters in the params dict is delayed until
+        # a match has been found by adding them to the param_stack. This way superfluous
+        # parameters are not set to the params dict while descending on branches that
+        # ultimately do not match.
 
         # NOTE(kgriffs): Base case
         if not nodes:
@@ -400,7 +415,11 @@ class CompiledRouter:
 
                 fast_return = not found_var_nodes
 
+        construct = None  # type: Any
+        setter = None  # type: Any
+        original_params_stack = params_stack.copy()
         for node in nodes:
+            params_stack = original_params_stack.copy()
             if node.is_var:
                 if node.is_complex:
                     # NOTE(richardolsson): Complex nodes are nodes which
@@ -417,10 +436,13 @@ class CompiledRouter:
 
                     if node.var_converter_map:
                         parent.append_child(_CxPrefetchGroupsFromPatternMatch())
-                        parent = self._generate_conversion_ast(parent, node)
+                        parent = self._generate_conversion_ast(parent, node, params_stack)
 
                     else:
-                        parent.append_child(_CxSetParamsFromPatternMatch())
+                        construct = _CxVariableFromPatternMatch(len(params_stack) + 1)
+                        setter = _CxSetParamsFromDict(construct.dict_variable_name)
+                        params_stack.append(setter)
+                        parent.append_child(construct)
 
                 else:
                     # NOTE(kgriffs): Simple nodes just capture the entire path
@@ -442,15 +464,14 @@ class CompiledRouter:
                         converter_idx = len(self._converters)
                         self._converters.append(converter_obj)
 
-                        construct = _CxIfConverterField(
-                            field_name,
-                            converter_idx,
-                        )
+                        construct = _CxIfConverterField(len(params_stack) + 1, converter_idx)
+                        setter = _CxSetParamFromValue(field_name, construct.field_variable_name)
+                        params_stack.append(setter)
 
                         parent.append_child(construct)
                         parent = construct
                     else:
-                        parent.append_child(_CxSetParam(node.var_name, level))
+                        params_stack.append(_CxSetParamFromPath(node.var_name, level))
 
                     # NOTE(kgriffs): We don't allow multiple simple var nodes
                     # to exist at the same level, e.g.:
@@ -479,6 +500,7 @@ class CompiledRouter:
                 parent,
                 return_values,
                 patterns,
+                params_stack.copy(),
                 level + 1,
                 fast_return
             )
@@ -491,6 +513,8 @@ class CompiledRouter:
                 # the segments for the requested route; otherwise we could
                 # mistakenly match "/foo/23/bar" against "/foo/{id}".
                 construct = _CxIfPathLength('==', level + 1)
+                for params in params_stack:
+                    construct.append_child(params)
                 construct.append_child(_CxReturnValue(resource_idx))
                 parent.append_child(construct)
 
@@ -502,7 +526,9 @@ class CompiledRouter:
         if not found_simple and fast_return:
             parent.append_child(_CxReturnNone())
 
-    def _generate_conversion_ast(self, parent, node):
+    def _generate_conversion_ast(self, parent, node: 'CompiledRouterNode', params_stack: list):
+        construct = None  # type: Any
+        setter = None  # type: Any
         # 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:
@@ -517,10 +543,9 @@ class CompiledRouter:
 
             parent.append_child(_CxSetFragmentFromField(field_name))
 
-            construct = _CxIfConverterField(
-                field_name,
-                converter_idx,
-            )
+            construct = _CxIfConverterField(len(params_stack) + 1, converter_idx)
+            setter = _CxSetParamFromValue(field_name, construct.field_variable_name)
+            params_stack.append(setter)
 
             parent.append_child(construct)
             parent = construct
@@ -528,7 +553,10 @@ class CompiledRouter:
         # NOTE(kgriffs): Add remaining fields that were not
         # converted, if any.
         if node.num_fields > len(node.var_converter_map):
-            parent.append_child(_CxSetParamsFromPatternMatchPrefetched())
+            construct = _CxVariableFromPatternMatchPrefetched(len(params_stack) + 1)
+            setter = _CxSetParamsFromDict(construct.dict_variable_name)
+            params_stack.append(setter)
+            parent.append_child(construct)
 
         return parent
 
@@ -553,7 +581,8 @@ class CompiledRouter:
             self._roots,
             self._ast,
             self._return_values,
-            self._patterns
+            self._patterns,
+            params_stack=[]
         )
 
         src_lines.append(self._ast.src(0))
@@ -859,7 +888,7 @@ class _CxParent:
 
 class _CxIfPathLength(_CxParent):
     def __init__(self, comparison, length):
-        super(_CxIfPathLength, self).__init__()
+        super().__init__()
         self._comparison = comparison
         self._length = length
 
@@ -875,7 +904,7 @@ class _CxIfPathLength(_CxParent):
 
 class _CxIfPathSegmentLiteral(_CxParent):
     def __init__(self, segment_idx, literal):
-        super(_CxIfPathSegmentLiteral, self).__init__()
+        super().__init__()
         self._segment_idx = segment_idx
         self._literal = literal
 
@@ -891,7 +920,7 @@ class _CxIfPathSegmentLiteral(_CxParent):
 
 class _CxIfPathSegmentPattern(_CxParent):
     def __init__(self, segment_idx, pattern_idx, pattern_text):
-        super(_CxIfPathSegmentPattern, self).__init__()
+        super().__init__()
         self._segment_idx = segment_idx
         self._pattern_idx = pattern_idx
         self._pattern_text = pattern_text
@@ -912,21 +941,22 @@ class _CxIfPathSegmentPattern(_CxParent):
 
 
 class _CxIfConverterField(_CxParent):
-    def __init__(self, field_name, converter_idx):
-        super(_CxIfConverterField, self).__init__()
-        self._field_name = field_name
+    def __init__(self, unique_idx, converter_idx):
+        super().__init__()
         self._converter_idx = converter_idx
+        self._unique_idx = unique_idx
+        self.field_variable_name = 'field_value_{0}'.format(unique_idx)
 
     def src(self, indentation):
         lines = [
-            '{0}field_value = converters[{1}].convert(fragment)'.format(
+            '{0}{1} = converters[{2}].convert(fragment)'.format(
                 _TAB_STR * indentation,
+                self.field_variable_name,
                 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,
+            '{0}if {1} is not None:'.format(
+                _TAB_STR * indentation,
+                self.field_variable_name
             ),
             self._children_src(indentation + 1),
         ]
@@ -956,17 +986,27 @@ class _CxSetFragmentFromPath:
         )
 
 
-class _CxSetParamsFromPatternMatch:
+class _CxVariableFromPatternMatch:
+    def __init__(self, unique_idx):
+        self._unique_idx = unique_idx
+        self.dict_variable_name = 'dict_match_{0}'.format(unique_idx)
+
     def src(self, indentation):
-        return '{0}params.update(match.groupdict())'.format(
-            _TAB_STR * indentation
+        return '{0}{1} = match.groupdict()'.format(
+            _TAB_STR * indentation,
+            self.dict_variable_name
         )
 
 
-class _CxSetParamsFromPatternMatchPrefetched:
+class _CxVariableFromPatternMatchPrefetched:
+    def __init__(self, unique_idx):
+        self._unique_idx = unique_idx
+        self.dict_variable_name = 'dict_groups_{0}'.format(unique_idx)
+
     def src(self, indentation):
-        return '{0}params.update(groups)'.format(
-            _TAB_STR * indentation
+        return '{0}{1} = groups'.format(
+            _TAB_STR * indentation,
+            self.dict_variable_name
         )
 
 
@@ -993,7 +1033,7 @@ class _CxReturnValue:
         )
 
 
-class _CxSetParam:
+class _CxSetParamFromPath:
     def __init__(self, param_name, segment_idx):
         self._param_name = param_name
         self._segment_idx = segment_idx
@@ -1004,3 +1044,27 @@ class _CxSetParam:
             self._param_name,
             self._segment_idx,
         )
+
+
+class _CxSetParamFromValue:
+    def __init__(self, param_name, field_value_name):
+        self._param_name = param_name
+        self._field_value_name = field_value_name
+
+    def src(self, indentation):
+        return "{0}params['{1}'] = {2}".format(
+            _TAB_STR * indentation,
+            self._param_name,
+            self._field_value_name,
+        )
+
+
+class _CxSetParamsFromDict:
+    def __init__(self, dict_value_name):
+        self._dict_value_name = dict_value_name
+
+    def src(self, indentation):
+        return '{0}params.update({1})'.format(
+            _TAB_STR * indentation,
+            self._dict_value_name,
+        )


Test patch:
diff --git a/tests/test_default_router.py b/tests/test_default_router.py
index 13c357e..50c0ef5 100644
--- a/tests/test_default_router.py
+++ b/tests/test_default_router.py
@@ -653,3 +653,31 @@ def test_options_converters_invalid_name_on_update(router):
             'valid_name': SpamConverter,
             '7eleven': SpamConverter,
         })
+
+
+@pytest.fixture
+def param_router():
+    r = DefaultRouter()
+
+    r.add_route('/c/foo/{bar}/baz', ResourceWithId(1))
+    r.add_route('/c/{foo}/bar/other', ResourceWithId(2))
+    r.add_route('/c/foo/{a:int}-{b}/a', ResourceWithId(3))
+    r.add_route('/upload/{service}/auth/token', ResourceWithId(4))
+    r.add_route('/upload/youtube/{project_id}/share', ResourceWithId(5))
+    r.add_route('/x/y/{a}.{b}/z', ResourceWithId(6))
+    r.add_route('/x/{y}/o.o/w', ResourceWithId(7))
+    return r
+
+
+@pytest.mark.parametrize('route, expected, num', (
+    ('/c/foo/arg/baz', {'bar': 'arg'}, 1),
+    ('/c/foo/bar/other', {'foo': 'foo'}, 2),
+    ('/c/foo/42-7/baz', {'bar': '42-7'}, 1),
+    ('/upload/youtube/auth/token', {'service': 'youtube'}, 4),
+    ('/x/y/o.o/w', {'y': 'y'}, 7),
+))
+def test_params_in_non_taken_branches(param_router, route, expected, num):
+    resource, __, params, __ = param_router.find(route)
+
+    assert resource.resource_id == num
+    assert params == expected