gabotorresruiz commented on code in PR #44393:
URL: https://github.com/apache/superset/pull/44393#discussion_r4050551997
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -956,31 +1154,36 @@ async def update_chart( # noqa: C901
# Validate before caching the form_data — same rationale as above.
if validation_config is not None:
with
event_logger.log_context(action="mcp.update_chart.validation"):
- validation_error = _validate_update_against_dataset(
+ validation_error = _validate_update_against_target(
validation_config,
preview_or_error,
chart,
- dataset_id=request.dataset_id,
+ rebind_id,
+ rebind_type,
)
if validation_error is not None:
return validation_error
- elif request.dataset_id is not None:
+ elif rebind_id is not None:
# Dataset-only rebind: verify the target dataset exists before
# caching. Skip compile check — no new config to execute.
with
event_logger.log_context(action="mcp.update_chart.validation"):
- validation_error = _validate_update_against_dataset(
+ validation_error = _validate_update_against_target(
None,
- {},
+ preview_or_error,
Review Comment:
I would fix this one before merge, and it lands on the table path rather
than the semantic one.
Swapping `{}` for `preview_or_error` here means a source-only rebind in
preview mode now runs `_validate_adhoc_filter_columns` over the chart's
retained `adhoc_filters` against the **new** dataset. The save branch at line
1132 still passes `new_form_data or {}`, and for a table to table rebind
`_source_rebind_payload` emits no `params`, so there it stays `{}`. The two
branches now disagree about the same request.
I verified it on this branch against the merge base `35768da`, same chart
and same mocks, a chart whose `params` carry `adhoc_filters: [{"subject":
"legacy_dim", ...}]` rebound to a dataset that has no `legacy_dim`:
```
head generate_preview=True success=False invalid_column "Filter
references column(s) not in dataset: legacy_dim"
head generate_preview=False success=True
base generate_preview=True success=True
base generate_preview=False success=True
```
So `update_chart(identifier=X, dataset_id=Y)` with the default
`generate_preview=True` now refuses a rebind that still saves fine with
`generate_preview=False`, and the remedy that error suggests (`'filters': []`)
requires passing a `config`, which turns the source-only rebind into a
different operation. The comment two lines up still reads "verify the target
dataset exists before caching", which is what makes me read this as incidental
rather than intended.
Narrowest fix I can see that keeps the semantic gain:
```python
validation_error = _validate_update_against_target(
None,
preview_or_error
if DatasourceType.SEMANTIC_VIEW.value
in (rebind_type, _chart_datasource_type(chart))
else {},
chart,
rebind_id,
rebind_type,
run_compile_check=False,
)
```
A case in `test_update_chart_rebind.py` that rebinds a table chart carrying
an `adhoc_filters` entry the target dataset lacks, once with
`generate_preview=True` and once with `False`, would pin whichever behavior you
pick. The current suite misses it because every rebind test either supplies a
`config` or uses a chart with no retained filters.
##########
superset/mcp_service/chart/tool/generate_chart.py:
##########
@@ -741,7 +828,9 @@ async def generate_chart( # noqa: C901
)
# Convert dataset_id to int only if numeric
- if (
+ if target is not None:
+ dataset_id_for_preview = target.id
Review Comment:
Not a blocker. This is the one boundary left that still resolves a view id
as a dataset, and both new tools reach it on their own success path.
The branch at line 799 hands the saved chart to
`_get_chart_preview_internal`, which calls
`validate_chart_dataset(chart.datasource_id, check_access=True)` at
`superset/mcp_service/chart/tool/get_chart_preview.py:1282`, and that is a
`DatasetDAO.find_by_id` against `SqlaTable`. `update_chart` reaches the same
code from its `preview_formats` loop.
I ran `generate_chart(view_id=1, save_chart=True)` on this branch with the
default `generate_preview=True` and a mocked `CreateChartCommand` returning a
chart with `datasource_type="semantic_view"`: `DatasetDAO.find_by_id(1,
skip_base_filter=False)` fires, and the tool returns `success=True` with an
empty `previews` and the warning `Dataset (ID: 1) has been deleted or does not
exist`. Where a table happens to share that id, the gate then passes or fails
for reasons that have nothing to do with the view.
`test_generate_view_never_looks_up_colliding_table` misses it because it
sets `generate_preview=not save_chart`, so the saved branch never runs a
preview. Skipping `validate_chart_dataset` for `semantic_view` charts the way
`update_chart` already does at line 1009, plus a `save_chart=True,
generate_preview=True` variant of that test, would close it.
--
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]
