diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py index 1bb5510a806..02ca11f0cc2 100644 --- a/superset/mcp_service/auth.py +++ b/superset/mcp_service/auth.py @@ -277,6 +277,23 @@ def _setup_user_context() -> User | None: logger.debug("No Flask app context available for user setup") return None raise + except ValueError as e: + # JWT user resolution failed (e.g. SAML subject not in DB). + # If middleware already set g.user (request context exists), + # use that instead of failing closed. + from flask import has_request_context + + if has_request_context() and hasattr(g, "user") and g.user: + logger.warning( + "JWT user resolution failed (%s), using middleware-provided g.user=%s", + e, + g.user.username, + ) + # Assign to local so relationship validation below runs + # (same as the normal path) to prevent detached instance errors. + user = g.user + else: + raise # Validate user has necessary relationships loaded # (Force access to ensure they're loaded if lazy) 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 a32fef3cfea..83967209c05 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 @@ -415,12 +415,8 @@ def add_chart_to_existing_dashboard( published=updated_dashboard.published, created_on=updated_dashboard.created_on, changed_on=updated_dashboard.changed_on, - created_by=updated_dashboard.created_by.username - if updated_dashboard.created_by - else None, - changed_by=updated_dashboard.changed_by.username - if updated_dashboard.changed_by - else None, + created_by=updated_dashboard.created_by_name or None, + changed_by=updated_dashboard.changed_by_name or None, uuid=str(updated_dashboard.uuid) if updated_dashboard.uuid else None, url=f"{get_superset_base_url()}/superset/dashboard/{updated_dashboard.id}/", chart_count=len(updated_dashboard.slices), diff --git a/superset/mcp_service/dashboard/tool/generate_dashboard.py b/superset/mcp_service/dashboard/tool/generate_dashboard.py index 8feee01ab68..5f0ed4ee5cc 100644 --- a/superset/mcp_service/dashboard/tool/generate_dashboard.py +++ b/superset/mcp_service/dashboard/tool/generate_dashboard.py @@ -231,10 +231,8 @@ def generate_dashboard( # Prepare dashboard data and create dashboard with event_logger.log_context(action="mcp.generate_dashboard.db_write"): - dashboard_data = { + dashboard_data: Dict[str, Any] = { "dashboard_title": dashboard_title, - "slug": None, # Let Superset auto-generate slug - "css": "", "json_metadata": json.dumps( { "filter_scopes": {}, @@ -265,8 +263,23 @@ def generate_dashboard( dashboard_data["description"] = request.description # Create the dashboard using Superset's command pattern - command = CreateDashboardCommand(dashboard_data) - dashboard = command.run() + try: + command = CreateDashboardCommand(dashboard_data) + dashboard = command.run() + except Exception as cmd_err: + # Surface the root cause from @transaction's error wrapping + root_cause = cmd_err.__cause__ or cmd_err + logger.error( + "CreateDashboardCommand failed: %s (cause: %s)", + cmd_err, + root_cause, + exc_info=True, + ) + return GenerateDashboardResponse( + dashboard=None, + dashboard_url=None, + error=f"Failed to create dashboard: {root_cause}", + ) # Convert to our response format from superset.mcp_service.dashboard.schemas import ( @@ -282,8 +295,8 @@ def generate_dashboard( published=dashboard.published, created_on=dashboard.created_on, changed_on=dashboard.changed_on, - created_by=dashboard.created_by.username if dashboard.created_by else None, - changed_by=dashboard.changed_by.username if dashboard.changed_by else None, + created_by=dashboard.created_by_name or None, + changed_by=dashboard.changed_by_name or None, uuid=str(dashboard.uuid) if dashboard.uuid else None, url=f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/", chart_count=len(request.chart_ids), diff --git a/superset/mcp_service/middleware.py b/superset/mcp_service/middleware.py index 0fadd648ae8..b0ffc4f5f7c 100644 --- a/superset/mcp_service/middleware.py +++ b/superset/mcp_service/middleware.py @@ -18,10 +18,13 @@ import logging import time from collections import defaultdict -from typing import Any, Awaitable, Callable, Dict, Protocol +from typing import Any, Awaitable, Callable, Dict, Protocol, Sequence +import mcp.types as mt from fastmcp.exceptions import ToolError from fastmcp.server.middleware import Middleware, MiddlewareContext +from fastmcp.server.middleware.middleware import CallNext +from fastmcp.tools.tool import Tool, ToolResult from flask import has_app_context from pydantic import ValidationError from sqlalchemy.exc import OperationalError, TimeoutError @@ -259,6 +262,62 @@ class PrivateToolMiddleware(Middleware): return await call_next(context) +class StructuredContentStripperMiddleware(Middleware): + """Strip ``outputSchema`` and ``structured_content`` to prevent encoding errors. + + FastMCP 3.x auto-generates ``outputSchema`` in tool definitions + (``tools/list``) and ``structuredContent`` in tool call responses + (``tools/call``) when the tool has a typed return annotation. + + Some MCP client transports (e.g. Claude.ai's MCP bridge) cannot handle + ``structuredContent`` dicts, causing ``TypeError: encoding without a + string argument``. Additionally, if ``outputSchema`` is advertised but + ``structuredContent`` is stripped from the response, clients may raise + ``Output validation error: outputSchema defined but no structured output + returned``. + + This middleware handles both sides: + - ``on_list_tools``: removes ``output_schema`` from every tool definition + - ``on_call_tool``: removes ``structured_content`` from every tool result + """ + + async def on_list_tools( + self, + context: MiddlewareContext[mt.ListToolsRequest], + call_next: CallNext[mt.ListToolsRequest, Sequence[Tool]], + ) -> Sequence[Tool]: + tools = await call_next(context) + return [ + t.model_copy(update={"output_schema": None}) + if t.output_schema is not None + else t + for t in tools + ] + + async def on_call_tool( + self, + context: MiddlewareContext[mt.CallToolRequestParams], + call_next: Callable[[MiddlewareContext], Awaitable[ToolResult]], + ) -> ToolResult: + try: + result = await call_next(context) + except Exception as e: + # When exceptions propagate past the middleware chain to the + # MCP SDK layer, they become CallToolResult(isError=True). + # Some transports (Claude.ai's MCP bridge) cannot encode these + # error responses, producing "encoding without a string argument". + # Catch ALL exceptions (not just specific types) because any + # unhandled exception — including ToolError from + # GlobalErrorHandlerMiddleware, ValueError, TypeError, etc. — + # will cause encoding failures on the wire. + return ToolResult( + content=[mt.TextContent(type="text", text=f"Error: {e}")], + ) + if isinstance(result, ToolResult) and result.structured_content is not None: + result = ToolResult(content=result.content, meta=result.meta) + return result + + class GlobalErrorHandlerMiddleware(Middleware): """ Global error handler middleware that provides consistent error responses diff --git a/superset/mcp_service/server.py b/superset/mcp_service/server.py index 732e8f4622f..aff5cd8b760 100644 --- a/superset/mcp_service/server.py +++ b/superset/mcp_service/server.py @@ -39,6 +39,7 @@ from superset.mcp_service.middleware import ( create_response_size_guard_middleware, GlobalErrorHandlerMiddleware, LoggingMiddleware, + StructuredContentStripperMiddleware, ) from superset.mcp_service.storage import _create_redis_store @@ -363,9 +364,15 @@ def run_server( # Add logging middleware (logs all tool calls with duration tracking) middleware_list.append(LoggingMiddleware()) - # Add global error handler (outermost – catches all exceptions) + # Add global error handler (catches all exceptions, raises ToolError) middleware_list.append(GlobalErrorHandlerMiddleware()) + # Strip outputSchema from tool definitions and structuredContent from + # tool responses to prevent encoding errors on Claude.ai's MCP bridge. + # MUST be outermost so it catches ToolError from GlobalErrorHandler + # and converts to plain text before the MCP SDK tries to encode it. + middleware_list.append(StructuredContentStripperMiddleware()) + mcp_instance = init_fastmcp_server( auth=auth_provider, middleware=middleware_list or None, diff --git a/superset/mcp_service/sql_lab/tool/save_sql_query.py b/superset/mcp_service/sql_lab/tool/save_sql_query.py index 448ce9e1924..65c76e6bb93 100644 --- a/superset/mcp_service/sql_lab/tool/save_sql_query.py +++ b/superset/mcp_service/sql_lab/tool/save_sql_query.py @@ -121,10 +121,10 @@ async def save_sql_query( id=saved_query.id, label=saved_query.label, sql=saved_query.sql, - database_id=request.database_id, - schema_name=request.schema_name, + database_id=saved_query.db_id, + schema_name=saved_query.schema or None, catalog=getattr(saved_query, "catalog", None), - description=request.description, + description=saved_query.description or None, url=saved_query_url, ) diff --git a/superset/mcp_service/utils/schema_utils.py b/superset/mcp_service/utils/schema_utils.py index e382ec3dce8..34f99be88d3 100644 --- a/superset/mcp_service/utils/schema_utils.py +++ b/superset/mcp_service/utils/schema_utils.py @@ -512,6 +512,7 @@ def _apply_signature_for_fastmcp( wrapper: Any, original_func: Callable[..., Any], request_annotation: Any, + request_default: Any = None, ) -> None: """Apply annotations and signature to wrapper, stripping ctx for FastMCP. @@ -539,7 +540,10 @@ def _apply_signature_for_fastmcp( if _is_context_param(param, name, FMContext): continue if name == "request": - new_params.append(param.replace(annotation=request_annotation)) + replacement = {"annotation": request_annotation} + if request_default is not None: + replacement["default"] = request_default + new_params.append(param.replace(**replacement)) else: new_params.append(param) wrapper.__signature__ = orig_sig.replace(parameters=new_params) diff --git a/superset/sql/execution/executor.py b/superset/sql/execution/executor.py index e021920023d..698baca29a4 100644 --- a/superset/sql/execution/executor.py +++ b/superset/sql/execution/executor.py @@ -839,13 +839,22 @@ class SQLExecutor: or app.config.get("CACHE_DEFAULT_TIMEOUT", 300) ) - # Serialize statement results for caching + # Serialize statement results for caching. + # Convert DataFrames to list-of-dicts so the cache backend + # does not need to pickle pandas objects (which can fail to + # deserialize correctly with some backends or pandas versions). + import pandas as pd + cached_data = { "statements": [ { "original_sql": stmt.original_sql, "executed_sql": stmt.executed_sql, - "data": stmt.data, + "data": ( + stmt.data.to_dict(orient="records") + if isinstance(stmt.data, pd.DataFrame) + else stmt.data + ), "row_count": stmt.row_count, "execution_time_ms": stmt.execution_time_ms, } 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 c990fdde2a5..6aa9b27b688 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 @@ -96,6 +96,8 @@ def _mock_dashboard(id: int = 1, title: str = "Test Dashboard") -> Mock: dashboard.created_by.username = "test_user" dashboard.changed_by = Mock() dashboard.changed_by.username = "test_user" + dashboard.created_by_name = "test_user" + dashboard.changed_by_name = "test_user" dashboard.uuid = f"dashboard-uuid-{id}" dashboard.slices = [] dashboard.owners = [] diff --git a/tests/unit_tests/mcp_service/sql_lab/tool/test_save_sql_query.py b/tests/unit_tests/mcp_service/sql_lab/tool/test_save_sql_query.py index 1d736f18cc9..39443e4ef17 100644 --- a/tests/unit_tests/mcp_service/sql_lab/tool/test_save_sql_query.py +++ b/tests/unit_tests/mcp_service/sql_lab/tool/test_save_sql_query.py @@ -226,6 +226,9 @@ class TestSaveSqlQueryToolLogic: mock_sq.id = 42 mock_sq.label = "Revenue Query" mock_sq.sql = "SELECT SUM(revenue) FROM sales" + mock_sq.db_id = 1 + mock_sq.schema = "" + mock_sq.description = "" mock_sq.catalog = None request = SaveSqlQueryRequest( @@ -390,6 +393,9 @@ class TestSaveSqlQueryToolLogic: mock_sq.id = 10 mock_sq.label = "Test" mock_sq.sql = "SELECT 1" + mock_sq.db_id = 1 + mock_sq.schema = "public" + mock_sq.description = "" mock_sq.catalog = None request = SaveSqlQueryRequest(