codeant-ai-for-open-source[bot] commented on code in PR #42070:
URL: https://github.com/apache/superset/pull/42070#discussion_r3599005185
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2009,6 +2009,105 @@ def validate_percentiles_and_dimensions(self) ->
"BoxPlotChartConfig":
return self
+class WaterfallChartConfig(UnknownFieldCheckMixin):
+ """Config for waterfall charts (viz_type ``waterfall``)."""
+
+ model_config = ConfigDict(extra="ignore", populate_by_name=True)
+
+ chart_type: Literal["waterfall"] = "waterfall"
+ x_axis: ColumnRef = Field(
+ ...,
+ description="Category or period column along the x-axis (often "
+ "temporal, e.g. month); each value is one waterfall step",
+ )
+ metric: ColumnRef = Field(
+ ...,
+ description="Metric whose per-step change is plotted (use aggregate "
+ "e.g. SUM for ad-hoc, or saved_metric=True for a saved metric)",
+ )
+ breakdown: ColumnRef | None = Field(
+ None,
+ validation_alias=AliasChoices("breakdown", "groupby"),
+ description="Optional single category column that breaks each "
+ "x-axis period into per-category steps (form_data 'groupby'; the "
+ "frontend Breakdowns control is single-select)",
+ )
+ time_grain: TimeGrain | None = Field(
+ None,
+ description="Time bucket for a temporal x_axis (PT1H, P1D, P1W, "
+ "P1M, P1Y); each bucket becomes one waterfall step. Ignored for a "
+ "non-temporal x_axis.",
+ validation_alias=AliasChoices("time_grain", "time_grain_sqla"),
+ )
+ show_total: bool = Field(
+ True, description="Append a total bar per period (frontend default)"
+ )
+ show_legend: bool = Field(
+ False, description="Show the legend (frontend default: off)"
+ )
+ increase_label: str = Field("Increase", max_length=50)
+ decrease_label: str = Field("Decrease", max_length=50)
+ total_label: str = Field("Total", max_length=50)
+ x_axis_time_format: str = Field(
+ "smart_date",
+ description="Time format for a temporal x-axis (e.g. 'smart_date',
'%Y-%m-%d')",
+ max_length=50,
+ )
+ y_axis_format: str = Field("SMART_NUMBER", max_length=50)
+ currency_format: CurrencyFormat | None = Field(
+ None,
+ description="Currency symbol applied to the metric value",
+ )
+ filters: List[FilterConfig] | None = Field(
+ None,
+ description="Structured filters (column/op/value). "
+ "Do NOT use adhoc_filters or raw SQL expressions.",
+ )
+ row_limit: int = Field(
+ 10000,
+ description="Max grouped rows (frontend shared default)",
+ ge=1,
+ le=50000,
+ )
+
+ @model_validator(mode="before")
+ @classmethod
+ def unwrap_list_breakdown(cls, data: Any) -> Any:
+ """Accept the native form_data shape where ``groupby`` is a list.
+
+ The frontend Breakdowns control is single-select but stores its value
+ as a one-item list (e.g. ``["region"]``), so a config round-tripped
+ from existing waterfall form_data sends a list. Unwrap a length-1
+ list to the single value; reject longer lists rather than silently
+ dropping breakdowns.
+ """
+ if isinstance(data, dict):
+ for key in ("groupby", "breakdown"):
+ value = data.get(key)
+ if isinstance(value, list):
+ if len(value) > 1:
+ raise ValueError(
+ "waterfall breakdown is single-select; pass at "
+ "most one column"
+ )
+ data[key] = value[0] if value else None
+ return data
Review Comment:
**Suggestion:** The list-unwrapping logic for `groupby`/`breakdown` assigns
the raw first list item directly, which turns a native form_data payload like
`groupby: ["region"]` into a plain string. That then fails `ColumnRef`
validation (expects an object shape), so round-tripping existing waterfall
form_data can break with validation errors. Normalize a string item into a
`{"name": ...}` object (while preserving dict items) when unwrapping the
single-entry list. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ generate_chart rejects valid waterfall configs from Superset form_data.
- ⚠️ LLM clients cannot reuse existing waterfall chart configurations.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the MCP generate_chart tool defined in
superset/mcp_service/chart/tool/generate_chart.py (async function
generate_chart at lines
98-100) with a JSON body that Pydantic will parse into GenerateChartRequest
(schemas.py
lines 2295-2305).
2. In the request payload, set config to a Superset-style waterfall
form_data dict
(accepted by ChartRequestNormalizerMixin at schemas.py lines 2140-2210)
containing:
{"chart_type": "waterfall", "x_axis": {"name": "month"}, "metric": {"name":
"revenue",
"aggregate": "SUM"}, "groupby": ["region"]}.
3. GenerateChartRequest.model_validate() constructs the nested ChartConfig
(schemas.py
lines 212-223); because chart_type="waterfall", Pydantic dispatches to
WaterfallChartConfig (schemas.py lines 113-173), running the
@model_validator(mode="before") unwrap_list_breakdown (schemas.py lines
174-195).
4. unwrap_list_breakdown sees data["groupby"] == ["region"], treats it as a
list, and
since len(value) == 1, assigns data["groupby"] = value[0], i.e. the bare
string "region"
(schemas.py lines 185-194).
5. Pydantic then tries to populate the breakdown: ColumnRef | None field
(schemas.py lines
129-135) using the validation alias AliasChoices("breakdown", "groupby"); it
receives the
string "region" instead of a dict and attempts to construct ColumnRef
(schemas.py lines
16-41), which expects a mapping with keys like "name" or "column_name" and
cannot parse a
plain string.
6. ColumnRef validation raises a ValidationError because the input is not a
valid object
shape; this bubbles up as a WaterfallChartConfig validation failure, causing
GenerateChartRequest validation to fail and the generate_chart tool call to
return a
schema error instead of creating a waterfall chart preview.
7. This misbehavior does not occur when groupby is provided as {"name":
"region"} or
[{"name": "region"}], as verified by tests in
tests/unit_tests/mcp_service/chart/test_waterfall_chart.py (tests
test_groupby_alias_for_breakdown and test_groupby_list_unwrapped). However,
native
Superset form_data patterns for groupby used elsewhere in the codebase (e.g.
tests/unit_tests/mcp_service/chart/test_preview_utils.py lines 71-80 where
groupby=["year"]) show that list-of-strings is a realistic shape, so
existing waterfall
form_data round-tripped into the MCP config will hit this bug unless the
list item is
normalized into a {"name": "region"}-style object.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=63147ae495014e8e8e9a0d2f2077317f&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=63147ae495014e8e8e9a0d2f2077317f&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/schemas.py
**Line:** 2086:2094
**Comment:**
*Type Error: The list-unwrapping logic for `groupby`/`breakdown`
assigns the raw first list item directly, which turns a native form_data
payload like `groupby: ["region"]` into a plain string. That then fails
`ColumnRef` validation (expects an object shape), so round-tripping existing
waterfall form_data can break with validation errors. Normalize a string item
into a `{"name": ...}` object (while preserving dict items) when unwrapping the
single-entry list.
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%2F42070&comment_hash=d18e78b0928b695530ca4d9028745cc549bc121e45f372ae43dba331f9c707a2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42070&comment_hash=d18e78b0928b695530ca4d9028745cc549bc121e45f372ae43dba331f9c707a2&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]
