fix(mcp): fix form_data null, dataset URL, ASCII preview, and chart rename (#39109)

Author: aminghadersohi

superset/mcp_service/chart/schemas.py

@@ -284,14 +284,25 @@ def serialize_chart_object(chart: ChartLike | None) -> ChartInfo | None:
     if not chart:
         return None
 
-    # Use the chart's native URL (explore URL) instead of screenshot URL
     from superset.mcp_service.utils.url_utils import get_superset_base_url
+    from superset.utils import json as utils_json
 
     chart_id = getattr(chart, "id", None)
     chart_url = None
     if chart_id:
         chart_url = f"{get_superset_base_url()}/explore/?slice_id={chart_id}"
 
+    # Parse form_data from the chart's params JSON string
+    chart_params = getattr(chart, "params", None)
+    chart_form_data = None
+    if chart_params and isinstance(chart_params, str):
+        try:
+            chart_form_data = utils_json.loads(chart_params)
+        except (TypeError, ValueError):
+            pass
+    elif isinstance(chart_params, dict):
+        chart_form_data = chart_params
+
     return ChartInfo(
         id=chart_id,
         slice_name=getattr(chart, "slice_name", None),
@@ -301,6 +312,7 @@ def serialize_chart_object(chart: ChartLike | None) -> ChartInfo | None:
         url=chart_url,
         description=getattr(chart, "description", None),
         cache_timeout=getattr(chart, "cache_timeout", None),
+        form_data=chart_form_data,
         changed_by=getattr(chart, "changed_by_name", None)
         or (str(chart.changed_by) if getattr(chart, "changed_by", None) else None),
         changed_by_name=getattr(chart, "changed_by_name", None),
@@ -1284,7 +1296,13 @@ class GenerateExploreLinkRequest(FormDataCacheControl):
 
 class UpdateChartRequest(QueryCacheControl):
     identifier: int | str = Field(..., description="Chart ID or UUID")
-    config: ChartConfig
+    config: ChartConfig | None = Field(
+        None,
+        description=(
+            "Chart configuration. Required for visualization changes. "
+            "Omit to only update the chart name."
+        ),
+    )
     chart_name: str | None = Field(
         None, description="Auto-generates if omitted", max_length=255
     )

superset/mcp_service/chart/tool/get_chart_preview.py

@@ -146,6 +146,16 @@ def generate(self) -> ASCIIPreview | ChartError:
                     if "column_name" in x_axis_config:
                         columns.append(x_axis_config["column_name"])
 
+            if not columns and not metrics:
+                return ChartError(
+                    error=(
+                        "Cannot generate ASCII preview: chart has no columns or "
+                        "metrics in its configuration. This chart type may not "
+                        "support ASCII preview."
+                    ),
+                    error_type="UnsupportedChart",
+                )
+
             factory = QueryContextFactory()
             query_context = factory.create(
                 datasource={

superset/mcp_service/chart/tool/update_chart.py

@@ -21,6 +21,7 @@
 
 import logging
 import time
+from typing import Any
 
 from fastmcp import Context
 from sqlalchemy.exc import SQLAlchemyError
@@ -46,6 +47,66 @@
 logger = logging.getLogger(__name__)
 
 
+def _find_chart(identifier: int | str) -> Any | None:
+    """Find a chart by numeric ID or UUID string."""
+    from superset.daos.chart import ChartDAO
+
+    if isinstance(identifier, int) or (
+        isinstance(identifier, str) and identifier.isdigit()
+    ):
+        chart_id = int(identifier) if isinstance(identifier, str) else identifier
+        return ChartDAO.find_by_id(chart_id)
+    return ChartDAO.find_by_id(identifier, id_column="uuid")
+
+
+def _build_update_payload(
+    request: UpdateChartRequest,
+    chart: Any,
+) -> dict[str, Any] | GenerateChartResponse:
+    """Build the update payload for a chart update.
+
+    Returns a dict payload on success, or a GenerateChartResponse error
+    when neither config nor chart_name is provided.
+    """
+    if request.config is not None:
+        dataset_id = chart.datasource_id if chart.datasource_id else None
+        new_form_data = map_config_to_form_data(request.config, dataset_id=dataset_id)
+        new_form_data.pop("_mcp_warnings", None)
+
+        chart_name = (
+            request.chart_name
+            if request.chart_name
+            else chart.slice_name or generate_chart_name(request.config)
+        )
+
+        return {
+            "slice_name": chart_name,
+            "viz_type": new_form_data["viz_type"],
+            "params": json.dumps(new_form_data),
+        }
+
+    # Name-only update: keep existing visualization, just rename
+    if not request.chart_name:
+        return GenerateChartResponse.model_validate(
+            {
+                "chart": None,
+                "error": {
+                    "error_type": "ValidationError",
+                    "message": ("Either 'config' or 'chart_name' must be provided."),
+                    "details": (
+                        "Either 'config' or 'chart_name' must be provided. "
+                        "Use config for visualization changes, chart_name "
+                        "for renaming."
+                    ),
+                },
+                "success": False,
+                "schema_version": "2.0",
+                "api_version": "v1",
+            }
+        )
+    return {"slice_name": request.chart_name}
+
+
 @tool(
     tags=["mutate"],
     class_permission_name="Chart",
@@ -105,29 +166,22 @@ async def update_chart(
     start_time = time.time()
 
     try:
-        # Find the existing chart
-        from superset.daos.chart import ChartDAO
-
         with event_logger.log_context(action="mcp.update_chart.chart_lookup"):
-            chart = None
-            if isinstance(request.identifier, int) or (
-                isinstance(request.identifier, str) and request.identifier.isdigit()
-            ):
-                chart_id = (
-                    int(request.identifier)
-                    if isinstance(request.identifier, str)
-                    else request.identifier
-                )
-                chart = ChartDAO.find_by_id(chart_id)
-            else:
-                # Try UUID lookup using DAO flexible method
-                chart = ChartDAO.find_by_id(request.identifier, id_column="uuid")
+            chart = _find_chart(request.identifier)
 
         if not chart:
             return GenerateChartResponse.model_validate(
                 {
                     "chart": None,
-                    "error": f"No chart found with identifier: {request.identifier}",
+                    "error": {
+                        "error_type": "NotFound",
+                        "message": (
+                            f"No chart found with identifier: {request.identifier}"
+                        ),
+                        "details": (
+                            f"No chart found with identifier: {request.identifier}"
+                        ),
+                    },
                     "success": False,
                     "schema_version": "2.0",
                     "api_version": "v1",
@@ -157,30 +211,15 @@ async def update_chart(
                 }
             )
 
-        # Map the new config to form_data format
-        # Get dataset_id from existing chart for column type checking
-        dataset_id = chart.datasource_id if chart.datasource_id else None
-        new_form_data = map_config_to_form_data(request.config, dataset_id=dataset_id)
-        new_form_data.pop("_mcp_warnings", None)
-
-        # Update chart using Superset's command
+        # Build update payload (config update or name-only rename)
         from superset.commands.chart.update import UpdateChartCommand
 
-        with event_logger.log_context(action="mcp.update_chart.db_write"):
-            # Generate new chart name if provided, otherwise keep existing
-            chart_name = (
-                request.chart_name
-                if request.chart_name
-                else chart.slice_name or generate_chart_name(request.config)
-            )
+        payload_or_error = _build_update_payload(request, chart)
+        if isinstance(payload_or_error, GenerateChartResponse):
+            return payload_or_error
 
-            update_payload = {
-                "slice_name": chart_name,
-                "viz_type": new_form_data["viz_type"],
-                "params": json.dumps(new_form_data),
-            }
-
-            command = UpdateChartCommand(chart.id, update_payload)
+        with event_logger.log_context(action="mcp.update_chart.db_write"):
+            command = UpdateChartCommand(chart.id, payload_or_error)
             updated_chart = command.run()
 
         # Generate semantic analysis
@@ -199,7 +238,11 @@ async def update_chart(
         chart_name = (
             updated_chart.slice_name
             if updated_chart and hasattr(updated_chart, "slice_name")
-            else generate_chart_name(request.config)
+            else (
+                generate_chart_name(request.config)
+                if request.config
+                else "Updated chart"
+            )
         )
         accessibility = AccessibilityMetadata(
             color_blind_safe=True,  # Would need actual analysis
@@ -288,7 +331,11 @@ async def update_chart(
         return GenerateChartResponse.model_validate(
             {
                 "chart": None,
-                "error": f"Chart update failed: {str(e)}",
+                "error": {
+                    "error_type": type(e).__name__,
+                    "message": f"Chart update failed: {e}",
+                    "details": str(e),
+                },
                 "performance": {
                     "query_duration_ms": execution_time,
                     "cache_status": "error",

superset/mcp_service/dataset/schemas.py

@@ -324,6 +324,9 @@ def _humanize_timestamp(dt: datetime | None) -> str | None:
 def serialize_dataset_object(dataset: Any) -> DatasetInfo | None:
     if not dataset:
         return None
+
+    from superset.mcp_service.utils.url_utils import get_superset_base_url
+
     params = getattr(dataset, "params", None)
     if isinstance(params, str):
         try:
@@ -387,7 +390,12 @@ def serialize_dataset_object(dataset: Any) -> DatasetInfo | None:
         if getattr(dataset, "uuid", None)
         else None,
         schema_perm=getattr(dataset, "schema_perm", None),
-        url=getattr(dataset, "url", None),
+        url=(
+            f"{get_superset_base_url()}/tablemodelview/edit/"
+            f"{getattr(dataset, 'id', None)}"
+            if getattr(dataset, "id", None)
+            else None
+        ),
         sql=getattr(dataset, "sql", None),
         main_dttm_col=getattr(dataset, "main_dttm_col", None),
         offset=getattr(dataset, "offset", None),