codeant-ai-for-open-source[bot] commented on code in PR #36935:
URL: https://github.com/apache/superset/pull/36935#discussion_r2666613761
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -835,7 +835,7 @@ class GenerateChartRequest(QueryCacheControl):
dataset_id: int | str = Field(..., description="Dataset identifier (ID,
UUID)")
config: ChartConfig = Field(..., description="Chart configuration")
save_chart: bool = Field(
- default=True,
+ default=False,
description="Whether to permanently save the chart in Superset",
)
generate_preview: bool = Field(
Review Comment:
**Suggestion:** Logic bug: a request where both `save_chart` and
`generate_preview` are False will do nothing (no chart saved and no preview
generated). Add a post-validation to `GenerateChartRequest` to reject requests
that have both flags false so callers can't send no-op requests that downstream
code may not handle. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
@model_validator(mode="after")
def validate_save_or_preview(self) -> "GenerateChartRequest":
"""Ensure at least one of save_chart or generate_preview is
enabled."""
if not getattr(self, "save_chart", False) and not getattr(
self, "generate_preview", False
):
raise ValueError(
"At least one of 'save_chart' or 'generate_preview' must be
True"
)
return self
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This validator catches a real, lightweight correctness issue: a request with
both save_chart and generate_preview set to False is likely a no-op and
probably not intended. The class already uses pydantic's model_validator and
adding an "after" validator is consistent with existing patterns (see
validate_cache_timeout). The change is low-risk, addresses a potential wasted
work / confusion, and is implemented cleanly as a model-level check. Be aware
it can be a breaking behavioral change if external callers intentionally relied
on allowing both to be False.
</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/schemas.py
**Line:** 841:841
**Comment:**
*Logic Error: Logic bug: a request where both `save_chart` and
`generate_preview` are False will do nothing (no chart saved and no preview
generated). Add a post-validation to `GenerateChartRequest` to reject requests
that have both flags false so callers can't send no-op requests that downstream
code may not handle.
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>
--
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]
