From 20b4ae1ef90d311a8588ba164cd7e812220a1f2f Mon Sep 17 00:00:00 2001 From: Yongjie Zhao Date: Thu, 27 Jan 2022 22:58:06 +0800 Subject: [PATCH] fix: null value and empty string in filter (#18171) --- .../index.tsx | 17 +++-- .../getSimpleSQLExpression.test.ts | 8 ++ .../src/explore/exploreUtils/index.js | 11 ++- superset-frontend/src/utils/common.js | 3 +- superset/connectors/base/models.py | 8 +- superset/connectors/druid/models.py | 4 +- superset/connectors/sqla/models.py | 11 +-- superset/constants.py | 1 + superset/views/core.py | 4 +- tests/integration_tests/sqla_models_tests.py | 76 ++++++++++++++++--- 10 files changed, 102 insertions(+), 41 deletions(-) diff --git a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx index 42baf74bdd1cf..ceddf7cbd79fe 100644 --- a/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx +++ b/superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx @@ -38,6 +38,7 @@ import AdhocFilter, { } from 'src/explore/components/controls/FilterControl/AdhocFilter'; import { Input } from 'src/common/components'; import { propertyComparator } from 'src/components/Select/Select'; +import { optionLabel } from 'src/utils/common'; const StyledInput = styled(Input)` margin-bottom: ${({ theme }) => theme.gridUnit * 4}px; @@ -226,7 +227,9 @@ const AdhocFilterEditPopoverSimpleTabContent: React.FC = props => { isOperatorRelevant, onComparatorChange, } = useSimpleTabFilterProps(props); - const [suggestions, setSuggestions] = useState>([]); + const [suggestions, setSuggestions] = useState< + Record<'label' | 'value', any>[] + >([]); const [comparator, setComparator] = useState(props.adhocFilter.comparator); const [loadingComparatorSuggestions, setLoadingComparatorSuggestions] = useState(false); @@ -346,7 +349,12 @@ const AdhocFilterEditPopoverSimpleTabContent: React.FC = props => { endpoint: `/superset/filter/${datasource.type}/${datasource.id}/${col}/`, }) .then(({ json }) => { - setSuggestions(json); + setSuggestions( + json.map((suggestion: null | number | boolean | string) => ({ + value: suggestion, + label: optionLabel(suggestion), + })), + ); setLoadingComparatorSuggestions(false); }) .catch(() => { @@ -402,10 +410,7 @@ const AdhocFilterEditPopoverSimpleTabContent: React.FC = props => { {MULTI_OPERATORS.has(operatorId) || suggestions.length > 0 ? ( ({ - value: suggestion, - label: String(suggestion), - }))} + options={suggestions} {...comparatorSelectProps} sortComparator={propertyComparator( typeof suggestions[0] === 'number' ? 'value' : 'label', diff --git a/superset-frontend/src/explore/exploreUtils/getSimpleSQLExpression.test.ts b/superset-frontend/src/explore/exploreUtils/getSimpleSQLExpression.test.ts index 54fc9022f5752..343f9efeecb46 100644 --- a/superset-frontend/src/explore/exploreUtils/getSimpleSQLExpression.test.ts +++ b/superset-frontend/src/explore/exploreUtils/getSimpleSQLExpression.test.ts @@ -16,7 +16,9 @@ * specific language governing permissions and limitations * under the License. */ +import { EMPTY_STRING, NULL_STRING } from 'src/utils/common'; import { getSimpleSQLExpression } from '.'; +import { Operators } from '../constants'; const params = { subject: 'subject', @@ -36,6 +38,12 @@ test('Should return "" if subject is falsy', () => { ).toBe(''); }); +test('Should return null string and empty string', () => { + expect(getSimpleSQLExpression(params.subject, Operators.IN, [null, ''])).toBe( + `subject ${Operators.IN} (${NULL_STRING}, ${EMPTY_STRING})`, + ); +}); + test('Should return subject if operator is falsy', () => { expect(getSimpleSQLExpression(params.subject, '', params.comparator)).toBe( params.subject, diff --git a/superset-frontend/src/explore/exploreUtils/index.js b/superset-frontend/src/explore/exploreUtils/index.js index 7cb3b8da69f15..9e739351d574f 100644 --- a/superset-frontend/src/explore/exploreUtils/index.js +++ b/superset-frontend/src/explore/exploreUtils/index.js @@ -35,6 +35,7 @@ import { OPERATOR_ENUM_TO_OPERATOR_TYPE, } from 'src/explore/constants'; import { DashboardStandaloneMode } from 'src/dashboard/util/constants'; +import { optionLabel } from '../../utils/common'; const MAX_URL_LENGTH = 8000; @@ -374,10 +375,12 @@ export const getSimpleSQLExpression = (subject, operator, comparator) => { firstValue !== undefined && Number.isNaN(Number(firstValue)); const quote = isString ? "'" : ''; const [prefix, suffix] = isMulti ? ['(', ')'] : ['', '']; - const formattedComparators = comparatorArray.map( - val => - `${quote}${isString ? String(val).replace("'", "''") : val}${quote}`, - ); + const formattedComparators = comparatorArray + .map(val => optionLabel(val)) + .map( + val => + `${quote}${isString ? String(val).replace("'", "''") : val}${quote}`, + ); if (comparatorArray.length > 0) { expression += ` ${prefix}${formattedComparators.join(', ')}${suffix}`; } diff --git a/superset-frontend/src/utils/common.js b/superset-frontend/src/utils/common.js index 567ec3d968f49..3e077075406e2 100644 --- a/superset-frontend/src/utils/common.js +++ b/superset-frontend/src/utils/common.js @@ -24,6 +24,7 @@ import { // ATTENTION: If you change any constants, make sure to also change constants.py +export const EMPTY_STRING = ''; export const NULL_STRING = ''; export const TRUE_STRING = 'TRUE'; export const FALSE_STRING = 'FALSE'; @@ -61,7 +62,7 @@ export function optionLabel(opt) { return NULL_STRING; } if (opt === '') { - return ''; + return EMPTY_STRING; } if (opt === true) { return ''; diff --git a/superset/connectors/base/models.py b/superset/connectors/base/models.py index 06b44c45c85e6..918927cca6ff5 100644 --- a/superset/connectors/base/models.py +++ b/superset/connectors/base/models.py @@ -25,7 +25,7 @@ from sqlalchemy.orm import foreign, Query, relationship, RelationshipProperty, Session from superset import is_feature_enabled, security_manager -from superset.constants import NULL_STRING +from superset.constants import EMPTY_STRING, NULL_STRING from superset.datasets.commands.exceptions import DatasetNotFoundError from superset.models.helpers import AuditMixinNullable, ImportExportMixin, QueryResult from superset.models.slice import Slice @@ -406,7 +406,7 @@ def handle_single_value(value: Optional[FilterValue]) -> Optional[FilterValue]: return utils.cast_to_num(value) if value == NULL_STRING: return None - if value == "": + if value == EMPTY_STRING: return "" if target_column_type == utils.GenericDataType.BOOLEAN: return utils.cast_to_boolean(value) @@ -441,9 +441,7 @@ def query(self, query_obj: QueryObjectDict) -> QueryResult: """ raise NotImplementedError() - def values_for_column( - self, column_name: str, limit: int = 10000, contain_null: bool = True, - ) -> List[Any]: + def values_for_column(self, column_name: str, limit: int = 10000) -> List[Any]: """Given a column, returns an iterable of distinct values This is used to populate the dropdown showing a list of diff --git a/superset/connectors/druid/models.py b/superset/connectors/druid/models.py index aa86d7ab89383..32edb695279c0 100644 --- a/superset/connectors/druid/models.py +++ b/superset/connectors/druid/models.py @@ -948,9 +948,7 @@ def metrics_and_post_aggs( ) return aggs, post_aggs - def values_for_column( - self, column_name: str, limit: int = 10000, contain_null: bool = True, - ) -> List[Any]: + def values_for_column(self, column_name: str, limit: int = 10000) -> List[Any]: """Retrieve some values for the given column""" logger.info( "Getting values for columns [{}] limited to [{}]".format(column_name, limit) diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 311889b090596..74e8e6c3d1dbe 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -176,9 +176,7 @@ def query(self, query_obj: QueryObjectDict) -> QueryResult: def get_query_str(self, query_obj: QueryObjectDict) -> str: raise NotImplementedError() - def values_for_column( - self, column_name: str, limit: int = 10000, contain_null: bool = True, - ) -> List[Any]: + def values_for_column(self, column_name: str, limit: int = 10000) -> List[Any]: raise NotImplementedError() @@ -738,9 +736,7 @@ def get_fetch_values_predicate(self) -> TextClause: ) ) from ex - def values_for_column( - self, column_name: str, limit: int = 10000, contain_null: bool = True, - ) -> List[Any]: + def values_for_column(self, column_name: str, limit: int = 10000) -> List[Any]: """Runs query against sqla to retrieve some sample values for the given column. """ @@ -756,9 +752,6 @@ def values_for_column( if limit: qry = qry.limit(limit) - if not contain_null: - qry = qry.where(target_col.get_sqla_col().isnot(None)) - if self.fetch_values_predicate: qry = qry.where(self.get_fetch_values_predicate()) diff --git a/superset/constants.py b/superset/constants.py index 1e02bcc0cba71..7cfa72e4b36b4 100644 --- a/superset/constants.py +++ b/superset/constants.py @@ -21,6 +21,7 @@ from enum import Enum NULL_STRING = "" +EMPTY_STRING = "" CHANGE_ME_SECRET_KEY = "CHANGE_ME_TO_A_COMPLEX_RANDOM_SECRET" diff --git a/superset/views/core.py b/superset/views/core.py index 84ef5eba8a446..81952e657f1ca 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -920,9 +920,7 @@ def filter( # pylint: disable=no-self-use datasource.raise_for_access() row_limit = apply_max_row_limit(config["FILTER_SELECT_ROW_LIMIT"]) payload = json.dumps( - datasource.values_for_column( - column_name=column, limit=row_limit, contain_null=False, - ), + datasource.values_for_column(column_name=column, limit=row_limit), default=utils.json_int_dttm_ser, ignore_nan=True, ) diff --git a/tests/integration_tests/sqla_models_tests.py b/tests/integration_tests/sqla_models_tests.py index eb5450780032a..105316d4c9945 100644 --- a/tests/integration_tests/sqla_models_tests.py +++ b/tests/integration_tests/sqla_models_tests.py @@ -30,7 +30,8 @@ from sqlalchemy.sql.elements import TextClause from superset import db -from superset.connectors.sqla.models import SqlaTable, TableColumn +from superset.connectors.sqla.models import SqlaTable, TableColumn, SqlMetric +from superset.constants import EMPTY_STRING, NULL_STRING from superset.db_engine_specs.bigquery import BigQueryEngineSpec from superset.db_engine_specs.druid import DruidEngineSpec from superset.exceptions import QueryObjectValidationError @@ -477,22 +478,77 @@ def test_labels_expected_on_mutated_query(self): def test_values_for_column(self): table = SqlaTable( table_name="test_null_in_column", - sql="SELECT 'foo' as foo UNION SELECT 'bar' UNION SELECT NULL", + sql=( + "SELECT 'foo' as foo " + "UNION SELECT '' " + "UNION SELECT NULL " + "UNION SELECT 'null'" + ), database=get_example_database(), ) TableColumn(column_name="foo", type="VARCHAR(255)", table=table) + SqlMetric(metric_name="count", expression="count(*)", table=table) - with_null = table.values_for_column( - column_name="foo", limit=10000, contain_null=True - ) + # null value, empty string and text should be retrieved + with_null = table.values_for_column(column_name="foo", limit=10000) assert None in with_null - assert len(with_null) == 3 + assert len(with_null) == 4 + + # null value should be replaced + result_object = table.query( + { + "metrics": ["count"], + "filter": [{"col": "foo", "val": [NULL_STRING], "op": "IN"}], + "is_timeseries": False, + } + ) + assert result_object.df["count"][0] == 1 + + # also accept None value + result_object = table.query( + { + "metrics": ["count"], + "filter": [{"col": "foo", "val": [None], "op": "IN"}], + "is_timeseries": False, + } + ) + assert result_object.df["count"][0] == 1 + + # empty string should be replaced + result_object = table.query( + { + "metrics": ["count"], + "filter": [{"col": "foo", "val": [EMPTY_STRING], "op": "IN"}], + "is_timeseries": False, + } + ) + assert result_object.df["count"][0] == 1 - without_null = table.values_for_column( - column_name="foo", limit=10000, contain_null=False + # also accept "" string + result_object = table.query( + { + "metrics": ["count"], + "filter": [{"col": "foo", "val": [""], "op": "IN"}], + "is_timeseries": False, + } + ) + assert result_object.df["count"][0] == 1 + + # both replaced + result_object = table.query( + { + "metrics": ["count"], + "filter": [ + { + "col": "foo", + "val": [EMPTY_STRING, NULL_STRING, "null", "foo"], + "op": "IN", + } + ], + "is_timeseries": False, + } ) - assert None not in without_null - assert len(without_null) == 2 + assert result_object.df["count"][0] == 4 @pytest.mark.parametrize(