From daefedebcdaa8ea0a8989f8ded40ccb10565c1b7 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Tue, 31 Mar 2026 18:50:20 +0200 Subject: [PATCH] fix(mcp): batch fix for execute_sql crashes, null timestamps, and deck.gl errors (#38977) --- superset/mcp_service/chart/schemas.py | 12 ++++++-- .../mcp_service/chart/tool/get_chart_data.py | 23 +++++++++++++++ .../mcp_service/chart/tool/list_charts.py | 1 + superset/mcp_service/dashboard/schemas.py | 16 ++++++++-- .../dashboard/tool/list_dashboards.py | 1 + superset/mcp_service/dataset/schemas.py | 12 ++++++-- .../mcp_service/dataset/tool/list_datasets.py | 1 + .../mcp_service/sql_lab/tool/execute_sql.py | 29 ++++++++++--------- .../dataset/tool/test_dataset_tools.py | 10 +++++-- .../sql_lab/tool/test_execute_sql.py | 14 +++++---- 10 files changed, 92 insertions(+), 27 deletions(-) diff --git a/superset/mcp_service/chart/schemas.py b/superset/mcp_service/chart/schemas.py index ce7d1ea397f..143d60a422f 100644 --- a/superset/mcp_service/chart/schemas.py +++ b/superset/mcp_service/chart/schemas.py @@ -25,6 +25,7 @@ import difflib from datetime import datetime, timezone from typing import Annotated, Any, Dict, List, Literal, Protocol +import humanize from pydantic import ( AliasChoices, AliasPath, @@ -272,6 +273,13 @@ class GetChartInfoRequest(BaseModel): return self +def _humanize_timestamp(dt: datetime | None) -> str | None: + """Convert a datetime to a humanized string like '2 hours ago'.""" + if dt is None: + return None + return humanize.naturaltime(datetime.now() - dt) + + def serialize_chart_object(chart: ChartLike | None) -> ChartInfo | None: if not chart: return None @@ -297,11 +305,11 @@ def serialize_chart_object(chart: ChartLike | None) -> ChartInfo | None: or (str(chart.changed_by) if getattr(chart, "changed_by", None) else None), changed_by_name=getattr(chart, "changed_by_name", None), changed_on=getattr(chart, "changed_on", None), - changed_on_humanized=getattr(chart, "changed_on_humanized", None), + changed_on_humanized=_humanize_timestamp(getattr(chart, "changed_on", None)), created_by=getattr(chart, "created_by_name", None) or (str(chart.created_by) if getattr(chart, "created_by", None) else None), created_on=getattr(chart, "created_on", None), - created_on_humanized=getattr(chart, "created_on_humanized", None), + created_on_humanized=_humanize_timestamp(getattr(chart, "created_on", None)), uuid=str(getattr(chart, "uuid", "")) if getattr(chart, "uuid", None) else None, tags=[ TagInfo.model_validate(tag, from_attributes=True) diff --git a/superset/mcp_service/chart/tool/get_chart_data.py b/superset/mcp_service/chart/tool/get_chart_data.py index a958433b9f8..34f7f0fb3a7 100644 --- a/superset/mcp_service/chart/tool/get_chart_data.py +++ b/superset/mcp_service/chart/tool/get_chart_data.py @@ -369,6 +369,29 @@ async def get_chart_data( # noqa: C901 # Bubble charts use x/y/size as separate metric fields. viz_type = chart.viz_type or "" + # Deck.gl chart types store spatial data (lat/lon) + # rather than traditional metrics/groupby. They + # require a saved query_context to retrieve data. + # Match by prefix to cover all current and future + # deck.gl viz types (deck_arc, deck_scatter, etc.). + if viz_type.startswith("deck_"): + await ctx.warning( + "Chart %s is a deck.gl visualization (%s) with no " + "saved query_context. Data retrieval requires " + "re-saving the chart in Superset." % (chart.id, viz_type) + ) + return ChartError( + error=( + f"Chart {chart.id} is a deck.gl visualization " + f"(type: {viz_type}) with no saved query_context. " + f"Deck.gl charts use spatial data (lat/lon) that " + f"cannot be reconstructed from form_data alone. " + f"Please open this chart in Superset and re-save " + f"it to generate a query_context." + ), + error_type="MissingQueryContext", + ) + singular_metric_no_groupby = ( "big_number", "big_number_total", diff --git a/superset/mcp_service/chart/tool/list_charts.py b/superset/mcp_service/chart/tool/list_charts.py index 3f9bcfe88eb..f952b43f1e1 100644 --- a/superset/mcp_service/chart/tool/list_charts.py +++ b/superset/mcp_service/chart/tool/list_charts.py @@ -47,6 +47,7 @@ DEFAULT_CHART_COLUMNS = [ "slice_name", "viz_type", "url", + "changed_on", "changed_on_humanized", ] diff --git a/superset/mcp_service/dashboard/schemas.py b/superset/mcp_service/dashboard/schemas.py index 99d42b2fa35..be2860e07cb 100644 --- a/superset/mcp_service/dashboard/schemas.py +++ b/superset/mcp_service/dashboard/schemas.py @@ -68,6 +68,7 @@ from __future__ import annotations from datetime import datetime from typing import Annotated, Any, Dict, List, Literal, TYPE_CHECKING +import humanize from pydantic import ( BaseModel, ConfigDict, @@ -515,6 +516,13 @@ def dashboard_serializer(dashboard: "Dashboard") -> DashboardInfo: ) +def _humanize_timestamp(dt: datetime | None) -> str | None: + """Convert a datetime to a humanized string like '2 hours ago'.""" + if dt is None: + return None + return humanize.naturaltime(datetime.now() - dt) + + def serialize_dashboard_object(dashboard: Any) -> DashboardInfo: """Simple dashboard serializer that safely handles object attributes.""" from superset.mcp_service.utils.url_utils import get_superset_base_url @@ -537,10 +545,14 @@ def serialize_dashboard_object(dashboard: Any) -> DashboardInfo: published=getattr(dashboard, "published", None), changed_by=getattr(dashboard, "changed_by_name", None), changed_on=getattr(dashboard, "changed_on", None), - changed_on_humanized=getattr(dashboard, "changed_on_humanized", None), + changed_on_humanized=_humanize_timestamp( + getattr(dashboard, "changed_on", None) + ), created_by=getattr(dashboard, "created_by_name", None), created_on=getattr(dashboard, "created_on", None), - created_on_humanized=getattr(dashboard, "created_on_humanized", None), + created_on_humanized=_humanize_timestamp( + getattr(dashboard, "created_on", None) + ), description=getattr(dashboard, "description", None), css=getattr(dashboard, "css", None), certified_by=getattr(dashboard, "certified_by", None), diff --git a/superset/mcp_service/dashboard/tool/list_dashboards.py b/superset/mcp_service/dashboard/tool/list_dashboards.py index ff277406140..380291ce20a 100644 --- a/superset/mcp_service/dashboard/tool/list_dashboards.py +++ b/superset/mcp_service/dashboard/tool/list_dashboards.py @@ -49,6 +49,7 @@ DEFAULT_DASHBOARD_COLUMNS = [ "dashboard_title", "slug", "url", + "changed_on", "changed_on_humanized", ] diff --git a/superset/mcp_service/dataset/schemas.py b/superset/mcp_service/dataset/schemas.py index 2d88b8bbc38..df5970ea061 100644 --- a/superset/mcp_service/dataset/schemas.py +++ b/superset/mcp_service/dataset/schemas.py @@ -24,6 +24,7 @@ from __future__ import annotations from datetime import datetime from typing import Annotated, Any, Dict, List, Literal +import humanize from pydantic import ( BaseModel, ConfigDict, @@ -307,6 +308,13 @@ def _parse_json_field(obj: Any, field_name: str) -> Dict[str, Any] | None: return value +def _humanize_timestamp(dt: datetime | None) -> str | None: + """Convert a datetime to a humanized string like '2 hours ago'.""" + if dt is None: + return None + return humanize.naturaltime(datetime.now() - dt) + + def serialize_dataset_object(dataset: Any) -> DatasetInfo | None: if not dataset: return None @@ -349,11 +357,11 @@ def serialize_dataset_object(dataset: Any) -> DatasetInfo | None: changed_by=getattr(dataset, "changed_by_name", None) or (str(dataset.changed_by) if getattr(dataset, "changed_by", None) else None), changed_on=getattr(dataset, "changed_on", None), - changed_on_humanized=getattr(dataset, "changed_on_humanized", None), + changed_on_humanized=_humanize_timestamp(getattr(dataset, "changed_on", None)), created_by=getattr(dataset, "created_by_name", None) or (str(dataset.created_by) if getattr(dataset, "created_by", None) else None), created_on=getattr(dataset, "created_on", None), - created_on_humanized=getattr(dataset, "created_on_humanized", None), + created_on_humanized=_humanize_timestamp(getattr(dataset, "created_on", None)), tags=[ TagInfo.model_validate(tag, from_attributes=True) for tag in getattr(dataset, "tags", []) diff --git a/superset/mcp_service/dataset/tool/list_datasets.py b/superset/mcp_service/dataset/tool/list_datasets.py index 95e37fcaece..96d493e5b98 100644 --- a/superset/mcp_service/dataset/tool/list_datasets.py +++ b/superset/mcp_service/dataset/tool/list_datasets.py @@ -48,6 +48,7 @@ DEFAULT_DATASET_COLUMNS = [ "id", "table_name", "schema", + "changed_on", "changed_on_humanized", ] diff --git a/superset/mcp_service/sql_lab/tool/execute_sql.py b/superset/mcp_service/sql_lab/tool/execute_sql.py index 18d89b3b271..edfbc9c8a50 100644 --- a/superset/mcp_service/sql_lab/tool/execute_sql.py +++ b/superset/mcp_service/sql_lab/tool/execute_sql.py @@ -36,8 +36,7 @@ from superset_core.queries.types import ( QueryStatus, ) -from superset.errors import ErrorLevel, SupersetError, SupersetErrorType -from superset.exceptions import SupersetErrorException, SupersetSecurityException +from superset.errors import SupersetErrorType from superset.extensions import event_logger from superset.mcp_service.sql_lab.schemas import ( ColumnInfo, @@ -91,21 +90,23 @@ async def execute_sql(request: ExecuteSqlRequest, ctx: Context) -> ExecuteSqlRes db.session.query(Database).filter_by(id=request.database_id).first() ) if not database: - raise SupersetErrorException( - SupersetError( - message=f"Database with ID {request.database_id} not found", - error_type=SupersetErrorType.DATABASE_NOT_FOUND_ERROR, - level=ErrorLevel.ERROR, - ) + await ctx.error( + "Database not found: database_id=%s" % request.database_id + ) + return ExecuteSqlResponse( + success=False, + error=f"Database with ID {request.database_id} not found", + error_type=SupersetErrorType.DATABASE_NOT_FOUND_ERROR.value, ) if not security_manager.can_access_database(database): - raise SupersetSecurityException( - SupersetError( - message=(f"Access denied to database {database.database_name}"), - error_type=SupersetErrorType.DATABASE_SECURITY_ACCESS_ERROR, - level=ErrorLevel.ERROR, - ) + await ctx.error( + "Access denied to database: %s" % database.database_name + ) + return ExecuteSqlResponse( + success=False, + error=f"Access denied to database {database.database_name}", + error_type=SupersetErrorType.DATABASE_SECURITY_ACCESS_ERROR.value, ) # 2. Build QueryOptions and execute query diff --git a/tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py b/tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py index 48742973863..0b63e013a8c 100644 --- a/tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py +++ b/tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py @@ -1236,6 +1236,7 @@ class TestDatasetDefaultColumnFiltering: "id", "table_name", "schema", + "changed_on", "changed_on_humanized", } @@ -1319,7 +1320,13 @@ class TestDatasetDefaultColumnFiltering: dataset_item = data["datasets"][0] # Verify ONLY default columns are present in the response item - expected_keys = {"id", "table_name", "schema", "changed_on_humanized"} + expected_keys = { + "id", + "table_name", + "schema", + "changed_on", + "changed_on_humanized", + } actual_keys = set(dataset_item.keys()) # The response should only contain the default columns, NOT all columns @@ -1335,7 +1342,6 @@ class TestDatasetDefaultColumnFiltering: "description", "database_name", "changed_by", - "changed_on", "columns", "metrics", ] diff --git a/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py b/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py index b536578ca1f..bcd66cbe20e 100644 --- a/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py +++ b/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py @@ -237,7 +237,7 @@ class TestExecuteSql: mock_security_manager, # noqa: PT019 mcp_server, ): - """Test error when database is not found.""" + """Test graceful error when database is not found.""" # mock_security_manager is patched but not used (error happens first) del mock_security_manager # Silence unused variable warning mock_db.session.query.return_value.filter_by.return_value.first.return_value = ( @@ -251,8 +251,10 @@ class TestExecuteSql: } async with Client(mcp_server) as client: - with pytest.raises(ToolError, match="Database with ID 999 not found"): - await client.call_tool("execute_sql", {"request": request}) + result = await client.call_tool("execute_sql", {"request": request}) + data = result.structured_content + assert data["success"] is False + assert "Database with ID 999 not found" in data["error"] @patch("superset.security_manager", new_callable=MagicMock) @patch("superset.db") @@ -274,8 +276,10 @@ class TestExecuteSql: } async with Client(mcp_server) as client: - with pytest.raises(ToolError, match="Access denied to database"): - await client.call_tool("execute_sql", {"request": request}) + result = await client.call_tool("execute_sql", {"request": request}) + data = result.structured_content + assert data["success"] is False + assert "Access denied to database" in data["error"] @patch("superset.security_manager") @patch("superset.db")