From 1f761c61ddf7de23b9258c15895f01ac8af6bc95 Mon Sep 17 00:00:00 2001
From: vera-liu <vera.liu@airbnb.com>
Date: Mon, 12 Sep 2016 13:41:05 -0700
Subject: [PATCH] Single quote filter values with comma (#1084)

* Single quote filter values with comma

* refactor for codeclimate limite

* Added unit tests and tooltip
---
 caravel/data/energy.json.gz            | Bin 983 -> 985 bytes
 caravel/models.py                      |  13 +++++++++----
 caravel/templates/caravel/explore.html |   7 +++++--
 caravel/viz.py                         |   9 ++++++---
 tests/core_tests.py                    |  10 ++++++++++
 5 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/caravel/data/energy.json.gz b/caravel/data/energy.json.gz
index b2f47a148b4f1a0c9dbd244e6e2f9803c4697dd6..624d71db683a9c08d19ad7e3625d502ca6d2e7d8 100644
GIT binary patch
delta 938
zcmV;b16BOj2iXS*ABzYG5k=Eikq9de*do!?u`-fOo9Xo3D?5=v7=JPuvDU}dr<vqY
zmgr2<gj}Rd&Q|16vDR@*Q#oVWd@`c)Nu=bQi{CuX-@MQy%hz_>gVx_;JF!A3Jtp^p
zC#nP*wtL~AYyQHN7ST^a)0j+Xx_-Z9?P1ido-;|RFSomU9}c?X5hEk>oWD@bMOxtu
z-JFd(HnbbArN|Vy-G6bo#ZO!Yh6`>e$re22*@E1N^kmFr@yipYY1JkNwsQdXay(Mx
zgiOu960-w0I${f!>L8<r(5TUzEqD~Vu|CWs$gu4KMIHid5w|!(m|~q6_olO;opjpW
zaEgi1oTk+XV<Q#|CL?YjpEI7s1{3Dj({>-=&e^k&rm)}2Cx0OEI_(btbdj(~o8sbn
zbv$QOV-ea_yREBG&`Zohb!uF9wjT!Hr2Q6y9O%WA@@^a1MkE}0ckTJ0V<F<=#e;aF
zQ6#)|<O5zwlQsE^d(Bwl6)YcaLZdl18M8W=bjy?C`4_6lREmfxwJyQK(q!GsuF^PD
zx?*Jy>b?&*&3}?*E@^aWsQCv|$y2>+=xGD;9aE-|4a=H8NN#-8hvMluxfV(>RoQ+M
z++#|UL_}1xn2a-)5R>#z#<SJl-opt{2uqn}`LzvZfR(Jq8D}$YMhr$S7TjcwNXr`?
z1R4WUq&vVlQd_%DGP^_0a7NTP50a*8DWtAVO9LoLKz|*YPA1TYC2cD=MYm46N!~K=
z!^FI#GM{hB_jmbhOK0IT4>s~O+Ig|PTEQx29@M&g@LKEYwa=PhVC!mPkX0mck12il
zUmdn`I;jiQ0509vF;z9$x8)*;waxZn^&y0}Qk+=>^a;ebAB+fVe9%3+uJG3^*6iyX
zV&+mZGJli!ta8xY9g{u7u!Ce18n>L}z-OLRPcP`mfclB$=<eVQ;mO6vv7e^UmdcY|
z)23og;gDk{YMFju-C4Ly&>e>}a!5X$kp0|k8zJ^Mq*-F%1AERbIN`li|2W&o*suY#
z>j*h)81_Rs%t9;3cH@DP$EBzt1&1Wyj+z2FA%FLCzATVyOE+U>5V;3c+!oKE3kfvQ
zfxAIx{XwGyjYDlf>uaZ@5bvPP)CTu$*%sH3?gn6A?T@EBY7X++bsh(~%PmC3QI!W$
z>sf#cDG+DZV+8$5m9Ac}ms-3#oVBIJ1M<rlC`~ZeF?{>To#3vOdtXfk!JJ*|=*Q7N
MkOzHcDk~WP0GIvHr~m)}

delta 936
zcmV;Z16Tan2iFG(ABzYGq1o_Okq9deI3m&1u`-fOo9Xo3E8CGl7=Kb3vDU}dr@7=&
z8tYWjm|P@G&e!BovDR@*Q#ohad@`c)NhIW)i{E^fy?LQ=nyu}&2d%%yc4CE6dQ9#G
zk5vgYZ1=)J*ZhSkEux=<CNnajN%4Nm+QX<@y<n15Uv78zJ{)w%BSuE%Ie(#=i=@IC
zx;YznY-l%JOOYyayMN<wi=Vg*3>Vx`oGy97(<QkP$;p_>{Ff(8)2dAlZ07*%<#?pX
z37MLIC1wY1bi|e{(LqKHp;4m+Tk<G$V||!OkYU>eiaZ3^B5rYnFvU7C?oH-FJL$B$
z;S>|21x>0E#zricOh(*5KBqjM8BCa8PuqQfJ7>>An!<i7pMQYF>$E=r&_&E5ZHkNQ
zb*>N#Ml}|qUB%nF3kBZGb!Yrx@J;G(G02HtOey!a(QQP+(RbIM4>}fNmcMuqPc({z
zx0ZasD`_$(dvUKBi@l=d!%b+k;3i|%2a|4joIn3U6`4vAF{O$!JS<I$es-12Ql%?a
z_MrCraMLtiWq-0pSB9E>FqJ&fyM~@NAm1@%irKKN`GX|KM}5ejo|9{#6jPP$H^Dt7
zG>%0?HJg!f%3@-Y{>gZ@+TVLP0dip}(=5NX!3?mP)i~pH&dr#?$i<SItPx3hqk}+W
zK#Ft+I7e!0izKr<<P2v-jq@OBqE<rc+Qc+~k_6PD>3?(reOS`Aa$|Jsq?_a|^FB<>
zODeM&mwbPh&9`(OKKEcFU!$EB+p86<V&*}u%LlKuu3r1B2?n;VCI(qW68D(WhyT@K
zYp0XCPz~VHeH~L(lYLt*f>_&ZA66eicq_%3H9((0eEY$Ou*L`7v+D|f&0@{I&LCzk
zB_lJ9&wne2&D~MiGY~sSHlcCLNd|o8arFd)jt!`vSdQ-w&Jdnnd>s3U3T>%8*)?q{
z))WpoYND3u2iBd1%LLtXI3tJT!wK2X-L?^8k3*Uz20pOo+=3I{OZAVljf@Q&K)a5R
z!-io$n8Pfzf^4@QD0y6p8d7jb0`90OkP~vh;D4(eSy;LmD}%^AsN%MG23<;^i4NQk
zI_nP_CFm^F2DH9*ItuX)+DvV5-<EBD73ppO_SOD)x})YG7q0U-$X#wBDvqi=kXp|I
zTu6a9yB;IxSE_XNlD*X8-Qlb)%^#3o#z1L;S;X+|BX@$kR_=X083c27t)m}D{{Uv4
Kp#{<}82|v~sl=xM

diff --git a/caravel/models.py b/caravel/models.py
index c0752ff74..b8b550612 100644
--- a/caravel/models.py
+++ b/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)
diff --git a/caravel/templates/caravel/explore.html b/caravel/templates/caravel/explore.html
index d12b97c15..be87414c9 100644
--- a/caravel/templates/caravel/explore.html
+++ b/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>
diff --git a/caravel/viz.py b/caravel/viz.py
index 20899f0df..61f3d5d75 100755
--- a/caravel/viz.py
+++ b/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):
diff --git a/tests/core_tests.py b/tests/core_tests.py
index cefa2d852..1898dfcaa 100644
--- a/tests/core_tests.py
+++ b/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/')