diff --git a/docs/docs/using-superset/using-ai-with-superset.mdx b/docs/docs/using-superset/using-ai-with-superset.mdx index 5100bbb941b5..4d9cec77f5b7 100644 --- a/docs/docs/using-superset/using-ai-with-superset.mdx +++ b/docs/docs/using-superset/using-ai-with-superset.mdx @@ -355,6 +355,53 @@ JSON, impact reporting still checks its saved form data and other accessible cha | `delete_dashboard` | Delete a dashboard by ID, UUID, or slug; leaves its charts intact (soft-deletes to trash when `SOFT_DELETE` is enabled; fails if alerts/reports are still attached, checked before editorship; otherwise returns `permission_denied` if the caller isn't an editor of the dashboard — owners, Admins, and explicitly granted editors qualify) | | `restore_dashboard` | Restore a soft-deleted dashboard from trash by ID or UUID (slug lookup does not cover trashed dashboards); not gated on `SOFT_DELETE`, so rows trashed while it was enabled stay recoverable, but only soft-deleted dashboards can be restored — permanently deleted dashboards are unrecoverable; returns `permission_denied` if the caller can see the dashboard but isn't an editor of it, or `NotFound` if the dashboard is outside the caller's RBAC visibility entirely | +### Dashboard text components + +Use `manage_dashboard_markdown` to add, update, or remove markdown tiles, +section headers, and dividers without replacing the dashboard's raw layout. +The caller needs Dashboard write permission and editorship of the dashboard. + +```json +{ + "request": { + "dashboard_id": 42, + "add": [ + {"component_type": "header", "text": "Sales overview"}, + {"component_type": "markdown", "code": "**Updated daily**", "width": 12}, + {"component_type": "divider"} + ] + } +} +``` + +Markdown tiles are appended in their own rows; headers and dividers are appended +directly to the grid or tab. Each addition can specify `target_tab` by name or +component ID (available from `get_dashboard_layout`), including nested tabs. +Without it, additions use the first top-level tab, or the grid on dashboards +without top-level tabs. Markdown width is 1–12 columns (default 4); height is in +8-pixel grid units (default 50). Width updates +must fit the space remaining beside other components in the same 12-column row; +otherwise, the error reports the available width. Components removed in the same +request do not count toward that row width. If any operation is rejected, the +layout remains unchanged. + +The response includes generated `added_component_ids` and a `components` summary. +Reuse those IDs in `update`, for example +`{"id": "MARKDOWN-…", "code": "Revised notes"}`, or in `remove`, for example +`["MARKDOWN-…"]`. Updates merge only supplied non-null fields and cannot change a +component's type or placement. Each update requires at least one non-null field +besides `id`; ID-only or all-null updates are rejected. The reserved dashboard-title `HEADER_ID` is not a +section header and cannot be changed through this tool. + +Operations in one request are validated before saving. Existing charts, native +filters, and unrelated dashboard properties are preserved. Removing a tile prunes +empty row/column wrappers but retains tabs. Invalid component IDs, incompatible +fields, invalid layouts, and conflicting update/removal operations return errors +without saving partial changes. Markdown uses the dashboard's existing renderer +and HTML sanitization configuration. Concurrent layout edits are last-write-wins; +avoid simultaneous edits from the UI and MCP clients. This tool does not reorder +or move existing components; use the dashboard editor for those operations. + ### SQL | Tool | Description | diff --git a/superset/mcp_service/app.py b/superset/mcp_service/app.py index 2d32f41b12ac..3e3ad6406d8d 100644 --- a/superset/mcp_service/app.py +++ b/superset/mcp_service/app.py @@ -135,6 +135,7 @@ def get_default_instructions( - add_chart_to_existing_dashboard: Add a chart to an existing dashboard (requires write access) - delete_dashboard: Delete a dashboard by ID/UUID/slug (requires editor rights — owner or Admin; destructive; does not delete its charts; soft-deletes to trash when the SOFT_DELETE feature flag is on, permanent otherwise) - manage_native_filters: Add, update, remove, or reorder native filters on a dashboard (requires write access; supports filter_select, filter_time, filter_range, and filter_timegrain) +- manage_dashboard_markdown: Add, update, or remove markdown/header/divider layout components on a dashboard (requires write access) - apply_dashboard_filters: Apply values to a dashboard's existing native filters for the calling user and return a shareable permalink (read access; does NOT change the saved dashboard) - remove_chart_from_dashboard: Remove a chart from an existing dashboard (requires write access) - restore_dashboard: Restore a soft-deleted dashboard from trash by ID/UUID (requires editor rights — owner or Admin; only applies to dashboards trashed under the SOFT_DELETE feature flag) @@ -531,7 +532,8 @@ def get_default_instructions( create_dataset_metric, delete_dataset_metric, update_dataset_metric, save_sql_query, add_chart_to_existing_dashboard, manage_native_filters, remove_chart_from_dashboard, update_chart_preview, manage_dashboard_owners, - manage_dashboard_roles, manage_dashboard_certification) require write + manage_dashboard_roles, manage_dashboard_certification, + manage_dashboard_markdown) require write permissions. These tools are only listed for users who have the necessary access. If a write tool does not appear in the tool list, the current user lacks write access. - execute_sql requires SQL Lab access (execute_sql_query permission), which is separate @@ -839,6 +841,7 @@ def create_mcp_app( get_dashboard_layout, list_dashboards, manage_dashboard_certification, + manage_dashboard_markdown, manage_dashboard_owners, manage_dashboard_roles, manage_native_filters, diff --git a/superset/mcp_service/dashboard/constants.py b/superset/mcp_service/dashboard/constants.py index 6010816eb3ec..20763f2a43cd 100644 --- a/superset/mcp_service/dashboard/constants.py +++ b/superset/mcp_service/dashboard/constants.py @@ -26,6 +26,10 @@ GRID_DEFAULT_CHART_WIDTH = 4 GRID_COLUMN_COUNT = 12 +GRID_MIN_ROW_UNITS = 5 +GRID_MAX_ROW_UNITS = 100 +GRID_ID = "GRID_ID" +HEADER_ID = "HEADER_ID" def generate_id(prefix: str) -> str: diff --git a/superset/mcp_service/dashboard/layout_placement.py b/superset/mcp_service/dashboard/layout_placement.py new file mode 100644 index 000000000000..00e1ea1c574e --- /dev/null +++ b/superset/mcp_service/dashboard/layout_placement.py @@ -0,0 +1,309 @@ +# 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. + +""" +Shared layout-placement helpers for MCP tools that insert new components +into a dashboard's ``position_json`` tree. + +Extracted from ``add_chart_to_existing_dashboard`` so that other +layout-mutating tools (e.g. ``manage_dashboard_markdown``) can resolve the +same GRID/TABS insertion target and scaffold the same GRID/ROOT structure +without duplicating the tab-matching logic. +""" + +import re +from collections import deque +from typing import Any, Dict + +from superset.mcp_service.dashboard.constants import generate_id + +# Compiled regex for stripping common emoji Unicode ranges from tab text. +# Uses specific Unicode blocks to avoid overly permissive ranges. +_EMOJI_RE = re.compile( + "[" + "\U0001f300-\U0001f5ff" # Misc Symbols and Pictographs + "\U0001f600-\U0001f64f" # Emoticons + "\U0001f680-\U0001f6ff" # Transport and Map Symbols + "\U0001f900-\U0001f9ff" # Supplemental Symbols and Pictographs + "\U0001fa70-\U0001faff" # Symbols and Pictographs Extended-A + "☀-⛿" # Misc Symbols + "✀-➿" # Dingbats + "︀-️" # Variation Selectors + "‍" # Zero-width joiner + "]+" +) + + +# 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_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 find_next_row_position(layout: Dict[str, Any]) -> str: + """ + Generate a unique ROW ID for a new row in the dashboard layout. + + Uses UUID-based IDs (e.g. ``ROW-a1b2c3d4``) instead of numeric indices + so that the IDs are compatible with real dashboard layouts that use + nanoid-style identifiers. + + Returns: + A new unique ROW ID string. + """ + row_key = generate_id("ROW") + # Ensure uniqueness (extremely unlikely collision, but safe) + while row_key in layout: + row_key = generate_id("ROW") + return row_key + + +def _normalize_tab_text(text: str | None) -> str: + """Strip emoji and extra whitespace from tab text for flexible matching.""" + if not text: + return "" + cleaned = _EMOJI_RE.sub("", text) + return cleaned.strip().lower() + + +def _match_tab_in_children( + layout: Dict[str, Any], + tabs_children: list[str], + target_tab: str, +) -> str | None: + """Search tabs_children for a tab matching target_tab by ID or name. + + Matching is flexible: exact ID match, exact text match, or + case-insensitive text match after stripping emoji characters. + """ + target_normalized = _normalize_tab_text(target_tab) + for tab_id in tabs_children: + tab = layout.get(tab_id) + if not tab or tab.get("type") != "TAB": + continue + tab_text = (tab.get("meta") or {}).get("text", "") + # Exact match on ID or text + if target_tab in (tab_id, tab_text): + return tab_id + # Flexible match: case-insensitive, emoji-stripped + if target_normalized and _normalize_tab_text(tab_text) == target_normalized: + return tab_id + return None + + +def _collect_tabs_groups( + layout: Dict[str, Any], *, include_nested: bool = True +) -> list[list[str]]: + """Collect reachable TABS groups, with ROOT/GRID groups taking precedence. + + Traverse children rather than trusting parent metadata. The non-recursive + mode preserves the default insertion target for callers without target_tab. + """ + groups: list[list[str]] = [] + pending = deque(["ROOT_ID", "GRID_ID"]) + visited: set[str] = set() + collected: set[str] = set() + while pending: + parent_key = pending.popleft() + if parent_key in visited: + continue + visited.add(parent_key) + parent = layout.get(parent_key) + if not isinstance(parent, dict): + continue + for child_id in parent.get("children", []): + child = layout.get(child_id) + if not isinstance(child, dict): + continue + if include_nested: + pending.append(child_id) + if child.get("type") == "TABS" and child_id not in collected: + collected.add(child_id) + tabs_children = child.get("children", []) + if tabs_children: + groups.append(tabs_children) + return groups + + +def _first_tab_from_groups( + layout: Dict[str, Any], groups: list[list[str]] +) -> str | None: + """Return the first valid TAB ID from the collected groups.""" + for tabs_children in groups: + first_tab_id = tabs_children[0] + first_tab = layout.get(first_tab_id) + if first_tab and first_tab.get("type") == "TAB": + return first_tab_id + return None + + +def collect_available_tab_names(layout: Dict[str, Any]) -> list[str]: + """Collect display entries (label + component ID) for all TAB components. + + Always includes the component ID so callers can retry unambiguously even + when multiple tabs share the same display name or a label is blank. + """ + entries: list[str] = [] + for tabs_children in _collect_tabs_groups(layout): + for tab_id in tabs_children: + tab = layout.get(tab_id) + if not tab or tab.get("type") != "TAB": + continue + text = (tab.get("meta") or {}).get("text", "") + entries.append(f"{text} ({tab_id})" if text else tab_id) + return entries + + +def find_tab_insert_target( + layout: Dict[str, Any], target_tab: str | None = None +) -> str | None: + """ + Detect if the dashboard uses tabs and return the appropriate tab's ID. + + When *target_tab* is ``None`` the function returns the first TAB child so + that new rows are placed inside the tab structure rather than directly + under ``GRID_ID``. + + When *target_tab* is provided the function tries to match it against tab + ``meta.text`` (display name) or the raw component ID. If no match is + found ``None`` is returned — the caller is responsible for surfacing an + error rather than silently placing the chart in the wrong tab. + + Returns: + The ID of the matched (or first) TAB component, or ``None``. + """ + groups = _collect_tabs_groups(layout, include_nested=target_tab is not None) + + if target_tab is not None: + for tabs_children in groups: + matched = _match_tab_in_children(layout, tabs_children, target_tab) + if matched: + return matched + # target_tab specified but not found — signal mismatch to the caller. + return None + + return _first_tab_from_groups(layout, groups) + + +def ensure_layout_structure( + layout: Dict[str, Any], component_key: str, parent_id: str +) -> None: + """ + Ensure the dashboard layout has proper GRID and ROOT structure, + and add the new component to the correct parent container. + + Args: + layout: The mutable layout dict to update. + component_key: The top-level component ID to insert (a ROW, or a + GRID/TAB-level component such as HEADER or DIVIDER). + parent_id: The container to add the component to (GRID_ID or a TAB ID). + """ + # Ensure GRID structure exists + if "GRID_ID" not in layout: + layout["GRID_ID"] = { + "children": [], + "id": "GRID_ID", + "parents": ["ROOT_ID"], + "type": "GRID", + } + + # Add the component to the target parent container + if parent := layout.get(parent_id): + if "children" not in parent: + parent["children"] = [] + parent["children"].append(component_key) + else: + # Fallback: add to GRID_ID + if "children" not in layout["GRID_ID"]: + layout["GRID_ID"]["children"] = [] + layout["GRID_ID"]["children"].append(component_key) + + # Update ROOT_ID if it exists, or create it + if "ROOT_ID" in layout: + if "children" not in layout["ROOT_ID"]: + layout["ROOT_ID"]["children"] = [] + # Only add GRID_ID to ROOT_ID when TABS are not already a direct + # child of ROOT_ID. Real Superset dashboards with tabs place a + # TABS container directly under ROOT_ID (ROOT_ID → TABS → TABs). + # Adding GRID_ID as a sibling of TABS confuses the frontend layout + # engine and makes charts invisible. + root_children = layout["ROOT_ID"]["children"] + has_tabs_under_root = any( + layout.get(c, {}).get("type") == "TABS" for c in root_children + ) + if not has_tabs_under_root and "GRID_ID" not in root_children: + root_children.append("GRID_ID") + else: + # Create ROOT_ID if it doesn't exist + layout["ROOT_ID"] = { + "children": ["GRID_ID"], + "id": "ROOT_ID", + "type": "ROOT", + } + + # Ensure dashboard version + if "DASHBOARD_VERSION_KEY" not in layout: + layout["DASHBOARD_VERSION_KEY"] = "v2" diff --git a/superset/mcp_service/dashboard/layout_validation.py b/superset/mcp_service/dashboard/layout_validation.py index d0643d5aa4ae..8a07151a2088 100644 --- a/superset/mcp_service/dashboard/layout_validation.py +++ b/superset/mcp_service/dashboard/layout_validation.py @@ -224,7 +224,7 @@ def _find_cycle(components: dict[str, dict[str, Any]]) -> str | None: def validate_dashboard_layout( # noqa: C901 - layout: dict[str, Any], expected_chart_ids: Collection[int] + layout: dict[str, Any], expected_chart_ids: Collection[int] | None ) -> str | None: """Return an error when an MCP layout replacement is unsafe to persist. @@ -236,6 +236,12 @@ def validate_dashboard_layout( # noqa: C901 ``HEADER_ID`` is dashboard metadata rather than a rendered tree child. Superset also retains an empty, detached ``GRID_ID`` when top-level tabs are used; both are allowed as explicit reserved-node exceptions. + + Pass ``expected_chart_ids=None`` to run only the structural checks. Edits + that never touch ``CHART`` nodes use this, because a dashboard may + legitimately have associated charts that are not yet in its layout (e.g. + after "Save & add to dashboard" from Explore; the frontend appends those + during hydration). """ components, error = _validate_component_shapes(layout) if error: @@ -303,6 +309,9 @@ def validate_dashboard_layout( # noqa: C901 continue return f"Layout component {component_id} is unreachable from ROOT_ID." + if expected_chart_ids is None: + return None + expected = set(expected_chart_ids) if missing := sorted(expected - reachable_chart_ids): return f"Layout would hide dashboard charts: {missing}." diff --git a/superset/mcp_service/dashboard/schemas.py b/superset/mcp_service/dashboard/schemas.py index cde220c7ca64..0a9436054b8d 100644 --- a/superset/mcp_service/dashboard/schemas.py +++ b/superset/mcp_service/dashboard/schemas.py @@ -107,6 +107,12 @@ PaginatedResponse, ) from superset.mcp_service.common.time_range_validation import validate_time_range +from superset.mcp_service.dashboard.constants import ( + GRID_COLUMN_COUNT, + GRID_DEFAULT_CHART_WIDTH, + GRID_MAX_ROW_UNITS, + GRID_MIN_ROW_UNITS, +) from superset.mcp_service.privacy import ( filter_user_directory_fields, strip_user_directory_fields_from_schema, @@ -122,6 +128,7 @@ OmittedFieldsBuilder, ) from superset.mcp_service.utils.sanitization import ( + _remove_dangerous_unicode, sanitize_user_input, sanitize_user_input_with_changes, ) @@ -2557,6 +2564,268 @@ class ManageNativeFiltersResponse(BaseModel): ) +# --------------------------------------------------------------------------- +# manage_dashboard_markdown schemas +# --------------------------------------------------------------------------- + + +class BaseNewDashboardComponentSpec(BaseModel): + """Common placement fields shared by all new markdown/header/divider specs.""" + + model_config = ConfigDict(extra="forbid") + + target_tab: str | None = Field( + None, + description=( + "Tab to add the component to, matched by display name or " + "component ID (see get_dashboard_layout for available tabs). " + "Omit to use the first tab, or the grid if there are no tabs; " + "specify a target when the component should land in a " + "specific one rather than the first tab." + ), + ) + + +class MarkdownComponentSpec(BaseNewDashboardComponentSpec): + """Spec for a new markdown/text tile. + + Placed in its own new row (a MARKDOWN component sits alongside charts, + not as a full-width band), so it composes with existing rows/charts on + the target grid or tab. + """ + + component_type: Literal["markdown"] = Field( + ..., description="Discriminator - must be 'markdown'" + ) + code: str = Field( + ..., min_length=1, description="Markdown (and safe inline HTML) source" + ) + width: int = Field( + GRID_DEFAULT_CHART_WIDTH, + ge=1, + le=GRID_COLUMN_COUNT, + description=( + f"Tile width in grid columns (1-{GRID_COLUMN_COUNT}, " + f"default {GRID_DEFAULT_CHART_WIDTH})" + ), + ) + height: int = Field( + 50, + ge=GRID_MIN_ROW_UNITS, + le=GRID_MAX_ROW_UNITS, + description=( + f"Tile height in grid units ({GRID_MIN_ROW_UNITS}-" + f"{GRID_MAX_ROW_UNITS}; one unit is " + "8 pixels; default 50)" + ), + ) + + +_HEADER_TEXT_MAX_LENGTH = 500 + + +def _sanitize_header_text(value: str) -> str: + """Normalize header text, which the frontend renders as plain React text. + + React escapes the value on render, so HTML stripping or entity escaping + here would only corrupt legitimate text such as ``Revenue < 1M`` (the + dashboard builder stores whatever the user types). Only length and + invisible/control characters are enforced. + """ + text = _remove_dangerous_unicode(value).strip() + if not text: + raise ValueError("text cannot be empty.") + if len(text) > _HEADER_TEXT_MAX_LENGTH: + raise ValueError( + f"text too long ({len(text)} characters). Maximum allowed length " + f"is {_HEADER_TEXT_MAX_LENGTH} characters." + ) + return text + + +class HeaderComponentSpec(BaseNewDashboardComponentSpec): + """Spec for a new section header band. + + Placed directly on the target grid/tab (not inside a row) so it spans + the full dashboard width, matching how the dashboard builder places + dragged header components. + """ + + component_type: Literal["header"] = Field( + ..., description="Discriminator - must be 'header'" + ) + text: str = Field(..., min_length=1, description="Header display text") + header_size: Literal["SMALL_HEADER", "MEDIUM_HEADER", "LARGE_HEADER"] = Field( + "MEDIUM_HEADER", description="Header text size" + ) + background: Literal["BACKGROUND_TRANSPARENT", "BACKGROUND_WHITE"] = Field( + "BACKGROUND_TRANSPARENT", description="Header band background" + ) + + @field_validator("text") + @classmethod + def sanitize_text(cls, v: str) -> str: + """Normalize header text; it renders as plain React text.""" + return _sanitize_header_text(v) + + +class DividerComponentSpec(BaseNewDashboardComponentSpec): + """Spec for a new horizontal divider. + + Placed directly on the target grid/tab (not inside a row), same as + ``HeaderComponentSpec``. Carries no content — only placement. + """ + + component_type: Literal["divider"] = Field( + ..., description="Discriminator - must be 'divider'" + ) + + +NewDashboardComponentSpec = Annotated[ + MarkdownComponentSpec | HeaderComponentSpec | DividerComponentSpec, + Field(discriminator="component_type"), +] + + +class DashboardComponentUpdateSpec(BaseModel): + """Partial update for an existing markdown/header/divider component. + + ``id`` and at least one non-null update field are required. Provided + fields are merged into the existing component. Fields that only apply to + one component type (e.g. ``code`` for markdown, ``text``/``header_size`` + for header) are rejected when used against the wrong component type. + A component's type cannot be changed; remove and re-add instead. + """ + + model_config = ConfigDict(extra="forbid") + + id: str = Field(..., min_length=1, description="ID of the component to update") + code: str | None = Field( + None, min_length=1, description="New markdown source (markdown only)" + ) + width: int | None = Field( + None, + ge=1, + le=GRID_COLUMN_COUNT, + description="New tile width in grid columns (markdown only)", + ) + height: int | None = Field( + None, + ge=GRID_MIN_ROW_UNITS, + le=GRID_MAX_ROW_UNITS, + description=( + f"New tile height in 8-pixel grid units ({GRID_MIN_ROW_UNITS}-" + f"{GRID_MAX_ROW_UNITS}; " + "markdown only)" + ), + ) + text: str | None = Field(None, description="New header text (header only)") + header_size: Literal["SMALL_HEADER", "MEDIUM_HEADER", "LARGE_HEADER"] | None = ( + Field(None, description="New header text size (header only)") + ) + background: Literal["BACKGROUND_TRANSPARENT", "BACKGROUND_WHITE"] | None = Field( + None, description="New header band background (header only)" + ) + + @field_validator("text") + @classmethod + def sanitize_text(cls, v: str | None) -> str | None: + """Normalize header text; it renders as plain React text.""" + return None if v is None else _sanitize_header_text(v) + + @model_validator(mode="after") + def _require_update_field(self) -> "DashboardComponentUpdateSpec": + """Reject updates that cannot change any component metadata.""" + if not any( + getattr(self, field) is not None + for field in ( + "code", + "width", + "height", + "text", + "header_size", + "background", + ) + ): + raise ValueError( + "At least one non-null update field besides id is required" + ) + return self + + +class DashboardComponentSummary(BaseModel): + """Summary of a markdown/header/divider component for LLM consumption.""" + + id: str = Field(description="Layout component ID") + component_type: Literal["markdown", "header", "divider"] = Field( + description="Component type" + ) + meta: Dict[str, Any] = Field( + default_factory=dict, + description=( + "Component metadata (e.g. code/width/height for markdown; " + "text/headerSize/background for header; empty for divider)" + ), + ) + + +class ManageDashboardMarkdownRequest(BaseModel): + """Request schema for the manage_dashboard_markdown tool.""" + + model_config = ConfigDict(extra="forbid") + + dashboard_id: int = Field( + ..., strict=True, gt=0, description="ID of the dashboard to modify" + ) + add: List[NewDashboardComponentSpec] = Field( + default_factory=list, + description="New markdown/header/divider components to create.", + ) + update: List[DashboardComponentUpdateSpec] = Field( + default_factory=list, + description="Partial updates to existing components, addressed by component ID", + ) + remove: List[str] = Field( + default_factory=list, + description="IDs of markdown/header/divider components to delete", + ) + + @model_validator(mode="after") + def _require_at_least_one_operation(self) -> "ManageDashboardMarkdownRequest": + """Reject requests with no component operations.""" + if not self.add and not self.update and not self.remove: + raise ValueError("At least one operation (add, update, remove) is required") + return self + + +class ManageDashboardMarkdownResponse(DashboardMutationErrorFields): + """Response schema for the manage_dashboard_markdown tool.""" + + dashboard_id: int | None = Field(None, description="ID of the dashboard") + dashboard_url: str | None = Field( + None, description="URL to view the updated dashboard" + ) + added_component_ids: List[str] = Field( + default_factory=list, + description=( + "Server-generated IDs of the newly created components, in request order" + ), + ) + updated_component_ids: List[str] = Field( + default_factory=list, description="IDs of the components that were updated" + ) + removed_component_ids: List[str] = Field( + default_factory=list, description="IDs of the components that were removed" + ) + components: List[DashboardComponentSummary] = Field( + default_factory=list, + description=( + "All markdown/header/divider components on the dashboard after " + "the operation" + ), + ) + + # --------------------------------------------------------------------------- # apply_dashboard_filters schemas # --------------------------------------------------------------------------- diff --git a/superset/mcp_service/dashboard/tool/__init__.py b/superset/mcp_service/dashboard/tool/__init__.py index 15524734c21e..0941a52dd086 100644 --- a/superset/mcp_service/dashboard/tool/__init__.py +++ b/superset/mcp_service/dashboard/tool/__init__.py @@ -26,6 +26,7 @@ from .get_dashboard_layout import get_dashboard_layout from .list_dashboards import list_dashboards from .manage_dashboard_certification import manage_dashboard_certification +from .manage_dashboard_markdown import manage_dashboard_markdown from .manage_dashboard_owners import manage_dashboard_owners from .manage_dashboard_roles import manage_dashboard_roles from .manage_native_filters import manage_native_filters @@ -44,6 +45,7 @@ "add_chart_to_existing_dashboard", "apply_dashboard_filters", "manage_dashboard_certification", + "manage_dashboard_markdown", "manage_dashboard_owners", "manage_dashboard_roles", "manage_native_filters", diff --git a/superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py b/superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py index 1e4c0c44ae8d..363c1e6f7bdb 100644 --- a/superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py +++ b/superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py @@ -22,7 +22,6 @@ """ import logging -import re from typing import Any, Dict from fastmcp import Context @@ -36,6 +35,12 @@ GRID_COLUMN_COUNT, GRID_DEFAULT_CHART_WIDTH, ) +from superset.mcp_service.dashboard.layout_placement import ( + collect_available_tab_names, + ensure_layout_structure, + find_next_row_position, + find_tab_insert_target, +) from superset.mcp_service.dashboard.layout_validation import rebuild_parent_chains from superset.mcp_service.dashboard.schemas import ( AddChartToDashboardRequest, @@ -49,154 +54,6 @@ logger = logging.getLogger(__name__) -# Compiled regex for stripping common emoji Unicode ranges from tab text. -# Uses specific Unicode blocks to avoid overly permissive ranges. -_EMOJI_RE = re.compile( - "[" - "\U0001f300-\U0001f5ff" # Misc Symbols and Pictographs - "\U0001f600-\U0001f64f" # Emoticons - "\U0001f680-\U0001f6ff" # Transport and Map Symbols - "\U0001f900-\U0001f9ff" # Supplemental Symbols and Pictographs - "\U0001fa70-\U0001faff" # Symbols and Pictographs Extended-A - "\u2600-\u26ff" # Misc Symbols - "\u2700-\u27bf" # Dingbats - "\ufe00-\ufe0f" # Variation Selectors - "\u200d" # Zero-width joiner - "]+" -) - - -def _find_next_row_position(layout: Dict[str, Any]) -> str: - """ - Generate a unique ROW ID for a new row in the dashboard layout. - - Uses UUID-based IDs (e.g. ``ROW-a1b2c3d4``) instead of numeric indices - so that the IDs are compatible with real dashboard layouts that use - nanoid-style identifiers. - - Returns: - A new unique ROW ID string. - """ - row_key = generate_id("ROW") - # Ensure uniqueness (extremely unlikely collision, but safe) - while row_key in layout: - row_key = generate_id("ROW") - return row_key - - -def _normalize_tab_text(text: str | None) -> str: - """Strip emoji and extra whitespace from tab text for flexible matching.""" - if not text: - return "" - cleaned = _EMOJI_RE.sub("", text) - return cleaned.strip().lower() - - -def _match_tab_in_children( - layout: Dict[str, Any], - tabs_children: list[str], - target_tab: str, -) -> str | None: - """Search tabs_children for a tab matching target_tab by ID or name. - - Matching is flexible: exact ID match, exact text match, or - case-insensitive text match after stripping emoji characters. - """ - target_normalized = _normalize_tab_text(target_tab) - for tab_id in tabs_children: - tab = layout.get(tab_id) - if not tab or tab.get("type") != "TAB": - continue - tab_text = (tab.get("meta") or {}).get("text", "") - # Exact match on ID or text - if target_tab in (tab_id, tab_text): - return tab_id - # Flexible match: case-insensitive, emoji-stripped - if target_normalized and _normalize_tab_text(tab_text) == target_normalized: - return tab_id - return None - - -def _collect_tabs_groups(layout: Dict[str, Any]) -> list[list[str]]: - """Collect all TABS groups from ROOT_ID and GRID_ID children. - - Superset dashboards can place TABS under either ROOT_ID or GRID_ID - depending on how the layout was constructed. - """ - groups: list[list[str]] = [] - for parent_key in ("ROOT_ID", "GRID_ID"): - parent = layout.get(parent_key) - if not parent: - continue - for child_id in parent.get("children", []): - child = layout.get(child_id) - if not child or child.get("type") != "TABS": - continue - tabs_children = child.get("children", []) - if tabs_children: - groups.append(tabs_children) - return groups - - -def _first_tab_from_groups( - layout: Dict[str, Any], groups: list[list[str]] -) -> str | None: - """Return the first valid TAB ID from the collected groups.""" - for tabs_children in groups: - first_tab_id = tabs_children[0] - first_tab = layout.get(first_tab_id) - if first_tab and first_tab.get("type") == "TAB": - return first_tab_id - return None - - -def _collect_available_tab_names(layout: Dict[str, Any]) -> list[str]: - """Collect display entries (label + component ID) for all TAB components. - - Always includes the component ID so callers can retry unambiguously even - when multiple tabs share the same display name or a label is blank. - """ - entries: list[str] = [] - for tabs_children in _collect_tabs_groups(layout): - for tab_id in tabs_children: - tab = layout.get(tab_id) - if not tab or tab.get("type") != "TAB": - continue - text = (tab.get("meta") or {}).get("text", "") - entries.append(f"{text} ({tab_id})" if text else tab_id) - return entries - - -def _find_tab_insert_target( - layout: Dict[str, Any], target_tab: str | None = None -) -> str | None: - """ - Detect if the dashboard uses tabs and return the appropriate tab's ID. - - When *target_tab* is ``None`` the function returns the first TAB child so - that new rows are placed inside the tab structure rather than directly - under ``GRID_ID``. - - When *target_tab* is provided the function tries to match it against tab - ``meta.text`` (display name) or the raw component ID. If no match is - found ``None`` is returned — the caller is responsible for surfacing an - error rather than silently placing the chart in the wrong tab. - - Returns: - The ID of the matched (or first) TAB component, or ``None``. - """ - groups = _collect_tabs_groups(layout) - - if target_tab is not None: - for tabs_children in groups: - matched = _match_tab_in_children(layout, tabs_children, target_tab) - if matched: - return matched - # target_tab specified but not found — signal mismatch to the caller. - return None - - return _first_tab_from_groups(layout, groups) - def _add_chart_to_layout( layout: Dict[str, Any], @@ -268,66 +125,6 @@ def _add_chart_to_layout( return chart_key, column_key, row_key -def _ensure_layout_structure( - layout: Dict[str, Any], row_key: str, parent_id: str -) -> None: - """ - Ensure the dashboard layout has proper GRID and ROOT structure, - and add the new row to the correct parent container. - - Args: - layout: The mutable layout dict to update. - row_key: The ROW component ID to insert. - parent_id: The container to add the row to (GRID_ID or a TAB ID). - """ - # Ensure GRID structure exists - if "GRID_ID" not in layout: - layout["GRID_ID"] = { - "children": [], - "id": "GRID_ID", - "parents": ["ROOT_ID"], - "type": "GRID", - } - - # Add row to the target parent container - if parent := layout.get(parent_id): - if "children" not in parent: - parent["children"] = [] - parent["children"].append(row_key) - else: - # Fallback: add to GRID_ID - if "children" not in layout["GRID_ID"]: - layout["GRID_ID"]["children"] = [] - layout["GRID_ID"]["children"].append(row_key) - - # Update ROOT_ID if it exists, or create it - if "ROOT_ID" in layout: - if "children" not in layout["ROOT_ID"]: - layout["ROOT_ID"]["children"] = [] - # Only add GRID_ID to ROOT_ID when TABS are not already a direct - # child of ROOT_ID. Real Superset dashboards with tabs place a - # TABS container directly under ROOT_ID (ROOT_ID → TABS → TABs). - # Adding GRID_ID as a sibling of TABS confuses the frontend layout - # engine and makes charts invisible. - root_children = layout["ROOT_ID"]["children"] - has_tabs_under_root = any( - layout.get(c, {}).get("type") == "TABS" for c in root_children - ) - if not has_tabs_under_root and "GRID_ID" not in root_children: - root_children.append("GRID_ID") - else: - # Create ROOT_ID if it doesn't exist - layout["ROOT_ID"] = { - "children": ["GRID_ID"], - "id": "ROOT_ID", - "type": "ROOT", - } - - # Ensure dashboard version - if "DASHBOARD_VERSION_KEY" not in layout: - layout["DASHBOARD_VERSION_KEY"] = "v2" - - def _resolve_parent_container( layout: Dict[str, Any], dashboard_id: int, @@ -338,10 +135,10 @@ def _resolve_parent_container( When *target_tab* is specified and not found the caller receives a descriptive error listing available tabs rather than a silent fallback. """ - tab_target = _find_tab_insert_target(layout, target_tab=target_tab) + tab_target = find_tab_insert_target(layout, target_tab=target_tab) if target_tab is not None and tab_target is None: - available = _collect_available_tab_names(layout) + available = collect_available_tab_names(layout) if available: tab_list = ", ".join(available) return None, AddChartToDashboardResponse( @@ -494,7 +291,7 @@ def add_chart_to_existing_dashboard( # noqa: C901 — complexity is structural current_layout = {} # Generate a unique ROW ID for the new row - row_key = _find_next_row_position(current_layout) + row_key = find_next_row_position(current_layout) # Detect tabbed dashboards and resolve target_tab by name or ID. parent_id, tab_error = _resolve_parent_container( @@ -513,7 +310,7 @@ def add_chart_to_existing_dashboard( # noqa: C901 — complexity is structural ) # Ensure proper layout structure - _ensure_layout_structure(current_layout, row_key, parent_id) + ensure_layout_structure(current_layout, row_key, parent_id) # The new row/column/chart nodes were added with empty # ``parents`` (see ``_add_chart_to_layout``); rebuild every diff --git a/superset/mcp_service/dashboard/tool/manage_dashboard_markdown.py b/superset/mcp_service/dashboard/tool/manage_dashboard_markdown.py new file mode 100644 index 000000000000..b56eb50fc1ba --- /dev/null +++ b/superset/mcp_service/dashboard/tool/manage_dashboard_markdown.py @@ -0,0 +1,513 @@ +# 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: manage_dashboard_markdown + +Adds, updates, and removes markdown/header/divider layout components on a +dashboard by translating high-level operations into ``position_json`` tree +edits. Analogous to ``manage_native_filters``, which does the same for the +flat native-filter list in ``json_metadata`` — this tool exists so callers +don't have to hand-build the full raw layout tree just to add a text tile +or section header (see ``generate_dashboard``'s ``position_json`` docstring). +""" + +import logging +from collections import Counter +from typing import Any, Dict, Iterable + +from fastmcp import Context +from sqlalchemy.exc import SQLAlchemyError +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import db, event_logger +from superset.mcp_service.dashboard.constants import ( + generate_id, + GRID_COLUMN_COUNT, + GRID_ID, + HEADER_ID, +) +from superset.mcp_service.dashboard.layout_placement import ( + collect_available_tab_names, + ensure_layout_structure, + find_next_row_position, + find_parent_key, + find_tab_insert_target, + remove_component_and_prune, +) +from superset.mcp_service.dashboard.layout_validation import ( + normalize_chart_id, + rebuild_parent_chains, + validate_dashboard_layout, +) +from superset.mcp_service.dashboard.schemas import ( + DashboardComponentSummary, + DashboardComponentUpdateSpec, + HeaderComponentSpec, + ManageDashboardMarkdownRequest, + ManageDashboardMarkdownResponse, + MarkdownComponentSpec, + NewDashboardComponentSpec, +) +from superset.mcp_service.dashboard.tool.governance_utils import ( + dashboard_url, + find_and_authorize_dashboard, +) +from superset.utils import json + +logger = logging.getLogger(__name__) + + +def _layout_chart_ids(layout: Dict[str, Any]) -> set[int]: + """Return the chart IDs referenced by CHART nodes in ``layout``.""" + chart_ids: set[int] = set() + for component in layout.values(): + if isinstance(component, dict) and component.get("type") == "CHART": + meta = component.get("meta") + if ( + isinstance(meta, dict) + and (chart_id := normalize_chart_id(meta.get("chartId"))) is not None + ): + chart_ids.add(chart_id) + return chart_ids + + +# Maps the tool-facing discriminator to the position_json component type. +_LAYOUT_TYPE_BY_COMPONENT_TYPE: dict[str, str] = { + "markdown": "MARKDOWN", + "header": "HEADER", + "divider": "DIVIDER", +} +_COMPONENT_TYPE_BY_LAYOUT_TYPE = { + v: k for k, v in _LAYOUT_TYPE_BY_COMPONENT_TYPE.items() +} + +# HEADER and DIVIDER are full-width bands placed directly on GRID/TAB — ROW +# does not accept them as children (see layout_validation._PARENT_MAX_DEPTH). +# MARKDOWN composes with charts, so it is wrapped in its own new ROW instead. +_ROW_WRAPPED_LAYOUT_TYPES = frozenset({"MARKDOWN"}) + + +class _ComponentOperationError(Exception): + """Raised internally when a component operation fails validation.""" + + +def _build_component_meta(spec: NewDashboardComponentSpec) -> Dict[str, Any]: + """Build the position_json ``meta`` object for a new component spec.""" + if isinstance(spec, MarkdownComponentSpec): + return {"code": spec.code, "width": spec.width, "height": spec.height} + if isinstance(spec, HeaderComponentSpec): + return { + "text": spec.text, + "headerSize": spec.header_size, + "background": spec.background, + } + # DividerComponentSpec carries no content, only placement. + return {} + + +def _resolve_target_container(layout: Dict[str, Any], target_tab: str | None) -> str: + """Return the GRID_ID or TAB component ID a new component should attach to. + + Raises ``_ComponentOperationError`` when *target_tab* is specified but + does not match any tab, listing the available tabs (or noting there are + none) so the caller can retry unambiguously. + """ + tab_target = find_tab_insert_target(layout, target_tab=target_tab) + + if target_tab is not None and tab_target is None: + available = collect_available_tab_names(layout) + if available: + raise _ComponentOperationError( + f"Tab '{target_tab}' not found. Available tabs: {', '.join(available)}." + ) + raise _ComponentOperationError( + "Dashboard has no tabs. Remove target_tab to add to the " + "default grid layout." + ) + + return tab_target if tab_target else GRID_ID + + +def _add_component_to_layout( + layout: Dict[str, Any], spec: NewDashboardComponentSpec +) -> str: + """Insert a new markdown/header/divider component into *layout*. + + Returns the new component's ID. New nodes are linked with empty + ``parents`` — the caller rebuilds the whole tree's parent chains via + ``rebuild_parent_chains`` after all operations are applied. + """ + parent_id = _resolve_target_container(layout, spec.target_tab) + + layout_type = _LAYOUT_TYPE_BY_COMPONENT_TYPE[spec.component_type] + component_id = generate_id(layout_type) + while component_id in layout: + component_id = generate_id(layout_type) + layout[component_id] = { + "id": component_id, + "type": layout_type, + "children": [], + "meta": _build_component_meta(spec), + "parents": [], + } + + if layout_type in _ROW_WRAPPED_LAYOUT_TYPES: + row_key = find_next_row_position(layout) + layout[row_key] = { + "id": row_key, + "type": "ROW", + "children": [component_id], + "meta": {"background": "BACKGROUND_TRANSPARENT"}, + "parents": [], + } + ensure_layout_structure(layout, row_key, parent_id) + else: + ensure_layout_structure(layout, component_id, parent_id) + + return component_id + + +def _apply_component_update( # noqa: C901 + spec: DashboardComponentUpdateSpec, node: Dict[str, Any], component_type: str +) -> None: + """Merge *spec* into *node*'s meta in place. + + Raises ``_ComponentOperationError`` when a field that only applies to a + different component type is set. + """ + markdown_fields = ("code", "width", "height") + header_fields = ("text", "header_size", "background") + + if component_type != "markdown": + if set_fields := [f for f in markdown_fields if getattr(spec, f) is not None]: + raise _ComponentOperationError( + f"Component '{spec.id}' has type '{component_type}'; fields " + f"{set_fields} only apply to markdown components." + ) + if component_type != "header": + if set_fields := [f for f in header_fields if getattr(spec, f) is not None]: + raise _ComponentOperationError( + f"Component '{spec.id}' has type '{component_type}'; fields " + f"{set_fields} only apply to header components." + ) + + meta = dict(node.get("meta") or {}) + if component_type == "markdown": + if spec.code is not None: + meta["code"] = spec.code + if spec.width is not None: + meta["width"] = spec.width + if spec.height is not None: + meta["height"] = spec.height + elif component_type == "header": + if spec.text is not None: + meta["text"] = spec.text + if spec.header_size is not None: + meta["headerSize"] = spec.header_size + if spec.background is not None: + meta["background"] = spec.background + node["meta"] = meta + + +def _validate_markdown_width( + layout: Dict[str, Any], component_id: str, width: int +) -> None: + """Reject a resize that exceeds the space left by the row's siblings.""" + parent_id = find_parent_key(layout, component_id) + parent = layout.get(parent_id) if parent_id is not None else None + if not parent or parent.get("type") != "ROW": + return + sibling_width = sum( + (layout[child_id].get("meta") or {}).get("width", 0) + for child_id in parent.get("children", []) + if child_id != component_id + ) + if width > (available_width := max(0, GRID_COLUMN_COUNT - sibling_width)): + raise _ComponentOperationError( + f"Cannot resize component '{component_id}' to width {width}: " + f"available width in row '{parent_id}' is {available_width} columns." + ) + + +def _duplicate_ids(ids: Iterable[str]) -> list[str]: + """Return the sorted IDs that appear more than once in ``ids``.""" + return sorted(cid for cid, count in Counter(ids).items() if count > 1) + + +def _apply_updates( + layout: Dict[str, Any], + updates: list[DashboardComponentUpdateSpec], + existing_components: Dict[str, Dict[str, Any]], +) -> list[str]: + """Apply every update spec in order; returns the updated component IDs.""" + if duplicates := _duplicate_ids(spec.id for spec in updates): + raise _ComponentOperationError( + f"update contains duplicate component IDs: {duplicates}." + ) + + for spec in updates: + node = existing_components.get(spec.id) + if node is None: + raise _ComponentOperationError( + f"Cannot update component '{spec.id}': not a markdown/header/" + "divider component on this dashboard." + ) + component_type = _COMPONENT_TYPE_BY_LAYOUT_TYPE[node["type"]] + if component_type == "markdown" and spec.width is not None: + _validate_markdown_width(layout, spec.id, spec.width) + _apply_component_update(spec, layout[spec.id], component_type) + + return [spec.id for spec in updates] + + +def _manageable_components(layout: Dict[str, Any]) -> Dict[str, Dict[str, Any]]: + """Return the markdown/header/divider nodes of *layout* keyed by component ID.""" + return { + key: node + for key, node in layout.items() + if key != HEADER_ID + and isinstance(node, dict) + and node.get("type") in _COMPONENT_TYPE_BY_LAYOUT_TYPE + } + + +def _component_summaries(layout: Dict[str, Any]) -> list[DashboardComponentSummary]: + """Summarize every markdown/header/divider component currently in *layout*.""" + return [ + DashboardComponentSummary( + id=key, + component_type=_COMPONENT_TYPE_BY_LAYOUT_TYPE[node["type"]], + meta=node.get("meta") or {}, + ) + for key, node in _manageable_components(layout).items() + ] + + +@tool( + tags=["mutate"], + class_permission_name="Dashboard", + method_permission_name="write", + annotations=ToolAnnotations( + title="Manage dashboard markdown/header/divider components", + readOnlyHint=False, + destructiveHint=True, + idempotentHint=False, + openWorldHint=False, + ), +) +def manage_dashboard_markdown( # noqa: C901 + request: ManageDashboardMarkdownRequest, ctx: Context +) -> ManageDashboardMarkdownResponse: + """ + Add, update, and remove markdown/header/divider layout components. + + Companion to ``manage_native_filters``, but for the ``position_json`` + layout tree instead of ``json_metadata``: no need to hand-craft the full + raw layout to add a text tile or section header. Markdown tiles are + placed in their own new row so they compose with existing chart rows; + headers and dividers are placed as full-width bands directly on the + target grid or tab (matching how the dashboard builder places them). + + Component IDs are server-generated and returned in the response. + ``target_tab`` (add only) selects which tab a component lands in by + display name or ID; omit it for the first tab, or the grid without tabs. + + Example:: + + manage_dashboard_markdown(request={ + "dashboard_id": 42, + "add": [ + {"component_type": "header", "text": "Sales"}, + {"component_type": "markdown", "code": "**Updated daily**"}, + ], + }) + """ + logger.info( + "Managing dashboard markdown components: dashboard_id=%s", request.dashboard_id + ) + + dashboard, auth_error = find_and_authorize_dashboard( + request.dashboard_id, ManageDashboardMarkdownResponse + ) + if auth_error is not None: + return auth_error + assert dashboard is not None # narrows for mypy + + # Writing position_json directly bypasses UpdateDashboardCommand, so + # mirror its raise_if_managed_externally guard explicitly. + if dashboard.is_managed_externally: + return ManageDashboardMarkdownResponse( + dashboard_id=request.dashboard_id, + managed_externally=True, + error=( + f"Dashboard {request.dashboard_id} is managed externally; its " + "layout is owned by the external system and cannot be " + "changed here." + ), + ) + + try: + with event_logger.log_context( + action="mcp.manage_dashboard_markdown.validation" + ): + try: + current_layout = json.loads(dashboard.position_json or "{}") + except (json.JSONDecodeError, TypeError): + return ManageDashboardMarkdownResponse( + dashboard_id=request.dashboard_id, + error=( + f"Dashboard {request.dashboard_id} has a malformed " + "layout (position_json could not be parsed); cannot " + "safely modify it." + ), + ) + if not isinstance(current_layout, dict): + return ManageDashboardMarkdownResponse( + dashboard_id=request.dashboard_id, + error="Dashboard has a malformed layout: expected a JSON object.", + ) + # Validate before traversing or pruning; do not repair corrupt trees + # by silently dropping nodes. Empty dashboards can be scaffolded. + # Chart coverage is not enforced: this tool never touches CHART + # nodes, and associated charts missing from the layout (attached + # from Explore) are a normal state the frontend hydrates. + if current_layout: + if error := validate_dashboard_layout(current_layout, None): + return ManageDashboardMarkdownResponse( + dashboard_id=request.dashboard_id, + error=f"Dashboard has a malformed layout: {error}", + ) + + existing_components = _manageable_components(current_layout) + original_chart_ids = _layout_chart_ids(current_layout) + + if unknown_removals := [ + cid for cid in request.remove if cid not in existing_components + ]: + return ManageDashboardMarkdownResponse( + dashboard_id=request.dashboard_id, + error=( + "Cannot remove components that are not markdown/" + f"header/divider components on this dashboard: " + f"{unknown_removals}." + ), + ) + + if duplicate_removals := _duplicate_ids(request.remove): + return ManageDashboardMarkdownResponse( + dashboard_id=request.dashboard_id, + error=( + "remove contains duplicate component IDs: " + f"{duplicate_removals}." + ), + ) + + removed_ids = set(request.remove) + if conflicts := sorted( + {spec.id for spec in request.update if spec.id in removed_ids} + ): + return ManageDashboardMarkdownResponse( + dashboard_id=request.dashboard_id, + error=( + f"Components {conflicts} cannot be both updated and removed." + ), + ) + + try: + # Validate resizes against siblings retained by this request. + for component_id in request.remove: + remove_component_and_prune(current_layout, component_id) + + updated_ids = _apply_updates( + current_layout, request.update, existing_components + ) + + added_ids = [ + _add_component_to_layout(current_layout, spec) + for spec in request.add + ] + except _ComponentOperationError as exc: + return ManageDashboardMarkdownResponse( + dashboard_id=request.dashboard_id, error=str(exc) + ) + + # New/updated nodes may carry empty or stale ``parents``; rebuild + # every reachable component's chain from the actual children + # edges so filter-scope derivation sees a correct tree. See + # superset.dashboards.filter_scope.get_chart_ids_in_scope. + current_layout = rebuild_parent_chains(current_layout) + + # Chart nodes present before the edit must survive it unchanged. + if error := validate_dashboard_layout(current_layout, original_chart_ids): + return ManageDashboardMarkdownResponse( + dashboard_id=request.dashboard_id, + error=f"Resulting dashboard layout is invalid: {error}", + ) + + # Capture the receipt before committing. A failed refresh leaves ORM + # attributes expired, so reading them afterwards could mask a saved write. + result = ManageDashboardMarkdownResponse( + dashboard_id=dashboard.id, + dashboard_url=dashboard_url(dashboard), + added_component_ids=added_ids, + updated_component_ids=updated_ids, + removed_component_ids=list(request.remove), + components=_component_summaries(current_layout), + ) + with event_logger.log_context(action="mcp.manage_dashboard_markdown.db_write"): + dashboard.position_json = json.dumps(current_layout) + db.session.commit() # pylint: disable=consider-using-transaction + try: + db.session.refresh(dashboard) + except SQLAlchemyError: + logger.warning( + "Dashboard %s updated but refresh failed; continuing", + request.dashboard_id, + exc_info=True, + ) + + except SQLAlchemyError as db_err: + try: + db.session.rollback() # pylint: disable=consider-using-transaction + except SQLAlchemyError: + logger.warning( + "Database rollback failed during error handling", exc_info=True + ) + logger.error("Dashboard markdown update failed: %s", db_err, exc_info=True) + return ManageDashboardMarkdownResponse( + dashboard_id=request.dashboard_id, + error="Failed to update dashboard due to a database error.", + ) + except Exception as exc: + logger.exception( + "Unexpected error managing markdown components on dashboard %s: %s", + request.dashboard_id, + exc, + ) + raise + + logger.info( + "Managed dashboard markdown components on dashboard %s " + "(added=%d updated=%d removed=%d)", + request.dashboard_id, + len(added_ids), + len(updated_ids), + len(request.remove), + ) + + return result diff --git a/superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py b/superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py index 5d4008d9f05b..0557c499bf3d 100644 --- a/superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py +++ b/superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py @@ -35,6 +35,9 @@ from superset.commands.exceptions import CommandException, ForbiddenError from superset.extensions import event_logger +from superset.mcp_service.dashboard.layout_placement import ( + remove_component_and_prune, +) from superset.mcp_service.dashboard.layout_validation import ( normalize_chart_id, rebuild_parent_chains, @@ -51,11 +54,6 @@ 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*. @@ -72,57 +70,6 @@ def _find_chart_keys(layout: Dict[str, Any], chart_id: int) -> list[str]: ] -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. @@ -132,7 +79,7 @@ def _remove_chart_from_layout(layout: Dict[str, Any], chart_id: int) -> 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)) + removed.extend(remove_component_and_prune(layout, chart_key)) return removed diff --git a/superset/mcp_service/dashboard_scope.py b/superset/mcp_service/dashboard_scope.py index 7a3c22118869..36a9a3fc1f83 100644 --- a/superset/mcp_service/dashboard_scope.py +++ b/superset/mcp_service/dashboard_scope.py @@ -1298,6 +1298,7 @@ def refuse(request: Any, scope: DashboardScope) -> Any: "list_themes", "list_users", "manage_dashboard_certification", + "manage_dashboard_markdown", "manage_dashboard_owners", "manage_dashboard_roles", "manage_native_filters", diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py index e69b2fb64c05..a1e873bcc6bd 100644 --- a/superset/mcp_service/mcp_config.py +++ b/superset/mcp_service/mcp_config.py @@ -406,6 +406,7 @@ class MCPAuthConfigError(ValueError): "generate_dashboard", "generate_explore_link", "manage_dashboard_certification", + "manage_dashboard_markdown", "manage_dashboard_owners", "manage_dashboard_roles", "manage_native_filters", diff --git a/superset/mcp_service/utils/response_size_utils.py b/superset/mcp_service/utils/response_size_utils.py index ed2020f0a431..d217fd65a74d 100644 --- a/superset/mcp_service/utils/response_size_utils.py +++ b/superset/mcp_service/utils/response_size_utils.py @@ -652,6 +652,7 @@ def _spec( "generate_chart": _spec("chart", "chart", reports_success=True), "generate_dashboard": _spec("dashboard", "dashboard"), "manage_dashboard_certification": _spec("dashboard"), + "manage_dashboard_markdown": _spec("dashboard"), "manage_dashboard_owners": _spec("dashboard"), "manage_dashboard_roles": _spec("dashboard"), "manage_native_filters": _spec("dashboard"), diff --git a/tests/unit_tests/mcp_service/dashboard/test_layout_placement.py b/tests/unit_tests/mcp_service/dashboard/test_layout_placement.py new file mode 100644 index 000000000000..983810e0843f --- /dev/null +++ b/tests/unit_tests/mcp_service/dashboard/test_layout_placement.py @@ -0,0 +1,101 @@ +# 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. + +"""Regression tests for emoji-insensitive dashboard tab matching.""" + +import sys +from typing import Any + +import pytest + +from superset.mcp_service.dashboard.layout_placement import ( + _EMOJI_RE, + _normalize_tab_text, + collect_available_tab_names, + find_tab_insert_target, +) + + +def test_emoji_ranges_match_only_documented_code_points() -> None: + """Check every Unicode code point, including gaps and U+FFFD.""" + ranges: tuple[tuple[int, int], ...] = ( + (0x1F300, 0x1F5FF), + (0x1F600, 0x1F64F), + (0x1F680, 0x1F6FF), + (0x1F900, 0x1F9FF), + (0x1FA70, 0x1FAFF), + (0x2600, 0x26FF), + (0x2700, 0x27BF), + (0xFE00, 0xFE0F), + ) + expected: set[int] = {0x200D} + for start, end in ranges: + block: set[int] = set(range(start, end + 1)) + assert expected.isdisjoint(block) + expected.update(block) + + actual: set[int] = { + code_point + for code_point in range(sys.maxunicode + 1) + if _EMOJI_RE.fullmatch(chr(code_point)) + } + assert actual == expected + + +@pytest.mark.parametrize( + ("text", "expected"), + [ + (" 📊 Sales 🚀 ", "sales"), + ("😀 🤖 🪐 ☀ ✂ Revenue", "revenue"), + ("👩\u200d💻 Forecast ☀\ufe0f", "forecast"), + (" Sales [A-Z] ^_` \\ ", "sales [a-z] ^_` \\"), + ("Sales \ufffd", "sales \ufffd"), + ("Équipe 東京", "équipe 東京"), + (None, ""), + ("", ""), + ], +) +def test_normalize_tab_text(text: str | None, expected: str) -> None: + """Strip intended emoji while preserving ordinary labels and punctuation.""" + assert _normalize_tab_text(text) == expected + + +@pytest.mark.parametrize("parent_type", ["TAB", "COLUMN"]) +def test_nested_tab_matching_at_any_depth(parent_type: str) -> None: + """Find nested tabs without changing default or top-level match priority.""" + layout: dict[str, Any] = { + "ROOT_ID": {"children": ["GRID_ID"]}, + "GRID_ID": {"children": ["container", "TABS-top"]}, + "container": {"type": parent_type, "children": ["TABS-nested"]}, + "TABS-top": {"type": "TABS", "children": ["TAB-top"]}, + "TAB-top": {"type": "TAB", "meta": {"text": "Shared"}}, + "TABS-nested": {"type": "TABS", "children": ["TAB-nested", "TAB-shared"]}, + "TAB-nested": {"type": "TAB", "meta": {"text": "📊 Nested"}}, + "TAB-shared": {"type": "TAB", "meta": {"text": "Shared"}}, + } + assert find_tab_insert_target(layout) == "TAB-top" + assert find_tab_insert_target(layout, "Shared") == "TAB-top" + assert find_tab_insert_target(layout, "nested") == "TAB-nested" + assert find_tab_insert_target(layout, "TAB-nested") == "TAB-nested" + assert collect_available_tab_names(layout) == [ + "Shared (TAB-top)", + "📊 Nested (TAB-nested)", + "Shared (TAB-shared)", + ] + layout["GRID_ID"]["children"].remove("TABS-top") + assert find_tab_insert_target(layout) is None + assert find_tab_insert_target(layout, "nested") == "TAB-nested" diff --git a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py index f5eef0d86a62..c4a726579b46 100644 --- a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py +++ b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py @@ -30,12 +30,14 @@ from superset.mcp_service.app import mcp from superset.mcp_service.chart.chart_utils import DatasetValidationResult from superset.mcp_service.dashboard.constants import generate_id +from superset.mcp_service.dashboard.layout_placement import ( + collect_available_tab_names, + ensure_layout_structure, + find_next_row_position, + find_tab_insert_target, +) from superset.mcp_service.dashboard.tool.add_chart_to_existing_dashboard import ( _add_chart_to_layout, - _collect_available_tab_names, - _ensure_layout_structure, - _find_next_row_position, - _find_tab_insert_target, ) from superset.mcp_service.dashboard.tool.generate_dashboard import ( _generate_title_from_charts, @@ -1893,28 +1895,28 @@ def test_generate_id_uniqueness(self): assert len(ids) == 100 def test_find_next_row_position_empty_layout(self): - """Test _find_next_row_position with empty layout.""" - result = _find_next_row_position({}) + """Test find_next_row_position with empty layout.""" + result = find_next_row_position({}) assert isinstance(result, str) assert result.startswith("ROW-") def test_find_tab_insert_target_no_tabs(self): - """Test _find_tab_insert_target with no tabs.""" + """Test find_tab_insert_target with no tabs.""" layout = {"GRID_ID": {"children": ["ROW-1"], "type": "GRID"}} - assert _find_tab_insert_target(layout) is None + assert find_tab_insert_target(layout) is None def test_find_tab_insert_target_with_tabs(self): - """Test _find_tab_insert_target with tabbed dashboard.""" + """Test find_tab_insert_target with tabbed dashboard.""" layout = { "GRID_ID": {"children": ["TABS-main"], "type": "GRID"}, "TABS-main": {"children": ["TAB-first", "TAB-second"], "type": "TABS"}, "TAB-first": {"children": [], "type": "TAB"}, "TAB-second": {"children": [], "type": "TAB"}, } - assert _find_tab_insert_target(layout) == "TAB-first" + assert find_tab_insert_target(layout) == "TAB-first" def test_find_tab_insert_target_by_tab_name(self): - """Test _find_tab_insert_target resolves target_tab by display name.""" + """Test find_tab_insert_target resolves target_tab by display name.""" layout = { "GRID_ID": {"children": ["TABS-main"], "type": "GRID"}, "TABS-main": {"children": ["TAB-first", "TAB-second"], "type": "TABS"}, @@ -1929,10 +1931,10 @@ def test_find_tab_insert_target_by_tab_name(self): "meta": {"text": "Customers"}, }, } - assert _find_tab_insert_target(layout, target_tab="Customers") == "TAB-second" + assert find_tab_insert_target(layout, target_tab="Customers") == "TAB-second" def test_find_tab_insert_target_by_tab_id(self): - """Test _find_tab_insert_target resolves target_tab by component ID.""" + """Test find_tab_insert_target resolves target_tab by component ID.""" layout = { "GRID_ID": {"children": ["TABS-main"], "type": "GRID"}, "TABS-main": {"children": ["TAB-first", "TAB-second"], "type": "TABS"}, @@ -1947,10 +1949,10 @@ def test_find_tab_insert_target_by_tab_id(self): "meta": {"text": "Tab 2"}, }, } - assert _find_tab_insert_target(layout, target_tab="TAB-second") == "TAB-second" + assert find_tab_insert_target(layout, target_tab="TAB-second") == "TAB-second" def test_find_tab_insert_target_unmatched_returns_none(self): - """Test _find_tab_insert_target returns None when target_tab doesn't + """Test find_tab_insert_target returns None when target_tab doesn't match any tab name or ID, so the caller can return a descriptive error.""" layout = { "GRID_ID": {"children": ["TABS-main"], "type": "GRID"}, @@ -1966,7 +1968,7 @@ def test_find_tab_insert_target_unmatched_returns_none(self): "meta": {"text": "Tab 2"}, }, } - assert _find_tab_insert_target(layout, target_tab="Nonexistent Tab") is None + assert find_tab_insert_target(layout, target_tab="Nonexistent Tab") is None def test_find_tab_insert_target_empty_string_returns_none(self) -> None: """An empty-string target_tab is treated as specified-but-not-found, @@ -1976,10 +1978,10 @@ def test_find_tab_insert_target_empty_string_returns_none(self) -> None: "TABS-main": {"children": ["TAB-first"], "type": "TABS"}, "TAB-first": {"children": [], "type": "TAB", "meta": {"text": "Tab 1"}}, } - assert _find_tab_insert_target(layout, target_tab="") is None + assert find_tab_insert_target(layout, target_tab="") is None def test_find_tab_insert_target_tabs_under_root(self) -> None: - """Test _find_tab_insert_target when TABS are under ROOT_ID (real layout).""" + """Test find_tab_insert_target when TABS are under ROOT_ID (real layout).""" layout = { "ROOT_ID": {"children": ["TABS-xxx"], "type": "ROOT"}, "GRID_ID": {"children": [], "type": "GRID", "parents": ["ROOT_ID"]}, @@ -1987,10 +1989,10 @@ def test_find_tab_insert_target_tabs_under_root(self) -> None: "TAB-a": {"children": [], "type": "TAB", "meta": {"text": "Overview"}}, "TAB-b": {"children": [], "type": "TAB", "meta": {"text": "Details"}}, } - assert _find_tab_insert_target(layout) == "TAB-a" + assert find_tab_insert_target(layout) == "TAB-a" def test_find_tab_insert_target_tabs_under_root_by_name(self) -> None: - """Test _find_tab_insert_target matches tab name when TABS under ROOT_ID.""" + """Test find_tab_insert_target matches tab name when TABS under ROOT_ID.""" layout = { "ROOT_ID": {"children": ["TABS-xxx"], "type": "ROOT"}, "GRID_ID": {"children": [], "type": "GRID", "parents": ["ROOT_ID"]}, @@ -1998,31 +2000,31 @@ def test_find_tab_insert_target_tabs_under_root_by_name(self) -> None: "TAB-a": {"children": [], "type": "TAB", "meta": {"text": "Overview"}}, "TAB-b": {"children": [], "type": "TAB", "meta": {"text": "Details"}}, } - assert _find_tab_insert_target(layout, target_tab="Details") == "TAB-b" + assert find_tab_insert_target(layout, target_tab="Details") == "TAB-b" def test_find_tab_insert_target_no_grid(self) -> None: - """Test _find_tab_insert_target with missing GRID_ID.""" - assert _find_tab_insert_target({"ROOT_ID": {"type": "ROOT"}}) is None + """Test find_tab_insert_target with missing GRID_ID.""" + assert find_tab_insert_target({"ROOT_ID": {"type": "ROOT"}}) is None def test_collect_available_tab_names_returns_display_names(self) -> None: - """_collect_available_tab_names returns label + component ID for each tab.""" + """collect_available_tab_names returns label + component ID for each tab.""" layout = { "GRID_ID": {"children": ["TABS-x"], "type": "GRID"}, "TABS-x": {"children": ["TAB-a", "TAB-b"], "type": "TABS"}, "TAB-a": {"children": [], "type": "TAB", "meta": {"text": "Overview"}}, "TAB-b": {"children": [], "type": "TAB", "meta": {"text": "Details"}}, } - names = _collect_available_tab_names(layout) + names = collect_available_tab_names(layout) assert names == ["Overview (TAB-a)", "Details (TAB-b)"] def test_collect_available_tab_names_falls_back_to_id(self) -> None: - """_collect_available_tab_names uses component ID only when text is empty.""" + """collect_available_tab_names uses component ID only when text is empty.""" layout = { "GRID_ID": {"children": ["TABS-x"], "type": "GRID"}, "TABS-x": {"children": ["TAB-a"], "type": "TABS"}, "TAB-a": {"children": [], "type": "TAB", "meta": {}}, } - names = _collect_available_tab_names(layout) + names = collect_available_tab_names(layout) assert names == ["TAB-a"] def test_collect_available_tab_names_duplicate_names(self) -> None: @@ -2033,17 +2035,17 @@ def test_collect_available_tab_names_duplicate_names(self) -> None: "TAB-a": {"children": [], "type": "TAB", "meta": {"text": "Sales"}}, "TAB-b": {"children": [], "type": "TAB", "meta": {"text": "Sales"}}, } - names = _collect_available_tab_names(layout) + names = collect_available_tab_names(layout) assert names == ["Sales (TAB-a)", "Sales (TAB-b)"] assert names[0] != names[1] def test_collect_available_tab_names_no_tabs(self) -> None: - """_collect_available_tab_names returns empty list for non-tabbed dashboards.""" + """collect_available_tab_names returns empty list for non-tabbed dashboards.""" layout = { "GRID_ID": {"children": ["ROW-1"], "type": "GRID"}, "ROW-1": {"children": [], "type": "ROW"}, } - assert _collect_available_tab_names(layout) == [] + assert collect_available_tab_names(layout) == [] def test_add_chart_to_layout_creates_column(self): """Test that _add_chart_to_layout creates ROW > COLUMN > CHART.""" @@ -2072,9 +2074,9 @@ def test_add_chart_to_layout_creates_column(self): assert layout[chart_key]["meta"]["chartId"] == 42 def test_ensure_layout_structure_creates_missing(self): - """Test _ensure_layout_structure creates GRID and ROOT if missing.""" + """Test ensure_layout_structure creates GRID and ROOT if missing.""" layout: dict = {} - _ensure_layout_structure(layout, "ROW-test", "GRID_ID") + ensure_layout_structure(layout, "ROW-test", "GRID_ID") assert "ROOT_ID" in layout assert "GRID_ID" in layout @@ -2083,7 +2085,7 @@ def test_ensure_layout_structure_creates_missing(self): assert layout["DASHBOARD_VERSION_KEY"] == "v2" def test_ensure_layout_structure_adds_to_tab(self): - """Test _ensure_layout_structure adds row to tab parent.""" + """Test ensure_layout_structure adds row to tab parent.""" layout = { "ROOT_ID": {"children": ["GRID_ID"], "type": "ROOT"}, "GRID_ID": { @@ -2093,13 +2095,13 @@ def test_ensure_layout_structure_adds_to_tab(self): }, "TAB-first": {"children": ["ROW-existing"], "type": "TAB"}, } - _ensure_layout_structure(layout, "ROW-new", "TAB-first") + ensure_layout_structure(layout, "ROW-new", "TAB-first") assert "ROW-new" in layout["TAB-first"]["children"] assert "ROW-new" not in layout["GRID_ID"]["children"] def test_ensure_layout_structure_tabs_under_root_no_grid_added(self): - """Test _ensure_layout_structure does NOT add GRID_ID to ROOT_ID + """Test ensure_layout_structure does NOT add GRID_ID to ROOT_ID when TABS already exists as a ROOT_ID child. Real Superset tabbed dashboards place TABS under ROOT_ID, not @@ -2127,7 +2129,7 @@ def test_ensure_layout_structure_tabs_under_root_no_grid_added(self): "parents": ["ROOT_ID", "TABS-xxx"], }, } - _ensure_layout_structure(layout, "ROW-new", "TAB-a") + ensure_layout_structure(layout, "ROW-new", "TAB-a") # Row added to the correct tab assert "ROW-new" in layout["TAB-a"]["children"] @@ -2136,14 +2138,14 @@ def test_ensure_layout_structure_tabs_under_root_no_grid_added(self): assert layout["ROOT_ID"]["children"] == ["TABS-xxx"] def test_ensure_layout_structure_no_tabs_adds_grid_to_root(self): - """Test _ensure_layout_structure still adds GRID_ID to ROOT_ID + """Test ensure_layout_structure still adds GRID_ID to ROOT_ID when the dashboard has no tabs (non-tabbed dashboard regression check). """ layout = { "ROOT_ID": {"children": [], "type": "ROOT"}, "GRID_ID": {"children": [], "type": "GRID", "parents": ["ROOT_ID"]}, } - _ensure_layout_structure(layout, "ROW-new", "GRID_ID") + ensure_layout_structure(layout, "ROW-new", "GRID_ID") assert "GRID_ID" in layout["ROOT_ID"]["children"] assert "ROW-new" in layout["GRID_ID"]["children"] diff --git a/tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_markdown.py b/tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_markdown.py new file mode 100644 index 000000000000..6adbe7e00cc0 --- /dev/null +++ b/tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_markdown.py @@ -0,0 +1,1279 @@ +# 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. + +""" +Unit tests for the manage_dashboard_markdown MCP tool. + +Follows the pattern from test_manage_native_filters.py: +- Tests run through the async MCP Client (not direct function calls) +- auth/editorship are mocked via the directory's autouse mock_auth fixture + (tests/unit_tests/mcp_service/dashboard/tool/conftest.py) +- Patches applied at source locations (superset.daos.dashboard.*, etc.) + +Covers: +- Adding markdown/header/divider components to the default grid and to a tab +- Updating an existing component (type-specific field rejection) +- Removing a component (including pruning the wrapper ROW a markdown tile + leaves behind) +- Validation errors: unknown removal ID, update+remove conflict, duplicate + update IDs, malformed position_json, missing target tab +- Header text normalization (stored verbatim; invisible characters stripped) +- Associated charts missing from the layout, externally managed dashboards +- Dashboard not found / permission denied +- "at least one operation" request validation (ToolError at the call boundary) +""" + +from collections.abc import Iterator +from typing import Any +from unittest.mock import Mock, patch, PropertyMock + +import pytest +from fastmcp import Client, FastMCP +from fastmcp.exceptions import ToolError +from sqlalchemy.exc import SQLAlchemyError + +from superset.commands.dashboard.exceptions import DashboardNotFoundError +from superset.exceptions import SupersetSecurityException +from superset.utils import json + +DAO_GET = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug" + + +@pytest.fixture(autouse=True) +def mock_event_logging() -> Iterator[None]: + """Isolate dashboard commits from the event logger's separate audit commits.""" + with patch("superset.extensions.event_logger.log_context"): + yield + + +def _empty_grid_layout() -> dict[str, Any]: + """Build an empty frontend-compatible grid.""" + return { + "DASHBOARD_VERSION_KEY": "v2", + "ROOT_ID": {"type": "ROOT", "id": "ROOT_ID", "children": ["GRID_ID"]}, + "GRID_ID": {"type": "GRID", "id": "GRID_ID", "children": []}, + } + + +def _grid_layout_with_existing_components() -> dict[str, Any]: + """Build a grid with each supported component type.""" + layout = _empty_grid_layout() + layout["GRID_ID"]["children"] = [ + "ROW-existing1", + "HEADER-existing1", + "DIVIDER-existing1", + ] + layout["ROW-existing1"] = { + "type": "ROW", + "id": "ROW-existing1", + "children": ["MARKDOWN-existing1"], + "meta": {"background": "BACKGROUND_TRANSPARENT"}, + "parents": ["ROOT_ID", "GRID_ID"], + } + layout["MARKDOWN-existing1"] = { + "type": "MARKDOWN", + "id": "MARKDOWN-existing1", + "children": [], + "meta": {"code": "Hello", "width": 4, "height": 50}, + "parents": ["ROOT_ID", "GRID_ID", "ROW-existing1"], + } + layout["HEADER-existing1"] = { + "type": "HEADER", + "id": "HEADER-existing1", + "children": [], + "meta": { + "text": "Old header", + "headerSize": "MEDIUM_HEADER", + "background": "BACKGROUND_TRANSPARENT", + }, + "parents": ["ROOT_ID", "GRID_ID"], + } + layout["DIVIDER-existing1"] = { + "type": "DIVIDER", + "id": "DIVIDER-existing1", + "children": [], + "meta": {}, + "parents": ["ROOT_ID", "GRID_ID"], + } + return layout + + +def _tabbed_layout() -> dict[str, Any]: + """Build a top-level tab layout.""" + return { + "DASHBOARD_VERSION_KEY": "v2", + "ROOT_ID": {"type": "ROOT", "id": "ROOT_ID", "children": ["TABS-1"]}, + "TABS-1": { + "type": "TABS", + "id": "TABS-1", + "children": ["TAB-a", "TAB-b"], + "meta": {}, + }, + "TAB-a": { + "type": "TAB", + "id": "TAB-a", + "children": [], + "meta": {"text": "Overview"}, + "parents": ["ROOT_ID", "TABS-1"], + }, + "TAB-b": { + "type": "TAB", + "id": "TAB-b", + "children": [], + "meta": {"text": "Details"}, + "parents": ["ROOT_ID", "TABS-1"], + }, + } + + +def _mock_dashboard( + id: int = 1, + layout: dict[str, Any] | None = None, + chart_ids: list[int] | None = None, +) -> Mock: + """Build a dashboard without touching the metadata database.""" + dashboard = Mock() + dashboard.id = id + dashboard.dashboard_title = "Test Dashboard" + dashboard.slug = None + dashboard.is_managed_externally = False + dashboard.position_json = json.dumps( + layout if layout is not None else _empty_grid_layout() + ) + slices = [] + for chart_id in chart_ids or []: + slc = Mock() + slc.id = chart_id + slices.append(slc) + dashboard.slices = slices + return dashboard + + +async def _call(mcp_server: FastMCP, request: dict[str, Any]) -> dict[str, Any]: + """Exercise validation and serialization through the MCP boundary.""" + async with Client(mcp_server) as client: + result = await client.call_tool( + "manage_dashboard_markdown", {"request": request} + ) + return json.loads(result.content[0].text) + + +# --------------------------------------------------------------------------- +# Add +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_add_markdown_creates_new_row(mcp_server: FastMCP) -> None: + """Adding a markdown wraps it in a new row appended to the grid.""" + dashboard = _mock_dashboard() + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data = await _call( + mcp_server, + { + "dashboard_id": 1, + "add": [{"component_type": "markdown", "code": "**Hello**"}], + }, + ) + + assert data["error"] is None + assert len(data["added_component_ids"]) == 1 + markdown_id = data["added_component_ids"][0] + assert markdown_id.startswith("MARKDOWN-") + + saved_layout = json.loads(dashboard.position_json) + markdown_node = saved_layout[markdown_id] + assert markdown_node["type"] == "MARKDOWN" + assert markdown_node["meta"] == {"code": "**Hello**", "width": 4, "height": 50} + + # Markdown tiles are wrapped in their own new ROW, not placed directly + # under GRID_ID. + row_key = next( + key + for key, node in saved_layout.items() + if isinstance(node, dict) + and node.get("type") == "ROW" + and markdown_id in node.get("children", []) + ) + assert row_key in saved_layout["GRID_ID"]["children"] + + summary = next(c for c in data["components"] if c["id"] == markdown_id) + assert summary["component_type"] == "markdown" + + +@pytest.mark.asyncio +async def test_add_header_placed_directly_under_grid(mcp_server: FastMCP) -> None: + """A new header is attached directly to the grid, not wrapped in a row.""" + dashboard = _mock_dashboard() + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data = await _call( + mcp_server, + { + "dashboard_id": 1, + "add": [ + { + "component_type": "header", + "text": "Sales", + "header_size": "LARGE_HEADER", + } + ], + }, + ) + + assert data["error"] is None + header_id = data["added_component_ids"][0] + assert header_id.startswith("HEADER-") + + saved_layout = json.loads(dashboard.position_json) + # HEADER is a full-width band: a direct child of GRID_ID, not wrapped + # in a ROW (ROW does not accept HEADER children). + assert header_id in saved_layout["GRID_ID"]["children"] + assert saved_layout[header_id]["meta"] == { + "text": "Sales", + "headerSize": "LARGE_HEADER", + "background": "BACKGROUND_TRANSPARENT", + } + + +@pytest.mark.asyncio +async def test_add_divider_placed_directly_under_grid(mcp_server: FastMCP) -> None: + """A new divider is attached directly to the grid, not wrapped in a row.""" + dashboard = _mock_dashboard() + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data = await _call( + mcp_server, + {"dashboard_id": 1, "add": [{"component_type": "divider"}]}, + ) + + assert data["error"] is None + divider_id = data["added_component_ids"][0] + assert divider_id.startswith("DIVIDER-") + + saved_layout = json.loads(dashboard.position_json) + assert divider_id in saved_layout["GRID_ID"]["children"] + assert saved_layout[divider_id]["meta"] == {} + + +@pytest.mark.asyncio +async def test_add_multiple_components_in_request_order(mcp_server: FastMCP) -> None: + """Add each requested component type in a single batch.""" + dashboard: Mock = _mock_dashboard() + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data: dict[str, Any] = await _call( + mcp_server, + { + "dashboard_id": 1, + "add": [ + {"component_type": "header", "text": "Section 1"}, + {"component_type": "markdown", "code": "text"}, + {"component_type": "divider"}, + ], + }, + ) + + assert data["error"] is None + assert len(data["added_component_ids"]) == 3 + types: list[str] = [c["component_type"] for c in data["components"]] + assert set(types) == {"header", "markdown", "divider"} + + +@pytest.mark.asyncio +async def test_add_to_target_tab_by_name(mcp_server: FastMCP) -> None: + """Place a header only in the tab matching the requested name.""" + dashboard: Mock = _mock_dashboard(layout=_tabbed_layout()) + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data: dict[str, Any] = await _call( + mcp_server, + { + "dashboard_id": 1, + "add": [ + { + "component_type": "header", + "text": "Details header", + "target_tab": "Details", + } + ], + }, + ) + + assert data["error"] is None + header_id: str = data["added_component_ids"][0] + saved_layout: dict[str, Any] = json.loads(dashboard.position_json) + assert header_id in saved_layout["TAB-b"]["children"] + assert header_id not in saved_layout["TAB-a"]["children"] + + +@pytest.mark.asyncio +async def test_add_target_tab_not_found_lists_available_tabs( + mcp_server: FastMCP, +) -> None: + """Report available tabs when the requested tab does not exist.""" + dashboard: Mock = _mock_dashboard(layout=_tabbed_layout()) + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data: dict[str, Any] = await _call( + mcp_server, + { + "dashboard_id": 1, + "add": [ + { + "component_type": "divider", + "target_tab": "Nonexistent", + } + ], + }, + ) + + assert "Nonexistent" in data["error"] + assert "Overview" in data["error"] + assert "Details" in data["error"] + + +@pytest.mark.asyncio +async def test_add_target_tab_on_dashboard_without_tabs(mcp_server: FastMCP) -> None: + """Reject a named tab target when the dashboard has no tabs.""" + dashboard: Mock = _mock_dashboard() + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data: dict[str, Any] = await _call( + mcp_server, + { + "dashboard_id": 1, + "add": [{"component_type": "divider", "target_tab": "Anything"}], + }, + ) + + assert "no tabs" in data["error"] + + +# --------------------------------------------------------------------------- +# Update +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_update_markdown_code(mcp_server: FastMCP) -> None: + """Updating markdown code preserves the existing width and height.""" + dashboard: Mock = _mock_dashboard(layout=_grid_layout_with_existing_components()) + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data: dict[str, Any] = await _call( + mcp_server, + { + "dashboard_id": 1, + "update": [{"id": "MARKDOWN-existing1", "code": "**Updated**"}], + }, + ) + + assert data["error"] is None + assert data["updated_component_ids"] == ["MARKDOWN-existing1"] + saved_layout: dict[str, Any] = json.loads(dashboard.position_json) + meta: dict[str, Any] = saved_layout["MARKDOWN-existing1"]["meta"] + # Only `code` was passed; width/height are preserved from the existing + # config (a partial update merges into the full component, it does not + # replace it). + assert meta == {"code": "**Updated**", "width": 4, "height": 50} + + +@pytest.mark.asyncio +async def test_update_header_fields(mcp_server: FastMCP) -> None: + """Updating a header's text and size keeps its other metadata.""" + dashboard = _mock_dashboard(layout=_grid_layout_with_existing_components()) + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data = await _call( + mcp_server, + { + "dashboard_id": 1, + "update": [ + { + "id": "HEADER-existing1", + "text": "New header", + "header_size": "SMALL_HEADER", + } + ], + }, + ) + + assert data["error"] is None + saved_layout: dict[str, Any] = json.loads(dashboard.position_json) + meta: dict[str, Any] = saved_layout["HEADER-existing1"]["meta"] + assert meta == { + "text": "New header", + "headerSize": "SMALL_HEADER", + "background": "BACKGROUND_TRANSPARENT", + } + + +@pytest.mark.asyncio +async def test_update_rejects_field_from_wrong_component_type( + mcp_server: FastMCP, +) -> None: + """Reject markdown-only fields sent for a header component.""" + dashboard = _mock_dashboard(layout=_grid_layout_with_existing_components()) + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data = await _call( + mcp_server, + { + "dashboard_id": 1, + "update": [{"id": "HEADER-existing1", "code": "not markdown"}], + }, + ) + + assert "only apply to markdown components" in data["error"] + + +@pytest.mark.asyncio +async def test_update_unknown_component_id(mcp_server: FastMCP) -> None: + """Updating an ID that is not a manageable component returns an error.""" + dashboard = _mock_dashboard(layout=_grid_layout_with_existing_components()) + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data = await _call( + mcp_server, + { + "dashboard_id": 1, + "update": [{"id": "MARKDOWN-nonexistent", "code": "x"}], + }, + ) + + assert "MARKDOWN-nonexistent" in data["error"] + + +@pytest.mark.asyncio +async def test_duplicate_update_ids_rejected(mcp_server: FastMCP) -> None: + """Reject an update batch that lists the same component ID twice.""" + dashboard = _mock_dashboard(layout=_grid_layout_with_existing_components()) + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data = await _call( + mcp_server, + { + "dashboard_id": 1, + "update": [ + {"id": "MARKDOWN-existing1", "code": "a"}, + {"id": "MARKDOWN-existing1", "code": "b"}, + ], + }, + ) + + assert "duplicate component IDs" in data["error"] + + +# --------------------------------------------------------------------------- +# Remove +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_remove_header(mcp_server: FastMCP) -> None: + """Remove a header that hangs directly off the grid.""" + dashboard = _mock_dashboard(layout=_grid_layout_with_existing_components()) + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data = await _call( + mcp_server, + {"dashboard_id": 1, "remove": ["HEADER-existing1"]}, + ) + + assert data["error"] is None + assert data["removed_component_ids"] == ["HEADER-existing1"] + saved_layout = json.loads(dashboard.position_json) + assert "HEADER-existing1" not in saved_layout + assert "HEADER-existing1" not in saved_layout["GRID_ID"]["children"] + + +@pytest.mark.asyncio +async def test_remove_markdown_prunes_empty_wrapping_row(mcp_server: FastMCP) -> None: + """Removing a markdown prunes the wrapping row it leaves empty.""" + dashboard = _mock_dashboard(layout=_grid_layout_with_existing_components()) + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data = await _call( + mcp_server, + {"dashboard_id": 1, "remove": ["MARKDOWN-existing1"]}, + ) + + assert data["error"] is None + saved_layout = json.loads(dashboard.position_json) + assert "MARKDOWN-existing1" not in saved_layout + # The wrapping ROW-existing1 had only the removed markdown as a child, + # so it must be pruned too rather than left as an empty orphan. + assert "ROW-existing1" not in saved_layout + assert "ROW-existing1" not in saved_layout["GRID_ID"]["children"] + + +@pytest.mark.asyncio +async def test_remove_unknown_id_rejected(mcp_server: FastMCP) -> None: + """Reject removal of an ID that is not a manageable component.""" + dashboard = _mock_dashboard(layout=_grid_layout_with_existing_components()) + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data = await _call( + mcp_server, + {"dashboard_id": 1, "remove": ["MARKDOWN-nonexistent"]}, + ) + + assert "MARKDOWN-nonexistent" in data["error"] + assert ( + json.loads(dashboard.position_json) == _grid_layout_with_existing_components() + ) + + +@pytest.mark.asyncio +async def test_duplicate_remove_ids_rejected(mcp_server: FastMCP) -> None: + """Reject a remove list that names the same component twice.""" + dashboard = _mock_dashboard(layout=_grid_layout_with_existing_components()) + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session") as session, + ): + data = await _call( + mcp_server, + {"dashboard_id": 1, "remove": ["HEADER-existing1", "HEADER-existing1"]}, + ) + + assert "duplicate component IDs" in data["error"] + session.commit.assert_not_called() + assert ( + json.loads(dashboard.position_json) == _grid_layout_with_existing_components() + ) + + +@pytest.mark.asyncio +async def test_update_and_remove_conflict_rejected(mcp_server: FastMCP) -> None: + """Updating and removing the same component in one request is rejected.""" + dashboard = _mock_dashboard(layout=_grid_layout_with_existing_components()) + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data = await _call( + mcp_server, + { + "dashboard_id": 1, + "update": [{"id": "HEADER-existing1", "text": "x"}], + "remove": ["HEADER-existing1"], + }, + ) + + assert "HEADER-existing1" in data["error"] + assert "both updated and removed" in data["error"] + + +# --------------------------------------------------------------------------- +# Validation / error paths +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_malformed_position_json(mcp_server: FastMCP) -> None: + """Unparseable position_json is reported as an error without saving.""" + dashboard = _mock_dashboard() + dashboard.position_json = "not valid json" + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data = await _call( + mcp_server, + {"dashboard_id": 1, "add": [{"component_type": "divider"}]}, + ) + + assert "malformed layout" in data["error"] + + +@pytest.mark.asyncio +async def test_dashboard_not_found(mcp_server: FastMCP) -> None: + """A missing dashboard yields a not-found error response.""" + with patch(DAO_GET, side_effect=DashboardNotFoundError()): + data = await _call( + mcp_server, + {"dashboard_id": 999, "add": [{"component_type": "divider"}]}, + ) + + assert data["error"] is not None + assert data["permission_denied"] is False + + +@pytest.mark.asyncio +async def test_permission_denied(mcp_server: FastMCP) -> None: + """A caller without access to the dashboard gets an error and no write.""" + dashboard = _mock_dashboard() + + with ( + patch(DAO_GET, return_value=dashboard), + patch( + "superset.security_manager.raise_for_editorship", + side_effect=SupersetSecurityException(Mock()), + ), + ): + data = await _call( + mcp_server, + {"dashboard_id": 1, "add": [{"component_type": "divider"}]}, + ) + + assert data["permission_denied"] is True + + +@pytest.mark.asyncio +async def test_at_least_one_operation_required(mcp_server: FastMCP) -> None: + """Request-schema validation happens at the MCP tool-call boundary, + before the tool body runs, so the client raises ToolError rather than + returning a JSON error body (mirrors manage_native_filters).""" + dashboard = _mock_dashboard() + + with patch(DAO_GET, return_value=dashboard): + with pytest.raises(ToolError, match="At least one operation"): + await _call(mcp_server, {"dashboard_id": 1}) + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "text", + [ + "Revenue < 1M & Clients > 500", + "Month = March", + "Sales", + ], +) +async def test_header_text_stored_verbatim(mcp_server: FastMCP, text: str) -> None: + """Header text renders as plain React text, so it is stored as typed.""" + dashboard = _mock_dashboard() + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data = await _call( + mcp_server, + {"dashboard_id": 1, "add": [{"component_type": "header", "text": text}]}, + ) + + assert data["error"] is None + header_id = data["added_component_ids"][0] + saved_layout = json.loads(dashboard.position_json) + assert saved_layout[header_id]["meta"]["text"] == text + + +@pytest.mark.asyncio +async def test_header_text_update_stored_verbatim(mcp_server: FastMCP) -> None: + """Updated header text is not HTML-escaped or pattern-rejected.""" + dashboard = _mock_dashboard(layout=_grid_layout_with_existing_components()) + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data = await _call( + mcp_server, + { + "dashboard_id": 1, + "update": [{"id": "HEADER-existing1", "text": "Month = March < Q2"}], + }, + ) + + assert data["error"] is None + saved_layout = json.loads(dashboard.position_json) + assert saved_layout["HEADER-existing1"]["meta"]["text"] == "Month = March < Q2" + + +@pytest.mark.asyncio +async def test_header_text_strips_invisible_characters(mcp_server: FastMCP) -> None: + """Zero-width and control characters are removed from header text.""" + dashboard = _mock_dashboard() + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data = await _call( + mcp_server, + { + "dashboard_id": 1, + "add": [{"component_type": "header", "text": " Sa\u200bles\u0007 "}], + }, + ) + + assert data["error"] is None + header_id = data["added_component_ids"][0] + saved_layout = json.loads(dashboard.position_json) + assert saved_layout[header_id]["meta"]["text"] == "Sales" + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("text", "message"), + [("\u200b \u200b", "cannot be empty"), ("x" * 501, "too long")], +) +async def test_header_text_rejected( + mcp_server: FastMCP, text: str, message: str +) -> None: + """Header text that is empty after normalization or too long is rejected.""" + with patch(DAO_GET) as lookup: + with pytest.raises(ToolError, match=message): + await _call( + mcp_server, + { + "dashboard_id": 1, + "add": [{"component_type": "header", "text": text}], + }, + ) + lookup.assert_not_called() + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "operations", + [ + {"add": [{"component_type": "markdown", "code": "x", "width": 13}]}, + {"add": [{"component_type": "markdown", "code": "x", "height": 0}]}, + {"add": [{"component_type": "markdown", "code": "x", "height": 4}]}, + {"update": [{"id": "MARKDOWN-existing1", "height": 4}]}, + {"add": [{"component_type": "markdown", "code": "x", "height": 101}]}, + {"update": [{"id": "MARKDOWN-existing1", "height": 101}]}, + {"add": [{"component_type": "chart"}]}, + {"add": [{"component_type": "markdown"}]}, + {"add": [{"component_type": "divider", "code": "ignored?"}]}, + {"update": [{"id": "MARKDOWN-existing1", "component_type": "header"}]}, + {"update": [{"id": "HEADER-existing1", "header_size": "HUGE"}]}, + {"add": [{"component_type": "divider"}], "dashboard_id": True}, + ], +) +async def test_schema_rejects_invalid_operations( + mcp_server: FastMCP, operations: dict[str, Any] +) -> None: + """Invalid inputs cannot reach dashboard lookup or persistence.""" + with patch(DAO_GET) as lookup: + with pytest.raises(ToolError): + await _call(mcp_server, {"dashboard_id": 1, **operations}) + lookup.assert_not_called() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("position_json", ["[]", "null", '{"ROOT_ID": 1}']) +async def test_malformed_layout_is_not_replaced( + mcp_server: FastMCP, position_json: str +) -> None: + """Corrupt layouts are rejected rather than discarded or traversed.""" + dashboard = _mock_dashboard() + dashboard.position_json = position_json + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session") as session, + ): + data = await _call( + mcp_server, {"dashboard_id": 1, "add": [{"component_type": "divider"}]} + ) + assert "malformed layout" in data["error"] + assert dashboard.position_json == position_json + session.commit.assert_not_called() + + +@pytest.mark.asyncio +async def test_failed_batch_does_not_save_partial_updates(mcp_server: FastMCP) -> None: + """A failed add must not persist earlier valid updates and removals.""" + dashboard = _mock_dashboard(layout=_grid_layout_with_existing_components()) + before = dashboard.position_json + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session") as session, + ): + data = await _call( + mcp_server, + { + "dashboard_id": 1, + "update": [{"id": "MARKDOWN-existing1", "code": "changed"}], + "remove": ["HEADER-existing1"], + "add": [{"component_type": "divider", "target_tab": "missing"}], + }, + ) + assert data["error"] + assert dashboard.position_json == before + session.commit.assert_not_called() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("operation", ["update", "remove"]) +async def test_reserved_title_and_charts_cannot_be_mutated( + mcp_server: FastMCP, operation: str +) -> None: + """Only text tiles, not dashboard title metadata or charts, are editable.""" + layout = _grid_layout_with_existing_components() + layout["HEADER_ID"] = { + "id": "HEADER_ID", + "type": "HEADER", + "meta": {"text": "Title"}, + } + layout["CHART-1"] = {"id": "CHART-1", "type": "CHART", "meta": {"chartId": 7}} + layout["ROW-existing1"]["children"].append("CHART-1") + dashboard = _mock_dashboard(layout=layout, chart_ids=[7]) + before = dashboard.position_json + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session") as session, + ): + for component_id in ("HEADER_ID", "CHART-1"): + payload: list[dict[str, str]] | list[str] + if operation == "update": + payload = [{"id": component_id, "text": "changed"}] + else: + payload = [component_id] + data = await _call(mcp_server, {"dashboard_id": 1, operation: payload}) + assert data["error"] + assert dashboard.position_json == before + session.commit.assert_not_called() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("target_tab", [None, "TAB-a", "Overview"]) +async def test_markdown_tab_placement_rebuilds_full_ancestry( + mcp_server: FastMCP, target_tab: str | None +) -> None: + """Default and explicit tab placement produce filter-scope parent chains.""" + dashboard = _mock_dashboard(layout=_tabbed_layout()) + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session"), + ): + data = await _call( + mcp_server, + { + "dashboard_id": 1, + "add": [ + { + "component_type": "markdown", + "code": "text", + "target_tab": target_tab, + } + ], + }, + ) + assert data["error"] is None + layout = json.loads(dashboard.position_json) + node = layout[data["added_component_ids"][0]] + row_id = layout["TAB-a"]["children"][0] + assert node["parents"] == ["ROOT_ID", "TABS-1", "TAB-a", row_id] + assert layout["ROOT_ID"]["children"] == ["TABS-1"] + + +@pytest.mark.asyncio +async def test_batch_preserves_charts_metadata_and_markdown_source( + mcp_server: FastMCP, +) -> None: + """Layout edits preserve non-target nodes, associated charts, and filters.""" + layout = _grid_layout_with_existing_components() + layout["CHART-1"] = { + "id": "CHART-1", + "type": "CHART", + "children": [], + "meta": {"chartId": 7, "width": 4, "height": 50}, + "parents": ["ROOT_ID", "GRID_ID", "ROW-existing1"], + } + layout["ROW-existing1"]["children"].append("CHART-1") + dashboard = _mock_dashboard(layout=layout, chart_ids=[7]) + dashboard.json_metadata = '{"native_filter_configuration": [{"id": "filter-1"}]}' + before_metadata, before_slices = dashboard.json_metadata, dashboard.slices + source = "## Notes\n\n
**bold** & [link](https://example.com)
" + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session") as session, + ): + data = await _call( + mcp_server, + { + "dashboard_id": 1, + "remove": ["MARKDOWN-existing1"], + "update": [{"id": "HEADER-existing1", "text": "Revised"}], + "add": [{"component_type": "markdown", "code": source}], + }, + ) + assert data["error"] is None + saved = json.loads(dashboard.position_json) + assert saved["CHART-1"] == layout["CHART-1"] + assert saved["ROW-existing1"]["children"] == ["CHART-1"] + assert saved[data["added_component_ids"][0]]["meta"]["code"] == source + assert dashboard.slices is before_slices + assert dashboard.json_metadata == before_metadata + session.commit.assert_called_once() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("failure", ["commit", "refresh"]) +async def test_database_failure_reports_commit_outcome( + mcp_server: FastMCP, failure: str +) -> None: + """Rollback failed commits; a refresh failure must not invite duplicate adds.""" + dashboard = _mock_dashboard() + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session") as session, + ): + + def expire_and_fail(*args: object) -> None: + """Simulate inaccessible ORM attributes after a failed refresh.""" + if failure == "refresh": + type(dashboard).id = PropertyMock( + side_effect=SQLAlchemyError("expired") + ) + type(dashboard).slug = PropertyMock( + side_effect=SQLAlchemyError("expired") + ) + raise SQLAlchemyError("database error") + + getattr(session, failure).side_effect = expire_and_fail + data = await _call( + mcp_server, + { + "dashboard_id": 1, + "add": [{"component_type": "divider"}], + }, + ) + if failure == "commit": + assert data["error"] + assert not data["added_component_ids"] + session.rollback.assert_called_once() + else: + assert data["error"] is None + assert len(data["added_component_ids"]) == 1 + session.rollback.assert_not_called() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("width", [3, 4, 5, 12]) +async def test_markdown_resize_respects_row_capacity( + mcp_server: FastMCP, width: int +) -> None: + """Count chart siblings and reject overflow without persisting any edits.""" + layout = _grid_layout_with_existing_components() + layout["ROW-existing1"]["children"].append("CHART-1") + layout["CHART-1"] = { + "id": "CHART-1", + "type": "CHART", + "children": [], + "meta": {"chartId": 1, "width": 8, "height": 50}, + } + # Placement must be read from children, not stale parent metadata. + layout["MARKDOWN-existing1"]["parents"] = [] + dashboard = _mock_dashboard(layout=layout, chart_ids=[1]) + original = dashboard.position_json + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session") as session, + ): + data = await _call( + mcp_server, + { + "dashboard_id": 1, + "update": [ + {"id": "HEADER-existing1", "text": "Changed"}, + {"id": "MARKDOWN-existing1", "width": width}, + ], + }, + ) + if width > 4: + assert "available width" in data["error"] + assert "4 columns" in data["error"] + assert dashboard.position_json == original + session.commit.assert_not_called() + else: + assert data["error"] is None + saved = json.loads(dashboard.position_json) + assert saved["MARKDOWN-existing1"]["meta"]["width"] == width + session.commit.assert_called_once() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("remaining_width", [0, 2]) +async def test_markdown_resize_accounts_for_removals( + mcp_server: FastMCP, remaining_width: int +) -> None: + """Remove siblings before resizing, but reject remaining overflow atomically.""" + layout = _grid_layout_with_existing_components() + layout["MARKDOWN-existing1"]["meta"]["width"] = 6 + layout["ROW-existing1"]["children"].append("MARKDOWN-sibling") + layout["MARKDOWN-sibling"] = { + "id": "MARKDOWN-sibling", + "type": "MARKDOWN", + "children": [], + "meta": {"code": "Sibling", "width": 6 - remaining_width, "height": 50}, + } + if remaining_width: + layout["ROW-existing1"]["children"].append("CHART-1") + layout["CHART-1"] = { + "id": "CHART-1", + "type": "CHART", + "children": [], + "meta": {"chartId": 1, "width": remaining_width, "height": 50}, + } + dashboard = _mock_dashboard(layout=layout, chart_ids=[1] if remaining_width else []) + original = dashboard.position_json + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session") as session, + ): + data = await _call( + mcp_server, + { + "dashboard_id": 1, + "update": [ + {"id": "HEADER-existing1", "text": "Changed"}, + {"id": "MARKDOWN-existing1", "width": 12}, + ], + "remove": ["MARKDOWN-sibling"], + }, + ) + if remaining_width: + assert "available width" in data["error"] + assert "10 columns" in data["error"] + assert dashboard.position_json == original + session.commit.assert_not_called() + else: + assert data["error"] is None + assert data["removed_component_ids"] == ["MARKDOWN-sibling"] + assert data["updated_component_ids"] == [ + "HEADER-existing1", + "MARKDOWN-existing1", + ] + saved = json.loads(dashboard.position_json) + assert saved["MARKDOWN-existing1"]["meta"]["width"] == 12 + assert saved["ROW-existing1"]["children"] == ["MARKDOWN-existing1"] + assert "MARKDOWN-sibling" not in saved + session.commit.assert_called_once() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("target_tab", ["Nested", "TAB-nested", "missing", None]) +async def test_add_to_nested_tab(mcp_server: FastMCP, target_tab: str | None) -> None: + """Resolve nested targets, list them on errors, and retain the default tab.""" + layout = _tabbed_layout() + layout["TAB-a"]["children"] = ["TABS-nested"] + layout["TABS-nested"] = { + "id": "TABS-nested", + "type": "TABS", + "children": ["TAB-nested"], + "meta": {}, + } + layout["TAB-nested"] = { + "id": "TAB-nested", + "type": "TAB", + "children": [], + "meta": {"text": "Nested"}, + } + dashboard = _mock_dashboard(layout=layout) + original = dashboard.position_json + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session") as session, + ): + data = await _call( + mcp_server, + { + "dashboard_id": 1, + "add": [ + { + "component_type": "markdown", + "code": "Nested notes", + "target_tab": target_tab, + } + ], + }, + ) + if target_tab == "missing": + assert "Nested (TAB-nested)" in data["error"] + assert dashboard.position_json == original + session.commit.assert_not_called() + else: + assert data["error"] is None + saved = json.loads(dashboard.position_json) + component_id = data["added_component_ids"][0] + target_id = "TAB-a" if target_tab is None else "TAB-nested" + row_id = saved[component_id]["parents"][-1] + assert row_id in saved[target_id]["children"] + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "fields", + [ + {}, + {"code": None}, + { + "code": None, + "width": None, + "height": None, + "text": None, + "header_size": None, + "background": None, + }, + ], +) +async def test_update_requires_non_null_field( + mcp_server: FastMCP, fields: dict[str, Any] +) -> None: + """Empty updates fail validation before lookup or persistence.""" + with ( + patch(DAO_GET) as lookup, + patch("superset.extensions.db.session") as session, + ): + with pytest.raises( + ToolError, match="At least one non-null update field besides id is required" + ): + await _call( + mcp_server, + { + "dashboard_id": 1, + "update": [{"id": "MARKDOWN-existing1", **fields}], + }, + ) + lookup.assert_not_called() + session.commit.assert_not_called() + + +@pytest.mark.asyncio +async def test_add_header_to_dashboard_with_chart_attached_from_explore( + mcp_server: FastMCP, +) -> None: + """Associated charts not yet laid out (added from Explore) do not block edits.""" + layout = _empty_grid_layout() + layout["GRID_ID"]["children"] = ["ROW-1"] + layout["ROW-1"] = { + "type": "ROW", + "id": "ROW-1", + "children": ["CHART-1"], + "meta": {"background": "BACKGROUND_TRANSPARENT"}, + "parents": ["ROOT_ID", "GRID_ID"], + } + chart_node = { + "type": "CHART", + "id": "CHART-1", + "children": [], + "meta": {"chartId": 7, "width": 4, "height": 50}, + "parents": ["ROOT_ID", "GRID_ID", "ROW-1"], + } + layout["CHART-1"] = chart_node + # Chart 8 is associated with the dashboard but not in position_json. + dashboard = _mock_dashboard(layout=layout, chart_ids=[7, 8]) + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session") as session, + ): + data = await _call( + mcp_server, + {"dashboard_id": 1, "add": [{"component_type": "header", "text": "Hi"}]}, + ) + + assert data["error"] is None + session.commit.assert_called_once() + saved_layout = json.loads(dashboard.position_json) + assert saved_layout["CHART-1"] == chart_node + assert saved_layout["ROW-1"]["children"] == ["CHART-1"] + + +@pytest.mark.asyncio +async def test_add_to_empty_layout_with_attached_chart(mcp_server: FastMCP) -> None: + """An empty layout with associated charts is scaffolded, not refused.""" + dashboard = _mock_dashboard(chart_ids=[7]) + dashboard.position_json = "" + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session") as session, + ): + data = await _call( + mcp_server, {"dashboard_id": 1, "add": [{"component_type": "divider"}]} + ) + + assert data["error"] is None + session.commit.assert_called_once() + + +@pytest.mark.asyncio +async def test_externally_managed_dashboard_is_refused(mcp_server: FastMCP) -> None: + """Externally managed dashboards are refused, matching UpdateDashboardCommand.""" + dashboard = _mock_dashboard() + dashboard.is_managed_externally = True + before = dashboard.position_json + + with ( + patch(DAO_GET, return_value=dashboard), + patch("superset.extensions.db.session") as session, + ): + data = await _call( + mcp_server, {"dashboard_id": 1, "add": [{"component_type": "divider"}]} + ) + + assert "managed externally" in data["error"] + assert data["managed_externally"] is True + assert data["permission_denied"] is False + assert dashboard.position_json == before + session.commit.assert_not_called() diff --git a/tests/unit_tests/mcp_service/test_mcp_tool_registration.py b/tests/unit_tests/mcp_service/test_mcp_tool_registration.py index 42856765b088..e7cf093384a0 100644 --- a/tests/unit_tests/mcp_service/test_mcp_tool_registration.py +++ b/tests/unit_tests/mcp_service/test_mcp_tool_registration.py @@ -99,6 +99,7 @@ "list_themes": "themes", "list_users": "users", "manage_dashboard_certification": "changed_fields", + "manage_dashboard_markdown": "components", "manage_dashboard_owners": "owners", "manage_dashboard_roles": "roles", "manage_native_filters": "filters", @@ -143,6 +144,7 @@ "generate_dashboard", "generate_explore_link", "manage_dashboard_certification", + "manage_dashboard_markdown", "manage_dashboard_owners", "manage_dashboard_roles", "manage_native_filters", @@ -177,6 +179,7 @@ "delete_dataset", "delete_dataset_metric", "execute_sql", + "manage_dashboard_markdown", "manage_dashboard_owners", "manage_dashboard_roles", "manage_native_filters", diff --git a/tests/unit_tests/mcp_service/test_tool_inventory.py b/tests/unit_tests/mcp_service/test_tool_inventory.py index 19bd66e4fdbd..944a2c519889 100644 --- a/tests/unit_tests/mcp_service/test_tool_inventory.py +++ b/tests/unit_tests/mcp_service/test_tool_inventory.py @@ -121,6 +121,7 @@ def budgeted_bytes(text: str) -> int: "list_themes": 3_000, "list_users": 2_900, "manage_dashboard_certification": 1_900, + "manage_dashboard_markdown": 7_000, "manage_dashboard_owners": 2_200, "manage_dashboard_roles": 1_900, "manage_native_filters": 9_100,