Repository: falconry/falcon
Pull request #1833 (2020-12-22 04:01:39)

fix(parse_query_string): only the first set of values for the same key are are split
Address #1777

To Do:

- [x] Decide if this is a good idea. 😄 
- [x] Add news fragment(s)
- [x] Port fix to pyx implementation
- [x] Ensure full test coverage

codecov[bot]: # [Codecov](https://codecov.io/gh/falconry/falcon/pull/1833?src=pr&el=h1) Report
> Merging [#1833](https://codecov.io/gh/falconry/falcon/pull/1833?src=pr&el=desc) (a334879) into [master](https://codecov.io/gh/falconry/falcon/commit/0be136c5e2e4591b4435877bba777e5af4069460?el=desc) (0be136c) will **decrease** coverage by `65.61%`.
> The diff coverage is `0.00%`.

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

```diff
@@             Coverage Diff              @@
##            master    #1833       +/-   ##
============================================
- Coverage   100.00%   34.38%   -65.62%     
============================================
  Files           54       54               
  Lines         5135     5144        +9     
  Branches       825      830        +5     
============================================
- Hits          5135     1769     -3366     
- Misses           0     3335     +3335     
- Partials         0       40       +40     
```


| [Impacted Files](https://codecov.io/gh/falconry/falcon/pull/1833?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [falcon/util/uri.py](https://codecov.io/gh/falconry/falcon/pull/1833/diff?src=pr&el=tree#diff-ZmFsY29uL3V0aWwvdXJpLnB5) | `29.45% <0.00%> (-70.55%)` | :arrow_down: |
| [falcon/util/reader.py](https://codecov.io/gh/falconry/falcon/pull/1833/diff?src=pr&el=tree#diff-ZmFsY29uL3V0aWwvcmVhZGVyLnB5) | `12.62% <0.00%> (-87.38%)` | :arrow_down: |
| [falcon/asgi/stream.py](https://codecov.io/gh/falconry/falcon/pull/1833/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvc3RyZWFtLnB5) | `13.58% <0.00%> (-86.42%)` | :arrow_down: |
| [falcon/asgi/structures.py](https://codecov.io/gh/falconry/falcon/pull/1833/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvc3RydWN0dXJlcy5weQ==) | `14.58% <0.00%> (-85.42%)` | :arrow_down: |
| [falcon/middleware.py](https://codecov.io/gh/falconry/falcon/pull/1833/diff?src=pr&el=tree#diff-ZmFsY29uL21pZGRsZXdhcmUucHk=) | `14.89% <0.00%> (-85.11%)` | :arrow_down: |
| [falcon/asgi/reader.py](https://codecov.io/gh/falconry/falcon/pull/1833/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvcmVhZGVyLnB5) | `16.00% <0.00%> (-84.00%)` | :arrow_down: |
| [falcon/testing/helpers.py](https://codecov.io/gh/falconry/falcon/pull/1833/diff?src=pr&el=tree#diff-ZmFsY29uL3Rlc3RpbmcvaGVscGVycy5weQ==) | `18.60% <0.00%> (-81.40%)` | :arrow_down: |
| [falcon/asgi/app.py](https://codecov.io/gh/falconry/falcon/pull/1833/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvYXBwLnB5) | `19.63% <0.00%> (-80.37%)` | :arrow_down: |
| [falcon/asgi/ws.py](https://codecov.io/gh/falconry/falcon/pull/1833/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvd3MucHk=) | `22.66% <0.00%> (-77.34%)` | :arrow_down: |
| [falcon/asgi/request.py](https://codecov.io/gh/falconry/falcon/pull/1833/diff?src=pr&el=tree#diff-ZmFsY29uL2FzZ2kvcmVxdWVzdC5weQ==) | `23.69% <0.00%> (-76.31%)` | :arrow_down: |
| ... and [37 more](https://codecov.io/gh/falconry/falcon/pull/1833/diff?src=pr&el=tree-more) | |

------

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

kgriffs: @vytas7 if this PR seems like a good idea, do you want to contribute the cython fix?
vytas7: Sure. It should be fairly simple since that code path is not fully cythonized.

And I do think it is a good idea, I'm not sure I understood the original intent tbh. It could have been an optimization, but the case itself seems to be quite uncommon: normally you would choose either or.
vytas7: @kgriffs Done!
While at it, I've also removed one roundtrip via Python call, but I haven't benchmarked much. It _should_ be a slight improvement, although the whole csv path isn't particularly optimized, it's rather cythonized Python code.
kgriffs: >  but the case itself seems to be quite uncommon: normally you would choose either or.

Agreed. However we have a reputation to uphold. 😸 
kgriffs: @vytas7 thanks for your help on this!

Patch:
diff --git a/docs/_newsfragments/1597.bugfix.rst b/docs/_newsfragments/1597.bugfix.rst
new file mode 100644
index 0000000..c3c8604
--- /dev/null
+++ b/docs/_newsfragments/1597.bugfix.rst
@@ -0,0 +1,5 @@
+When :attr:`~falcon.RequestOptions.auto_parse_qs_csv` is enabled, the framework
+now correctly parses all occurrences of the same parameter in the query string,
+rather than only splitting the values in the first occurrence. For example,
+whereas previously ``t=1,2&t=3,4`` would become ``['1', '2', '3,4']``, now the
+resulting list will be ``['1', '2', '3', '4']``
diff --git a/falcon/cyutil/uri.pyx b/falcon/cyutil/uri.pyx
index 47662fb..da3b2b2 100644
--- a/falcon/cyutil/uri.pyx
+++ b/falcon/cyutil/uri.pyx
@@ -1,4 +1,4 @@
-# Copyright 2019 by Vytautas Liuolia.
+# Copyright 2019-2020 by Vytautas Liuolia.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -101,7 +101,7 @@ cdef unicode cy_decode(unsigned char* data, Py_ssize_t start, Py_ssize_t end,
         PyMem_Free(result)
 
 
-cdef cy_handle_csv(dict result, bint keep_blank, unicode key, unicode value):
+cdef cy_handle_csv(dict result, bint keep_blank, unicode key, bytes value):
     # NOTE(kgriffs): Falcon supports a more compact form of lists, in which the
     # elements are comma-separated and assigned to a single param instance. If
     # it turns out that very few people use this, it can be deprecated at some
@@ -111,27 +111,39 @@ cdef cy_handle_csv(dict result, bint keep_blank, unicode key, unicode value):
     # option so we largely reimplement the unoptimized Python version here.
 
     cdef old_value = result.get(key)
+    cdef list additional_values
+    cdef unicode decoded
 
     # NOTE(steffgrez): Falcon decodes value at the last moment. So query parser
     # won't mix up between percent-encoded comma (as value) and comma-separated
     # list (as reserved character for sub-delimiter).
-    if old_value is None:
-        if ',' in value:
-            if not keep_blank:
-                # NOTE(kgriffs): Normalize the result in the case that some
-                # elements are empty strings, such that the result will be the
-                # same for 'foo=1,,3' as 'foo=1&foo=&foo=3'.
-                result[key] = [decode(element) for element in value.split(',')
-                               if element]
-            else:
-                result[key] = [decode(element) for element in value.split(',')]
+    if b',' in value:
+        # NOTE(kgriffs,vytas): Normalize the result in the case that some
+        #   elements are empty strings, such that the result will be the same
+        #   for 'foo=1,,3' as 'foo=1&foo=&foo=3'
+        #   (but only if keep_blank is set to False).
+        additional_values = [
+            cy_decode(element, 0, len(element), 0, True)
+            for element in value.split(b',') if keep_blank or element
+        ]
+
+        if old_value is None:
+            result[key] = additional_values
+        elif isinstance(old_value, list):
+            old_value.extend(additional_values)
         else:
-            result[key] = decode(value)
+            additional_values.insert(0, old_value)
+            result[key] = additional_values
 
-    elif isinstance(old_value, list):
-        old_value.append(decode(value))
     else:
-        result[key] = [old_value, decode(value)]
+        decoded = cy_decode(value, 0, len(value), 0, True)
+
+        if old_value is None:
+            result[key] = decoded
+        elif isinstance(old_value, list):
+            old_value.append(decoded)
+        else:
+            result[key] = [old_value, decoded]
 
 
 cdef cy_parse_query_string(unsigned char* data, Py_ssize_t length,
@@ -165,8 +177,7 @@ cdef cy_parse_query_string(unsigned char* data, Py_ssize_t length,
                 if partition >= 0:
                     key = cy_decode(data, start, partition, encoded_start_key, True)
                     if csv and encoded_start_val >= 0:
-                        cy_handle_csv(result, keep_blank, key,
-                                      data[partition+1:pos].decode())
+                        cy_handle_csv(result, keep_blank, key, data[partition+1:pos])
                         start = pos + 1
                         encoded_start_key = -1
                         encoded_start_val = -1
@@ -218,8 +229,7 @@ cdef cy_parse_query_string(unsigned char* data, Py_ssize_t length,
         if partition >= 0:
             key = cy_decode(data, start, partition, encoded_start_key, True)
             if csv and encoded_start_val >= 0:
-                cy_handle_csv(result, keep_blank, key,
-                              data[partition+1:length].decode())
+                cy_handle_csv(result, keep_blank, key, data[partition+1:length])
                 return result
 
             value = cy_decode(data, partition+1, length, encoded_start_val, True)
diff --git a/falcon/request.py b/falcon/request.py
index bb077b6..f2eb78d 100644
--- a/falcon/request.py
+++ b/falcon/request.py
@@ -1957,14 +1957,17 @@ class RequestOptions:
                 also http://goo.gl/6rlcux).
 
         auto_parse_qs_csv: Set to ``True`` to split query string values on
-            any non-percent-encoded commas (default ``False``). When ``False``,
+            any non-percent-encoded commas (default ``False``).
+
+            When ``False``,
             values containing commas are left as-is. In this mode, list items
             are taken only from multiples of the same parameter name within the
-            query string (i.e. ``/?t=1,2,3&t=4`` becomes ``['1,2,3', '4']``).
+            query string (i.e. ``t=1,2,3&t=4`` becomes ``['1,2,3', '4']``).
+
             When `auto_parse_qs_csv` is set to ``True``, the query string value
             is also split on non-percent-encoded commas and these items
-            are added to the final list (i.e. ``/?t=1,2,3&t=4``
-            becomes ``['1', '2', '3', '4']``).
+            are added to the final list (i.e. ``t=1,2,3&t=4,5``
+            becomes ``['1', '2', '3', '4', '5']``).
 
             Warning:
                 Enabling this option will cause the framework to misinterpret
diff --git a/falcon/util/uri.py b/falcon/util/uri.py
index c7cf2e0..22a6e8d 100644
--- a/falcon/util/uri.py
+++ b/falcon/util/uri.py
@@ -346,13 +346,36 @@ def parse_query_string(query_string, keep_blank=False, csv=True):
             # a list, or append the next value to the list.
             old_value = params[k]
 
-            if is_encoded:
-                v = decode(v)
+            if csv and ',' in v:
+                # NOTE(kgriffs): Falcon supports a more compact form of
+                # lists, in which the elements are comma-separated and
+                # assigned to a single param instance. If it turns out that
+                # very few people use this, it can be deprecated at some
+                # point.
+                v = v.split(',')
+
+                if not keep_blank:
+                    # NOTE(kgriffs): Normalize the result in the case that
+                    # some elements are empty strings, such that the result
+                    # will be the same for 'foo=1,,3' as 'foo=1&foo=&foo=3'.
+                    additional_values = [decode(element) for element in v if element]
+                else:
+                    additional_values = [decode(element) for element in v]
+
+                if isinstance(old_value, list):
+                    old_value.extend(additional_values)
+                else:
+                    additional_values.insert(0, old_value)
+                    params[k] = additional_values
 
-            if isinstance(old_value, list):
-                old_value.append(v)
             else:
-                params[k] = [old_value, v]
+                if is_encoded:
+                    v = decode(v)
+
+                if isinstance(old_value, list):
+                    old_value.append(v)
+                else:
+                    params[k] = [old_value, v]
 
         else:
             if csv and ',' in v:


Test patch:
diff --git a/tests/test_query_params.py b/tests/test_query_params.py
index 78284af..9a4def2 100644
--- a/tests/test_query_params.py
+++ b/tests/test_query_params.py
@@ -182,6 +182,60 @@ class TestQueryParams:
         assert req.get_param('id') in ['23', '42', '2']
         assert req.get_param_as_list('id', int) == [23, 42, 2]
 
+    def test_option_auto_parse_qs_csv_multiple_fields_false(
+        self,
+        simulate_request,
+        client,
+        resource
+    ):
+        client.app.add_route('/', resource)
+
+        query_string = 't=1,2&t=3,4'
+        simulate_request(client=client, path='/', query_string=query_string)
+
+        req = resource.captured_req
+
+        assert req.params['t'] == ['1,2', '3,4']
+        assert req.get_param('t') in ['1,2', '3,4']
+        assert req.get_param_as_list('t') == ['1,2', '3,4']
+
+    @pytest.mark.parametrize(
+        'qs, keep_blank, expected',
+        [
+            ('t=1&t=3,4', False, ['1', '3', '4']),
+            ('t=1&t=2&t=3,4', False, ['1', '2', '3', '4']),
+            ('t=1,2&t=3,4', False, ['1', '2', '3', '4']),
+            ('t=1,,2&t=3,4', False, ['1', '2', '3', '4']),
+            ('t=1,,2&t=3,4', True, ['1', '', '2', '3', '4']),
+            ('t=1,2&t=3,4,,5', False, ['1', '2', '3', '4', '5']),
+            ('t=1&t=,1,4,,5', False, ['1', '1', '4', '5']),
+            ('t=1&t=,1,4,,5', True, ['1', '', '1', '4', '', '5']),
+            ('t=1&t=,1,4,,5&t=a,b,c', True, ['1', '', '1', '4', '', '5', 'a', 'b', 'c']),
+        ]
+    )
+    def test_option_auto_parse_qs_csv_multiple_fields_true(
+        self,
+        simulate_request,
+        client,
+        resource,
+
+        qs,
+        keep_blank,
+        expected,
+    ):
+        client.app.add_route('/', resource)
+        client.app.req_options.auto_parse_qs_csv = True
+        client.app.req_options.keep_blank_qs_values = keep_blank
+
+        query_string = qs
+        simulate_request(client=client, path='/', query_string=query_string)
+
+        req = resource.captured_req
+
+        assert req.params['t'] == expected
+        assert req.get_param('t') in expected
+        assert req.get_param_as_list('t') == expected
+
     def test_option_auto_parse_qs_csv_complex_false(self, simulate_request, client, resource):
         client.app.add_route('/', resource)
         client.app.req_options.auto_parse_qs_csv = False