From 3b7834510d052d0ea79f431c6184b29da2baab50 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Fri, 26 Jun 2026 15:00:57 +0000 Subject: [PATCH] fix(mcp): fix pre_validate aliases and sql_expression normalization Three pre_validate methods were checking only canonical field names, but the Pydantic schemas accept validation aliases. For example PieChartConfig.dimension accepts "groupby" as an alias, so sending {"groupby": ...} would pass the schema but be incorrectly rejected by pre_validate. Four normalize_column_refs implementations did not guard against sql_expression metrics (name=None), causing AttributeError when _get_canonical_column_name(None, ...) was called. XYChartPlugin already handled this correctly; the fix brings the other plugins in line. Pre-validate alias fixes: - TableChartPlugin: accept columns/all_columns/groupby (AliasChoices) - PieChartPlugin: accept dimension/groupby (AliasChoices) - PivotTableChartPlugin: accept rows/groupby/dimension (AliasChoices) sql_expression normalization guards: - BigNumberChartPlugin: skip metric normalization when sql_expression set - PieChartPlugin: same for metric field - MixedTimeseriesChartPlugin: skip in _norm_list helper - PivotTableChartPlugin: skip in _norm_col_list helper --- superset/mcp_service/chart/plugins/big_number.py | 4 +++- superset/mcp_service/chart/plugins/mixed_timeseries.py | 2 ++ superset/mcp_service/chart/plugins/pie.py | 6 ++++-- superset/mcp_service/chart/plugins/pivot_table.py | 9 +++++++-- superset/mcp_service/chart/plugins/table.py | 7 +++++-- 5 files changed, 21 insertions(+), 7 deletions(-) diff --git a/superset/mcp_service/chart/plugins/big_number.py b/superset/mcp_service/chart/plugins/big_number.py index 82618e86031..2e3931a0be9 100644 --- a/superset/mcp_service/chart/plugins/big_number.py +++ b/superset/mcp_service/chart/plugins/big_number.py @@ -208,7 +208,9 @@ class BigNumberChartPlugin(BaseChartPlugin): config_dict = config.model_dump() if config_dict.get("metric"): - if config_dict["metric"].get("saved_metric"): + if config_dict["metric"].get("sql_expression"): + pass + elif config_dict["metric"].get("saved_metric"): config_dict["metric"]["name"] = ( DatasetValidator._get_canonical_metric_name( config_dict["metric"]["name"], dataset_context diff --git a/superset/mcp_service/chart/plugins/mixed_timeseries.py b/superset/mcp_service/chart/plugins/mixed_timeseries.py index 2bd45243126..050a5626def 100644 --- a/superset/mcp_service/chart/plugins/mixed_timeseries.py +++ b/superset/mcp_service/chart/plugins/mixed_timeseries.py @@ -132,6 +132,8 @@ class MixedTimeseriesChartPlugin(BaseChartPlugin): def _norm_list(key: str) -> None: if config_dict.get(key): for col in config_dict[key]: + if col.get("sql_expression"): + continue if col.get("saved_metric"): col["name"] = DatasetValidator._get_canonical_metric_name( col["name"], dataset_context diff --git a/superset/mcp_service/chart/plugins/pie.py b/superset/mcp_service/chart/plugins/pie.py index 0e31d219927..ffb78a86343 100644 --- a/superset/mcp_service/chart/plugins/pie.py +++ b/superset/mcp_service/chart/plugins/pie.py @@ -47,7 +47,7 @@ class PieChartPlugin(BaseChartPlugin): ) -> ChartGenerationError | None: missing_fields = [] - if "dimension" not in config: + if "dimension" not in config and "groupby" not in config: missing_fields.append("'dimension' (category column for slices)") if "metric" not in config: missing_fields.append("'metric' (value metric for slice sizes)") @@ -104,7 +104,9 @@ class PieChartPlugin(BaseChartPlugin): ) ) if config_dict.get("metric"): - if config_dict["metric"].get("saved_metric"): + if config_dict["metric"].get("sql_expression"): + pass + elif config_dict["metric"].get("saved_metric"): config_dict["metric"]["name"] = ( DatasetValidator._get_canonical_metric_name( config_dict["metric"]["name"], dataset_context diff --git a/superset/mcp_service/chart/plugins/pivot_table.py b/superset/mcp_service/chart/plugins/pivot_table.py index 1437e0fd525..ebf83800cb8 100644 --- a/superset/mcp_service/chart/plugins/pivot_table.py +++ b/superset/mcp_service/chart/plugins/pivot_table.py @@ -47,7 +47,7 @@ class PivotTableChartPlugin(BaseChartPlugin): ) -> ChartGenerationError | None: missing_fields = [] - if not config.get("rows"): + if not (config.get("rows") or config.get("groupby") or config.get("dimension")): missing_fields.append("'rows' (row grouping columns)") if not config.get("metrics"): missing_fields.append("'metrics' (aggregation metrics)") @@ -67,7 +67,10 @@ class PivotTableChartPlugin(BaseChartPlugin): error_code="MISSING_PIVOT_FIELDS", ) - if not isinstance(config.get("rows", []), list): + rows_val = ( + config.get("rows") or config.get("groupby") or config.get("dimension") or [] + ) + if not isinstance(rows_val, list): return ChartGenerationError( error_type="invalid_rows_format", message="Rows must be a list of columns", @@ -123,6 +126,8 @@ class PivotTableChartPlugin(BaseChartPlugin): def _norm_col_list(key: str) -> None: if config_dict.get(key): for col in config_dict[key]: + if col.get("sql_expression"): + continue if col.get("saved_metric"): col["name"] = DatasetValidator._get_canonical_metric_name( col["name"], dataset_context diff --git a/superset/mcp_service/chart/plugins/table.py b/superset/mcp_service/chart/plugins/table.py index dc0d3d06c5d..ba6976d0c45 100644 --- a/superset/mcp_service/chart/plugins/table.py +++ b/superset/mcp_service/chart/plugins/table.py @@ -46,7 +46,10 @@ class TableChartPlugin(BaseChartPlugin): self, config: dict[str, Any], ) -> ChartGenerationError | None: - if not config.get("columns"): + columns = ( + config.get("columns") or config.get("all_columns") or config.get("groupby") + ) + if not columns: return ChartGenerationError( error_type="missing_columns", message="Table chart missing required field: columns", @@ -63,7 +66,7 @@ class TableChartPlugin(BaseChartPlugin): error_code="MISSING_COLUMNS", ) - if not isinstance(config.get("columns", []), list): + if not isinstance(columns, list): return ChartGenerationError( error_type="invalid_columns_format", message="Columns must be a list",