Skip to content

Commit af9ac74

Browse files
fix(mcp): handle string chartId in layout and non-dict position_json
- _find_chart_keys now matches chartId as both int and str to handle imported/hand-edited layouts where chartId may be stored as a string - validate position_json parses to a dict before passing to layout helpers, preventing AttributeError on malformed-but-valid JSON (e.g. "[]") - test: add return type annotation to mock_auth fixture; add docstrings to _chart_node and _call_remove test helpers Addresses CodeAnt AI review findings on #40958.
1 parent 1ae5e61 commit af9ac74

2 files changed

Lines changed: 9 additions & 2 deletions

File tree

superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,12 +59,14 @@ def _find_chart_keys(layout: Dict[str, Any], chart_id: int) -> list[str]:
5959
A chart can legitimately appear more than once in a layout (e.g. under
6060
multiple tabs), so all occurrences are returned.
6161
"""
62+
# Accept both int and string chartId — position_json is user/frontend-authored
63+
# and imported or hand-edited layouts may store chartId as a string.
6264
return [
6365
key
6466
for key, node in layout.items()
6567
if isinstance(node, dict)
6668
and node.get("type") == "CHART"
67-
and (node.get("meta") or {}).get("chartId") == chart_id
69+
and (node.get("meta") or {}).get("chartId") in (chart_id, str(chart_id))
6870
]
6971

7072

@@ -270,6 +272,8 @@ def remove_chart_from_dashboard( # noqa: C901 — complexity is structural (lay
270272
current_layout = json.loads(dashboard.position_json or "{}")
271273
except (json.JSONDecodeError, TypeError):
272274
current_layout = {}
275+
if not isinstance(current_layout, dict):
276+
current_layout = {}
273277

274278
remaining_slices = [
275279
slc for slc in dashboard.slices if slc.id != request.chart_id

tests/unit_tests/mcp_service/dashboard/tool/test_remove_chart_from_dashboard.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
"""
3636

3737
import logging
38+
from collections.abc import Generator
3839
from typing import Any
3940
from unittest.mock import Mock, patch
4041

@@ -64,7 +65,7 @@ def mcp_server() -> object:
6465

6566

6667
@pytest.fixture(autouse=True)
67-
def mock_auth():
68+
def mock_auth() -> Generator[Mock, None, None]:
6869
"""Mock authentication for all tests."""
6970
with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user:
7071
mock_user = Mock()
@@ -127,6 +128,7 @@ def _mock_dashboard(
127128

128129

129130
def _chart_node(key: str, chart_id: int, parents: list[str]) -> dict[str, Any]:
131+
"""Build a minimal CHART layout node dict for use in test layouts."""
130132
return {
131133
"children": [],
132134
"id": key,
@@ -254,6 +256,7 @@ def _tabbed_layout() -> dict[str, Any]:
254256
async def _call_remove(
255257
mcp_server: object, dashboard_id: int = 1, chart_id: int = 10
256258
) -> dict[str, Any]:
259+
"""Call remove_chart_from_dashboard via the MCP client and return structured content."""
257260
async with Client(mcp_server) as client:
258261
result = await client.call_tool(
259262
"remove_chart_from_dashboard",

0 commit comments

Comments
 (0)