Copilot commented on code in PR #39795: URL: https://github.com/apache/superset/pull/39795#discussion_r3236332983
########## superset/utils/rison_filters.py: ########## @@ -0,0 +1,226 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Parser for Rison URL filters that converts simplified filter syntax +to Superset's adhoc_filters format. +""" + +from __future__ import annotations + +import logging +from typing import Any, Optional, Union + +import prison +from flask import request + +logger = logging.getLogger(__name__) + + +class RisonFilterParser: + """ + Parse Rison filter syntax from URL parameter 'f' and convert to adhoc_filters. + + Supports: + - Simple equality: f=(country:USA) + - Lists (IN): f=(country:!(USA,Canada)) + - NOT operator: f=(NOT:(country:USA)) + - OR operator: f=(OR:!(condition1,condition2)) + - Comparison operators: f=(sales:(gt:100000)) + - BETWEEN: f=(date:(between:!(2024-01-01,2024-12-31))) + - LIKE: f=(name:(like:'%smith%')) + """ + + OPERATORS: dict[str, str] = { + "gt": ">", + "gte": ">=", + "lt": "<", + "lte": "<=", + "between": "BETWEEN", + "like": "LIKE", + "ilike": "ILIKE", + "ne": "!=", + "eq": "==", + } + + def parse(self, filter_string: Optional[str] = None) -> list[dict[str, Any]]: + """ + Parse Rison filter string and convert to adhoc_filters format. + + Args: + filter_string: Rison-encoded filter string, or None to get from request + + Returns: + List of adhoc_filter dictionaries + """ + if filter_string is None: + filter_string = request.args.get("f") + + if not filter_string: + return [] + + try: + filters_obj = prison.loads(filter_string) + return self._convert_to_adhoc_filters(filters_obj) + except Exception: + logger.warning( + "Failed to parse Rison filters: %s", filter_string, exc_info=True + ) + return [] + + def _convert_to_adhoc_filters( + self, filters_obj: Union[dict[str, Any], list[Any], Any] + ) -> list[dict[str, Any]]: + if not isinstance(filters_obj, dict): + return [] + + adhoc_filters: list[dict[str, Any]] = [] + + for key, value in filters_obj.items(): + if key == "OR": + adhoc_filters.extend(self._handle_or_operator(value)) + elif key == "NOT": + adhoc_filters.extend(self._handle_not_operator(value)) + else: + filter_dict = self._create_filter(key, value) + if filter_dict: + adhoc_filters.append(filter_dict) + + return adhoc_filters + + def _create_filter( + self, column: str, value: Any, negate: bool = False + ) -> Optional[dict[str, Any]]: + filter_dict: dict[str, Any] = { + "expressionType": "SIMPLE", + "clause": "WHERE", + "subject": column, + } + + if isinstance(value, list): + filter_dict["operator"] = "NOT IN" if negate else "IN" + filter_dict["comparator"] = value + elif isinstance(value, dict): + operator_info = self._parse_operator_dict(value) + if operator_info: + operator, comparator = operator_info + if negate and operator == "==": + operator = "!=" + elif negate and operator == "IN": + operator = "NOT IN" + filter_dict["operator"] = operator + filter_dict["comparator"] = comparator + else: + return None + else: + filter_dict["operator"] = "!=" if negate else "==" + filter_dict["comparator"] = value + + return filter_dict + + def _parse_operator_dict( + self, op_dict: dict[str, Any] + ) -> Optional[tuple[str, Any]]: + if not op_dict: + return None + + for op_key, op_value in op_dict.items(): + if op_key in self.OPERATORS: + operator = self.OPERATORS[op_key] + if ( + operator == "BETWEEN" + and isinstance(op_value, list) + and len(op_value) == 2 + ): + return operator, op_value + return operator, op_value + if op_key == "in": + return "IN", op_value if isinstance(op_value, list) else [op_value] + if op_key == "nin": + return "NOT IN", op_value if isinstance(op_value, list) else [op_value] + + return None + + def _handle_or_operator(self, or_value: Any) -> list[dict[str, Any]]: + if not isinstance(or_value, list): + return [] + + sql_parts: list[str] = [] + + for item in or_value: + if isinstance(item, dict): + for col, val in item.items(): + if col not in ("OR", "NOT"): + sql_part = self._build_sql_condition(col, val) + if sql_part: + sql_parts.append(sql_part) + + if sql_parts: + return [ + { + "expressionType": "SQL", + "clause": "WHERE", + "sqlExpression": f"({' OR '.join(sql_parts)})", + } + ] + + return [] + + def _build_sql_condition(self, column: str, value: Any) -> Optional[str]: + if isinstance(value, list): + values_str = ", ".join( + [f"'{v}'" if isinstance(v, str) else str(v) for v in value] + ) + return f"{column} IN ({values_str})" + + if isinstance(value, dict): + operator_info = self._parse_operator_dict(value) + if operator_info: + op, comp = operator_info + if op == "BETWEEN" and isinstance(comp, list): + return f"{column} BETWEEN '{comp[0]}' AND '{comp[1]}'" + if op == "LIKE": + return f"{column} LIKE '{comp}'" + comp_str = f"'{comp}'" if isinstance(comp, str) else str(comp) + return f"{column} {op} {comp_str}" + + val_str = f"'{value}'" if isinstance(value, str) else str(value) + return f"{column} = {val_str}" Review Comment: The OR-path builds `expressionType: SQL` by interpolating untrusted URL-provided `column` and `value` directly into SQL (including unescaped quotes), which can enable SQL injection / malformed SQL expressions. Since these filters originate from `request.args`, this needs strong hardening: validate/whitelist column identifiers (or quote identifiers safely), escape string literals (at minimum single quotes), and consider avoiding `expressionType: SQL` generation entirely for URL inputs (e.g., treat OR as unsupported/unmatched or represent it via a safe structured filter format if available). ########## superset/utils/rison_filters.py: ########## @@ -0,0 +1,226 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Parser for Rison URL filters that converts simplified filter syntax +to Superset's adhoc_filters format. +""" + +from __future__ import annotations + +import logging +from typing import Any, Optional, Union + +import prison +from flask import request + +logger = logging.getLogger(__name__) + + +class RisonFilterParser: + """ + Parse Rison filter syntax from URL parameter 'f' and convert to adhoc_filters. + + Supports: + - Simple equality: f=(country:USA) + - Lists (IN): f=(country:!(USA,Canada)) + - NOT operator: f=(NOT:(country:USA)) + - OR operator: f=(OR:!(condition1,condition2)) + - Comparison operators: f=(sales:(gt:100000)) + - BETWEEN: f=(date:(between:!(2024-01-01,2024-12-31))) + - LIKE: f=(name:(like:'%smith%')) + """ + + OPERATORS: dict[str, str] = { + "gt": ">", + "gte": ">=", + "lt": "<", + "lte": "<=", + "between": "BETWEEN", + "like": "LIKE", + "ilike": "ILIKE", + "ne": "!=", + "eq": "==", + } + + def parse(self, filter_string: Optional[str] = None) -> list[dict[str, Any]]: + """ + Parse Rison filter string and convert to adhoc_filters format. + + Args: + filter_string: Rison-encoded filter string, or None to get from request + + Returns: + List of adhoc_filter dictionaries + """ + if filter_string is None: + filter_string = request.args.get("f") Review Comment: `parse()` reads `flask.request` when `filter_string` is omitted. If `merge_rison_filters()` (or other callers) are used in a non-request context (e.g., async tasks, CLI, unit tests outside request fixtures), `request.args` access can raise a `RuntimeError` due to missing request context. Consider catching `RuntimeError` (request context missing) and returning `[]`, or refactor so request access happens only at the call site that is guaranteed to run under a request. ########## superset/utils/rison_filters.py: ########## @@ -0,0 +1,226 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Parser for Rison URL filters that converts simplified filter syntax +to Superset's adhoc_filters format. +""" + +from __future__ import annotations + +import logging +from typing import Any, Optional, Union + +import prison +from flask import request + +logger = logging.getLogger(__name__) + + +class RisonFilterParser: + """ + Parse Rison filter syntax from URL parameter 'f' and convert to adhoc_filters. + + Supports: + - Simple equality: f=(country:USA) + - Lists (IN): f=(country:!(USA,Canada)) + - NOT operator: f=(NOT:(country:USA)) + - OR operator: f=(OR:!(condition1,condition2)) + - Comparison operators: f=(sales:(gt:100000)) + - BETWEEN: f=(date:(between:!(2024-01-01,2024-12-31))) + - LIKE: f=(name:(like:'%smith%')) + """ + + OPERATORS: dict[str, str] = { + "gt": ">", + "gte": ">=", + "lt": "<", + "lte": "<=", + "between": "BETWEEN", + "like": "LIKE", + "ilike": "ILIKE", + "ne": "!=", + "eq": "==", + } + + def parse(self, filter_string: Optional[str] = None) -> list[dict[str, Any]]: Review Comment: `parse()` reads `flask.request` when `filter_string` is omitted. If `merge_rison_filters()` (or other callers) are used in a non-request context (e.g., async tasks, CLI, unit tests outside request fixtures), `request.args` access can raise a `RuntimeError` due to missing request context. Consider catching `RuntimeError` (request context missing) and returning `[]`, or refactor so request access happens only at the call site that is guaranteed to run under a request. ########## superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx: ########## @@ -94,9 +127,47 @@ const HorizontalFilterBar: FC<HorizontalBarProps> = ({ [chartIds, chartLayoutItems, dataMask, verboseMaps], ); + const activeUrlFilters = useMemo(() => getUrlFilterIndicators(), []); + + const handleRemoveUrlFilter = useCallback( + (filterToRemove: UrlFilterIndicator) => { + const risonParam = getRisonFilterParam(); + if (!risonParam) return; + + const currentFilters = parseRisonFilters(risonParam); + const remaining = currentFilters.filter( + f => f.subject !== filterToRemove.filter.subject, + ); + updateUrlWithUnmatchedFilters(remaining); + }, + [], + ); Review Comment: `activeUrlFilters` is memoized with an empty dependency array, so the horizontal URL filter chips won’t update when the URL changes (including after removing a filter via `updateUrlWithUnmatchedFilters`). This makes removals appear to do nothing until some unrelated re-render happens. Consider deriving filters from router location (e.g., `useLocation().search`) or storing them in local state and updating state in `handleRemoveUrlFilter` (similar to the vertical collapse component). ########## superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx: ########## @@ -107,9 +107,16 @@ const publishDataMask = debounce( const previousParams = new URLSearchParams(search); const newParams = new URLSearchParams(); let dataMaskKey: string | null; + let risonFilterValue: string | null = null; + previousParams.forEach((value, key) => { if (!EXCLUDED_URL_PARAMS.includes(key)) { - newParams.append(key, value); + if (key === 'f') { + // Preserve the original Rison filter value to avoid encoding + risonFilterValue = value; + } else { + newParams.append(key, value); + } } }); Review Comment: This attempts to preserve Rison encoding, but `URLSearchParams` provides a *decoded* value (and treats `+` as space), so `risonFilterValue` may no longer match the original raw query bytes. Re-inserting the decoded value without encoding can generate an invalid query string or change the filter payload. A safer approach is to preserve the raw `f=...` substring from the original `search` (e.g., via regex capture on `search`), or to re-encode with `encodeURIComponent` consistently (and avoid manual concatenation). ########## superset-frontend/src/dashboard/util/risonFilters.ts: ########## @@ -0,0 +1,497 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { + QueryObjectFilterClause, + PartialFilters, + DataMaskStateWithId, +} from '@superset-ui/core'; +import rison from 'rison'; + +export interface RisonFilter { + subject: string; + operator: string; + comparator: string | number | boolean | (string | number)[]; +} + +export interface IntelligentRisonInjectionResult { + updatedDataMask: DataMaskStateWithId; + unmatchedFilters: RisonFilter[]; +} + +/** + * Parse individual filter condition + */ +function parseFilterCondition(key: string, value: unknown): RisonFilter { + // Handle comparison operators: (gt:100), (between:!(1,10)) + if (typeof value === 'object' && value !== null && !Array.isArray(value)) { + const [operator, operatorValue] = Object.entries( + value as Record<string, unknown>, + )[0]; + + switch (operator) { + case 'gt': + return { + subject: key, + operator: '>', + comparator: operatorValue as string | number, + }; + case 'gte': + return { + subject: key, + operator: '>=', + comparator: operatorValue as string | number, + }; + case 'lt': + return { + subject: key, + operator: '<', + comparator: operatorValue as string | number, + }; + case 'lte': + return { + subject: key, + operator: '<=', + comparator: operatorValue as string | number, + }; + case 'between': + return { + subject: key, + operator: 'BETWEEN', + comparator: operatorValue as (string | number)[], + }; + case 'like': + return { + subject: key, + operator: 'LIKE', + comparator: operatorValue as string, + }; + default: + return { + subject: key, + operator: '==', + comparator: value as unknown as string | number, + }; + } + } + + // Handle IN operator: !(value1,value2) + if (Array.isArray(value)) { + return { + subject: key, + operator: 'IN', + comparator: value as (string | number)[], + }; + } + + // Handle simple equality + return { + subject: key, + operator: '==', + comparator: value as string | number | boolean, + }; +} + +/** + * Parse Rison filter syntax from URL parameter. + * Supports formats like: (country:USA,year:2024) + */ +export function parseRisonFilters(risonString: string): RisonFilter[] { + try { + const parsed = rison.decode(risonString); + const filters: RisonFilter[] = []; + + if (!parsed || typeof parsed !== 'object') { + return filters; + } + + const parsedObj = parsed as Record<string, unknown>; + + // Handle OR operator: OR:!(condition1,condition2) + if (parsedObj.OR && Array.isArray(parsedObj.OR)) { + (parsedObj.OR as Record<string, unknown>[]).forEach(condition => { + if (typeof condition === 'object') { + Object.entries(condition).forEach(([key, value]) => { + filters.push(parseFilterCondition(key, value)); + }); + } + }); + return filters; + } + + // Handle NOT operator: NOT:(condition) + if (parsedObj.NOT && typeof parsedObj.NOT === 'object') { + Object.entries(parsedObj.NOT as Record<string, unknown>).forEach( + ([key, value]) => { + const filter = parseFilterCondition(key, value); + if (filter.operator === '==') { + filter.operator = '!='; + } else if (filter.operator === 'IN') { + filter.operator = 'NOT IN'; + } + filters.push(filter); + }, + ); + return filters; + } + + // Handle regular filters + Object.entries(parsedObj).forEach(([key, value]) => { + if (key !== 'OR' && key !== 'NOT') { + filters.push(parseFilterCondition(key, value)); + } + }); + + return filters; + } catch (error) { + console.warn('Failed to parse Rison filters:', error); + return []; + } +} + +/** + * Convert Rison filters to Superset adhoc filter format + */ +export function risonToAdhocFilters( + risonFilters: RisonFilter[], +): QueryObjectFilterClause[] { + return risonFilters.map( + filter => + ({ + expressionType: 'SIMPLE' as const, + clause: 'WHERE' as const, + subject: filter.subject, + operator: filter.operator, + comparator: filter.comparator, + }) as unknown as QueryObjectFilterClause, + ); +} + +/** + * Prettify Rison filter URL by replacing encoded characters. + * Uses browser history API to update URL without page reload. + */ +export function prettifyRisonFilterUrl(): void { + try { + const currentUrl = window.location.href; + + if (!currentUrl.includes('&f=') && !currentUrl.includes('?f=')) { + return; + } + + const urlMatch = currentUrl.match(/([?&])f=([^&]*)/); + if (!urlMatch) { + return; + } + + const separator = urlMatch[1]; + let risonValue = urlMatch[2]; + + if (!risonValue.includes('%') && !risonValue.includes('+')) { + return; + } + + let previousValue = ''; + let decodeAttempts = 0; + while (risonValue !== previousValue && decodeAttempts < 5) { + previousValue = risonValue; + try { + if (risonValue.includes('%')) { + risonValue = decodeURIComponent(risonValue); + } + } catch { + break; + } + decodeAttempts += 1; + } + + risonValue = risonValue.replace(/\+/g, ' '); + + const matchIndex = urlMatch.index ?? 0; + const beforeRison = currentUrl.substring(0, matchIndex); + const afterRison = currentUrl.substring(matchIndex + urlMatch[0].length); + const prettifiedUrl = `${beforeRison}${separator}f=${risonValue}${afterRison}`; + + if (prettifiedUrl !== currentUrl) { + window.history.replaceState(window.history.state, '', prettifiedUrl); + } + } catch (error) { + console.warn('Failed to prettify Rison URL:', error); + } +} + +/** + * Get Rison filter parameter from URL + */ +export function getRisonFilterParam(): string | null { + const params = new URLSearchParams(window.location.search); + return params.get('f'); +} + +/** + * Convert an array of RisonFilter back to Rison string format + */ +export function risonFiltersToString(filters: RisonFilter[]): string { + if (filters.length === 0) { + return ''; + } + + const risonObject: Record< + string, + string | number | boolean | (string | number)[] | Record<string, unknown> + > = {}; + + filters.forEach(filter => { + if (filter.operator === 'IN' && Array.isArray(filter.comparator)) { + risonObject[filter.subject] = filter.comparator; + } else if (filter.operator === '==') { + risonObject[filter.subject] = filter.comparator; + } else { + const operatorMap: Record<string, string> = { + '>': 'gt', + '>=': 'gte', + '<': 'lt', + '<=': 'lte', + BETWEEN: 'between', + LIKE: 'like', + }; + + const risonOp = operatorMap[filter.operator] || filter.operator; + risonObject[filter.subject] = { [risonOp]: filter.comparator }; + } + }); + + try { + return rison.encode(risonObject); Review Comment: `risonFiltersToString()` will produce invalid/unparseable Rison filter syntax for operators like `!=` and `NOT IN` (and potentially `ILIKE`). For example, it will emit `{ '!=': 'USA' }`, but `parseRisonFilters()` doesn't interpret `'!='` as NOT—so removing/updating URL filters can silently change semantics and/or break round-tripping. Add explicit encoding for NOT and NOT IN (e.g., `NOT:(col:value)` and `NOT:(col:!(...))`), and ensure decode/encode symmetry for the operator set you support. ########## superset-frontend/src/dashboard/util/risonFilters.ts: ########## @@ -0,0 +1,497 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { + QueryObjectFilterClause, + PartialFilters, + DataMaskStateWithId, +} from '@superset-ui/core'; +import rison from 'rison'; + +export interface RisonFilter { + subject: string; + operator: string; + comparator: string | number | boolean | (string | number)[]; +} + +export interface IntelligentRisonInjectionResult { + updatedDataMask: DataMaskStateWithId; + unmatchedFilters: RisonFilter[]; +} + +/** + * Parse individual filter condition + */ +function parseFilterCondition(key: string, value: unknown): RisonFilter { + // Handle comparison operators: (gt:100), (between:!(1,10)) + if (typeof value === 'object' && value !== null && !Array.isArray(value)) { + const [operator, operatorValue] = Object.entries( + value as Record<string, unknown>, + )[0]; + + switch (operator) { + case 'gt': + return { + subject: key, + operator: '>', + comparator: operatorValue as string | number, + }; + case 'gte': + return { + subject: key, + operator: '>=', + comparator: operatorValue as string | number, + }; + case 'lt': + return { + subject: key, + operator: '<', + comparator: operatorValue as string | number, + }; + case 'lte': + return { + subject: key, + operator: '<=', + comparator: operatorValue as string | number, + }; + case 'between': + return { + subject: key, + operator: 'BETWEEN', + comparator: operatorValue as (string | number)[], + }; + case 'like': + return { + subject: key, + operator: 'LIKE', + comparator: operatorValue as string, + }; + default: + return { + subject: key, + operator: '==', + comparator: value as unknown as string | number, + }; + } + } + + // Handle IN operator: !(value1,value2) + if (Array.isArray(value)) { + return { + subject: key, + operator: 'IN', + comparator: value as (string | number)[], + }; + } + + // Handle simple equality + return { + subject: key, + operator: '==', + comparator: value as string | number | boolean, + }; +} + +/** + * Parse Rison filter syntax from URL parameter. + * Supports formats like: (country:USA,year:2024) + */ +export function parseRisonFilters(risonString: string): RisonFilter[] { + try { + const parsed = rison.decode(risonString); + const filters: RisonFilter[] = []; + + if (!parsed || typeof parsed !== 'object') { + return filters; + } + + const parsedObj = parsed as Record<string, unknown>; + + // Handle OR operator: OR:!(condition1,condition2) + if (parsedObj.OR && Array.isArray(parsedObj.OR)) { + (parsedObj.OR as Record<string, unknown>[]).forEach(condition => { + if (typeof condition === 'object') { + Object.entries(condition).forEach(([key, value]) => { + filters.push(parseFilterCondition(key, value)); + }); + } + }); + return filters; + } + + // Handle NOT operator: NOT:(condition) + if (parsedObj.NOT && typeof parsedObj.NOT === 'object') { + Object.entries(parsedObj.NOT as Record<string, unknown>).forEach( + ([key, value]) => { + const filter = parseFilterCondition(key, value); + if (filter.operator === '==') { + filter.operator = '!='; + } else if (filter.operator === 'IN') { + filter.operator = 'NOT IN'; + } + filters.push(filter); + }, + ); + return filters; + } + + // Handle regular filters + Object.entries(parsedObj).forEach(([key, value]) => { + if (key !== 'OR' && key !== 'NOT') { + filters.push(parseFilterCondition(key, value)); + } + }); + + return filters; + } catch (error) { + console.warn('Failed to parse Rison filters:', error); + return []; + } +} + +/** + * Convert Rison filters to Superset adhoc filter format + */ +export function risonToAdhocFilters( + risonFilters: RisonFilter[], +): QueryObjectFilterClause[] { + return risonFilters.map( + filter => + ({ + expressionType: 'SIMPLE' as const, + clause: 'WHERE' as const, + subject: filter.subject, + operator: filter.operator, + comparator: filter.comparator, + }) as unknown as QueryObjectFilterClause, + ); +} + +/** + * Prettify Rison filter URL by replacing encoded characters. + * Uses browser history API to update URL without page reload. + */ +export function prettifyRisonFilterUrl(): void { + try { + const currentUrl = window.location.href; + + if (!currentUrl.includes('&f=') && !currentUrl.includes('?f=')) { + return; + } + + const urlMatch = currentUrl.match(/([?&])f=([^&]*)/); + if (!urlMatch) { + return; + } + + const separator = urlMatch[1]; + let risonValue = urlMatch[2]; + + if (!risonValue.includes('%') && !risonValue.includes('+')) { + return; + } + + let previousValue = ''; + let decodeAttempts = 0; + while (risonValue !== previousValue && decodeAttempts < 5) { + previousValue = risonValue; + try { + if (risonValue.includes('%')) { + risonValue = decodeURIComponent(risonValue); + } + } catch { + break; + } + decodeAttempts += 1; + } + + risonValue = risonValue.replace(/\+/g, ' '); + + const matchIndex = urlMatch.index ?? 0; + const beforeRison = currentUrl.substring(0, matchIndex); + const afterRison = currentUrl.substring(matchIndex + urlMatch[0].length); + const prettifiedUrl = `${beforeRison}${separator}f=${risonValue}${afterRison}`; + + if (prettifiedUrl !== currentUrl) { + window.history.replaceState(window.history.state, '', prettifiedUrl); + } + } catch (error) { + console.warn('Failed to prettify Rison URL:', error); + } +} + +/** + * Get Rison filter parameter from URL + */ +export function getRisonFilterParam(): string | null { + const params = new URLSearchParams(window.location.search); + return params.get('f'); +} + +/** + * Convert an array of RisonFilter back to Rison string format + */ +export function risonFiltersToString(filters: RisonFilter[]): string { + if (filters.length === 0) { + return ''; + } + + const risonObject: Record< + string, + string | number | boolean | (string | number)[] | Record<string, unknown> + > = {}; + + filters.forEach(filter => { + if (filter.operator === 'IN' && Array.isArray(filter.comparator)) { + risonObject[filter.subject] = filter.comparator; + } else if (filter.operator === '==') { + risonObject[filter.subject] = filter.comparator; + } else { + const operatorMap: Record<string, string> = { + '>': 'gt', + '>=': 'gte', + '<': 'lt', + '<=': 'lte', + BETWEEN: 'between', + LIKE: 'like', + }; + + const risonOp = operatorMap[filter.operator] || filter.operator; + risonObject[filter.subject] = { [risonOp]: filter.comparator }; + } + }); + + try { + return rison.encode(risonObject); Review Comment: `risonFiltersToString()` will produce invalid/unparseable Rison filter syntax for operators like `!=` and `NOT IN` (and potentially `ILIKE`). For example, it will emit `{ '!=': 'USA' }`, but `parseRisonFilters()` doesn't interpret `'!='` as NOT—so removing/updating URL filters can silently change semantics and/or break round-tripping. Add explicit encoding for NOT and NOT IN (e.g., `NOT:(col:value)` and `NOT:(col:!(...))`), and ensure decode/encode symmetry for the operator set you support. ########## superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/VerticalCollapse.tsx: ########## @@ -0,0 +1,173 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { useMemo, useState, useCallback } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { css, useTheme, SupersetTheme } from '@apache-superset/core/theme'; +import { Icons } from '@superset-ui/core/components/Icons'; +import { FilterBarOrientation } from 'src/dashboard/types'; +import { + updateUrlWithUnmatchedFilters, + getRisonFilterParam, + parseRisonFilters, +} from 'src/dashboard/util/risonFilters'; +import UrlFilterTag from './UrlFilterTag'; +import { UrlFilterIndicator } from './selectors'; + +const UrlFiltersVerticalCollapse = (props: { + urlFilters: UrlFilterIndicator[]; +}) => { + const { urlFilters: initialFilters } = props; + const theme = useTheme(); + const [isOpen, setIsOpen] = useState(true); + const [urlFilters, setUrlFilters] = + useState<UrlFilterIndicator[]>(initialFilters); + + const toggleSection = useCallback(() => { + setIsOpen(prev => !prev); + }, []); + + const handleRemoveFilter = useCallback( + (filterToRemove: UrlFilterIndicator) => { + const risonParam = getRisonFilterParam(); + if (!risonParam) return; + + const currentFilters = parseRisonFilters(risonParam); + const remaining = currentFilters.filter( + f => f.subject !== filterToRemove.filter.subject, + ); + + updateUrlWithUnmatchedFilters(remaining); + setUrlFilters(prev => + prev.filter(f => f.subject !== filterToRemove.subject), + ); Review Comment: Removal and React keys are based only on `subject`. If multiple URL filters share the same subject (possible with OR lists, or multiple conditions on the same column), clicking close will remove *all* filters for that subject and the `key` will collide, causing rendering bugs. Use a stable unique identifier per chip (e.g., include `subject+operator+JSON.stringify(comparator)` or carry an index/id from parsing) and remove based on that same unique identity rather than just `subject`. ########## superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx: ########## @@ -148,9 +155,16 @@ const publishDataMask = debounce( if (appRoot !== '/' && replacementPathname.startsWith(appRoot)) { replacementPathname = replacementPathname.substring(appRoot.length); } + // Manually reconstruct the search string to preserve Rison filter encoding + let searchString = newParams.toString(); + if (risonFilterValue) { + const separator = searchString ? '&' : ''; + searchString = `${searchString}${separator}f=${risonFilterValue}`; + } + history.replace({ pathname: replacementPathname, - search: newParams.toString(), + search: searchString, }); Review Comment: This attempts to preserve Rison encoding, but `URLSearchParams` provides a *decoded* value (and treats `+` as space), so `risonFilterValue` may no longer match the original raw query bytes. Re-inserting the decoded value without encoding can generate an invalid query string or change the filter payload. A safer approach is to preserve the raw `f=...` substring from the original `search` (e.g., via regex capture on `search`), or to re-encode with `encodeURIComponent` consistently (and avoid manual concatenation). ########## superset-frontend/src/dashboard/components/nativeFilters/FilterBar/UrlFilters/VerticalCollapse.tsx: ########## @@ -0,0 +1,173 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { useMemo, useState, useCallback } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { css, useTheme, SupersetTheme } from '@apache-superset/core/theme'; +import { Icons } from '@superset-ui/core/components/Icons'; +import { FilterBarOrientation } from 'src/dashboard/types'; +import { + updateUrlWithUnmatchedFilters, + getRisonFilterParam, + parseRisonFilters, +} from 'src/dashboard/util/risonFilters'; +import UrlFilterTag from './UrlFilterTag'; +import { UrlFilterIndicator } from './selectors'; + +const UrlFiltersVerticalCollapse = (props: { + urlFilters: UrlFilterIndicator[]; +}) => { + const { urlFilters: initialFilters } = props; + const theme = useTheme(); + const [isOpen, setIsOpen] = useState(true); + const [urlFilters, setUrlFilters] = + useState<UrlFilterIndicator[]>(initialFilters); + + const toggleSection = useCallback(() => { + setIsOpen(prev => !prev); + }, []); + + const handleRemoveFilter = useCallback( + (filterToRemove: UrlFilterIndicator) => { + const risonParam = getRisonFilterParam(); + if (!risonParam) return; + + const currentFilters = parseRisonFilters(risonParam); + const remaining = currentFilters.filter( + f => f.subject !== filterToRemove.filter.subject, + ); + + updateUrlWithUnmatchedFilters(remaining); + setUrlFilters(prev => + prev.filter(f => f.subject !== filterToRemove.subject), + ); + }, + [], + ); + + const sectionContainerStyle = useCallback( + (theme: SupersetTheme) => css` + margin-bottom: ${theme.sizeUnit * 3}px; + padding: 0 ${theme.sizeUnit * 4}px; + `, + [], + ); + + const sectionHeaderStyle = useCallback( + (theme: SupersetTheme) => css` + display: flex; + align-items: center; + justify-content: space-between; + padding: ${theme.sizeUnit * 2}px 0; + cursor: pointer; + user-select: none; + + &:hover { + background: ${theme.colorBgTextHover}; + margin: 0 -${theme.sizeUnit * 2}px; + padding: ${theme.sizeUnit * 2}px; + border-radius: ${theme.borderRadius}px; + } + `, + [], + ); + + const sectionTitleStyle = useCallback( + (theme: SupersetTheme) => css` + margin: 0; + font-size: ${theme.fontSize}px; + font-weight: ${theme.fontWeightStrong}; + color: ${theme.colorText}; + line-height: 1.3; + display: flex; + align-items: center; + gap: ${theme.sizeUnit}px; + `, + [], + ); + + const sectionContentStyle = useCallback( + (theme: SupersetTheme) => css` + padding: ${theme.sizeUnit * 2}px 0; + `, + [], + ); + + const dividerStyle = useCallback( + (theme: SupersetTheme) => css` + height: 1px; + background: ${theme.colorSplit}; + margin: ${theme.sizeUnit * 2}px 0; + `, + [], + ); + + const iconStyle = useCallback( + (open: boolean, theme: SupersetTheme) => css` + transform: ${open ? 'rotate(0deg)' : 'rotate(180deg)'}; + transition: transform 0.2s ease; + color: ${theme.colorTextSecondary}; + `, + [], + ); + + const filterIndicators = useMemo( + () => + urlFilters.map(filter => ( + <UrlFilterTag + key={filter.subject} + filter={filter} + orientation={FilterBarOrientation.Vertical} + onRemove={handleRemoveFilter} + /> + )), + [urlFilters, handleRemoveFilter], + ); Review Comment: Removal and React keys are based only on `subject`. If multiple URL filters share the same subject (possible with OR lists, or multiple conditions on the same column), clicking close will remove *all* filters for that subject and the `key` will collide, causing rendering bugs. Use a stable unique identifier per chip (e.g., include `subject+operator+JSON.stringify(comparator)` or carry an index/id from parsing) and remove based on that same unique identity rather than just `subject`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
