aminghadersohi commented on code in PR #38375:
URL: https://github.com/apache/superset/pull/38375#discussion_r2882533377
##########
superset/mcp_service/chart/validation/schema_validator.py:
##########
@@ -126,37 +126,52 @@ def _pre_validate(
return False, ChartGenerationError(
error_type="missing_chart_type",
message="Missing required field: chart_type",
- details="Chart configuration must specify 'chart_type' as
either 'xy' "
- "or 'table'",
+ details="Chart configuration must specify 'chart_type'",
suggestions=[
"Add 'chart_type': 'xy' for line/bar/area/scatter charts",
"Add 'chart_type': 'table' for table visualizations",
- "Example: 'config': {'chart_type': 'xy', ...}",
+ "Add 'chart_type': 'pie' for pie or donut charts",
+ "Add 'chart_type': 'pivot_table' for interactive pivot
tables",
+ "Add 'chart_type': 'mixed_timeseries' for dual-series time
charts",
],
error_code="MISSING_CHART_TYPE",
)
- if chart_type not in ["xy", "table"]:
+ return SchemaValidator._pre_validate_chart_type(chart_type, config)
+
+ @staticmethod
+ def _pre_validate_chart_type(
+ chart_type: str,
+ config: Dict[str, Any],
+ ) -> Tuple[bool, ChartGenerationError | None]:
+ """Validate chart type and dispatch to type-specific pre-validation."""
+ chart_type_validators = {
+ "xy": SchemaValidator._pre_validate_xy_config,
+ "table": SchemaValidator._pre_validate_table_config,
+ "pie": SchemaValidator._pre_validate_pie_config,
+ "pivot_table": SchemaValidator._pre_validate_pivot_table_config,
+ "mixed_timeseries":
SchemaValidator._pre_validate_mixed_timeseries_config,
+ }
+
+ if chart_type not in chart_type_validators:
+ valid_types = ", ".join(chart_type_validators.keys())
return False, ChartGenerationError(
error_type="invalid_chart_type",
message=f"Invalid chart_type: '{chart_type}'",
- details=f"Chart type '{chart_type}' is not supported. Must be
'xy' or "
- f"'table'",
+ details=f"Chart type '{chart_type}' is not supported. "
+ f"Must be one of: {valid_types}",
suggestions=[
"Use 'chart_type': 'xy' for line, bar, area, or scatter
charts",
"Use 'chart_type': 'table' for tabular data display",
+ "Use 'chart_type': 'pie' for pie or donut charts",
+ "Use 'chart_type': 'pivot_table' for interactive pivot
tables",
+ "Use 'chart_type': 'mixed_timeseries' for dual-series time
charts",
"Check spelling and ensure lowercase",
],
error_code="INVALID_CHART_TYPE",
)
- # Pre-validate structure based on chart type
- if chart_type == "xy":
- return SchemaValidator._pre_validate_xy_config(config)
- elif chart_type == "table":
- return SchemaValidator._pre_validate_table_config(config)
-
- return True, None
+ return chart_type_validators[chart_type](config)
Review Comment:
Good catch - fixed in 3fbc4ce. Added `isinstance(chart_type, str)` guard
before the dict lookup so non-string values (list, dict, int, etc.) return a
structured `ChartGenerationError` instead of raising `TypeError`. Also added
parametrized tests covering list, dict, int, and bool inputs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
