codeant-ai-for-open-source[bot] commented on code in PR #38375:
URL: https://github.com/apache/superset/pull/38375#discussion_r2881267598
##########
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:
**Suggestion:** If `chart_type` is not a string (for example, a list or dict
due to malformed client input), checking membership in `chart_type_validators`
will raise a `TypeError` instead of returning a structured
`ChartGenerationError`, causing the validator to crash on bad input instead of
failing gracefully. To avoid this, explicitly validate that `chart_type` is a
string before using it as a dict key and treat non-string values as an invalid
chart type. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Malformed MCP chart requests can crash validation flow.
- ⚠️ MCP chart generation may return HTTP 500 on bad input.
- ⚠️ Clients receive no structured `ChartGenerationError` response.
- ⚠️ Harder to debug invalid chart_type issues for integrators.
```
</details>
```suggestion
@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,
}
# Ensure chart_type is a string before using it as a dict key
if not isinstance(chart_type, str):
valid_types = ", ".join(chart_type_validators.keys())
return False, ChartGenerationError(
error_type="invalid_chart_type",
message=f"Invalid chart_type type:
{type(chart_type).__name__}",
details="The 'chart_type' field must be a string. "
f"Must be one of: {valid_types}",
suggestions=[
"Set 'chart_type' to a string value such as 'xy',
'table', "
"'pie', 'pivot_table', or 'mixed_timeseries'",
"Check that your JSON encoder is not serializing
chart_type "
"as an object or array",
],
error_code="INVALID_CHART_TYPE",
)
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. "
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",
)
return chart_type_validators[chart_type](config)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In any Python context (tests, MCP service), call
`SchemaValidator.validate_request()`
from `superset/mcp_service/chart/validation/schema_validator.py` with a
payload like:
`{"dataset_id": 1, "config": {"chart_type": ["xy"]}}`.
2. Inside `validate_request()` (same file, lines ~36–58), `_pre_validate()`
is invoked,
which extracts `config` and then `chart_type = config.get("chart_type")`.
3. Since `chart_type` is a non-empty list (`["xy"]`), the truthiness check
`if not
chart_type:` in `_pre_validate()` (same file, lines ~84–101) is skipped, and
it calls
`SchemaValidator._pre_validate_chart_type(chart_type, config)` at line 140.
4. In `_pre_validate_chart_type()` at
`superset/mcp_service/chart/validation/schema_validator.py:143`, Python
evaluates `if
chart_type not in chart_type_validators:` where `chart_type_validators` is a
dict. Because
`chart_type` is a list (an unhashable type), this membership test raises
`TypeError:
unhashable type: 'list'`, causing `validate_request()` to raise an uncaught
exception
instead of returning a structured `ChartGenerationError` with
`error_code="INVALID_CHART_TYPE"`.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/validation/schema_validator.py
**Line:** 142:174
**Comment:**
*Type Error: If `chart_type` is not a string (for example, a list or
dict due to malformed client input), checking membership in
`chart_type_validators` will raise a `TypeError` instead of returning a
structured `ChartGenerationError`, causing the validator to crash on bad input
instead of failing gracefully. To avoid this, explicitly validate that
`chart_type` is a string before using it as a dict key and treat non-string
values as an invalid chart type.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38375&comment_hash=8515bcec14429e7917c39ed37b26558835a3bfbc710b0a3a1df29985b399e3cd&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38375&comment_hash=8515bcec14429e7917c39ed37b26558835a3bfbc710b0a3a1df29985b399e3cd&reaction=dislike'>👎</a>
--
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]
