Repository: falconry/falcon
Pull request #1085 (2017-07-14 17:00:58)

fix(routing): Fix field converter argstr going past closing paren

codecov[bot]: # [Codecov](https://codecov.io/gh/falconry/falcon/pull/1085?src=pr&el=h1) Report
> Merging [#1085](https://codecov.io/gh/falconry/falcon/pull/1085?src=pr&el=desc) into [master](https://codecov.io/gh/falconry/falcon/commit/b70ae05c2d78b06e4f21d2e118371c97e8c60829?src=pr&el=desc) will **decrease** coverage by `1.18%`.
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff             @@
##           master    #1085      +/-   ##
==========================================
- Coverage     100%   98.81%   -1.19%     
==========================================
  Files          38       38              
  Lines        2354     2354              
  Branches      332      319      -13     
==========================================
- Hits         2354     2326      -28     
- Misses          0       15      +15     
- Partials        0       13      +13
```


| [Impacted Files](https://codecov.io/gh/falconry/falcon/pull/1085?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [falcon/routing/compiled.py](https://codecov.io/gh/falconry/falcon/pull/1085?src=pr&el=tree#diff-ZmFsY29uL3JvdXRpbmcvY29tcGlsZWQucHk=) | `100% <ø> (ø)` | :arrow_up: |
| [falcon/response\_helpers.py](https://codecov.io/gh/falconry/falcon/pull/1085?src=pr&el=tree#diff-ZmFsY29uL3Jlc3BvbnNlX2hlbHBlcnMucHk=) | `83.33% <0%> (-16.67%)` | :arrow_down: |
| [falcon/response.py](https://codecov.io/gh/falconry/falcon/pull/1085?src=pr&el=tree#diff-ZmFsY29uL3Jlc3BvbnNlLnB5) | `94.37% <0%> (-5.63%)` | :arrow_down: |
| [falcon/util/misc.py](https://codecov.io/gh/falconry/falcon/pull/1085?src=pr&el=tree#diff-ZmFsY29uL3V0aWwvbWlzYy5weQ==) | `96.87% <0%> (-3.13%)` | :arrow_down: |
| [falcon/testing/helpers.py](https://codecov.io/gh/falconry/falcon/pull/1085?src=pr&el=tree#diff-ZmFsY29uL3Rlc3RpbmcvaGVscGVycy5weQ==) | `97.33% <0%> (-2.67%)` | :arrow_down: |
| [falcon/request.py](https://codecov.io/gh/falconry/falcon/pull/1085?src=pr&el=tree#diff-ZmFsY29uL3JlcXVlc3QucHk=) | `98.43% <0%> (-1.57%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/falconry/falcon/pull/1085?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/1085?src=pr&el=footer). Last update [b70ae05...b81d2d9](https://codecov.io/gh/falconry/falcon/pull/1085?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/falcon/routing/compiled.py b/falcon/routing/compiled.py
index f11c057..2c6e10b 100644
--- a/falcon/routing/compiled.py
+++ b/falcon/routing/compiled.py
@@ -25,7 +25,14 @@ from falcon.routing import converters
 
 _TAB_STR = ' ' * 4
 _FIELD_PATTERN = re.compile(
-    '{((?P<fname>[^}:]*)((?P<cname_sep>:(?P<cname>[^}\(]*))(\((?P<argstr>.*)\))?)?)}'
+    # NOTE(kgriffs): This disallows the use of the '}' character within
+    # an argstr. However, we don't really have a way of escaping
+    # curly brackets in URI templates at the moment, so users should
+    # see this as a similar restriction and so somewhat unsurprising.
+    #
+    # We may want to create a contextual parser at some point to
+    # work around this problem.
+    '{((?P<fname>[^}:]*)((?P<cname_sep>:(?P<cname>[^}\(]*))(\((?P<argstr>[^}]*)\))?)?)}'
 )
 _IDENTIFIER_PATTERN = re.compile('[A-Za-z_][A-Za-z0-9_]*$')
 


Test patch:
diff --git a/tests/test_uri_templates.py b/tests/test_uri_templates.py
index 70ac510..c6d3670 100644
--- a/tests/test_uri_templates.py
+++ b/tests/test_uri_templates.py
@@ -16,7 +16,9 @@ from falcon import testing
 
 
 _TEST_UUID = uuid.uuid4()
+_TEST_UUID_2 = uuid.uuid4()
 _TEST_UUID_STR = str(_TEST_UUID)
+_TEST_UUID_STR_2 = str(_TEST_UUID_2)
 _TEST_UUID_STR_SANS_HYPHENS = _TEST_UUID_STR.replace('-', '')
 
 
@@ -209,16 +211,31 @@ def test_datetime_converter(client, resource, uri_template, path, dt_expected):
         assert resource.captured_kwargs['timestamp'] == dt_expected
 
 
-@pytest.mark.parametrize('uri_template, path, uuid_expected', [
+@pytest.mark.parametrize('uri_template, path, expected', [
     (
         '/widgets/{widget_id:uuid}',
         '/widgets/' + _TEST_UUID_STR,
-        _TEST_UUID
+        {'widget_id': _TEST_UUID}
     ),
     (
         '/widgets/{widget_id:uuid}/orders',
         '/widgets/' + _TEST_UUID_STR_SANS_HYPHENS + '/orders',
-        _TEST_UUID
+        {'widget_id': _TEST_UUID}
+    ),
+    (
+        '/versions/diff/{left:uuid()}...{right:uuid()}',
+        '/versions/diff/{0}...{1}'.format(_TEST_UUID_STR, _TEST_UUID_STR_2),
+        {'left': _TEST_UUID, 'right': _TEST_UUID_2, }
+    ),
+    (
+        '/versions/diff/{left:uuid}...{right:uuid()}',
+        '/versions/diff/{0}...{1}'.format(_TEST_UUID_STR, _TEST_UUID_STR_2),
+        {'left': _TEST_UUID, 'right': _TEST_UUID_2, }
+    ),
+    (
+        '/versions/diff/{left:uuid()}...{right:uuid}',
+        '/versions/diff/{0}...{1}'.format(_TEST_UUID_STR, _TEST_UUID_STR_2),
+        {'left': _TEST_UUID, 'right': _TEST_UUID_2, }
     ),
     (
         '/widgets/{widget_id:uuid}/orders',
@@ -226,18 +243,18 @@ def test_datetime_converter(client, resource, uri_template, path, dt_expected):
         None
     ),
 ])
-def test_uuid_converter(client, resource, uri_template, path, uuid_expected):
+def test_uuid_converter(client, resource, uri_template, path, expected):
     client.app.add_route(uri_template, resource)
 
     result = client.simulate_get(path)
 
-    if uuid_expected is None:
+    if expected is None:
         assert result.status_code == 404
         assert not resource.called
     else:
         assert result.status_code == 200
         assert resource.called
-        assert resource.captured_kwargs['widget_id'] == uuid_expected
+        assert resource.captured_kwargs == expected
 
 
 def test_uuid_converter_complex_segment(client, resource):
@@ -257,19 +274,39 @@ def test_uuid_converter_complex_segment(client, resource):
     assert resource.captured_kwargs['last'] == last_uuid
 
 
-def test_converter_custom(client, resource):
+@pytest.mark.parametrize('uri_template, path, expected', [
+    (
+        '/{food:spam}',
+        '/something',
+        {'food': 'spam!'}
+    ),
+    (
+        '/{food:spam(")")}:{food_too:spam("()")}',
+        '/bacon:eggs',
+        {'food': 'spam!', 'food_too': 'spam!'}
+    ),
+    (
+        '/({food:spam()}){food_too:spam("()")}',
+        '/(bacon)eggs',
+        {'food': 'spam!', 'food_too': 'spam!'}
+    ),
+])
+def test_converter_custom(client, resource, uri_template, path, expected):
     class SpamConverter(object):
+        def __init__(self, useless_text=None):
+            pass
+
         def convert(self, fragment):
             return 'spam!'
 
     client.app.router_options.converters['spam'] = SpamConverter
-    client.app.add_route('/{food:spam}', resource)
+    client.app.add_route(uri_template, resource)
 
-    result = client.simulate_get('/something')
+    result = client.simulate_get(path)
 
     assert result.status_code == 200
     assert resource.called
-    assert resource.captured_kwargs['food'] == 'spam!'
+    assert resource.captured_kwargs == expected
 
 
 def test_single_trailing_slash(client):