You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by jl...@apache.org on 2026/04/10 20:45:50 UTC
(superset) 01/01: fix(mcp): handle all chart types in column normalization and reject adhoc_filters
This is an automated email from the ASF dual-hosted git repository.
jli pushed a commit to branch sc-103356-mcp-adhoc-filters-fix
in repository https://gitbox.apache.org/repos/asf/superset.git
commit f10fed78eb65d25d8efde1807e1e3210abcc418e
Author: sadpandajoe <jc...@gmail.com>
AuthorDate: Fri Apr 10 20:45:19 2026 +0000
fix(mcp): handle all chart types in column normalization and reject adhoc_filters
Two fixes for sc-103356:
1. normalize_column_names() previously fell through to
TableChartConfig.model_validate() for non-XY/Table chart types
(pie, pivot_table, mixed_timeseries, etc.), which raised a Pydantic
ValidationError due to discriminator mismatch. That exception wasn't
caught by the narrow except-tuple in the pipeline, surfacing as an
opaque validation_system_error. Now each chart type is reconstructed
with its own model, and the except clause catches all exceptions.
2. When adhoc_filters is passed in config, it was silently dropped by
Pydantic's extra="ignore". Now schema_validator detects it early and
returns a clear error with guidance to use the 'filters' field instead.
Co-Authored-By: Claude Opus 4.6 <no...@anthropic.com>
---
.../chart/validation/dataset_validator.py | 35 +++++-
superset/mcp_service/chart/validation/pipeline.py | 10 +-
.../chart/validation/schema_validator.py | 28 +++++
.../mcp_service/chart/test_new_chart_types.py | 114 ++++++++++++++++++
.../validation/test_column_name_normalization.py | 127 +++++++++++++++++++++
5 files changed, 305 insertions(+), 9 deletions(-)
diff --git a/superset/mcp_service/chart/validation/dataset_validator.py b/superset/mcp_service/chart/validation/dataset_validator.py
index 202d7cc5afc..e1b777f5b3b 100644
--- a/superset/mcp_service/chart/validation/dataset_validator.py
+++ b/superset/mcp_service/chart/validation/dataset_validator.py
@@ -25,7 +25,13 @@ import logging
from typing import Any, Dict, List, Tuple
from superset.mcp_service.chart.schemas import (
+ BigNumberChartConfig,
+ ChartConfig,
ColumnRef,
+ HandlebarsChartConfig,
+ MixedTimeseriesChartConfig,
+ PieChartConfig,
+ PivotTableChartConfig,
TableChartConfig,
XYChartConfig,
)
@@ -319,10 +325,10 @@ class DatasetValidator:
@staticmethod
def normalize_column_names(
- config: TableChartConfig | XYChartConfig,
+ config: ChartConfig,
dataset_id: int | str,
dataset_context: DatasetContext | None = None,
- ) -> TableChartConfig | XYChartConfig:
+ ) -> ChartConfig:
"""
Normalize column names in config to match the canonical dataset column names.
@@ -354,13 +360,32 @@ class DatasetValidator:
elif isinstance(config, TableChartConfig):
DatasetValidator._normalize_table_config(config_dict, dataset_context)
- # Normalize filter columns (common to both config types)
+ # Normalize filter columns (common to all config types)
DatasetValidator._normalize_filters(config_dict, dataset_context)
- # Reconstruct the config with normalized names
+ # Reconstruct the config with normalized names using the correct type.
+ # Previously this fell through to TableChartConfig.model_validate() for
+ # non-XY/Table types (pie, pivot_table, mixed_timeseries), which raised
+ # a Pydantic ValidationError because the discriminator value didn't match
+ # "table". That exception was not caught by the narrow except-tuple in
+ # ValidationPipeline._normalize_column_names and surfaced to users as an
+ # opaque validation_system_error.
if isinstance(config, XYChartConfig):
return XYChartConfig.model_validate(config_dict)
- return TableChartConfig.model_validate(config_dict)
+ if isinstance(config, TableChartConfig):
+ return TableChartConfig.model_validate(config_dict)
+ if isinstance(config, PieChartConfig):
+ return PieChartConfig.model_validate(config_dict)
+ if isinstance(config, PivotTableChartConfig):
+ return PivotTableChartConfig.model_validate(config_dict)
+ if isinstance(config, MixedTimeseriesChartConfig):
+ return MixedTimeseriesChartConfig.model_validate(config_dict)
+ if isinstance(config, HandlebarsChartConfig):
+ return HandlebarsChartConfig.model_validate(config_dict)
+ if isinstance(config, BigNumberChartConfig):
+ return BigNumberChartConfig.model_validate(config_dict)
+ # Unknown type — return original config unchanged
+ return config
@staticmethod
def _get_column_suggestions(
diff --git a/superset/mcp_service/chart/validation/pipeline.py b/superset/mcp_service/chart/validation/pipeline.py
index 6923de0bd0b..b864e5787cc 100644
--- a/superset/mcp_service/chart/validation/pipeline.py
+++ b/superset/mcp_service/chart/validation/pipeline.py
@@ -325,10 +325,12 @@ class ValidationPipeline:
return GenerateChartRequest.model_validate(request_dict)
- except (ImportError, AttributeError, KeyError, ValueError, TypeError) as e:
- # If normalization fails, return the original request
- # Validation has already passed, so this is a non-critical failure
- logger.warning("Column name normalization failed: %s", e)
+ except Exception: # noqa: BLE001
+ # If normalization fails, return the original request.
+ # Validation has already passed, so this is a non-critical failure.
+ # Use broad except because Pydantic v2 ValidationError does not
+ # inherit from ValueError, so the previous narrow tuple missed it.
+ logger.exception("Column name normalization failed unexpectedly")
return request
@staticmethod
diff --git a/superset/mcp_service/chart/validation/schema_validator.py b/superset/mcp_service/chart/validation/schema_validator.py
index c82728b11bd..021c8e41ea3 100644
--- a/superset/mcp_service/chart/validation/schema_validator.py
+++ b/superset/mcp_service/chart/validation/schema_validator.py
@@ -120,6 +120,34 @@ class SchemaValidator:
error_code="INVALID_CONFIG_FORMAT",
)
+ # Check for adhoc_filters — Superset's native format is not supported.
+ # Without this check, the field is silently dropped by Pydantic's
+ # extra="ignore" and filters are never applied, giving the caller no
+ # indication that something went wrong.
+ if "adhoc_filters" in config:
+ return False, ChartGenerationError(
+ error_type="unsupported_adhoc_filters",
+ message="'adhoc_filters' is not supported in generate_chart",
+ details=(
+ "The 'adhoc_filters' field uses Superset's internal format and "
+ "is not accepted by the generate_chart tool. Use the 'filters' "
+ "field instead, which takes a simplified list of column/op/value "
+ "objects."
+ ),
+ suggestions=[
+ "Replace 'adhoc_filters' with 'filters' in the config",
+ "Format: 'filters': [{'column': 'col_name', 'op': '=', "
+ "'value': 'some_value'}]",
+ "Supported operators: =, >, <, >=, <=, !=, LIKE, ILIKE, "
+ "NOT LIKE, IN, NOT IN",
+ "Example: 'filters': [{'column': 'region', 'op': '=', "
+ "'value': 'US'}]",
+ "For multiple values use IN: 'filters': [{'column': 'status', "
+ "'op': 'IN', 'value': ['active', 'pending']}]",
+ ],
+ error_code="UNSUPPORTED_ADHOC_FILTERS",
+ )
+
# Check chart_type early
chart_type = config.get("chart_type")
if not chart_type:
diff --git a/tests/unit_tests/mcp_service/chart/test_new_chart_types.py b/tests/unit_tests/mcp_service/chart/test_new_chart_types.py
index 9469f63b39a..d56eddbe576 100644
--- a/tests/unit_tests/mcp_service/chart/test_new_chart_types.py
+++ b/tests/unit_tests/mcp_service/chart/test_new_chart_types.py
@@ -22,6 +22,7 @@ Tests cover schema validation, form_data mapping, chart name generation,
and schema validator pre-validation for all three new chart types.
"""
+from typing import Any
from unittest.mock import patch
import pytest
@@ -975,3 +976,116 @@ class TestSchemaValidatorNewTypes:
assert is_valid is False
assert error is not None
assert error.error_code == "INVALID_CHART_TYPE"
+
+
+# ============================================================
+# adhoc_filters detection tests (sc-103356)
+# ============================================================
+
+
+class TestAdhocFiltersDetection:
+ """Tests for sc-103356: generate_chart should surface a clear error when
+ adhoc_filters is passed in the config instead of silently dropping it."""
+
+ @pytest.mark.parametrize(
+ "chart_type,extra_fields",
+ [
+ (
+ "xy",
+ {"x": {"name": "date"}, "y": [{"name": "rev", "aggregate": "SUM"}]},
+ ),
+ (
+ "table",
+ {"columns": [{"name": "product"}]},
+ ),
+ (
+ "pie",
+ {
+ "dimension": {"name": "region"},
+ "metric": {"name": "rev", "aggregate": "SUM"},
+ },
+ ),
+ (
+ "pivot_table",
+ {
+ "rows": [{"name": "region"}],
+ "metrics": [{"name": "rev", "aggregate": "SUM"}],
+ },
+ ),
+ (
+ "mixed_timeseries",
+ {
+ "x": {"name": "date"},
+ "y": [{"name": "rev", "aggregate": "SUM"}],
+ "y_secondary": [{"name": "cnt", "aggregate": "COUNT"}],
+ },
+ ),
+ ],
+ )
+ def test_adhoc_filters_in_config_returns_clear_error(
+ self, chart_type: str, extra_fields: dict[str, Any]
+ ) -> None:
+ """adhoc_filters in config must be rejected with UNSUPPORTED_ADHOC_FILTERS."""
+ config = {
+ "chart_type": chart_type,
+ **extra_fields,
+ "adhoc_filters": [
+ {
+ "expressionType": "SIMPLE",
+ "subject": "region",
+ "operator": "==",
+ "comparator": "US",
+ "clause": "WHERE",
+ }
+ ],
+ }
+ data = {"dataset_id": 1, "config": config}
+ is_valid, _, error = SchemaValidator.validate_request(data)
+
+ assert is_valid is False
+ assert error is not None
+ assert error.error_code == "UNSUPPORTED_ADHOC_FILTERS"
+ assert "adhoc_filters" in error.message
+ assert "filters" in (error.details or "")
+ # Suggestions must guide callers towards the correct field
+ assert error.suggestions is not None
+ assert any("filters" in s for s in error.suggestions)
+
+ def test_adhoc_filters_empty_list_still_rejected(self) -> None:
+ """An empty adhoc_filters list should also be rejected."""
+ data = {
+ "dataset_id": 1,
+ "config": {
+ "chart_type": "xy",
+ "x": {"name": "date"},
+ "y": [{"name": "rev", "aggregate": "SUM"}],
+ "adhoc_filters": [],
+ },
+ }
+ is_valid, _, error = SchemaValidator.validate_request(data)
+
+ assert is_valid is False
+ assert error is not None
+ assert error.error_code == "UNSUPPORTED_ADHOC_FILTERS"
+
+ def test_filters_field_still_accepted(self) -> None:
+ """The proper 'filters' field must continue to work after the fix."""
+ data = {
+ "dataset_id": 1,
+ "config": {
+ "chart_type": "xy",
+ "x": {"name": "date"},
+ "y": [{"name": "rev", "aggregate": "SUM"}],
+ "filters": [{"column": "region", "op": "=", "value": "US"}],
+ },
+ }
+ is_valid, request, error = SchemaValidator.validate_request(data)
+
+ assert is_valid is True
+ assert error is None
+ assert request is not None
+ # config is stored as a raw dict in GenerateChartRequest
+ assert isinstance(request.config, dict)
+ assert request.config.get("filters") is not None
+ assert len(request.config["filters"]) == 1
+ assert request.config["filters"][0]["column"] == "region"
diff --git a/tests/unit_tests/mcp_service/chart/validation/test_column_name_normalization.py b/tests/unit_tests/mcp_service/chart/validation/test_column_name_normalization.py
index ab663ebc811..ded43ca3dc1 100644
--- a/tests/unit_tests/mcp_service/chart/validation/test_column_name_normalization.py
+++ b/tests/unit_tests/mcp_service/chart/validation/test_column_name_normalization.py
@@ -30,6 +30,9 @@ import pytest
from superset.mcp_service.chart.schemas import (
ColumnRef,
FilterConfig,
+ MixedTimeseriesChartConfig,
+ PieChartConfig,
+ PivotTableChartConfig,
TableChartConfig,
XYChartConfig,
)
@@ -259,6 +262,7 @@ class TestNormalizeColumnNames:
normalized = DatasetValidator.normalize_column_names(config, dataset_id=18)
+ assert isinstance(normalized, TableChartConfig)
assert normalized.columns[0].name == "OrderDate"
assert normalized.columns[1].name == "ProductLine"
assert normalized.columns[2].name == "Sales"
@@ -729,3 +733,126 @@ class TestValidateSavedMetrics:
assert not is_valid
assert error is not None
assert error.error_code == "INVALID_SAVED_METRIC"
+
+
+# ============================================================
+# Tests for non-XY/Table chart type normalization (sc-103356)
+# ============================================================
+
+
+@pytest.fixture
+def pie_dataset_context() -> DatasetContext:
+ return DatasetContext(
+ id=99,
+ table_name="orders",
+ schema="public",
+ database_name="examples",
+ available_columns=[
+ {"name": "Region", "type": "VARCHAR", "is_temporal": False},
+ {"name": "Revenue", "type": "DECIMAL", "is_numeric": True},
+ {"name": "Category", "type": "VARCHAR", "is_temporal": False},
+ ],
+ available_metrics=[],
+ )
+
+
+class TestNormalizeNonXYTableChartTypes:
+ """Tests for sc-103356: normalize_column_names used to crash with an
+ uncaught Pydantic ValidationError for PieChartConfig, PivotTableChartConfig,
+ and MixedTimeseriesChartConfig because the fallback path always called
+ TableChartConfig.model_validate(), which rejected the mismatched discriminator."""
+
+ @patch.object(DatasetValidator, "_get_dataset_context")
+ def test_pie_chart_config_normalizes_without_error(
+ self, mock_get_context, pie_dataset_context: DatasetContext
+ ) -> None:
+ """PieChartConfig must survive normalize_column_names unchanged."""
+ mock_get_context.return_value = pie_dataset_context
+
+ config = PieChartConfig(
+ chart_type="pie",
+ dimension=ColumnRef(name="region"),
+ metric=ColumnRef(name="revenue", aggregate="SUM"),
+ )
+
+ # Before the fix this raised pydantic.ValidationError (not caught by
+ # the narrow except-tuple), which bubbled up as validation_system_error.
+ normalized = DatasetValidator.normalize_column_names(config, dataset_id=99)
+
+ assert isinstance(normalized, PieChartConfig)
+ assert normalized.chart_type == "pie"
+
+ @patch.object(DatasetValidator, "_get_dataset_context")
+ def test_pivot_table_config_normalizes_without_error(
+ self, mock_get_context, pie_dataset_context: DatasetContext
+ ) -> None:
+ """PivotTableChartConfig must survive normalize_column_names unchanged."""
+ mock_get_context.return_value = pie_dataset_context
+
+ config = PivotTableChartConfig(
+ chart_type="pivot_table",
+ rows=[ColumnRef(name="region")],
+ metrics=[ColumnRef(name="revenue", aggregate="SUM")],
+ )
+
+ normalized = DatasetValidator.normalize_column_names(config, dataset_id=99)
+
+ assert isinstance(normalized, PivotTableChartConfig)
+ assert normalized.chart_type == "pivot_table"
+
+ @patch.object(DatasetValidator, "_get_dataset_context")
+ def test_mixed_timeseries_config_normalizes_without_error(
+ self, mock_get_context, pie_dataset_context: DatasetContext
+ ) -> None:
+ """MixedTimeseriesChartConfig must survive normalize_column_names unchanged."""
+ mock_get_context.return_value = pie_dataset_context
+
+ config = MixedTimeseriesChartConfig(
+ chart_type="mixed_timeseries",
+ x=ColumnRef(name="region"),
+ y=[ColumnRef(name="revenue", aggregate="SUM")],
+ y_secondary=[ColumnRef(name="revenue", aggregate="COUNT")],
+ )
+
+ normalized = DatasetValidator.normalize_column_names(config, dataset_id=99)
+
+ assert isinstance(normalized, MixedTimeseriesChartConfig)
+ assert normalized.chart_type == "mixed_timeseries"
+
+ @patch.object(DatasetValidator, "_get_dataset_context")
+ def test_pie_chart_filter_columns_normalized(
+ self, mock_get_context, pie_dataset_context: DatasetContext
+ ) -> None:
+ """Filters on a PieChartConfig should have their column names normalized."""
+ mock_get_context.return_value = pie_dataset_context
+
+ config = PieChartConfig(
+ chart_type="pie",
+ dimension=ColumnRef(name="region"),
+ metric=ColumnRef(name="revenue", aggregate="SUM"),
+ filters=[FilterConfig(column="category", op="=", value="Electronics")],
+ )
+
+ normalized = DatasetValidator.normalize_column_names(config, dataset_id=99)
+
+ assert isinstance(normalized, PieChartConfig)
+ assert normalized.filters is not None
+ # "category" matches "Category" case-insensitively -> normalized to "Category"
+ assert normalized.filters[0].column == "Category"
+
+ @patch.object(DatasetValidator, "_get_dataset_context")
+ def test_pie_no_dataset_context_returns_original(
+ self, mock_get_context
+ ) -> None:
+ """When dataset context is unavailable the original config is returned."""
+ mock_get_context.return_value = None
+
+ config = PieChartConfig(
+ chart_type="pie",
+ dimension=ColumnRef(name="region"),
+ metric=ColumnRef(name="revenue", aggregate="SUM"),
+ )
+
+ result = DatasetValidator.normalize_column_names(config, dataset_id=99)
+
+ assert result is config