fix(mcp): detect unknown chart config fields and suggest correct ones (#38848)

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

tests/unit_tests/mcp_service/chart/test_chart_schemas.py

@@ -235,17 +235,16 @@ class TestXYChartConfig:
         )
         assert config.kind == "area"
 
-    def test_unknown_fields_ignored(self) -> None:
-        """Test that unknown fields are silently ignored (extra='ignore')."""
-        config = XYChartConfig(
-            chart_type="xy",
-            x=ColumnRef(name="territory"),
-            y=[ColumnRef(name="sales", aggregate="SUM")],
-            kind="bar",
-            unknown_field="bad",
-        )
-        assert config.kind == "bar"
-        assert not hasattr(config, "unknown_field")
+    def test_unknown_fields_raise_error(self) -> None:
+        """Test that unknown fields raise ValueError with suggestions."""
+        with pytest.raises(ValidationError, match="Unknown field"):
+            XYChartConfig(
+                chart_type="xy",
+                x=ColumnRef(name="territory"),
+                y=[ColumnRef(name="sales", aggregate="SUM")],
+                kind="bar",
+                unknown_field="bad",
+            )
 
     def test_series_alias_accepted(self) -> None:
         """Test that 'series' is accepted as alias for 'group_by'."""
@@ -430,12 +429,144 @@ class TestRowLimit:
 class TestTableChartConfigExtraFields:
     """Test TableChartConfig rejects unknown fields."""
 
-    def test_unknown_fields_ignored(self) -> None:
-        """Test that unknown fields are silently ignored (extra='ignore')."""
-        config = TableChartConfig(
-            chart_type="table",
-            columns=[ColumnRef(name="product")],
-            foo="bar",
+    def test_unknown_fields_raise_error(self) -> None:
+        """Test that unknown fields raise ValueError with valid field list."""
+        with pytest.raises(ValidationError, match="Unknown field 'foo'"):
+            TableChartConfig(
+                chart_type="table",
+                columns=[ColumnRef(name="product")],
+                foo="bar",
+            )
+
+
+class TestAliasChoices:
+    """Test that common Superset form_data aliases are accepted."""
+
+    def test_xy_stack_alias_for_stacked(self) -> None:
+        """Test that 'stack' is accepted as alias for 'stacked'."""
+        config = XYChartConfig.model_validate(
+            {
+                "chart_type": "xy",
+                "x": {"name": "category"},
+                "y": [{"name": "sales", "aggregate": "SUM"}],
+                "stack": True,
+            }
         )
-        assert len(config.columns) == 1
-        assert not hasattr(config, "foo")
+        assert config.stacked is True
+
+    def test_xy_stacked_still_works(self) -> None:
+        """Test that 'stacked' still works as primary field name."""
+        config = XYChartConfig(
+            chart_type="xy",
+            x=ColumnRef(name="category"),
+            y=[ColumnRef(name="sales", aggregate="SUM")],
+            stacked=True,
+        )
+        assert config.stacked is True
+
+    def test_xy_time_grain_sqla_alias(self) -> None:
+        """Test that 'time_grain_sqla' is accepted as alias for 'time_grain'."""
+        config = XYChartConfig.model_validate(
+            {
+                "chart_type": "xy",
+                "x": {"name": "order_date"},
+                "y": [{"name": "sales", "aggregate": "SUM"}],
+                "time_grain_sqla": "P1D",
+            }
+        )
+        assert config.time_grain is not None
+
+    def test_table_order_by_alias_for_sort_by(self) -> None:
+        """Test that 'order_by' is accepted as alias for 'sort_by'."""
+        config = TableChartConfig.model_validate(
+            {
+                "chart_type": "table",
+                "columns": [{"name": "product"}],
+                "order_by": ["product"],
+            }
+        )
+        assert config.sort_by == ["product"]
+
+    def test_mixed_timeseries_time_grain_sqla_alias(self) -> None:
+        """Test that 'time_grain_sqla' works for MixedTimeseriesChartConfig."""
+        from superset.mcp_service.chart.schemas import MixedTimeseriesChartConfig
+
+        config = MixedTimeseriesChartConfig.model_validate(
+            {
+                "chart_type": "mixed_timeseries",
+                "x": {"name": "order_date"},
+                "y": [{"name": "sales", "aggregate": "SUM"}],
+                "y_secondary": [{"name": "profit", "aggregate": "SUM"}],
+                "time_grain_sqla": "P1M",
+            }
+        )
+        assert config.time_grain is not None
+
+
+class TestUnknownFieldDetection:
+    """Test that unknown fields produce helpful error messages."""
+
+    def test_near_miss_suggests_correct_field(self) -> None:
+        """Test that a near-miss field name produces 'did you mean?' suggestion."""
+        with pytest.raises(ValidationError, match="did you mean"):
+            XYChartConfig.model_validate(
+                {
+                    "chart_type": "xy",
+                    "x": {"name": "category"},
+                    "y": [{"name": "sales", "aggregate": "SUM"}],
+                    "stacks": True,
+                }
+            )
+
+    def test_completely_unknown_field_lists_valid_fields(self) -> None:
+        """Test that a completely unknown field lists valid fields."""
+        with pytest.raises(ValidationError, match="Valid fields:"):
+            XYChartConfig.model_validate(
+                {
+                    "chart_type": "xy",
+                    "x": {"name": "category"},
+                    "y": [{"name": "sales", "aggregate": "SUM"}],
+                    "zzz_nonexistent": True,
+                }
+            )
+
+    def test_pie_chart_unknown_field(self) -> None:
+        """Test unknown field detection on PieChartConfig."""
+        from superset.mcp_service.chart.schemas import PieChartConfig
+
+        with pytest.raises(ValidationError, match="Unknown field"):
+            PieChartConfig.model_validate(
+                {
+                    "chart_type": "pie",
+                    "dimension": {"name": "category"},
+                    "metric": {"name": "sales", "aggregate": "SUM"},
+                    "bad_field": True,
+                }
+            )
+
+    def test_table_chart_unknown_field(self) -> None:
+        """Test unknown field detection on TableChartConfig."""
+        with pytest.raises(ValidationError, match="Unknown field"):
+            TableChartConfig.model_validate(
+                {
+                    "chart_type": "table",
+                    "columns": [{"name": "product"}],
+                    "invalid_param": "test",
+                }
+            )
+
+    def test_known_aliases_not_flagged_as_unknown(self) -> None:
+        """Test that known aliases pass validation without errors."""
+        config = XYChartConfig.model_validate(
+            {
+                "chart_type": "xy",
+                "x_axis": {"name": "category"},
+                "metrics": [{"name": "sales", "aggregate": "SUM"}],
+                "groupby": [{"name": "region"}],
+                "stack": True,
+                "time_grain_sqla": "P1D",
+            }
+        )
+        assert config.stacked is True
+        assert config.row_limit == 10000
+        assert config.group_by is not None

tests/unit_tests/mcp_service/chart/test_handlebars_chart.py

@@ -119,7 +119,7 @@ class TestHandlebarsChartConfig:
             )
 
     def test_extra_fields_forbidden(self) -> None:
