feat(charts): validate post-processing options against operation schemas [DRAFT]

ChartDataPostProcessingOperationSchema accepted `options` as a free-form Dict;
the per-operation option schemas (aggregate, rolling, prophet, pivot, ...)
existed only for OpenAPI docs and were never wired into validation.

Add a validates_schema hook that maps each operation to its option schema and
validates `options` against it. Validation is lenient (unknown=EXCLUDE) so it
surfaces wrong types / out-of-range values on declared fields without rejecting
payloads that carry extra keys; operations without a dedicated schema are
unaffected.

DRAFT: these option schemas were never used for validation and have latent
issues (e.g. ChartDataAggregateOptionsSchema.groupby is accidentally a tuple,
so it isn't validated). Each option schema should be audited against the real
pandas_postprocessing signatures before strict (unknown=RAISE) validation is
considered, to avoid rejecting currently-valid requests.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

superset/charts/schemas.py

@@ -22,7 +22,15 @@ from typing import Any, TYPE_CHECKING
 
 from flask import current_app
 from flask_babel import gettext as _
-from marshmallow import EXCLUDE, fields, post_load, Schema, validate
+from marshmallow import (
+    EXCLUDE,
+    fields,
+    post_load,
+    Schema,
+    validate,
+    validates_schema,
+    ValidationError,
+)
 from marshmallow.validate import Length, Range
 from marshmallow_union import Union
 
@@ -937,6 +945,42 @@ class ChartDataPostProcessingOperationSchema(Schema):
         },
     )
 
+    # Map post-processing operation -> its options schema, for operations that
+    # declare one. Operations without a dedicated schema are not structurally
+    # validated here.
+    _OPTIONS_SCHEMAS: dict[str, type[Schema]] = {
+        "aggregate": ChartDataAggregateOptionsSchema,
+        "rolling": ChartDataRollingOptionsSchema,
+        "select": ChartDataSelectOptionsSchema,
+        "sort": ChartDataSortOptionsSchema,
+        "contribution": ChartDataContributionOptionsSchema,
+        "prophet": ChartDataProphetOptionsSchema,
+        "boxplot": ChartDataBoxplotOptionsSchema,
+        "pivot": ChartDataPivotOptionsSchema,
+        "geohash_decode": ChartDataGeohashDecodeOptionsSchema,
+        "geohash_encode": ChartDataGeohashEncodeOptionsSchema,
+        "geodetic_parse": ChartDataGeodeticParseOptionsSchema,
+    }
+
+    @validates_schema
+    def validate_options(self, data: dict[str, Any], **kwargs: Any) -> None:
+        """Validate ``options`` against the operation's option schema.
+
+        Validation is lenient (unknown keys are ignored) so it surfaces wrong
+        types / out-of-range values on declared fields without rejecting
+        payloads that carry extra keys.
+        """
+        operation = data.get("operation")
+        options = data.get("options")
+        if not isinstance(operation, str) or not isinstance(options, dict):
+            return
+        schema_cls = self._OPTIONS_SCHEMAS.get(operation)
+        if schema_cls is None:
+            return
+        errors = schema_cls(unknown=EXCLUDE).validate(options)
+        if errors:
+            raise ValidationError({"options": errors})
+
 
 class ChartDataFilterSchema(Schema):
     col = fields.Raw(

superset/mcp_service/dashboard/schemas.py

@@ -480,17 +480,7 @@ class AddChartToDashboardRequest(BaseModel):
     )
     chart_id: int = Field(..., description="ID of the chart to add to the dashboard")
     target_tab: str | None = Field(
-        None,
-        min_length=1,
-        description=(
-            "Tab to place the chart in. Accepts a tab display name "
-            "(e.g. 'Sales') or a tab component ID (e.g. 'TAB-abc123'). "
-            "Display-name matching is case-insensitive and strips all emoji; "
-            "component ID matching is case-sensitive and exact. "
-            "When not found, the error response lists all available tab names. "
-            "When omitted on a tabbed dashboard the chart is placed in the "
-            "first tab."
-        ),
+        None, description="Target tab name (if dashboard has tabs)"
     )
 
 

superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py

