diff --git a/superset/mcp_service/chart/tool/generate_chart.py b/superset/mcp_service/chart/tool/generate_chart.py index 7b7f3a17895..bb8157adf4d 100644 --- a/superset/mcp_service/chart/tool/generate_chart.py +++ b/superset/mcp_service/chart/tool/generate_chart.py @@ -123,6 +123,9 @@ async def generate_chart( # noqa: C901 "Chart configuration details: config=%s" % (request.config.model_dump(),) ) + # Track runtime warnings to include in response + runtime_warnings: list[str] = [] + try: # Run comprehensive validation pipeline await ctx.report_progress(1, 5, "Running validation pipeline") @@ -131,22 +134,34 @@ async def generate_chart( # noqa: C901 ) from superset.mcp_service.chart.validation import ValidationPipeline - is_valid, parsed_request, validation_error = ( - ValidationPipeline.validate_request(request.model_dump()) + validation_result = ValidationPipeline.validate_request_with_warnings( + request.model_dump() ) - if is_valid and parsed_request is not None: + + if validation_result.is_valid and validation_result.request is not None: # Use the validated request going forward - request = parsed_request - if not is_valid: + request = validation_result.request + + # Capture runtime warnings (informational, not blocking) + if validation_result.warnings: + runtime_warnings = validation_result.warnings.get("warnings", []) + if runtime_warnings: + await ctx.info( + "Runtime suggestions: %s" % ("; ".join(runtime_warnings[:3]),) + ) + + if not validation_result.is_valid: execution_time = int((time.time() - start_time) * 1000) - assert validation_error is not None # Type narrowing for mypy + if validation_result.error is None: + raise RuntimeError("Validation failed but error object is missing") await ctx.error( - "Chart validation failed: error=%s" % (validation_error.model_dump(),) + "Chart validation failed: error=%s" + % (validation_result.error.model_dump(),) ) return GenerateChartResponse.model_validate( { "chart": None, - "error": validation_error.model_dump(), + "error": validation_result.error.model_dump(), "performance": { "query_duration_ms": execution_time, "cache_status": "error", @@ -471,6 +486,8 @@ async def generate_chart( # noqa: C901 else {}, "performance": performance.model_dump() if performance else None, "accessibility": accessibility.model_dump() if accessibility else None, + # Runtime warnings (informational suggestions, not errors) + "warnings": runtime_warnings, "success": True, "schema_version": "2.0", "api_version": "v1", diff --git a/superset/mcp_service/chart/validation/pipeline.py b/superset/mcp_service/chart/validation/pipeline.py index 8d595aa97ab..948f9d2e62d 100644 --- a/superset/mcp_service/chart/validation/pipeline.py +++ b/superset/mcp_service/chart/validation/pipeline.py @@ -101,12 +101,28 @@ def _sanitize_validation_error(error: Exception) -> str: return error_str +class ValidationResult: + """Result of validation pipeline including optional warnings.""" + + def __init__( + self, + is_valid: bool, + request: GenerateChartRequest | None = None, + error: ChartGenerationError | None = None, + warnings: Dict[str, Any] | None = None, + ): + self.is_valid = is_valid + self.request = request + self.error = error + self.warnings = warnings # Runtime warnings (informational, not blocking) + + class ValidationPipeline: """ Main validation orchestrator that runs validations in sequence: 1. Schema validation (structure and types) 2. Dataset validation (columns exist) - 3. Runtime validation (performance, compatibility) + 3. Runtime validation (performance, compatibility) - returns warnings, not errors """ @staticmethod @@ -121,6 +137,24 @@ class ValidationPipeline: Returns: Tuple of (is_valid, parsed_request, error) + + Note: Use validate_request_with_warnings() to also get runtime warnings. + """ + result = ValidationPipeline.validate_request_with_warnings(request_data) + return result.is_valid, result.request, result.error + + @staticmethod + def validate_request_with_warnings( + request_data: Dict[str, Any], + ) -> ValidationResult: + """ + Validate a chart generation request and return warnings as metadata. + + Args: + request_data: Raw request data dictionary + + Returns: + ValidationResult with is_valid, request, error, and optional warnings """ try: # Layer 1: Schema validation @@ -128,27 +162,28 @@ class ValidationPipeline: is_valid, request, error = SchemaValidator.validate_request(request_data) if not is_valid: - return False, None, error + return ValidationResult(is_valid=False, error=error) # Ensure request is not None if request is None: - return False, None, error + return ValidationResult(is_valid=False, error=error) # Layer 2: Dataset validation is_valid, error = ValidationPipeline._validate_dataset( request.config, request.dataset_id ) if not is_valid: - return False, request, error + return ValidationResult(is_valid=False, request=request, error=error) - # Layer 3: Runtime validation - is_valid, error = ValidationPipeline._validate_runtime( + # Layer 3: Runtime validation - returns warnings as metadata, not errors + _is_valid, warnings_metadata = ValidationPipeline._validate_runtime( request.config, request.dataset_id ) - if not is_valid: - return False, request, error + # Runtime validation always returns True now, warnings are informational - return True, request, None + return ValidationResult( + is_valid=True, request=request, warnings=warnings_metadata + ) except Exception as e: logger.exception("Validation pipeline error") @@ -164,7 +199,7 @@ class ValidationPipeline: template_vars={"reason": sanitized_reason}, error_code="VALIDATION_PIPELINE_ERROR", ) - return False, None, error + return ValidationResult(is_valid=False, error=error) @staticmethod def _validate_dataset( @@ -189,8 +224,15 @@ class ValidationPipeline: @staticmethod def _validate_runtime( config: ChartConfig, dataset_id: int | str - ) -> Tuple[bool, ChartGenerationError | None]: - """Validate runtime issues (performance, compatibility).""" + ) -> Tuple[bool, Dict[str, Any] | None]: + """ + Validate runtime issues (performance, compatibility). + + Returns: + Tuple of (is_valid, warnings_metadata) + - is_valid: Always True (runtime warnings don't block generation) + - warnings_metadata: Dict with warnings/suggestions, or None + """ try: from .runtime import RuntimeValidator diff --git a/superset/mcp_service/chart/validation/runtime/__init__.py b/superset/mcp_service/chart/validation/runtime/__init__.py index 659ada61de2..7228d73481b 100644 --- a/superset/mcp_service/chart/validation/runtime/__init__.py +++ b/superset/mcp_service/chart/validation/runtime/__init__.py @@ -21,13 +21,12 @@ Validates performance, compatibility, and user experience issues. """ import logging -from typing import List, Tuple +from typing import Any, Dict, List, Tuple from superset.mcp_service.chart.schemas import ( ChartConfig, XYChartConfig, ) -from superset.mcp_service.common.error_schemas import ChartGenerationError logger = logging.getLogger(__name__) @@ -38,16 +37,21 @@ class RuntimeValidator: @staticmethod def validate_runtime_issues( config: ChartConfig, dataset_id: int | str - ) -> Tuple[bool, ChartGenerationError | None]: + ) -> Tuple[bool, Dict[str, Any] | None]: """ Validate runtime issues that could affect chart rendering or performance. + Warnings are returned as informational metadata, NOT as errors. + Chart generation proceeds regardless of warnings. + Args: config: Chart configuration to validate dataset_id: Dataset identifier Returns: - Tuple of (is_valid, error) + Tuple of (is_valid, warnings_metadata) + - is_valid: Always True (warnings don't block generation) + - warnings_metadata: Dict with warnings and suggestions, or None """ warnings: List[str] = [] suggestions: List[str] = [] @@ -75,16 +79,21 @@ class RuntimeValidator: warnings.extend(type_warnings) suggestions.extend(type_suggestions) - # Semantic warnings are informational, not blocking errors. - # Log them for debugging but allow chart generation to proceed. + # Return warnings as metadata, NOT as errors + # Warnings should inform, not block chart generation if warnings: logger.info( - "Runtime semantic warnings for dataset %s: %s", + "Runtime validation warnings for dataset %s: %s", dataset_id, - "; ".join(warnings[:3]) + ("..." if len(warnings) > 3 else ""), + warnings[:3], + ) + return ( + True, + { + "warnings": warnings[:5], # Limit to 5 warnings + "suggestions": suggestions[:5], # Limit to 5 suggestions + }, ) - if suggestions: - logger.info("Suggestions: %s", "; ".join(suggestions[:3])) return True, None 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 ba8cc3fb9f9..c49677cb99f 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 @@ -67,11 +67,16 @@ class TestRuntimeValidatorNonBlocking: "Currency format '$,.2f' may not display dates correctly" ] - is_valid, error = RuntimeValidator.validate_runtime_issues(config, 1) + is_valid, warnings_metadata = RuntimeValidator.validate_runtime_issues( + config, 1 + ) # Should still return valid despite warnings assert is_valid is True - assert error is None + # Warnings are returned as metadata, not as blocking errors + assert warnings_metadata is not None + assert "warnings" in warnings_metadata + assert len(warnings_metadata["warnings"]) > 0 def test_validate_runtime_issues_non_blocking_with_cardinality_warnings(self): """Test that cardinality warnings do NOT block chart generation.""" @@ -92,11 +97,16 @@ class TestRuntimeValidatorNonBlocking: ["Consider using aggregation or filtering"], ) - is_valid, error = RuntimeValidator.validate_runtime_issues(config, 1) + is_valid, warnings_metadata = RuntimeValidator.validate_runtime_issues( + config, 1 + ) # Should still return valid despite high cardinality warning assert is_valid is True - assert error is None + # Warnings are returned as metadata, not as blocking errors + assert warnings_metadata is not None + assert "warnings" in warnings_metadata + assert len(warnings_metadata["warnings"]) > 0 def test_validate_runtime_issues_non_blocking_with_chart_type_suggestions(self): """Test that chart type suggestions do NOT block chart generation.""" @@ -117,11 +127,16 @@ class TestRuntimeValidatorNonBlocking: ["Consider using bar chart for better visualization"], ) - is_valid, error = RuntimeValidator.validate_runtime_issues(config, 1) + is_valid, warnings_metadata = RuntimeValidator.validate_runtime_issues( + config, 1 + ) # Should still return valid despite suggestion assert is_valid is True - assert error is None + # Warnings are returned as metadata, not as blocking errors + assert warnings_metadata is not None + assert "warnings" in warnings_metadata + assert len(warnings_metadata["warnings"]) > 0 def test_validate_runtime_issues_non_blocking_with_multiple_warnings(self): """Test that multiple warnings combined do NOT block chart generation.""" @@ -158,11 +173,17 @@ class TestRuntimeValidatorNonBlocking: ["Chart type suggestion"], ) - is_valid, error = RuntimeValidator.validate_runtime_issues(config, 1) + is_valid, warnings_metadata = RuntimeValidator.validate_runtime_issues( + config, 1 + ) # Should still return valid despite multiple warnings assert is_valid is True - assert error is None + # Warnings are returned as metadata, not as blocking errors + assert warnings_metadata is not None + assert "warnings" in warnings_metadata + # Multiple warnings from different validators should all be collected + assert len(warnings_metadata["warnings"]) >= 3 def test_validate_runtime_issues_logs_warnings(self): """Test that warnings are logged for debugging purposes.""" @@ -184,12 +205,16 @@ class TestRuntimeValidatorNonBlocking: ): mock_format.return_value = ["Test warning message"] - is_valid, error = RuntimeValidator.validate_runtime_issues(config, 1) + is_valid, warnings_metadata = RuntimeValidator.validate_runtime_issues( + config, 1 + ) # Verify warnings were logged assert mock_logger.info.called assert is_valid is True - assert error is None + # Warnings are returned as metadata + assert warnings_metadata is not None + assert "warnings" in warnings_metadata def test_validate_table_chart_skips_xy_validations(self): """Test that table charts skip XY-specific validations.""" @@ -211,7 +236,14 @@ class TestRuntimeValidatorNonBlocking: "superset.mcp_service.chart.validation.runtime.RuntimeValidator." "_validate_cardinality" ) as mock_cardinality, + patch( + "superset.mcp_service.chart.validation.runtime.RuntimeValidator." + "_validate_chart_type" + ) as mock_chart_type, ): + # Mock chart type validator to return no warnings + mock_chart_type.return_value = ([], []) + is_valid, error = RuntimeValidator.validate_runtime_issues(config, 1) # Format and cardinality validation should not be called for table charts