bito-code-review[bot] commented on code in PR #40853:
URL: https://github.com/apache/superset/pull/40853#discussion_r3383372229
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -459,14 +505,17 @@ async def update_chart( # noqa: C901
if parsed_config is not None:
with
event_logger.log_context(action="mcp.update_chart.validation"):
validation_error = _validate_update_against_dataset(
- parsed_config, preview_or_error, chart
+ parsed_config,
+ preview_or_error,
+ chart,
+ dataset_id=request.dataset_id,
)
if validation_error is not None:
return validation_error
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing validation in preview path</b></div>
<div id="fix">
The preview path (generate_preview=True) has the same validation gap as the
persist path. When only `dataset_id` is provided, validation is skipped. Add a
check: `if request.dataset_id is not None and parsed_config is None:
validation_error = _validate_update_against_dataset(None, {}, chart,
dataset_id=request.dataset_id)`
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/mcp_service/chart/tool/update_chart.py (lines 505-514) ---
505: # Validate before caching the form_data — same rationale
as above.
506: + # Also validate dataset-only updates to catch
invalid/inaccessible datasets
507: + if parsed_config is not None or request.dataset_id is not
None:
508: if parsed_config is not None:
509: with
event_logger.log_context(action="mcp.update_chart.validation"):
510: validation_error =
_validate_update_against_dataset(
511: parsed_config,
512: preview_or_error,
513: chart,
514: dataset_id=request.dataset_id,
515: )
516: + else:
517: + # Dataset-only update: validate the new dataset
exists
518: + with
event_logger.log_context(action="mcp.update_chart.validation"):
519: + validation_error =
_validate_update_against_dataset(
520: + None,
521: preview_or_error,
522: chart,
523: dataset_id=request.dataset_id,
524: )
```
</div>
</details>
</div>
<small><i>Code Review Run #77fe3f</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -116,13 +124,24 @@ def _build_update_payload(
else chart.slice_name or generate_chart_name(parsed_config)
)
- return {
+ payload: dict[str, Any] = {
"slice_name": chart_name,
"viz_type": new_form_data["viz_type"],
"params": json.dumps(new_form_data),
# Clear stale query_context so get_chart_data uses the updated
params.
"query_context": None,
}
+ if request.dataset_id is not None:
+ payload["datasource_id"] = request.dataset_id
+ payload["datasource_type"] = "table"
+ return payload
+
+ # Dataset-only update: rebind chart to a different dataset without
changing viz
+ if request.dataset_id is not None:
+ return {
+ "datasource_id": request.dataset_id,
+ "datasource_type": "table",
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing validation for dataset-only rebind</b></div>
<div id="fix">
When `dataset_id` is provided alone (without `config`), the code skips
validation entirely. The new dataset might not exist, might be inaccessible, or
might have incompatible schema. Add a validation call using
`_validate_update_against_dataset` with the new `dataset_id` before returning
the payload at line 140.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/mcp_service/chart/tool/update_chart.py (lines 139-144) ---
139: # Dataset-only update: rebind chart to a different dataset without
changing viz
140: if request.dataset_id is not None:
141: + # Validate the new dataset exists and is accessible before
rebinding
142: + validation_error = _validate_update_against_dataset(
143: + None, # parsed_config
144: + {}, # form_data (empty for dataset-only update)
145: + chart,
146: + dataset_id=request.dataset_id,
147: + )
148: + if validation_error is not None:
149: + return validation_error
150: return {
151: "datasource_id": request.dataset_id,
152: "datasource_type": "table",
153: }
```
</div>
</details>
</div>
<small><i>Code Review Run #77fe3f</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
