bito-code-review[bot] commented on code in PR #40853:
URL: https://github.com/apache/superset/pull/40853#discussion_r3389822372
##########
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:
<!-- Bito Reply -->
The implementation correctly addresses the suggestion by adding the required
validation call using `_validate_update_against_dataset` before the dataset
rebind occurs. By passing `None` for the config and an empty dictionary for the
form data, the code ensures that the dataset existence and accessibility are
verified without triggering unnecessary compilation checks, which is
appropriate for a dataset-only update.
**superset/mcp_service/chart/tool/update_chart.py**
```
if request.dataset_id is not None:
# Validate the new dataset exists and is accessible before rebinding
validation_error = _validate_update_against_dataset(
None, # parsed_config
{}, # form_data (empty for dataset-only update)
chart,
dataset_id=request.dataset_id,
)
if validation_error is not None:
return validation_error
```
##########
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:
<!-- Bito Reply -->
The suggestion to add validation in the preview path for dataset-only
updates is appropriate. It ensures that dataset existence and accessibility are
verified even when no chart configuration is provided, maintaining consistency
with the persist path.
**superset/mcp_service/chart/tool/update_chart.py**
```
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: )
```
--
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]
