codeant-ai-for-open-source[bot] commented on code in PR #41599:
URL: https://github.com/apache/superset/pull/41599#discussion_r3599230975
##########
superset/mcp_service/chart/tool/generate_chart.py:
##########
@@ -111,39 +111,55 @@ async def generate_chart( # noqa: C901
IMPORTANT: The 'chart_type' field in the config is a DISCRIMINATOR that
determines
which chart configuration schema to use. It MUST be included and MUST
match the
- other fields in your configuration:
+ other fields in your configuration. There are exactly 9 valid chart_type
values,
+ listed below. Values such as 'line', 'bar', 'area', and 'scatter' are
'kind'
+ values WITHIN chart_type='xy', not chart_type values themselves:
- - Use chart_type='xy' for charts with x and y axes (line, bar, area,
scatter)
- Required fields: y (x is optional — defaults to dataset's primary
datetime column)
+ - chart_type='xy' for charts with x and y axes (line, bar, area, scatter).
+ Required fields: y (x is optional — defaults to dataset's primary
+ datetime column). Use 'kind' to pick line/bar/area/scatter
+ (default kind='line').
- - Use chart_type='table' for tabular visualizations
+ - chart_type='table' for tabular visualizations.
Required fields: columns
- - Use chart_type='pie' for pie/donut charts
+ - chart_type='pie' for pie/donut charts.
Required fields: dimension, metric
- - Use chart_type='pivot_table' for pivot table visualizations
- Required fields: rows, metrics
+ - chart_type='pivot_table' for pivot table visualizations.
+ Required fields: rows, metrics (columns is optional, for cross-tabs)
- - Use chart_type='mixed_timeseries' for dual-axis time-series charts
- Required fields: x, y, y_secondary
+ - chart_type='mixed_timeseries' for dual-axis time-series charts.
+ Required fields: x, y (primary metrics), y_secondary (secondary metrics)
- - Use chart_type='handlebars' for custom template-based visualizations
+ - chart_type='handlebars' for custom template-based visualizations.
Required fields: handlebars_template
Review Comment:
**Suggestion:** The required-fields guidance for the Handlebars chart type
is incomplete: with the default `query_mode='aggregate'`, validation also
requires `metrics`, so documenting only `handlebars_template` will cause
clients to submit payloads that fail at runtime. Update this section to
describe the conditional requirement (`metrics` for aggregate mode, or
`columns` for raw mode) so the docstring matches actual schema/validator
behavior. [docstring mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Handlebars charts fail when using docstring-only guidance.
- ⚠️ MCP clients may retry invalid handlebars requests repeatedly.
- ⚠️ LLM tools mislearn required fields for handlebars charts.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. From an MCP client, call the `generate_chart` tool implemented by `async
def
generate_chart()` in `superset/mcp_service/chart/tool/generate_chart.py:98`,
constructing
the request payload based on the docstring lines 135–136 which state that for
`chart_type='handlebars'` the only required field is `handlebars_template`.
2. Send a request like:
`{"dataset_id": 1, "config": {"chart_type": "handlebars",
"handlebars_template":
"<ul>{{#each data}}<li>{{this.name}}</li>{{/each}}</ul>"}}`
omitting both `metrics` and `columns` because the docstring does not
document them as
required.
3. Inside `generate_chart`, the validation pipeline is invoked via
`ValidationPipeline.validate_request_with_warnings(request.model_dump())` at
`superset/mcp_service/chart/tool/generate_chart.py:187–191`, which calls
`SchemaValidator.validate_request()` in
`superset/mcp_service/chart/validation/schema_validator.py:40–56`.
4. `SchemaValidator._pre_validate_chart_type()` at
`schema_validator.py:110–88` fetches
`HandlebarsChartPlugin` and calls its `pre_validate(config)` method defined
in
`superset/mcp_service/chart/plugins/handlebars.py:45–124`; that method uses
`query_mode =
config.get("query_mode", "aggregate")` (line 82) and, when
`query_mode=="aggregate"`,
requires `metrics` (lines 110–123) while the Pydantic schema
`HandlebarsChartConfig` in
`superset/mcp_service/chart/schemas.py:1282–103` also enforces
metrics/columns via
`validate_query_fields`. Because the request only includes
`handlebars_template`,
`pre_validate` returns a `ChartGenerationError` and
`SchemaValidator.validate_request()`
returns `is_valid=False`, causing
`ValidationPipeline.validate_request_with_warnings()`
(pipeline.py:47–50) and then `generate_chart()` to respond with a validation
error instead
of a chart preview. This mismatch between docstring (template-only required)
and
validators (template plus metrics/columns depending on query_mode) will
cause any client
following the docstring to consistently submit invalid handlebars chart
configurations.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=aadce721f42f4ba98d93db5c4514f477&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=aadce721f42f4ba98d93db5c4514f477&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/generate_chart.py
**Line:** 135:136
**Comment:**
*Docstring Mismatch: The required-fields guidance for the Handlebars
chart type is incomplete: with the default `query_mode='aggregate'`, validation
also requires `metrics`, so documenting only `handlebars_template` will cause
clients to submit payloads that fail at runtime. Update this section to
describe the conditional requirement (`metrics` for aggregate mode, or
`columns` for raw mode) so the docstring matches actual schema/validator
behavior.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41599&comment_hash=a4decd2954e4aaa4a36441c92c8015a0e3b13af6b8bd3ec89a9bfb448d36879e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41599&comment_hash=a4decd2954e4aaa4a36441c92c8015a0e3b13af6b8bd3ec89a9bfb448d36879e&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]
