codeant-ai-for-open-source[bot] commented on code in PR #39922:
URL: https://github.com/apache/superset/pull/39922#discussion_r3398668366
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -1665,24 +1707,28 @@ def reject_sql_expression_on_dimensions(self) ->
"XYChartConfig":
@model_validator(mode="after")
def validate_unique_column_labels(self) -> "XYChartConfig":
"""Ensure all column labels are unique across x, y, and group_by."""
- labels_seen: dict[str, str] = {}
+ # Key is (saved_metric, label) so a saved metric and a regular column
+ # with the same input name are not flagged as duplicates — saved
metrics
+ # resolve to their actual casing from the dataset during normalization.
+ labels_seen: dict[tuple[bool, str], str] = {}
duplicates: list[str] = []
# Add x-axis label if present (x may be None, resolved later).
# The dimension validator rejects sql_expression on x, so name is set.
if self.x is not None:
x_label = self.x.label or self.x.name or ""
- labels_seen[x_label] = "x"
+ labels_seen[(self.x.saved_metric, x_label)] = "x"
# Check Y-axis labels
for i, col in enumerate(self.y):
label = _metric_display_label(col)
- if label in labels_seen:
+ key = (col.saved_metric, label)
+ if key in labels_seen:
duplicates.append(
- f"y[{i}]: '{label}' (conflicts with {labels_seen[label]})"
+ f"y[{i}]: '{label}' (conflicts with {labels_seen[key]})"
)
else:
- labels_seen[label] = f"y[{i}]"
+ labels_seen[key] = f"y[{i}]"
Review Comment:
**Suggestion:** This XY duplicate-label validator has the same issue: using
`(saved_metric, label)` allows identical labels when one side is a saved metric
and the other is not. That contradicts the downstream form-data constraints
(which require unique labels) and can lead to compile-time duplicate-label
errors after this validation passes. Use the normalized display label as the
unique key regardless of `saved_metric`. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ XY charts with mixed metrics can still trigger duplicate-label errors.
- ⚠️ Some invalid XY configs pass schema but fail at query time.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Build an `XYChartConfig` (model in
`superset/mcp_service/chart/schemas.py:1613-1758`)
with `chart_type="xy"` and `y` containing two metrics that share the same
display label
but differ in `saved_metric`, for example `ColumnRef(name="revenue",
saved_metric=True)`
and `ColumnRef(name="revenue", aggregate="SUM", label="revenue")`.
2. When this config is instantiated (e.g. as `GenerateChartRequest.config` in
`superset/mcp_service/chart/schemas.py:1878-1904` or as part of
`UpdateChartPreviewRequest` in the preview tools), the
`XYChartConfig.validate_unique_column_labels()` model validator at
`schemas.py:1707-1758`
runs. For each Y metric it computes `label = _metric_display_label(col)`
(`schemas.py:1724`) and then `key = (col.saved_metric, label)`
(`schemas.py:1725`), so the
two metrics with label `"revenue"` generate distinct keys and bypass the
duplicate-label
check.
3. Downstream mapping in `map_xy_config()`
(`superset/mcp_service/chart/chart_utils.py:721-803`) converts both
ColumnRefs into
metrics in `form_data["metrics"]` with the same rendered label `"revenue"`
(saved metric
uses its name; the ad‑hoc metric uses the explicit label), and may also
include columns
with matching labels, yielding duplicate labels in the query definition.
4. When this `form_data` is executed through Superset's query pipeline (for
example via
`_compile_chart()` in `superset/mcp_service/chart/compile.py:143-200`, which
constructs a
`QueryContext` and runs `ChartDataCommand.validate()`),
`QueryObject._validate_no_have_duplicate_labels()` in
`superset/common/query_object.py:15-25` compares `metric_names +
column_names` and raises
a `QueryObjectValidationError` for the duplicated `"revenue"` label, even
though the XY
schema validator allowed the configuration.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c900696033f443dea1978896663261c8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c900696033f443dea1978896663261c8&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:** 1723:1731
**Comment:**
*Incorrect Condition Logic: This XY duplicate-label validator has the
same issue: using `(saved_metric, label)` allows identical labels when one side
is a saved metric and the other is not. That contradicts the downstream
form-data constraints (which require unique labels) and can lead to
compile-time duplicate-label errors after this validation passes. Use the
normalized display label as the unique key regardless of `saved_metric`.
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%2F39922&comment_hash=bdbc76d68c593a1d09f5c87d4b0704f08beb66ab00e3e469e6f31ba10305fab1&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=bdbc76d68c593a1d09f5c87d4b0704f08beb66ab00e3e469e6f31ba10305fab1&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]