@@ -149,46 +149,29 @@ def _first_tab_from_groups(
     return None
 
 
-def _collect_available_tab_names(layout: Dict[str, Any]) -> list[str]:
-    """Collect display names (or IDs) of all TAB components in the layout."""
-    names: 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", "")
-            names.append(text if text else tab_id)
-    return names
-
-
 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.
+    If *target_tab* is provided the function first tries to match it against
+    tab ``meta.text`` (display name) or the raw component ID.  When no match
+    is found (or *target_tab* is ``None``) the first ``TAB`` child is used as
+    a fallback so that new rows are still placed inside the tab structure
+    rather than directly under ``GRID_ID``.
 
     Returns:
-        The ID of the matched (or first) TAB component, or ``None``.
+        The ID of the matched (or first) TAB component, or ``None`` if the
+        dashboard does not use top-level tabs.
     """
     groups = _collect_tabs_groups(layout)
 
-    if target_tab is not None:
+    if target_tab:
         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)
 
@@ -333,45 +316,6 @@ def _ensure_layout_structure(
         layout["DASHBOARD_VERSION_KEY"] = "v2"
 
 
-def _resolve_parent_container(
-    layout: Dict[str, Any],
-    dashboard_id: int,
-    target_tab: str | None,
-) -> tuple[str, None] | tuple[None, AddChartToDashboardResponse]:
-    """Return (parent_id, None) on success or (None, error_response) on mismatch.
-
-    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)
-
-    if target_tab is not None and tab_target is None:
-        available = _collect_available_tab_names(layout)
-        if available:
-            tab_list = ", ".join(f"'{t}'" for t in available)
-            return None, AddChartToDashboardResponse(
-                dashboard=None,
-                dashboard_url=None,
-                position=None,
-                error=(
-                    f"Tab '{target_tab}' not found in dashboard {dashboard_id}. "
-                    f"Available tabs: {tab_list}."
-                ),
-            )
-        return None, AddChartToDashboardResponse(
-            dashboard=None,
-            dashboard_url=None,
-            position=None,
-            error=(
-                f"Dashboard {dashboard_id} has no tabs. "
-                "Remove the target_tab parameter to add the chart to "
-                "the default grid layout."
-            ),
-        )
-
-    return tab_target if tab_target else "GRID_ID", None
-
-
 def _find_and_authorize_dashboard(
     dashboard_id: int,
 ) -> tuple[Any, AddChartToDashboardResponse | None]:
@@ -425,7 +369,7 @@ def _find_and_authorize_dashboard(
         destructiveHint=False,
     ),
 )
