QT/superset2
1
0
Fork 0
mirror of https://github.com/apache/superset.git synced 2026-04-07 18:35:15 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(mcp): treat runtime validation warnings as informational, not errors (#37214)

Browse Source 
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Amin Ghadersohi
2026-02-05 10:23:51 -07:00
committed by GitHub
parent 20f1918dd6
commit a9dca529c1
4 changed files with 140 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files

33
superset/mcp_service/chart/tool/generate_chart.py
View File

@@ -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",

66
superset/mcp_service/chart/validation/pipeline.py
View File

@@ -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

29
superset/mcp_service/chart/validation/runtime/__init__.py
View File

@@ -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

52
tests/unit_tests/mcp_service/chart/validation/test_runtime_validator.py
View File

@@ -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