Repository: falconry/falcon

Issue:
Add option to opt-out from comma separated value parsing
I'm porting a project to Falcon and I stumbled upon an issue regarding its parsing of CSV values inside URIs. Let's say I have filtering engine that accepts queries such as this:

    http://great.dude/api/cars?query=added:yesterday,today+spoilers:red

I obviously want to make `req.get_param('query')` return `'added:yesterday,today spoilers:red'`, and not `['added:yesterday', 'today spoilers:red']`.

Right now this [isn't really configurable](https://github.com/falconry/falcon/blob/35987b2be85456f431bbda509e884a8b0b20ed11/falcon/util/uri.py#L312-L328) and I need to check if `get_param()` returns a `list` and then join it back if needed, which looks sort of silly. Fortunately, the ability to use custom request classes alleviates the issue to some extent.

I see a few ways to improve things upstream:

1. Offer explicit `get_param_as_string` that will possibly do `','.join(...)` under the hood.
2. Add an option to disable this mechanism as an additional option to `Api`.
3. Add an option to disable this mechanism as an additional option to `add_route()`.
4. Change `get_param` to always return string.

Option 4 makes the most sense to me, but it breaks BC. If option 4 is not feasible, I'd went with option 1.

Discussion:
kgriffs: Looks like we accidentally had some overlap in effort between #729 and #811. I attempted to combine the two into a new PR with a few tweeks to param naming and docstrings. Everyone please take a look and provide feedback. Thanks!
codecov-io: ## [Current coverage][cc-pull] is **100%**
> Merging [#820][cc-pull] into [master][cc-base-branch] will not change coverage

```diff
@@             master       #820   diff @@
==========================================
  Files            29         29          
  Lines          1789       1799    +10   
  Methods           0          0          
  Messages          0          0          
  Branches        299        303     +4   
==========================================
+ Hits           1789       1799    +10   
  Misses            0          0          
  Partials          0          0          
```

> Powered by [Codecov](https://codecov.io?src=pr). Last updated by [cf3cb50...d4e630a][cc-compare]
[cc-base-branch]: https://codecov.io/gh/falconry/falcon/branch/master?src=pr
[cc-compare]: https://codecov.io/gh/falconry/falcon/compare/cf3cb5029a51d4b7c980c7851328a02046db9b3e...d4e630a70cd1774519851a78a1d1fb80a83e8b7e
[cc-pull]: https://codecov.io/gh/falconry/falcon/pull/820?src=pr
orcsly: Looks good, thanks!
jmvrbanac: :+1: 
qwesda: The parameter name `csv` for the `parse_query_string` could have a more descriptive name. `parse_qs_csv` sounds logical to me, since the option general option is called `auto_parse_qs_csv`.

One evaluation in `parse_query_string` can be short-circuited:
[https://github.com/falconry/falcon/pull/820/files#diff-7d2a078ae72702ba816f18a9aa1c48b9R319](https://github.com/falconry/falcon/pull/820/files#diff-7d2a078ae72702ba816f18a9aa1c48b9R319)

Otherwise it looks good.
kgriffs: @qwesda since the method name already implies we are working with parsing query strings, is it necessary to also include that in the name of the kwarg?
qwesda: @kgriffs: I just thought something more explicit would be more in line with `keep_blank_qs_values`, which is pretty verbose. Having one verbose param and one very non-verbose seemed weird. 
kgriffs: @qwesda, ah, good point! I'll switch to `parse_qs_csv` and we can see how that looks.
kgriffs: @qwesda @jmvrbanac @orcsly I think this is ready for final review.
qwesda: @kgriffs looks ok
orcsly: Yup looks good.  Thanks!

Patch:
diff --git a/falcon/request.py b/falcon/request.py
index 597ac80..7359991 100644
--- a/falcon/request.py
+++ b/falcon/request.py
@@ -284,6 +284,7 @@ class Request(object):
                 self._params = parse_query_string(
                     self.query_string,
                     keep_blank_qs_values=self.options.keep_blank_qs_values,
+                    parse_qs_csv=self.options.auto_parse_qs_csv,
                 )
 
             else:
@@ -1153,6 +1154,7 @@ class Request(object):
             extra_params = parse_query_string(
                 body,
                 keep_blank_qs_values=self.options.keep_blank_qs_values,
+                parse_qs_csv=self.options.auto_parse_qs_csv,
             )
 
             self._params.update(extra_params)
@@ -1190,8 +1192,11 @@ class RequestOptions(object):
     """This class is a container for ``Request`` options.
 
     Attributes:
-        keep_blank_qs_values (bool): Set to ``True`` in order to retain
-            blank values in query string parameters (default ``False``).
+        keep_blank_qs_values (bool): Set to ``True`` to keep query string
+            fields even if they do not have a value (default ``False``).
+            For comma-separated values, this option also determines
+            whether or not empty elements in the parsed list are
+            retained.
         auto_parse_form_urlencoded: Set to ``True`` in order to
             automatically consume the request stream and merge the
             results into the request's query string params when the
@@ -1202,18 +1207,29 @@ class RequestOptions(object):
             Note:
                 The character encoding for fields, before
                 percent-encoding non-ASCII bytes, is assumed to be
-                UTF-8. The special `_charset_` field is ignored if present.
+                UTF-8. The special `_charset_` field is ignored if
+                present.
 
                 Falcon expects form-encoded request bodies to be
                 encoded according to the standard W3C algorithm (see
                 also http://goo.gl/6rlcux).
 
+        auto_parse_qs_csv: Set to ``False`` to treat commas in a query
+            string value as literal characters, rather than as a comma-
+            separated list (default ``True``). When this option is
+            enabled, the value will be split on any non-percent-encoded
+            commas. Disable this option when encoding lists as multiple
+            occurrences of the same parameter, and when values may be
+            encoded in alternative formats in which the comma character
+            is significant.
     """
     __slots__ = (
         'keep_blank_qs_values',
         'auto_parse_form_urlencoded',
+        'auto_parse_qs_csv',
     )
 
     def __init__(self):
         self.keep_blank_qs_values = False
         self.auto_parse_form_urlencoded = False
+        self.auto_parse_qs_csv = True
diff --git a/falcon/util/misc.py b/falcon/util/misc.py
index 5b02f05..12eb481 100644
--- a/falcon/util/misc.py
+++ b/falcon/util/misc.py
@@ -148,7 +148,7 @@ def http_date_to_dt(http_date, obs_date=False):
     raise ValueError('time data %r does not match known formats' % http_date)
 
 
-def to_query_str(params):
+def to_query_str(params, comma_delimited_lists=True):
     """Converts a dictionary of params to a query string.
 
     Args:
@@ -157,6 +157,10 @@ def to_query_str(params):
             something that can be converted into a ``str``. If `params`
             is a ``list``, it will be converted to a comma-delimited string
             of values (e.g., 'thing=1,2,3')
+        comma_delimited_lists (bool, default ``True``):
+            If set to ``False`` encode lists by specifying multiple instances
+            of the parameter (e.g., 'thing=1&thing=2&thing=3')
+
 
     Returns:
         str: A URI query string including the '?' prefix, or an empty string
@@ -175,7 +179,20 @@ def to_query_str(params):
         elif v is False:
             v = 'false'
         elif isinstance(v, list):
-            v = ','.join(map(str, v))
+            if comma_delimited_lists:
+                v = ','.join(map(str, v))
+            else:
+                for list_value in v:
+                    if list_value is True:
+                        list_value = 'true'
+                    elif list_value is False:
+                        list_value = 'false'
+                    else:
+                        list_value = str(list_value)
+
+                    query_str += k + '=' + list_value + '&'
+
+                continue
         else:
             v = str(v)
 
diff --git a/falcon/util/uri.py b/falcon/util/uri.py
index 2f68ec9..63ca45e 100644
--- a/falcon/util/uri.py
+++ b/falcon/util/uri.py
@@ -246,11 +246,12 @@ else:
         return decoded_uri.decode('utf-8', 'replace')
 
 
-def parse_query_string(query_string, keep_blank_qs_values=False):
+def parse_query_string(query_string, keep_blank_qs_values=False,
+                       parse_qs_csv=True):
     """Parse a query string into a dict.
 
     Query string parameters are assumed to use standard form-encoding. Only
-    parameters with values are parsed. for example, given 'foo=bar&flag',
+    parameters with values are returned. For example, given 'foo=bar&flag',
     this function would ignore 'flag' unless the `keep_blank_qs_values` option
     is set.
 
@@ -269,8 +270,16 @@ def parse_query_string(query_string, keep_blank_qs_values=False):
 
     Args:
         query_string (str): The query string to parse.
-        keep_blank_qs_values (bool): If set to ``True``, preserves boolean
-            fields and fields with no content as blank strings.
+        keep_blank_qs_values (bool): Set to ``True`` to return fields even if
+            they do not have a value (default ``False``). For comma-separated
+            values, this option also determines whether or not empty elements
+            in the parsed list are retained.
+        parse_qs_csv: Set to ``False`` in order to disable splitting query
+            parameters on ``,`` (default ``True``). Depending on the user agent,
+            encoding lists as multiple occurrences of the same parameter might
+            be preferable. In this case, setting `parse_qs_csv` to ``False``
+            will cause the framework to treat commas as literal characters in
+            each occurring parameter value.
 
     Returns:
         dict: A dictionary of (*name*, *value*) pairs, one per query
@@ -309,7 +318,7 @@ def parse_query_string(query_string, keep_blank_qs_values=False):
                 params[k] = [old_value, decode(v)]
 
         else:
-            if ',' in v:
+            if parse_qs_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


Test patch:
diff --git a/tests/test_options.py b/tests/test_options.py
index b3d9812..a3b8b72 100644
--- a/tests/test_options.py
+++ b/tests/test_options.py
@@ -1,16 +1,32 @@
+import ddt
+
 from falcon.request import RequestOptions
 import falcon.testing as testing
 
 
+@ddt.ddt
 class TestRequestOptions(testing.TestBase):
 
-    def test_correct_options(self):
+    def test_option_defaults(self):
         options = RequestOptions()
+
         self.assertFalse(options.keep_blank_qs_values)
-        options.keep_blank_qs_values = True
-        self.assertTrue(options.keep_blank_qs_values)
-        options.keep_blank_qs_values = False
-        self.assertFalse(options.keep_blank_qs_values)
+        self.assertFalse(options.auto_parse_form_urlencoded)
+        self.assertTrue(options.auto_parse_qs_csv)
+
+    @ddt.data(
+        'keep_blank_qs_values',
+        'auto_parse_form_urlencoded',
+        'auto_parse_qs_csv',
+    )
+    def test_options_toggle(self, option_name):
+        options = RequestOptions()
+
+        setattr(options, option_name, True)
+        self.assertTrue(getattr(options, option_name))
+
+        setattr(options, option_name, False)
+        self.assertFalse(getattr(options, option_name))
 
     def test_incorrect_options(self):
         options = RequestOptions()
diff --git a/tests/test_query_params.py b/tests/test_query_params.py
index c588f23..62c906d 100644
--- a/tests/test_query_params.py
+++ b/tests/test_query_params.py
@@ -65,6 +65,60 @@ class _TestQueryParams(testing.TestBase):
         self.assertEqual(req.get_param_as_list('id', int), [23, 42])
         self.assertEqual(req.get_param('q'), u'\u8c46 \u74e3')
 
+    def test_option_auto_parse_qs_csv_simple_false(self):
+        self.api.req_options.auto_parse_qs_csv = False
+
+        query_string = 'id=23,42,,&id=2'
+        self.simulate_request('/', query_string=query_string)
+
+        req = self.resource.req
+
+        self.assertEqual(req.params['id'], [u'23,42,,', u'2'])
+        self.assertIn(req.get_param('id'), [u'23,42,,', u'2'])
+        self.assertEqual(req.get_param_as_list('id'), [u'23,42,,', u'2'])
+
+    def test_option_auto_parse_qs_csv_simple_true(self):
+        self.api.req_options.auto_parse_qs_csv = True
+
+        query_string = 'id=23,42,,&id=2'
+        self.simulate_request('/', query_string=query_string)
+
+        req = self.resource.req
+
+        self.assertEqual(req.params['id'], [u'23', u'42', u'2'])
+        self.assertIn(req.get_param('id'), [u'23', u'42', u'2'])
+        self.assertEqual(req.get_param_as_list('id', int), [23, 42, 2])
+
+    def test_option_auto_parse_qs_csv_complex_false(self):
+        self.api.req_options.auto_parse_qs_csv = False
+
+        encoded_json = '%7B%22msg%22:%22Testing%201,2,3...%22,%22code%22:857%7D'
+        decoded_json = '{"msg":"Testing 1,2,3...","code":857}'
+
+        query_string = ('colors=red,green,blue&limit=1'
+                        '&list-ish1=f,,x&list-ish2=,0&list-ish3=a,,,b'
+                        '&empty1=&empty2=,&empty3=,,'
+                        '&thing=' + encoded_json)
+
+        self.simulate_request('/', query_string=query_string)
+
+        req = self.resource.req
+
+        self.assertIn(req.get_param('colors'), 'red,green,blue')
+        self.assertEqual(req.get_param_as_list('colors'), [u'red,green,blue'])
+
+        self.assertEqual(req.get_param_as_list('limit'), ['1'])
+
+        self.assertEqual(req.get_param_as_list('empty1'), None)
+        self.assertEqual(req.get_param_as_list('empty2'), [u','])
+        self.assertEqual(req.get_param_as_list('empty3'), [u',,'])
+
+        self.assertEqual(req.get_param_as_list('list-ish1'), [u'f,,x'])
+        self.assertEqual(req.get_param_as_list('list-ish2'), [u',0'])
+        self.assertEqual(req.get_param_as_list('list-ish3'), [u'a,,,b'])
+
+        self.assertEqual(req.get_param('thing'), decoded_json)
+
     def test_bad_percentage(self):
         query_string = 'x=%%20%+%&y=peregrine&z=%a%z%zz%1%20e'
         self.simulate_request('/', query_string=query_string)
diff --git a/tests/test_utils.py b/tests/test_utils.py
index 957a959..6b5f75d 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -128,6 +128,16 @@ class TestFalconUtils(testtools.TestCase):
             falcon.to_query_str({'things': ['a', 'b']}),
             '?things=a,b')
 
+        expected = ('?things=a&things=b&things=&things=None'
+                    '&things=true&things=false&things=0')
+
+        actual = falcon.to_query_str(
+            {'things': ['a', 'b', '', None, True, False, 0]},
+            comma_delimited_lists=False
+        )
+
+        self.assertEqual(actual, expected)
+
     def test_pack_query_params_several(self):
         garbage_in = {
             'limit': 17,