diff --git a/superset/mcp_service/chart/plugins/big_number.py b/superset/mcp_service/chart/plugins/big_number.py index 0147ec0f02c..82618e86031 100644 --- a/superset/mcp_service/chart/plugins/big_number.py +++ b/superset/mcp_service/chart/plugins/big_number.py @@ -79,7 +79,25 @@ class BigNumberChartPlugin(BaseChartPlugin): ], error_code="INVALID_BIG_NUMBER_METRIC_TYPE", ) - if not metric.get("aggregate") and not metric.get("saved_metric"): + if metric.get("sql_expression"): + label = metric.get("label") + if not isinstance(label, str) or not label.strip(): + return ChartGenerationError( + error_type="missing_sql_metric_label", + message="SQL expression metrics require a non-empty 'label'", + details=( + "When using a custom SQL expression as the Big Number metric, " + "a human-readable 'label' string is required so Superset can " + "display the metric name." + ), + suggestions=[ + "Add 'label': e.g. {'sql_expression': 'SUM(a)/SUM(b)', " + "'label': 'Conversion Rate'}", + "The label must be a non-empty string", + ], + error_code="MISSING_SQL_METRIC_LABEL", + ) + elif not metric.get("aggregate") and not metric.get("saved_metric"): return ChartGenerationError( error_type="missing_metric_aggregate", message=( diff --git a/superset/mcp_service/chart/plugins/xy.py b/superset/mcp_service/chart/plugins/xy.py index 1d542b49bbc..7ec7263cf89 100644 --- a/superset/mcp_service/chart/plugins/xy.py +++ b/superset/mcp_service/chart/plugins/xy.py @@ -119,6 +119,8 @@ class XYChartPlugin(BaseChartPlugin): config_dict["x"]["name"], dataset_context ) for y_col in config_dict.get("y") or []: + if y_col.get("sql_expression"): + continue # sql_expression metrics have no underlying column if y_col.get("saved_metric"): y_col["name"] = get_canonical_metric(y_col["name"], dataset_context) else: diff --git a/tests/unit_tests/mcp_service/chart/test_big_number_chart.py b/tests/unit_tests/mcp_service/chart/test_big_number_chart.py index fef4d562bb7..a19f22282e4 100644 --- a/tests/unit_tests/mcp_service/chart/test_big_number_chart.py +++ b/tests/unit_tests/mcp_service/chart/test_big_number_chart.py @@ -100,23 +100,29 @@ class TestBigNumberChartConfig: def test_sql_expression_with_label_passes_pre_validation(self) -> None: """A custom SQL metric is a valid third option alongside aggregate and saved_metric in Tier-1 validation.""" + from superset.mcp_service.chart.registry import get_registry + data = { "chart_type": "big_number", "metric": {"sql_expression": "SUM(a)/SUM(b)", "label": "Ratio"}, } - is_valid, error = SchemaValidator._pre_validate_big_number_config(data) - assert is_valid is True + plugin = get_registry().get("big_number") + assert plugin is not None + error = plugin.pre_validate(data) assert error is None def test_sql_expression_without_label_fails_pre_validation(self) -> None: """Tier-1 surfaces the label-required error with an LLM-actionable suggestion before the request reaches Pydantic's stricter error.""" + from superset.mcp_service.chart.registry import get_registry + data = { "chart_type": "big_number", "metric": {"sql_expression": "SUM(a)/SUM(b)"}, } - is_valid, error = SchemaValidator._pre_validate_big_number_config(data) - assert is_valid is False + plugin = get_registry().get("big_number") + assert plugin is not None + error = plugin.pre_validate(data) assert error is not None assert error.error_code == "MISSING_SQL_METRIC_LABEL" @@ -124,12 +130,15 @@ class TestBigNumberChartConfig: """Pre-validation runs on raw dict input before Pydantic coercion, so a non-string ``label`` (e.g. an int from a buggy client) must surface as a validation error, not an AttributeError from ``.strip()``.""" + from superset.mcp_service.chart.registry import get_registry + data = { "chart_type": "big_number", "metric": {"sql_expression": "SUM(a)/SUM(b)", "label": 123}, } - is_valid, error = SchemaValidator._pre_validate_big_number_config(data) - assert is_valid is False + plugin = get_registry().get("big_number") + assert plugin is not None + error = plugin.pre_validate(data) assert error is not None assert error.error_code == "MISSING_SQL_METRIC_LABEL" diff --git a/tests/unit_tests/mcp_service/chart/tool/test_update_chart.py b/tests/unit_tests/mcp_service/chart/tool/test_update_chart.py index 0fb24e6e8d5..ff00fb34f89 100755 --- a/tests/unit_tests/mcp_service/chart/tool/test_update_chart.py +++ b/tests/unit_tests/mcp_service/chart/tool/test_update_chart.py @@ -1358,6 +1358,7 @@ class TestUpdateChartColumnNormalization: mock_validate.return_value = CompileResult( success=True, error=None, error_code=None, tier="validation", error_obj=None ) + mock_create_preview.return_value = ("http://example.com/explore", None, []) # normalize_column_names returns the config unchanged def _passthrough(config, dataset_id): @@ -1418,7 +1419,8 @@ class TestUpdateChartColumnNormalization: mock_validate.return_value = CompileResult( success=True, error=None, error_code=None, tier="validation", error_obj=None ) - mock_normalize.side_effect = RuntimeError("DB connection failed") + mock_create_preview.return_value = ("http://example.com/explore", None, []) + mock_normalize.side_effect = ValueError("DB connection failed") request = { "identifier": 1, diff --git a/tests/unit_tests/mcp_service/chart/validation/test_runtime_validator.py b/tests/unit_tests/mcp_service/chart/validation/test_runtime_validator.py index 5c40111f14c..aa68fffaf16 100644 --- a/tests/unit_tests/mcp_service/chart/validation/test_runtime_validator.py +++ b/tests/unit_tests/mcp_service/chart/validation/test_runtime_validator.py @@ -236,8 +236,13 @@ class TestRuntimeValidatorNonBlocking: def test_validate_cardinality_returns_cleanly_when_x_name_is_none(self) -> None: """The dimension-rejection guard on XYChartConfig normally forbids x.name=None, but a model_construct bypass (or a future code path) - could land us here. The defensive guard must return cleanly without - calling into CardinalityValidator (which assumes a real column).""" + could land us here. The defensive guard in XYChartPlugin.get_runtime_warnings + must skip cardinality without crashing.""" + from superset.mcp_service.chart.plugins.xy import XYChartPlugin + from superset.mcp_service.chart.validation.runtime.format_validator import ( + FormatTypeValidator, + ) + col = ColumnRef.model_construct(name=None) config = XYChartConfig.model_construct( chart_type="xy", @@ -246,14 +251,19 @@ class TestRuntimeValidatorNonBlocking: kind="line", ) - with patch( - "superset.mcp_service.chart.validation.runtime." - "cardinality_validator.CardinalityValidator.check_cardinality" - ) as mock_check: - warnings, suggestions = RuntimeValidator._validate_cardinality( - config, dataset_id=1 - ) + plugin = XYChartPlugin() + with ( + patch.object( + FormatTypeValidator, + "validate_format_compatibility", + return_value=(True, []), + ), + patch( + "superset.mcp_service.chart.validation.runtime." + "cardinality_validator.CardinalityValidator.check_cardinality" + ) as mock_check, + ): + warnings = plugin.get_runtime_warnings(config, dataset_id=1) assert warnings == [] - assert suggestions == [] mock_check.assert_not_called()