Single quote filter values with comma (#1084)

* Single quote filter values with comma

* refactor for codeclimate limite

* Added unit tests and tooltip

caravel/models.py

@@ -3,6 +3,7 @@ from __future__ import absolute_import
 from __future__ import division
 from __future__ import print_function
 from __future__ import unicode_literals
+import re
 
 import functools
 import json
@@ -54,6 +55,7 @@ from caravel.utils import flasher, MetricPermException, DimSelector
 config = app.config
 
 QueryResult = namedtuple('namedtuple', ['df', 'query', 'duration'])
+FillterPattern = re.compile(r'''((?:[^,"']|"[^"]*"|'[^']*')+)''')
 
 
 class JavascriptPostAggregator(Postaggregator):
@@ -839,7 +841,8 @@ class SqlaTable(Model, Queryable, AuditMixinNullable):
         for col, op, eq in filter:
             col_obj = cols[col]
             if op in ('in', 'not in'):
-                values = eq.split(",")
+                splitted = FillterPattern.split(eq)[1::2]
+                values = [types.replace("'", '').strip() for types in splitted]
                 cond = col_obj.sqla_col.in_(values)
                 if op == 'not in':
                     cond = ~cond
@@ -1597,9 +1600,11 @@ class DruidDatasource(Model, AuditMixinNullable, Queryable):
                 cond = ~(Dimension(col) == eq)
             elif op in ('in', 'not in'):
                 fields = []
-                splitted = eq.split(',')
-                if len(splitted) > 1:
-                    for s in eq.split(','):
+                # Distinguish quoted values with regular value types
+                splitted = FillterPattern.split(eq)[1::2]
+                values = [types.replace("'", '') for types in splitted]
+                if len(values) > 1:
+                    for s in values:
                         s = s.strip()
                         fields.append(Dimension(col) == s)
                     cond = Filter(type="or", fields=fields)

caravel/templates/caravel/explore.html

@@ -106,8 +106,11 @@
                 <div class="panel-title">
                   {{ _("Filters") }}
                   <i  class="fa fa-question-circle-o" data-toggle="tooltip"
-                      data-placement="bottom"
-                      title="{{_("Filters are defined using comma delimited strings as in 'US,FR,Other'")}}. {{_("Leave the value field empty to filter empty strings or nulls")}}">
+                      data-placement="right"
+                      title="{{_("Filters are defined using comma delimited strings as in <US,FR,Other>")}}.
+                      {{_("Leave the value field empty to filter empty strings or nulls")}}.
+                      {{_("For filters with comma in values, wrap them in single quotes,
+                      as in <NY, 'Tahoe, CA', DC>")}}">
                   </i>
                 </div>
               </div>

caravel/viz.py

@@ -214,9 +214,12 @@ class BaseViz(object):
             extra_filters = json.loads(extra_filters)
             for slice_filters in extra_filters.values():
                 for col, vals in slice_filters.items():
-                    if col and vals:
-                        if col in self.datasource.filterable_column_names:
-                            filters += [(col, 'in', ",".join(vals))]
+                    if not (col and vals):
+                        continue
+                    elif col in self.datasource.filterable_column_names:
+                        # Quote values with comma to avoid conflict
+                        vals = ["'%s'" % x if "," in x else x for x in vals]
+                        filters += [(col, 'in', ",".join(vals))]
         return filters
 
     def query_obj(self):

tests/core_tests.py

@@ -308,6 +308,16 @@ class CoreTests(CaravelTestCase):
         assert 'datasource_for_gamma' in resp.data.decode('utf-8')
         assert 'datasource_not_for_gamma' not in resp.data.decode('utf-8')
 
+    def test_add_filter(self, username='admin'):
+        # navigate to energy_usage slice with "Electricity,heat" in filter values
+        data = (
+            "/caravel/explore/table/1/?viz_type=table&groupby=source&metric=count&flt_col_1=source&flt_op_1=in&flt_eq_1=%27Electricity%2Cheat%27"
+            "&userid=1&datasource_name=energy_usage&datasource_id=1&datasource_type=tablerdo_save=saveas")
+        resp = self.client.get(
+            data,
+            follow_redirects=True)
+        assert ("source" in resp.data.decode('utf-8'))
+
     def test_gamma(self):
         self.login(username='gamma')
         resp = self.client.get('/slicemodelview/list/')