codeant-ai-for-open-source[bot] commented on code in PR #40958: URL: https://github.com/apache/superset/pull/40958#discussion_r3482623130
########## superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py: ########## @@ -0,0 +1,454 @@ +# 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: remove_chart_from_dashboard + +This tool removes a chart from an existing dashboard. It is the inverse of +add_chart_to_existing_dashboard: it deletes the chart's CHART component(s) +from position_json (pruning ROW/COLUMN containers that become empty), +removes the chart from the dashboard's slices relationship, and cleans +stale references to the chart from json_metadata (expanded_slices, +timed_refresh_immune_slices, filter_scopes). +""" + +import logging +from typing import Any, Dict + +from fastmcp import Context +from sqlalchemy.exc import SQLAlchemyError +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.commands.exceptions import CommandException +from superset.extensions import event_logger +from superset.mcp_service.dashboard.schemas import ( + DashboardInfo, + RemoveChartFromDashboardRequest, + RemoveChartFromDashboardResponse, + serialize_chart_summary, +) +from superset.mcp_service.privacy import user_can_view_data_model_metadata +from superset.mcp_service.utils.url_utils import get_superset_base_url +from superset.utils import json + +logger = logging.getLogger(__name__) + +# Container types that should be deleted once they have no children left. +# TAB/TABS/GRID/ROOT containers are intentionally kept even when empty — +# deleting a TAB would silently change the dashboard's visible structure. +_PRUNABLE_TYPES = ("ROW", "COLUMN") + + +def _find_chart_keys(layout: Dict[str, Any], chart_id: int) -> list[str]: + """Return all layout keys of CHART components referencing *chart_id*. + + A chart can legitimately appear more than once in a layout (e.g. under + multiple tabs), so all occurrences are returned. + """ + # Accept both int and string chartId — position_json is user/frontend-authored + # and imported or hand-edited layouts may store chartId as a string. + return [ + key + for key, node in layout.items() + if isinstance(node, dict) + and node.get("type") == "CHART" + and (node.get("meta") or {}).get("chartId") in (chart_id, str(chart_id)) + ] + + +def _find_parent_key(layout: Dict[str, Any], component_key: str) -> str | None: + """Find the component whose children list contains *component_key*. + + The reverse lookup scans children lists instead of trusting the + ``parents`` metadata on the node, which can be stale in hand-edited or + programmatically generated layouts. + """ + for key, node in layout.items(): + if not isinstance(node, dict): + continue + children = node.get("children") + if isinstance(children, list) and component_key in children: + return key + return None + + +def _remove_component_and_prune( + layout: Dict[str, Any], component_key: str +) -> list[str]: + """Remove *component_key* from the layout and prune empty containers. + + Walks up the parent chain deleting ROW/COLUMN containers that become + empty as a result of the removal, so no orphaned wrapper nodes are left + behind. Returns the list of removed layout keys. + """ + removed: list[str] = [] + parent_key = _find_parent_key(layout, component_key) + + layout.pop(component_key, None) + removed.append(component_key) + + child_key = component_key + while parent_key is not None: + parent = layout.get(parent_key) + if not isinstance(parent, dict): + break + children = parent.get("children") + if isinstance(children, list): + parent["children"] = [c for c in children if c != child_key] + if parent.get("type") in _PRUNABLE_TYPES and not parent.get("children"): + grandparent_key = _find_parent_key(layout, parent_key) + layout.pop(parent_key, None) + removed.append(parent_key) + child_key = parent_key + parent_key = grandparent_key + else: + break + + return removed + + +def _remove_chart_from_layout(layout: Dict[str, Any], chart_id: int) -> list[str]: + """Remove every CHART component for *chart_id* from the layout. Review Comment: **Suggestion:** This performs the dashboard mutation and metadata mutation in separate commits, so failures on the second commit produce a "failed" response after the chart has already been removed. That creates partial state and misleading error semantics for callers. Keep these writes in one transaction (or make second-step failure non-fatal and clearly report partial success). [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Tool can report failure after successfully removing chart. - ⚠️ MCP agents may retry or compensate against already-mutated dashboards. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Use `_call_remove()` from `tests/unit_tests/mcp_service/dashboard/tool/test_remove_chart_from_dashboard.py:256-260` to call `remove_chart_from_dashboard` on a dashboard where the target chart appears in both `position_json` and `slices` and where json_metadata contains references to that chart (similar to `test_json_metadata_cleanup` at lines 252-40). 2. Let `UpdateDashboardCommand.run()` succeed so that the first transaction at `superset/commands/dashboard/update.py:58-79` commits an update applying the pruned layout and `remaining_slices` (invoked from `remove_chart_from_dashboard.py:60-61`), which detaches the chart from the dashboard and removes its CHART nodes, while `_clean_json_metadata` has already produced a cleaned `metadata` dict in memory at lines 52-58. 3. Simulate a failure in the second commit by causing `db.session.commit()` inside the `if metadata_changed and isinstance(metadata, dict)` block at `remove_chart_from_dashboard.py:118-125` to raise `SQLAlchemyError` (e.g. by patching `superset.db.session.commit` in a test or by inducing a transient DB error after the first transaction has committed). 4. The outer `try/except` at `remove_chart_from_dashboard.py:257-195` catches this SQLAlchemyError, performs `db.session.rollback()`, and returns a `RemoveChartFromDashboardResponse` with `error="Failed to remove chart from dashboard: ..."`, even though the earlier `UpdateDashboardCommand` transaction has already removed the chart from layout and slices, leaving callers with a failure response and a partially-updated dashboard state. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2fa43146287f441f8ee509d66e29442b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2fa43146287f441f8ee509d66e29442b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py **Line:** 60:125 **Comment:** *Logic Error: This performs the dashboard mutation and metadata mutation in separate commits, so failures on the second commit produce a "failed" response after the chart has already been removed. That creates partial state and misleading error semantics for callers. Keep these writes in one transaction (or make second-step failure non-fatal and clearly report partial success). Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40958&comment_hash=98c4ec1c9292851e60153810285d3cd3915af284995e2823df86c2c1aa8fb549&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40958&comment_hash=98c4ec1c9292851e60153810285d3cd3915af284995e2823df86c2c1aa8fb549&reaction=dislike'>👎</a> ########## superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py: ########## @@ -0,0 +1,454 @@ +# 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: remove_chart_from_dashboard + +This tool removes a chart from an existing dashboard. It is the inverse of +add_chart_to_existing_dashboard: it deletes the chart's CHART component(s) +from position_json (pruning ROW/COLUMN containers that become empty), +removes the chart from the dashboard's slices relationship, and cleans +stale references to the chart from json_metadata (expanded_slices, +timed_refresh_immune_slices, filter_scopes). +""" + +import logging +from typing import Any, Dict + +from fastmcp import Context +from sqlalchemy.exc import SQLAlchemyError +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.commands.exceptions import CommandException +from superset.extensions import event_logger +from superset.mcp_service.dashboard.schemas import ( + DashboardInfo, + RemoveChartFromDashboardRequest, + RemoveChartFromDashboardResponse, + serialize_chart_summary, +) +from superset.mcp_service.privacy import user_can_view_data_model_metadata +from superset.mcp_service.utils.url_utils import get_superset_base_url +from superset.utils import json + +logger = logging.getLogger(__name__) + +# Container types that should be deleted once they have no children left. +# TAB/TABS/GRID/ROOT containers are intentionally kept even when empty — +# deleting a TAB would silently change the dashboard's visible structure. +_PRUNABLE_TYPES = ("ROW", "COLUMN") + + +def _find_chart_keys(layout: Dict[str, Any], chart_id: int) -> list[str]: + """Return all layout keys of CHART components referencing *chart_id*. + + A chart can legitimately appear more than once in a layout (e.g. under + multiple tabs), so all occurrences are returned. + """ + # Accept both int and string chartId — position_json is user/frontend-authored + # and imported or hand-edited layouts may store chartId as a string. + return [ + key + for key, node in layout.items() + if isinstance(node, dict) + and node.get("type") == "CHART" + and (node.get("meta") or {}).get("chartId") in (chart_id, str(chart_id)) + ] + + +def _find_parent_key(layout: Dict[str, Any], component_key: str) -> str | None: + """Find the component whose children list contains *component_key*. + + The reverse lookup scans children lists instead of trusting the + ``parents`` metadata on the node, which can be stale in hand-edited or + programmatically generated layouts. + """ + for key, node in layout.items(): + if not isinstance(node, dict): + continue + children = node.get("children") + if isinstance(children, list) and component_key in children: + return key + return None + + +def _remove_component_and_prune( + layout: Dict[str, Any], component_key: str +) -> list[str]: + """Remove *component_key* from the layout and prune empty containers. + + Walks up the parent chain deleting ROW/COLUMN containers that become + empty as a result of the removal, so no orphaned wrapper nodes are left + behind. Returns the list of removed layout keys. + """ + removed: list[str] = [] + parent_key = _find_parent_key(layout, component_key) + + layout.pop(component_key, None) + removed.append(component_key) + + child_key = component_key + while parent_key is not None: + parent = layout.get(parent_key) + if not isinstance(parent, dict): + break + children = parent.get("children") + if isinstance(children, list): + parent["children"] = [c for c in children if c != child_key] + if parent.get("type") in _PRUNABLE_TYPES and not parent.get("children"): + grandparent_key = _find_parent_key(layout, parent_key) + layout.pop(parent_key, None) + removed.append(parent_key) + child_key = parent_key + parent_key = grandparent_key + else: + break + + return removed + + +def _remove_chart_from_layout(layout: Dict[str, Any], chart_id: int) -> list[str]: + """Remove every CHART component for *chart_id* from the layout. + + Returns all removed layout keys (charts plus pruned containers). + """ + removed: list[str] = [] + for chart_key in _find_chart_keys(layout, chart_id): + # The chart key may already be gone if it shared a pruned container. + if chart_key in layout: + removed.extend(_remove_component_and_prune(layout, chart_key)) + return removed + + +def _remove_id_from_list(values: Any, chart_id: int) -> tuple[Any, bool]: + """Return (new_list, changed) with *chart_id* removed from a list of IDs. + + Handles both int and str representations since json_metadata is + user/frontend-authored and not strictly typed. + """ + if not isinstance(values, list): + return values, False + filtered = [v for v in values if v != chart_id and v != str(chart_id)] + return filtered, len(filtered) != len(values) + + +def _clean_json_metadata(metadata: Dict[str, Any], chart_id: int) -> bool: + """Remove stale references to *chart_id* from a json_metadata dict. + + Cleans ``expanded_slices`` (dict keyed by chart ID), ``filter_scopes`` + (dict keyed by filter chart ID, with per-column ``immune`` ID lists), + and ``timed_refresh_immune_slices`` (list of chart IDs). Mutates + *metadata* in place and returns True when anything changed. + """ + changed = False + chart_key = str(chart_id) + + expanded_slices = metadata.get("expanded_slices") + if isinstance(expanded_slices, dict) and chart_key in expanded_slices: + del expanded_slices[chart_key] + changed = True + + immune_slices, immune_changed = _remove_id_from_list( + metadata.get("timed_refresh_immune_slices"), chart_id + ) + if immune_changed: + metadata["timed_refresh_immune_slices"] = immune_slices + changed = True + + filter_scopes = metadata.get("filter_scopes") + if isinstance(filter_scopes, dict): + if chart_key in filter_scopes: + del filter_scopes[chart_key] + changed = True + for column_scopes in filter_scopes.values(): + if not isinstance(column_scopes, dict): + continue + for column_config in column_scopes.values(): + if not isinstance(column_config, dict): + continue + immune, immune_changed = _remove_id_from_list( + column_config.get("immune"), chart_id + ) + if immune_changed: + column_config["immune"] = immune + changed = True + + return changed Review Comment: **Suggestion:** Metadata cleanup omits `default_filters`, so removing a chart can leave stale default filter entries keyed by that chart ID. Superset's dashboard metadata flow normally prunes chart-scoped keys when slices change; this bypass path should do the same to avoid invalid saved filter state referencing a non-member chart. [incomplete implementation] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Dashboard metadata keeps default_filters for removed filter charts. - ⚠️ Behavior diverges from standard set_dash_metadata pruning semantics. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Create or identify a dashboard whose `json_metadata` contains `default_filters` entries keyed by the chart ID to be removed (structure matching the usage in `superset/daos/dashboard.py:61-68` and `superset/views/utils.py:373-400`, e.g. `\"default_filters\": json.dumps({\"10\": {\"col\": [\"val\"]}})`), and ensure the chart with ID 10 is present in `dashboard.slices`. 2. Invoke the MCP tool via `_call_remove()` in `tests/unit_tests/mcp_service/dashboard/tool/test_remove_chart_from_dashboard.py:256-260`, calling `remove_chart_from_dashboard` with `dashboard_id` for this dashboard and `chart_id` set to the ID that appears in both `slices` and `default_filters`. 3. Inside `remove_chart_from_dashboard` (`superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py:46-58`), `metadata` is loaded from `dashboard.json_metadata` and passed to `_clean_json_metadata` at lines 149-190, which removes references from `expanded_slices`, `timed_refresh_immune_slices`, and `filter_scopes` but never touches the `default_filters` key, so `metadata_changed` only reflects changes in those sections while `default_filters` continues to reference the removed chart ID. 4. After `UpdateDashboardCommand.run()` commits the layout/slices changes at `remove_chart_from_dashboard.py:60-61`, the cleaned `metadata` (which still carries the old `default_filters` mapping) is written back to `updated_dashboard.json_metadata` and committed at lines 118-125; subsequent calls to build extra filters at `superset/views/utils.py:371-400` still decode and iterate this `default_filters` dict keyed by the removed chart ID, leading to persisted stale default-filter configuration even though the chart has been removed from the dashboard. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=81a4eabc183e44138656f678cff12a6e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=81a4eabc183e44138656f678cff12a6e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py **Line:** 149:190 **Comment:** *Incomplete Implementation: Metadata cleanup omits `default_filters`, so removing a chart can leave stale default filter entries keyed by that chart ID. Superset's dashboard metadata flow normally prunes chart-scoped keys when slices change; this bypass path should do the same to avoid invalid saved filter state referencing a non-member chart. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40958&comment_hash=c321fafd104fab15fc5b400662dc5439bfb36c77b5d61119607cfcc063a8b146&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40958&comment_hash=c321fafd104fab15fc5b400662dc5439bfb36c77b5d61119607cfcc063a8b146&reaction=dislike'>👎</a> -- 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]
