From e4b449d9e90474a9cc0c076344cecea4dbd3d73a Mon Sep 17 00:00:00 2001 From: Maxime Beauchemin Date: Mon, 19 Mar 2018 23:22:05 -0700 Subject: [PATCH 1/8] [WiP] [explore] proper filtering of NULLs and '' TODO: handling of Druid equivalents --- .../assets/src/components/AlteredSliceTag.jsx | 2 +- .../explore/components/controls/Filter.jsx | 8 ++++++- .../components/controls/FilterControl.jsx | 22 ++++++++++++++++++- superset/connectors/sqla/models.py | 10 +++++++-- 4 files changed, 37 insertions(+), 5 deletions(-) diff --git a/superset/assets/src/components/AlteredSliceTag.jsx b/superset/assets/src/components/AlteredSliceTag.jsx index eb24424e8d41f..ad1356b48b0c5 100644 --- a/superset/assets/src/components/AlteredSliceTag.jsx +++ b/superset/assets/src/components/AlteredSliceTag.jsx @@ -61,7 +61,7 @@ export default class AlteredSliceTag extends React.Component { return '[]'; } return value.map((v) => { - const filterVal = v.val.constructor === Array ? `[${v.val.join(', ')}]` : v.val; + const filterVal = v.val && v.val.constructor === Array ? `[${v.val.join(', ')}]` : v.val; return `${v.col} ${v.op} ${filterVal}`; }).join(', '); } else if (controls[key] && controls[key].type === 'BoundsControl') { diff --git a/superset/assets/src/explore/components/controls/Filter.jsx b/superset/assets/src/explore/components/controls/Filter.jsx index 49a9751f3be00..c405b8c0de3ec 100644 --- a/superset/assets/src/explore/components/controls/Filter.jsx +++ b/superset/assets/src/explore/components/controls/Filter.jsx @@ -16,6 +16,8 @@ const operatorsArr = [ { val: '<', type: 'string', havingOnly: true }, { val: 'regex', type: 'string', datasourceTypes: ['druid'] }, { val: 'LIKE', type: 'string', datasourceTypes: ['table'] }, + { val: 'IS NULL', type: null }, + { val: 'IS NOT NULL', type: null }, ]; const operators = {}; operatorsArr.forEach((op) => { @@ -90,6 +92,10 @@ export default class Filter extends React.Component { renderFilterFormControl(filter) { const operator = operators[filter.op]; + if (operator.type === null) { + // IS NULL or IS NOT NULL + return null; + } if (operator.useSelect && !this.props.having) { // TODO should use a simple Select, not a control here... return ( @@ -99,7 +105,7 @@ export default class Filter extends React.Component { name="filter-value" value={filter.val} isLoading={this.props.valuesLoading} - choices={this.props.valueChoices} + options={this.props.valueChoices} onChange={this.changeSelect.bind(this)} showHeader={false} /> diff --git a/superset/assets/src/explore/components/controls/FilterControl.jsx b/superset/assets/src/explore/components/controls/FilterControl.jsx index 041dd6fe2c181..14111854d4685 100644 --- a/superset/assets/src/explore/components/controls/FilterControl.jsx +++ b/superset/assets/src/explore/components/controls/FilterControl.jsx @@ -18,6 +18,25 @@ const defaultProps = { value: [], }; +function optionLabel(opt) { + if (opt === null) { + return ''; + } else if (opt === '') { + return ''; + } + return opt; +} +function optionValue(opt) { + if (opt === null) { + return ''; + } + return opt; +} +function optionFromValue(opt) { + // From a list of options, handles special values & labels + return { value: optionValue(opt), label: optionLabel(opt) }; +} + export default class FilterControl extends React.Component { constructor(props) { @@ -57,7 +76,8 @@ export default class FilterControl extends React.Component { success: (data) => { this.setState((prevState) => { const newStateFilters = Object.assign([], prevState.filters); - newStateFilters[index] = { valuesLoading: false, valueChoices: data }; + const valueChoices = data.map(opt => optionFromValue(opt)); + newStateFilters[index] = { valuesLoading: false, valueChoices }; return { filters: newStateFilters, activeRequest: null }; }); }, diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 7fbb7ae44d4ec..51282b39cd57b 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -574,11 +574,11 @@ def get_sqla_query( # sqla where_clause_and = [] having_clause_and = [] for flt in filter: - if not all([flt.get(s) for s in ['col', 'op', 'val']]): + if not all([flt.get(s) for s in ['col', 'op']]): continue col = flt['col'] op = flt['op'] - eq = flt['val'] + eq = flt.get('val') col_obj = cols.get(col) if col_obj: if op in ('in', 'not in'): @@ -596,6 +596,8 @@ def get_sqla_query( # sqla if v is not None: values.append(v) cond = col_obj.sqla_col.in_(values) + if '' in eq: + cond = or_(cond, col_obj.sqla_col == None) if op == 'not in': cond = ~cond where_clause_and.append(cond) @@ -616,6 +618,10 @@ def get_sqla_query( # sqla where_clause_and.append(col_obj.sqla_col <= eq) elif op == 'LIKE': where_clause_and.append(col_obj.sqla_col.like(eq)) + elif op == 'IS NULL': + where_clause_and.append(col_obj.sqla_col == None) + elif op == 'IS NOT NULL': + where_clause_and.append(col_obj.sqla_col != None) if extras: where = extras.get('where') if where: From a66801db4cf67c48704c16706da4ed85b709890f Mon Sep 17 00:00:00 2001 From: Maxime Beauchemin Date: Tue, 20 Mar 2018 09:31:31 -0700 Subject: [PATCH 2/8] Unit tests --- superset/assets/package.json | 1 + .../explore/components/FilterControl_spec.jsx | 10 ++++++++-- .../javascripts/explore/components/Filter_spec.jsx | 4 ++-- superset/connectors/sqla/models.py | 6 +++--- 4 files changed, 14 insertions(+), 7 deletions(-) diff --git a/superset/assets/package.json b/superset/assets/package.json index ceb34c74a2eb8..c4911b3101bde 100644 --- a/superset/assets/package.json +++ b/superset/assets/package.json @@ -74,6 +74,7 @@ "moment": "^2.20.1", "mousetrap": "^1.6.1", "mustache": "^2.2.1", + "npm": "^5.7.1", "nvd3": "1.8.6", "object.entries": "^1.0.4", "object.keys": "^0.1.0", diff --git a/superset/assets/spec/javascripts/explore/components/FilterControl_spec.jsx b/superset/assets/spec/javascripts/explore/components/FilterControl_spec.jsx index 69f8b2794f1f0..114e3e96b18a4 100644 --- a/superset/assets/spec/javascripts/explore/components/FilterControl_spec.jsx +++ b/superset/assets/spec/javascripts/explore/components/FilterControl_spec.jsx @@ -225,12 +225,18 @@ describe('FilterControl', () => { wrapper.instance().fetchFilterValues(0, 'col1'); expect(wrapper.state().activeRequest).to.equal(spyReq); // Sets active to null after success - $.ajax.getCall(0).args[0].success('choices'); + $.ajax.getCall(0).args[0].success(['opt1', 'opt2', null, '']); expect(wrapper.state().filters[0].valuesLoading).to.equal(false); - expect(wrapper.state().filters[0].valueChoices).to.equal('choices'); + expect(wrapper.state().filters[0].valueChoices).to.deep.equal([ + { value: 'opt1', label: 'opt1' }, + { value: 'opt2', label: 'opt2' }, + { value: '', label: '' }, + { value: '', label: '' }, + ]); expect(wrapper.state().activeRequest).to.equal(null); }); + it('cancels active request if another is submitted', () => { const spyReq = sinon.spy(); spyReq.abort = sinon.spy(); diff --git a/superset/assets/spec/javascripts/explore/components/Filter_spec.jsx b/superset/assets/spec/javascripts/explore/components/Filter_spec.jsx index 1a4c395807ec2..27425de9a3f6f 100644 --- a/superset/assets/spec/javascripts/explore/components/Filter_spec.jsx +++ b/superset/assets/spec/javascripts/explore/components/Filter_spec.jsx @@ -46,7 +46,7 @@ describe('Filter', () => { expect(wrapper.find(Select)).to.have.lengthOf(2); expect(wrapper.find(Button)).to.have.lengthOf(1); expect(wrapper.find(SelectControl)).to.have.lengthOf(1); - expect(wrapper.find('#select-op').prop('options')).to.have.lengthOf(8); + expect(wrapper.find('#select-op').prop('options')).to.have.lengthOf(10); }); it('renders five op choices for table datasource', () => { @@ -58,7 +58,7 @@ describe('Filter', () => { filterable_cols: ['country_name'], }; const druidWrapper = shallow(); - expect(druidWrapper.find('#select-op').prop('options')).to.have.lengthOf(9); + expect(druidWrapper.find('#select-op').prop('options')).to.have.lengthOf(11); }); it('renders six op choices for having filter', () => { diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 51282b39cd57b..eb6f6669df400 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -597,7 +597,7 @@ def get_sqla_query( # sqla values.append(v) cond = col_obj.sqla_col.in_(values) if '' in eq: - cond = or_(cond, col_obj.sqla_col == None) + cond = or_(cond, col_obj.sqla_col == None) # noqa if op == 'not in': cond = ~cond where_clause_and.append(cond) @@ -619,9 +619,9 @@ def get_sqla_query( # sqla elif op == 'LIKE': where_clause_and.append(col_obj.sqla_col.like(eq)) elif op == 'IS NULL': - where_clause_and.append(col_obj.sqla_col == None) + where_clause_and.append(col_obj.sqla_col == None) # noqa elif op == 'IS NOT NULL': - where_clause_and.append(col_obj.sqla_col != None) + where_clause_and.append(col_obj.sqla_col != None) # noqa if extras: where = extras.get('where') if where: From 6e7417055f417f4d48bb03a462741d060641c652 Mon Sep 17 00:00:00 2001 From: Maxime Beauchemin Date: Mon, 26 Mar 2018 18:13:32 +0000 Subject: [PATCH 3/8] Some refactoring --- superset/assets/src/SqlLab/actions.js | 3 +- superset/assets/src/chart/chartAction.js | 9 +++-- superset/assets/src/common.js | 6 ++++ .../assets/src/components/OnPasteSelect.jsx | 26 ++++++++++++++ .../explore/components/controls/Filter.jsx | 5 +-- .../components/controls/FilterControl.jsx | 22 +----------- superset/connectors/base/models.py | 29 +++++++++++++++ superset/connectors/druid/models.py | 36 +++++++------------ superset/connectors/sqla/models.py | 21 +++-------- superset/viz.py | 2 ++ tests/druid_func_tests.py | 1 + 11 files changed, 94 insertions(+), 66 deletions(-) diff --git a/superset/assets/src/SqlLab/actions.js b/superset/assets/src/SqlLab/actions.js index 04a9a5eae4c4b..644947023bcb8 100644 --- a/superset/assets/src/SqlLab/actions.js +++ b/superset/assets/src/SqlLab/actions.js @@ -2,6 +2,7 @@ import shortid from 'shortid'; import { now } from '../modules/dates'; import { t } from '../locales'; +import { COMMON_ERR_MESSAGES } from '../common'; const $ = require('jquery'); @@ -163,7 +164,7 @@ export function runQuery(query) { } } if (msg.indexOf('CSRF token') > 0) { - msg = t('Your session timed out, please refresh your page and try again.'); + msg = COMMON_ERR_MESSAGES.SESSION_TIMED_OUT; } dispatch(queryFailed(query, msg)); }, diff --git a/superset/assets/src/chart/chartAction.js b/superset/assets/src/chart/chartAction.js index b9338a91f5559..a2f01165d61ce 100644 --- a/superset/assets/src/chart/chartAction.js +++ b/superset/assets/src/chart/chartAction.js @@ -1,6 +1,7 @@ import { getExploreUrlAndPayload, getAnnotationJsonUrl } from '../explore/exploreUtils'; import { requiresQuery, ANNOTATION_SOURCE_TYPES } from '../modules/AnnotationTypes'; import { Logger, LOG_ACTIONS_LOAD_EVENT } from '../logger'; +import { COMMON_ERR_MESSAGES } from '../common'; const $ = window.$ = require('jquery'); @@ -160,12 +161,16 @@ export function runQuery(formData, force = false, timeout = 60, key) { errObject = err.responseJSON; } else if (err.stack) { errObject = { - error: 'Unexpected error: ' + err.description, + error: t('Unexpected error: ') + err.description, stacktrace: err.stack, }; + } else if (err.responseText && err.responseText.indexOf('CSRF') >= 0) { + errObject = { + error: COMMON_ERR_MESSAGES.SESSION_TIMED_OUT, + }; } else { errObject = { - error: 'Unexpected error.', + error: t('Unexpected error.'), }; } dispatch(chartUpdateFailed(errObject, key)); diff --git a/superset/assets/src/common.js b/superset/assets/src/common.js index d84f064065e82..cc509eb892e8b 100644 --- a/superset/assets/src/common.js +++ b/superset/assets/src/common.js @@ -1,5 +1,6 @@ /* eslint-disable global-require */ import $ from 'jquery'; +import { t } from './locales'; const utils = require('./modules/utils'); @@ -30,3 +31,8 @@ export function appSetup() { window.jQuery = $; require('bootstrap'); } + +// Error messages used in many places across applications +export const COMMON_ERR_MESSAGES = { + SESSION_TIMED_OUT: t('Your session timed out, please refresh your page and try again.'), +}; diff --git a/superset/assets/src/components/OnPasteSelect.jsx b/superset/assets/src/components/OnPasteSelect.jsx index 40bbbd09328d7..d715d2afa85af 100644 --- a/superset/assets/src/components/OnPasteSelect.jsx +++ b/superset/assets/src/components/OnPasteSelect.jsx @@ -2,6 +2,31 @@ import React from 'react'; import PropTypes from 'prop-types'; import Select from 'react-select'; + +function optionLabel(opt) { + if (opt === null) { + return ''; + } else if (opt === '') { + return ''; + } else if (opt === true) { + return ''; + } else if (opt === '') { + return ''; + } + return opt; +} +function optionValue(opt) { + if (opt === null) { + return ''; + } + return opt; +} +function optionFromValue(opt) { + // From a list of options, handles special values & labels + return { value: optionValue(opt), label: optionLabel(opt) }; +} + + export default class OnPasteSelect extends React.Component { onPaste(evt) { if (!this.props.multi) { @@ -55,6 +80,7 @@ export default class OnPasteSelect extends React.Component { this.pasteInput = ref; }; const inputProps = { onPaste: this.onPaste.bind(this) }; + console.log(this.props); return ( diff --git a/superset/assets/src/explore/components/controls/FilterControl.jsx b/superset/assets/src/explore/components/controls/FilterControl.jsx index 14111854d4685..041dd6fe2c181 100644 --- a/superset/assets/src/explore/components/controls/FilterControl.jsx +++ b/superset/assets/src/explore/components/controls/FilterControl.jsx @@ -18,25 +18,6 @@ const defaultProps = { value: [], }; -function optionLabel(opt) { - if (opt === null) { - return ''; - } else if (opt === '') { - return ''; - } - return opt; -} -function optionValue(opt) { - if (opt === null) { - return ''; - } - return opt; -} -function optionFromValue(opt) { - // From a list of options, handles special values & labels - return { value: optionValue(opt), label: optionLabel(opt) }; -} - export default class FilterControl extends React.Component { constructor(props) { @@ -76,8 +57,7 @@ export default class FilterControl extends React.Component { success: (data) => { this.setState((prevState) => { const newStateFilters = Object.assign([], prevState.filters); - const valueChoices = data.map(opt => optionFromValue(opt)); - newStateFilters[index] = { valuesLoading: false, valueChoices }; + newStateFilters[index] = { valuesLoading: false, valueChoices: data }; return { filters: newStateFilters, activeRequest: null }; }); }, diff --git a/superset/connectors/base/models.py b/superset/connectors/base/models.py index 68a020e36a9d3..143b75acf2bc5 100644 --- a/superset/connectors/base/models.py +++ b/superset/connectors/base/models.py @@ -6,6 +6,7 @@ import json +from past.builtins import basestring from sqlalchemy import ( and_, Boolean, Column, Integer, String, Text, ) @@ -185,6 +186,34 @@ def data(self): 'verbose_map': verbose_map, } + @staticmethod + def filter_values_handler( + values, target_column_is_numeric=False, is_list_target=False): + def handle_single_value(v): + # backward compatibility with previous components if isinstance(v, basestring): - v = v.strip("'").strip('"') + v = v.strip().strip("'").strip('"') if target_column_is_numeric: # For backwards compatibility and edge cases # where a column data type might have changed diff --git a/superset/connectors/druid/models.py b/superset/connectors/druid/models.py index 9807855eeb629..c847aca2a0b98 100644 --- a/superset/connectors/druid/models.py +++ b/superset/connectors/druid/models.py @@ -1341,18 +1341,22 @@ def increment_timestamp(ts): @classmethod def get_filters(cls, raw_filters, num_cols): # noqa + """Given Superset filter data structure, returns pydruid Filter(s)""" filters = None for flt in raw_filters: col = flt.get('col') op = flt.get('op') eq = flt.get('val') - if not col or not op: + if ( + not col or + not op or + (eq is None and op not in ('IS NULL', 'IS NOT NULL'))): continue cond = None - eq = cls.filter_values_handler( - eq, is_list_target=op in ('in', 'not in')) - is_numeric_col = col in num_cols + eq = cls.filter_values_handler( + eq, is_list_target=op in ('in', 'not in'), + target_column_is_numeric=is_numeric_col) if op == '==': cond = Dimension(col) == eq elif op == '!=': diff --git a/tests/druid_func_tests.py b/tests/druid_func_tests.py index 9c1bca9aa56d2..b750d3d87367d 100644 --- a/tests/druid_func_tests.py +++ b/tests/druid_func_tests.py @@ -122,12 +122,12 @@ def test_get_filters_handles_arrays_for_string_types(self): filtr = {'col': 'A', 'op': '==', 'val': []} res = DruidDatasource.get_filters([filtr], []) - self.assertEqual('', res.filter['filter']['value']) + self.assertEqual(None, res.filter['filter']['value']) def test_get_filters_handles_none_for_string_types(self): filtr = {'col': 'A', 'op': '==', 'val': None} res = DruidDatasource.get_filters([filtr], []) - self.assertEqual('', res.filter['filter']['value']) + self.assertEqual(None, res) def test_get_filters_extracts_values_in_quotes(self): filtr = {'col': 'A', 'op': 'in', 'val': [' "a" ']} From 01aa3a2865ab6b8c4835604d8e78f8e8fac7f478 Mon Sep 17 00:00:00 2001 From: Maxime Beauchemin Date: Thu, 12 Apr 2018 22:06:42 +0000 Subject: [PATCH 7/8] Addressing more comments --- superset/connectors/base/models.py | 13 +++++++------ superset/connectors/druid/models.py | 3 ++- superset/connectors/sqla/models.py | 3 ++- superset/viz.py | 2 -- tests/druid_func_tests.py | 8 ++++---- 5 files changed, 15 insertions(+), 14 deletions(-) diff --git a/superset/connectors/base/models.py b/superset/connectors/base/models.py index 87c0fc2c63ed7..8e4a2a22459d4 100644 --- a/superset/connectors/base/models.py +++ b/superset/connectors/base/models.py @@ -192,19 +192,20 @@ def filter_values_handler( def handle_single_value(v): # backward compatibility with previous