sadpandajoe commented on code in PR #44096: URL: https://github.com/apache/superset/pull/44096#discussion_r4017438567
########## superset/mcp_service/dashboard/tool/apply_dashboard_filters.py: ########## @@ -0,0 +1,459 @@ +# 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. + +""" +MCP tool: apply_dashboard_filters + +Applies native filter VALUES to a dashboard for the calling user by +storing a ``dataMask`` in a dashboard permalink. The dashboard's saved +native filter configuration is untouched, so nothing another viewer sees +changes -- which is what separates this tool from manage_native_filters, +which DEFINES the filters and writes to the shared dashboard. +""" + +import logging +from typing import Any + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.constants import EMPTY_FILTER_SQL_EXPRESSION, NO_TIME_RANGE +from superset.extensions import event_logger +from superset.mcp_service.dashboard.permalink import ( + build_dashboard_permalink_url, + create_dashboard_permalink, + get_dashboard_permalink_data_mask, +) +from superset.mcp_service.dashboard.schemas import ( + AppliedFilterSummary, + ApplyDashboardFiltersRequest, + ApplyDashboardFiltersResponse, + ApplyFilterValueSpec, + FilterSelectValue, +) +from superset.mcp_service.dashboard.tool.manage_native_filters import ( + current_native_filter_config, +) + +logger = logging.getLogger(__name__) + +# Filter types this tool knows how to apply a value to. Kept in step with the +# types manage_native_filters can create. +SUPPORTED_FILTER_TYPES: frozenset[str] = frozenset({"filter_select", "filter_time"}) + +# Display strings the frontend uses when labelling a selected value; mirrored +# here so a permalink's label reads the same as a UI-applied one. +_NULL_LABEL = "<NULL>" +_TRUE_LABEL = "TRUE" +_FALSE_LABEL = "FALSE" + + +class _FilterApplyError(Exception): + """Raised internally when a requested filter value cannot be applied.""" + + +def _publish_filters_applied(dashboard_id: int, permalink_key: str) -> bool: + """Best-effort principal nudge; filter state stays in the authorized permalink.""" + from superset.coordination.base import CoordinationService + from superset.realtime.publish import publish_realtime + from superset.websocket.channel import get_realtime_principal + + try: + if not CoordinationService.is_backend_defined(): + return False + principal = get_realtime_principal() + if principal is None: + return False + return publish_realtime( + topic="dashboard.filters_applied", + scope="principal", + payload={"dashboard_id": dashboard_id, "permalink_key": permalink_key}, + routes=[principal["channel"]], + ) + except Exception: # noqa: BLE001 pylint: disable=broad-except + logger.warning( + "Failed to publish filters applied for dashboard %s", dashboard_id + ) + return False + + +def _describe_filters(configs: list[dict[str, Any]]) -> str: + """Render the dashboard's filters as a name/ID list for error messages.""" + if not configs: + return "This dashboard has no native filters." + described = ", ".join( + f"{conf.get('name') or '(unnamed)'} (id={conf.get('id')}, " + f"type={conf.get('filterType')})" + for conf in configs + ) + return f"Filters on this dashboard: {described}." + + +def _resolve_filter(reference: str, configs: list[dict[str, Any]]) -> dict[str, Any]: + """Resolve a filter name or ID to its configuration. + + An exact ID match wins over a name match, so a filter whose display name + happens to equal another filter's ID cannot shadow that filter. + """ + for conf in configs: + if conf.get("id") == reference: + return conf + + wanted = reference.strip().casefold() + matches = [ + conf + for conf in configs + if isinstance(conf.get("name"), str) + and conf["name"].strip().casefold() == wanted + ] + if len(matches) == 1: + return matches[0] + if matches: + raise _FilterApplyError( + f"'{reference}' matches more than one filter on this dashboard " + f"({', '.join(str(conf.get('id')) for conf in matches)}). " + "Pass the filter ID instead of the name." + ) + raise _FilterApplyError( + f"No filter named '{reference}' was found on this dashboard. " + f"{_describe_filters(configs)}" + ) + + +def _value_label(value: FilterSelectValue) -> str: + """Format one selected value the way the dashboard UI labels it.""" + if value is None: + return _NULL_LABEL + if isinstance(value, bool): + return _TRUE_LABEL if value else _FALSE_LABEL + return str(value) + + +def _select_data_mask( + conf: dict[str, Any], values: list[FilterSelectValue] +) -> dict[str, Any]: + """Build the data mask a filter_select filter produces for ``values``. + + Mirrors the frontend's ``getSelectExtraFormData``: a non-empty selection + becomes an ``IN`` predicate on the filter's target column, and an empty + selection on a filter marked ``enableEmptyFilter`` becomes an impossible + predicate (the "required filter, nothing chosen" state) rather than no + filtering at all. + """ + targets = [target for target in (conf.get("targets") or []) if target] + column = (targets[0].get("column") or {}).get("name") if targets else None + if not column: + raise _FilterApplyError( + f"Filter '{conf.get('name') or conf.get('id')}' has no target " + "column, so a value cannot be applied to it." + ) + + control_values = conf.get("controlValues") or {} + if control_values.get("inverseSelection"): + raise _FilterApplyError( + f"Filter '{conf.get('name') or conf.get('id')}' enables inverse " + "selection, which this tool does not support." + ) + if (operator := control_values.get("operatorType", "exact")) != "exact": + raise _FilterApplyError( + f"Filter '{conf.get('name') or conf.get('id')}' uses matching " + f"operator '{operator}', which this tool does not support. " + "Only exact-match select filters are supported." + ) + + if values: + extra_form_data: dict[str, Any] = { + "filters": [{"col": column, "op": "IN", "val": list(values)}] + } + filter_state: dict[str, Any] = { + "value": list(values), + "label": ", ".join(_value_label(value) for value in values), + } + else: + extra_form_data = ( + { + "adhoc_filters": [ + { + "expressionType": "SQL", + "clause": "WHERE", + "sqlExpression": EMPTY_FILTER_SQL_EXPRESSION, + } + ] + } + if control_values.get("enableEmptyFilter") + else {} + ) + filter_state = {"value": None} + + return {"extraFormData": extra_form_data, "filterState": filter_state} + + +def _time_data_mask(time_range: str) -> dict[str, Any]: + """Build the data mask a filter_time filter produces for ``time_range``.""" + is_set = bool(time_range) and time_range != NO_TIME_RANGE Review Comment: "No filter" produces an empty mask here. For a required time filter with a saved default, the live notification applies that clear, but opening the returned permalink restores the default because dashboard hydration only recognizes the select-filter clear sentinel; could this reject or preserve required time clears consistently across both paths? ########## superset-frontend/src/dashboard/useDashboardFilterSync.ts: ########## @@ -0,0 +1,112 @@ +/** + * 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 { useEffect } from 'react'; +import { useDispatch, useStore } from 'react-redux'; +import { t } from '@apache-superset/core/translation'; +import { + addSuccessToast, + removeToast, +} from 'src/components/MessageToasts/actions'; +import { removeDataMask, updateDataMask } from 'src/dataMask/actions'; +import { subscribeRealtime } from 'src/middleware/realtime'; +import { RootState } from './types'; +import { getPermalinkValue } from './components/nativeFilters/FilterBar/keyValue'; + +/** Apply chat permalink state to the mounted dashboard, with one-shot undo. */ +export default function useDashboardFilterSync(dashboardId?: number) { + const dispatch = useDispatch(); + const store = useStore<RootState>(); + + useEffect(() => { + if (!dashboardId) return undefined; + + let active = true; + let request = 0; + let appliedRequest = 0; + let toastId: string | undefined; + const dismissToast = () => { + if (toastId) dispatch(removeToast(toastId)); + toastId = undefined; + }; + const unsubscribe = subscribeRealtime( + 'dashboard.filters_applied', + async payload => { + if ( + !active || + typeof payload !== 'object' || + payload === null || + !('dashboard_id' in payload) || + payload.dashboard_id !== dashboardId || + !('permalink_key' in payload) || + typeof payload.permalink_key !== 'string' || + !payload.permalink_key + ) { + return; + } + request += 1; + const version = request; + try { + const value = await getPermalinkValue(payload.permalink_key); + const mask = value?.state?.dataMask; + if (!active || version !== request || !mask) return; + const entries = Object.entries(mask); + if (!entries.length) return; + + // Read at application time, including edits made during the fetch. + const previous = store.getState().dataMask; + appliedRequest = version; + dismissToast(); + entries.forEach(([id, dataMask]) => { + dispatch(updateDataMask(id, dataMask)); + }); + let undone = false; + const toast = addSuccessToast(t('Filters applied from chat'), { + duration: 8000, + action: { + label: t('Undo'), + onClick: () => { + if (!active || undone || version !== appliedRequest) return; + undone = true; + entries.forEach(([id]) => { Review Comment: Undo restores every affected filter from the pre-application snapshot without checking for newer user edits. If someone changes Region after the chat update and then clicks Undo to revert another affected filter, this loop silently replaces the new Region selection with the older value; should it only restore entries that still match the mask applied by this notification? ########## superset-frontend/src/components/MessageToasts/Toast.tsx: ########## @@ -152,6 +153,17 @@ export default function Toast({ toast, onCloseToast }: ToastPresenterProps) { {icon} <Interweave content={toast.text} noHtml={!toast.allowHtml} /> </div> + {toast.action && ( Review Comment: This adds a keyboard-focusable Undo action to a toast that still disappears after eight seconds without pausing on hover or focus. A keyboard or screen-reader user may not reach the control before it vanishes, or may lose focus while interacting; should actionable toasts pause or extend their timeout, or use a persistent accessible control? -- 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]