-        with pytest.raises(ValueError, match="Extra inputs"):
+        with pytest.raises(ValueError, match="Unknown field 'unknown_field'"):
             HandlebarsChartConfig(
                 chart_type="handlebars",
                 handlebars_template="<p>test</p>",

tests/unit_tests/mcp_service/chart/test_new_chart_types.py

@@ -103,15 +103,14 @@ class TestPieChartConfigSchema:
         assert config.filters is not None
         assert len(config.filters) == 1
 
-    def test_pie_config_ignores_extra_fields(self) -> None:
-        config = PieChartConfig(
-            chart_type="pie",
-            dimension=ColumnRef(name="product"),
-            metric=ColumnRef(name="revenue", aggregate="SUM"),
-            unknown_field="bad",
-        )
-        assert config.dimension.name == "product"
-        assert not hasattr(config, "unknown_field")
+    def test_pie_config_rejects_extra_fields(self) -> None:
+        with pytest.raises(ValidationError, match="Unknown field"):
+            PieChartConfig(
+                chart_type="pie",
+                dimension=ColumnRef(name="product"),
+                metric=ColumnRef(name="revenue", aggregate="SUM"),
+                unknown_field="bad",
+            )
 
     def test_pie_config_missing_dimension(self) -> None:
         with pytest.raises(ValidationError):
@@ -324,15 +323,14 @@ class TestPivotTableChartConfigSchema:
                 metrics=[ColumnRef(name="revenue", aggregate="SUM")],
             )
 
-    def test_pivot_table_ignores_extra_fields(self) -> None:
-        config = PivotTableChartConfig(
-            chart_type="pivot_table",
-            rows=[ColumnRef(name="product")],
-            metrics=[ColumnRef(name="revenue", aggregate="SUM")],
-            unknown_field="bad",
-        )
-        assert config.rows[0].name == "product"
-        assert not hasattr(config, "unknown_field")
+    def test_pivot_table_rejects_extra_fields(self) -> None:
+        with pytest.raises(ValidationError, match="Unknown field"):
+            PivotTableChartConfig(
+                chart_type="pivot_table",
+                rows=[ColumnRef(name="product")],
+                metrics=[ColumnRef(name="revenue", aggregate="SUM")],
+                unknown_field="bad",
+            )
 
     def test_pivot_table_valid_aggregate_functions(self) -> None:
         for agg in ["Sum", "Average", "Median", "Count", "Minimum", "Maximum"]:
@@ -504,16 +502,15 @@ class TestMixedTimeseriesChartConfigSchema:
                 y_secondary=[ColumnRef(name="orders", aggregate="COUNT")],
             )
 
-    def test_mixed_timeseries_ignores_extra_fields(self) -> None:
-        config = MixedTimeseriesChartConfig(
-            chart_type="mixed_timeseries",
-            x=ColumnRef(name="date"),
-            y=[ColumnRef(name="revenue", aggregate="SUM")],
-            y_secondary=[ColumnRef(name="orders", aggregate="COUNT")],
-            unknown_field="bad",
-        )
-        assert config.x.name == "date"
-        assert not hasattr(config, "unknown_field")
+    def test_mixed_timeseries_rejects_extra_fields(self) -> None:
+        with pytest.raises(ValidationError, match="Unknown field"):
+            MixedTimeseriesChartConfig(
+                chart_type="mixed_timeseries",
+                x=ColumnRef(name="date"),
+                y=[ColumnRef(name="revenue", aggregate="SUM")],
+                y_secondary=[ColumnRef(name="orders", aggregate="COUNT")],
+                unknown_field="bad",
+            )
 
     def test_mixed_timeseries_default_row_limit(self) -> None:
         config = MixedTimeseriesChartConfig(