gabotorresruiz commented on code in PR #42659: URL: https://github.com/apache/superset/pull/42659#discussion_r3806386657
########## superset/mcp_service/dashboard/permalink.py: ########## @@ -0,0 +1,182 @@ +# 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. + +"""Helpers for resolving dashboard permalink keys and shared URLs.""" + +import logging +from dataclasses import dataclass +from typing import Callable, Generic, TypeVar +from urllib.parse import urlparse + +from flask import g, has_request_context + +from superset.commands.dashboard.exceptions import DashboardAccessDeniedError +from superset.commands.dashboard.permalink.get import GetDashboardPermalinkCommand +from superset.dashboards.permalink.exceptions import DashboardPermalinkGetFailedError +from superset.dashboards.permalink.types import DashboardPermalinkValue +from superset.mcp_service.auth import load_user_with_relationships +from superset.mcp_service.dashboard.schemas import ( + redact_filter_state_data_model_metadata, +) +from superset.mcp_service.privacy import user_can_view_data_model_metadata +from superset.mcp_service.utils import sanitize_for_llm_context + +logger = logging.getLogger(__name__) + +LookupResultT = TypeVar("LookupResultT") + + +@dataclass(frozen=True) +class DashboardLookupResult(Generic[LookupResultT]): + """Result of resolving either a dashboard identifier or permalink.""" + + result: LookupResultT | None + permalink_key: str | None = None + permalink_value: DashboardPermalinkValue | None = None + permalink_error: bool = False + + +@dataclass(frozen=True) +class DashboardPermalinkState: + """Sanitized permalink state belonging to a resolved dashboard.""" + + key: str + state: dict[str, object] + + +def extract_dashboard_permalink_key(value: str) -> str: + """Return a key from a dashboard permalink URL, or the bare input.""" + path_parts = [part for part in urlparse(value).path.split("/") if part] + if len(path_parts) >= 3 and path_parts[-3:-1] == ["dashboard", "p"]: + return path_parts[-1] + return value + + +def refresh_request_user_for_permalink_access() -> None: + """Reload the request user before permalink access checks.""" + if not has_request_context() or not getattr(g, "user", None): + return + current_user = g.user + if getattr(current_user, "is_anonymous", False): + return + username = getattr(current_user, "username", None) + email = getattr(current_user, "email", None) + if not username and not email: + return + refreshed_user = ( + load_user_with_relationships(username=username) + if username + else load_user_with_relationships(email=email) + ) + if refreshed_user is not None: + g.user = refreshed_user + + +def get_dashboard_permalink( + key_or_url: str, +) -> tuple[str, DashboardPermalinkValue] | None: + """Resolve a dashboard permalink key or shared URL, returning its state.""" + key = extract_dashboard_permalink_key(key_or_url) + refresh_request_user_for_permalink_access() + try: + value = GetDashboardPermalinkCommand(key).run() + except (DashboardAccessDeniedError, DashboardPermalinkGetFailedError) as ex: + logger.info("Dashboard permalink could not be resolved: %s", ex) + return None + return (key, value) if value else None + + +def lookup_dashboard_reference( + *, + identifier: int | str | None, + permalink_key: str | None, + lookup: Callable[[int | str], LookupResultT], + is_found: Callable[[LookupResultT], bool], +) -> DashboardLookupResult[LookupResultT]: + """Look up a dashboard while preserving identifier precedence. + + A supplied identifier selects the dashboard and an explicit permalink only + contributes state. Shared permalink URLs and permalink-only requests select + the dashboard embedded in the permalink. Ambiguous bare strings use normal + identifier lookup first, then fall back to permalink resolution. + """ + key = permalink_key + identifier_is_permalink_url = False + if isinstance(identifier, str): + extracted_key = extract_dashboard_permalink_key(identifier) + identifier_is_permalink_url = extracted_key != identifier + if identifier_is_permalink_url: + key = extracted_key + + if identifier is not None and not identifier_is_permalink_url: + result = lookup(identifier) + if is_found(result): + resolved = get_dashboard_permalink(key) if key else None + return DashboardLookupResult( + result=result, + permalink_key=resolved[0] if resolved else key, + permalink_value=resolved[1] if resolved else None, + ) + if permalink_key is not None or not isinstance(identifier, str): + return DashboardLookupResult(result=result, permalink_key=key) + else: + result = None + + reference = key or (identifier if isinstance(identifier, str) else None) + resolved = get_dashboard_permalink(reference) if reference else None + if resolved is None: + return DashboardLookupResult( + result=result, + permalink_key=reference, + permalink_error=True, + ) + key, value = resolved + return DashboardLookupResult( + result=lookup(value["dashboardId"]), + permalink_key=key, + permalink_value=value, + ) + + +def get_matching_dashboard_permalink_state( + lookup_result: DashboardLookupResult[LookupResultT], + dashboard_id: int | None, +) -> DashboardPermalinkState | None: + """Return sanitized permalink state when it belongs to ``dashboard_id``.""" + value = lookup_result.permalink_value + key = lookup_result.permalink_key + if value is None or key is None: + return None + try: + permalink_dashboard_id = int(value["dashboardId"]) Review Comment: This block worries me a bit. `CreateDashboardPermalinkCommand.run()` stores `value["dashboardId"]` as `str(dashboard.uuid)` (superset/commands/dashboard/permalink/create.py, since 07bcfa9b5f in Nov 2023), so for virtually every real permalink this `int(...)` raises, the helper returns `None`, and both tools drop the shared state with a "belongs to a different dashboard" warning even though it is the same dashboard. I verified this on a running instance built from this branch: creating a permalink through the real `POST /api/v1/dashboard/1/permalink` API stores `"dashboardId": "<dashboard uuid>"` in the key_value row, and calling `get_dashboard_info` over HTTP with that key resolves the correct dashboard but returns `is_permalink_state: false` with no `filter_state`, logging "permalink_key belongs to a different dashboard" even though the response's own `uuid` equals the permalink's `dashboardId`. The identical request against a master build on the same database returns the full filter state. So this breaks the new resolution flow and regresses the existing `permalink_key` one. The unit tests miss it because they all mock `dashboardId` as `"42"`. Both `DashboardInfo` and `DashboardLayout` already carry `uuid`, so one option is to compare against it as well: ```python reference = str(value.get("dashboardId", "")) if reference not in {str(dashboard_id), str(dashboard_uuid or "")}: return None ``` Or, since the permalink-only path resolves the dashboard from the permalink, you could mark that on `DashboardLookupResult` and skip re-verification there entirely, keeping the comparison only for the explicit identifier + permalink_key combination. Either way, could you add a test whose mocked permalink value uses a UUID-string `dashboardId` (that is what the create command stores) asserting `filter_state` is present, for both tools? Pre-2023 permalinks can also carry a slug here; uuid + id covers everything created since 3.1, so I would treat slug matching as optional. ########## superset/mcp_service/dashboard/tool/get_dashboard_info.py: ########## @@ -27,82 +27,65 @@ from typing import Any from fastmcp import Context -from flask import g, has_request_context from sqlalchemy.orm import subqueryload from superset_core.mcp.decorators import tool, ToolAnnotations -from superset.dashboards.permalink.exceptions import DashboardPermalinkGetFailedError -from superset.dashboards.permalink.types import DashboardPermalinkValue from superset.extensions import event_logger -from superset.mcp_service.auth import load_user_with_relationships +from superset.mcp_service.dashboard.permalink import ( + DashboardLookupResult, + get_matching_dashboard_permalink_state, + lookup_dashboard_reference, +) from superset.mcp_service.dashboard.schemas import ( dashboard_serializer, DashboardError, DashboardInfo, DEFAULT_GET_DASHBOARD_INFO_COLUMNS, GetDashboardInfoRequest, - redact_filter_state_data_model_metadata, ) from superset.mcp_service.mcp_core import ModelGetInfoCore -from superset.mcp_service.privacy import user_can_view_data_model_metadata -from superset.mcp_service.utils import sanitize_for_llm_context logger = logging.getLogger(__name__) -def _refresh_request_user_for_permalink_access() -> None: - """Reload the request user before permalink access checks.""" - if not has_request_context() or not getattr(g, "user", None): - return - - current_user = g.user - if getattr(current_user, "is_anonymous", False): - return - - username = getattr(current_user, "username", None) - email = getattr(current_user, "email", None) - if not username and not email: - return - - refreshed_user = ( - load_user_with_relationships(username=username) - if username - else load_user_with_relationships(email=email) - ) - if refreshed_user is not None: - g.user = refreshed_user - - def _apply_permalink_state( result: DashboardInfo, permalink_key: str, permalink_state: dict[str, object], ) -> DashboardInfo: - """Sanitize only the raw permalink fields added after serialization.""" + """Add sanitized permalink fields after dashboard serialization.""" payload = result.model_dump(mode="python") payload["permalink_key"] = permalink_key - payload["filter_state"] = sanitize_for_llm_context( - permalink_state, - field_path=("filter_state",), - excluded_field_names=frozenset(), - ) + payload["filter_state"] = permalink_state payload["is_permalink_state"] = True return DashboardInfo.model_validate(payload) -def _get_permalink_state(permalink_key: str) -> DashboardPermalinkValue | None: - """Retrieve dashboard filter state from permalink. - - Returns the permalink value containing dashboardId and state if found, - None otherwise. - """ - from superset.commands.dashboard.permalink.get import GetDashboardPermalinkCommand - - try: - return GetDashboardPermalinkCommand(permalink_key).run() - except DashboardPermalinkGetFailedError as e: - logger.warning("Failed to retrieve permalink state: %s", e) - return None +def _lookup_dashboard( + tool: ModelGetInfoCore, + request: GetDashboardInfoRequest, +) -> tuple[ + DashboardInfo | DashboardError, + DashboardLookupResult[DashboardInfo | DashboardError], +]: + """Resolve an ordinary identifier or dashboard permalink, then run lookup.""" + lookup_result = lookup_dashboard_reference( + identifier=request.identifier, + permalink_key=request.permalink_key, + lookup=tool.run_tool, + is_found=lambda result: isinstance(result, DashboardInfo), + ) + result = lookup_result.result + if result is None or lookup_result.permalink_error: Review Comment: Not a blocker. When `identifier` is a plain string that simply doesn't exist (a slug typo, no permalink involved), this branch discards the original not-found error and returns the permalink message. I verified on this branch: `{"identifier": "sales-dashbord"}` returns "Dashboard permalink could not be resolved. It may be invalid or expired; ask for a fresh shared dashboard link.", which will send the agent asking the user for a shared link they never mentioned, while the same call on master returns "DashboardInfo with identifier 'sales-dashbord' not found". Since `result` still holds the original error here, could we keep it whenever `result is not None` and reserve the permalink wording for permalink-only requests? get_dashboard_layout.py has the same pattern around line 104. A small test asserting the typo message names the identifier would lock it in. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