-def add_chart_to_existing_dashboard(  # noqa: C901
+def add_chart_to_existing_dashboard(
     request: AddChartToDashboardRequest, ctx: Context
 ) -> AddChartToDashboardResponse:
     """
@@ -499,16 +443,11 @@ def add_chart_to_existing_dashboard(  # noqa: C901
             # Generate a unique ROW ID for the new row
             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(
-                current_layout, request.dashboard_id, request.target_tab
+            # Detect tabbed dashboards and resolve target_tab by name or ID
+            tab_target = _find_tab_insert_target(
+                current_layout, target_tab=request.target_tab
             )
-            if tab_error is not None:
-                return tab_error
-            if parent_id is None:
-                raise RuntimeError(
-                    "unreachable: tab_error is None implies parent_id is str"
-                )
+            parent_id = tab_target if tab_target else "GRID_ID"
 
             # Add chart, column, and row to layout
             chart_key, column_key, row_key = _add_chart_to_layout(

tests/unit_tests/charts/test_schemas.py

@@ -152,3 +152,53 @@ def test_time_grain_validation_with_config_addons(app_context: None) -> None:
     }
     result = schema.load(custom_data)
     assert result["time_grain"] == "PT10M"
+
+
+def test_post_processing_operation_validates_options(app_context: None) -> None:
+    """options are validated against the operation's option schema (leniently)."""
+    from superset.charts.schemas import ChartDataPostProcessingOperationSchema
+
+    schema = ChartDataPostProcessingOperationSchema()
+
+    # Valid prophet options load.
+    schema.load(
+        {
+            "operation": "prophet",
+            "options": {
+                "time_grain": "P1D",
+                "periods": 7,
+                "confidence_interval": 0.8,
+            },
+        }
+    )
+
+    # Out-of-range confidence_interval (must be 0-1) on a declared field is
+    # rejected.
+    with pytest.raises(ValidationError) as exc_info:
+        schema.load(
+            {
+                "operation": "prophet",
+                "options": {
+                    "time_grain": "P1D",
+                    "periods": 7,
+                    "confidence_interval": 2.0,
+                },
+            }
+        )
+    assert "options" in exc_info.value.messages
+
+    # Extra/unknown keys are tolerated (lenient validation).
+    schema.load(
+        {
+            "operation": "prophet",
+            "options": {
+                "time_grain": "P1D",
+                "periods": 7,
+                "confidence_interval": 0.8,
+                "some_future_option": True,
+            },
+        }
+    )
+
+    # An operation without a dedicated schema accepts arbitrary options.
+    schema.load({"operation": "flatten", "options": {"anything": [1, 2, 3]}})

tests/unit_tests/mcp_service/dashboard/tool/test_add_chart_to_existing_dashboard.py

@@ -49,7 +49,7 @@ logger = logging.getLogger(__name__)
 # ---------------------------------------------------------------------------
 
 
-@pytest.fixture()
+@pytest.fixture
 def mcp_server() -> object:
     """Return the FastMCP app instance for use in MCP client tests."""
     return mcp
@@ -138,7 +138,7 @@ def _mock_dashboard(
 
 
 @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
-@pytest.mark.asyncio()
+@pytest.mark.asyncio
 async def test_dashboard_not_found(mock_find_by_id: Mock, mcp_server: object) -> None:
     """Returns a clear error when the target dashboard does not exist."""
     mock_find_by_id.return_value = None
@@ -157,7 +157,7 @@ async def test_dashboard_not_found(mock_find_by_id: Mock, mcp_server: object) ->
 
 @patch("superset.security_manager.raise_for_ownership")
 @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
-@pytest.mark.asyncio()
+@pytest.mark.asyncio
 async def test_permission_denied(
     mock_find_by_id: Mock, mock_raise_for_ownership: Mock, mcp_server: object
 ) -> None:
@@ -203,7 +203,7 @@ async def test_permission_denied(
 @patch("superset.db.session.get")
 @patch("superset.security_manager.raise_for_ownership")
 @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
-@pytest.mark.asyncio()
+@pytest.mark.asyncio
 async def test_chart_not_found(
     mock_find_by_id: Mock,
     mock_raise_for_ownership: Mock,
@@ -231,7 +231,7 @@ async def test_chart_not_found(
 @patch("superset.db.session.get")
 @patch("superset.security_manager.raise_for_ownership")
 @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
-@pytest.mark.asyncio()
+@pytest.mark.asyncio
 async def test_chart_already_in_dashboard(
     mock_find_by_id: Mock,
     mock_raise_for_ownership: Mock,
@@ -261,7 +261,7 @@ async def test_chart_already_in_dashboard(
 @patch("superset.db.session.get")
 @patch("superset.security_manager.raise_for_ownership")
 @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
-@pytest.mark.asyncio()
+@pytest.mark.asyncio
 async def test_successful_add(
     mock_find_by_id: Mock,
     mock_raise_for_ownership: Mock,
@@ -295,17 +295,3 @@ async def test_successful_add(
     assert "/superset/dashboard/1/" in content["dashboard_url"]
     assert content["position"] is not None
     assert "chart_key" in content["position"]
-
-
-def test_empty_target_tab_rejected_by_schema() -> None:
-    """Empty string target_tab is rejected at schema layer, not as 'Tab not found'."""
-    from pydantic import ValidationError
-
-    from superset.mcp_service.dashboard.schemas import AddChartToDashboardRequest
-
-    with pytest.raises(ValidationError):
-        AddChartToDashboardRequest(dashboard_id=1, chart_id=10, target_tab="")
-
-    # None is valid (tab omitted)
-    req = AddChartToDashboardRequest(dashboard_id=1, chart_id=10, target_tab=None)
-    assert req.target_tab is None

tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py

@@ -32,7 +32,6 @@ from superset.mcp_service.chart.chart_utils import DatasetValidationResult
 from superset.mcp_service.dashboard.constants import generate_id
 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,
@@ -1060,110 +1059,6 @@ class TestAddChartToExistingDashboard:
             assert "TAB-tab2" in chart_parents
             assert "TAB-tab1" not in chart_parents
 
-    @patch("superset.commands.dashboard.update.UpdateDashboardCommand")
-    @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
-    @patch("superset.db.session")
-    @pytest.mark.asyncio
-    async def test_add_chart_target_tab_not_found(
-        self, mock_db_session, mock_find_dashboard, mock_update_command, mcp_server
-    ):
-        """target_tab specified but no matching tab → descriptive error listing
-        available tabs, not a silent fallback to the first tab."""
-        mock_dashboard = _mock_dashboard(id=3, title="Tabbed Dashboard")
-        mock_dashboard.slices = [_mock_chart(id=10)]
-        mock_dashboard.position_json = json.dumps(
-            {
-                "ROOT_ID": {"children": ["GRID_ID"], "id": "ROOT_ID", "type": "ROOT"},
-                "GRID_ID": {
-                    "children": ["TABS-abc123"],
-                    "id": "GRID_ID",
-                    "parents": ["ROOT_ID"],
-                    "type": "GRID",
-                },
-                "TABS-abc123": {
-                    "children": ["TAB-tab1", "TAB-tab2"],
-                    "id": "TABS-abc123",
-                    "parents": ["ROOT_ID", "GRID_ID"],
-                    "type": "TABS",
-                },
-                "TAB-tab1": {
-                    "children": [],
-                    "id": "TAB-tab1",
-                    "meta": {"text": "Overview"},
-                    "parents": ["ROOT_ID", "GRID_ID", "TABS-abc123"],
-                    "type": "TAB",
-                },
-                "TAB-tab2": {
-                    "children": [],
-                    "id": "TAB-tab2",
-                    "meta": {"text": "Details"},
-                    "parents": ["ROOT_ID", "GRID_ID", "TABS-abc123"],
-                    "type": "TAB",
-                },
-                "DASHBOARD_VERSION_KEY": "v2",
-            }
-        )
-        mock_chart = _mock_chart(id=30)
-        mock_db_session.get.return_value = mock_chart
-        mock_find_dashboard.return_value = mock_dashboard
-
-        request = {"dashboard_id": 3, "chart_id": 30, "target_tab": "Nonexistent Tab"}
-
-        async with Client(mcp_server) as client:
-            result = await client.call_tool(
-                "add_chart_to_existing_dashboard", {"request": request}
-            )
-
-            assert result.structured_content["error"] is not None
-            error = result.structured_content["error"]
-            assert "Nonexistent Tab" in error
-            assert "not found" in error
-            # Available tabs should be listed
-            assert "Overview" in error
-            assert "Details" in error
-            # No layout mutation should have been persisted
-            mock_update_command.assert_not_called()
-
-    @patch("superset.commands.dashboard.update.UpdateDashboardCommand")
-    @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
-    @patch("superset.db.session")
-    @pytest.mark.asyncio
-    async def test_add_chart_target_tab_on_non_tabbed_dashboard(
-        self, mock_db_session, mock_find_dashboard, mock_update_command, mcp_server
-    ):
-        """target_tab on a dashboard with no tabs → descriptive error."""
-        mock_dashboard = _mock_dashboard(id=5, title="Flat Dashboard")
-        mock_dashboard.slices = []
-        mock_dashboard.position_json = json.dumps(
-            {
-                "ROOT_ID": {"children": ["GRID_ID"], "id": "ROOT_ID", "type": "ROOT"},
-                "GRID_ID": {
-                    "children": [],
-                    "id": "GRID_ID",
-                    "parents": ["ROOT_ID"],
-                    "type": "GRID",
-                },
-                "DASHBOARD_VERSION_KEY": "v2",
-            }
-        )
-        mock_chart = _mock_chart(id=99)
-        mock_db_session.get.return_value = mock_chart
-        mock_find_dashboard.return_value = mock_dashboard
-
-        request = {"dashboard_id": 5, "chart_id": 99, "target_tab": "Sales"}
-
-        async with Client(mcp_server) as client:
-            result = await client.call_tool(
-                "add_chart_to_existing_dashboard", {"request": request}
-            )
-
-            assert result.structured_content["error"] is not None
-            error = result.structured_content["error"]
-            assert "no tabs" in error.lower()
-            assert "target_tab" in error
-            # No layout mutation should have been persisted
-            mock_update_command.assert_not_called()
-
     @patch("superset.commands.dashboard.update.UpdateDashboardCommand")
     @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
     @patch("superset.db.session")
@@ -1416,9 +1311,9 @@ class TestLayoutHelpers:
         }
         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
-        match any tab name or ID, so the caller can return a descriptive error."""
+    def test_find_tab_insert_target_unmatched_falls_back_to_first(self):
+        """Test _find_tab_insert_target falls back to first tab when target_tab
+        doesn't match any tab name or ID."""
         layout = {
             "GRID_ID": {"children": ["TABS-main"], "type": "GRID"},
             "TABS-main": {"children": ["TAB-first", "TAB-second"], "type": "TABS"},
@@ -1433,17 +1328,9 @@ class TestLayoutHelpers:
                 "meta": {"text": "Tab 2"},
             },
         }
-        assert _find_tab_insert_target(layout, target_tab="Nonexistent Tab") is None
-
-    def test_find_tab_insert_target_empty_string_returns_none(self):
-        """An empty-string target_tab is treated as specified-but-not-found,
-        not as 'no tab requested', so it returns None rather than first tab."""
-        layout = {
-            "GRID_ID": {"children": ["TABS-main"], "type": "GRID"},
-            "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="Nonexistent Tab") == "TAB-first"
+        )
 
     def test_find_tab_insert_target_tabs_under_root(self):
         """Test _find_tab_insert_target when TABS are under ROOT_ID (real layout)."""
@@ -1471,35 +1358,6 @@ class TestLayoutHelpers:
         """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):
-        """_collect_available_tab_names returns each tab's display text."""
-        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)
-        assert names == ["Overview", "Details"]
-
-    def test_collect_available_tab_names_falls_back_to_id(self):
-        """_collect_available_tab_names uses component ID 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)
-        assert names == ["TAB-a"]
-
-    def test_collect_available_tab_names_no_tabs(self):
-        """_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) == []
-
     def test_add_chart_to_layout_creates_column(self):
         """Test that _add_chart_to_layout creates ROW > COLUMN > CHART."""
         layout = {