aminghadersohi commented on code in PR #40853:
URL: https://github.com/apache/superset/pull/40853#discussion_r3391505982
##########
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:
Fixed in c80a1b50. When both `dataset_id` and `chart_name` are provided
without `config`, `_build_update_payload` now includes `slice_name` in the
payload so the rename is persisted alongside the datasource rebind.
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -178,26 +204,38 @@ def _validate_update_against_dataset(
parsed_config: Any,
form_data: dict[str, Any],
chart: Any,
+ dataset_id: int | None = None,
+ run_compile_check: bool = True,
) -> GenerateChartResponse | None:
"""Run Tier 1 (schema) + Tier 2 (compile) validation against the chart's
dataset. Returns ``None`` on success, or a :class:`GenerateChartResponse`
error envelope on failure that callers should return as-is.
+
+ When ``dataset_id`` is provided, validates against that dataset instead of
+ the chart's existing datasource (used when rebinding to a new dataset).
+ Pass ``run_compile_check=False`` to skip the Tier 2 live-query check (used
+ for dataset-only rebinds where no new chart config is provided).
"""
from superset.daos.dataset import DatasetDAO
- dataset = getattr(chart, "datasource", None)
- if dataset is None and getattr(chart, "datasource_id", None) is not None:
- dataset = DatasetDAO.find_by_id(chart.datasource_id)
+ if dataset_id is not None:
+ dataset = DatasetDAO.find_by_id(dataset_id)
Review Comment:
Acknowledged. The intent here was to keep the pre-validation lightweight —
the actual write path enforces access via `UpdateChartCommand` (which calls
`security_manager.raise_for_access`). For `run_compile_check=False`
(dataset-only rebind) the only information that leaks before the write check is
whether the dataset ID exists, which is a minor exposure. Adding a full
`has_dataset_access` check here introduces complexity and test surface, and the
write path already gates the mutation. Happy to add it if the reviewer
considers it necessary.
--
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]
