bito-code-review[bot] commented on code in PR #44393:
URL: https://github.com/apache/superset/pull/44393#discussion_r4150330488
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -956,31 +1154,39 @@ 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:
- # Dataset-only rebind: verify the target dataset exists before
- # caching. Skip compile check — no new config to execute.
+ elif rebind_id is not None:
+ # Table-only rebinds preserve legacy existence-only validation.
+ # Semantic transitions must validate the retained query roles.
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
+ if DatasourceType.SEMANTIC_VIEW.value
+ in (rebind_type, _chart_datasource_type(chart))
+ else {},
Review Comment:
<!-- Bito Reply -->
The suggestion is locally correct in its intent to ensure that adhoc filters
are validated against the target dataset during a rebind. Applying this
suggestion improves the code by ensuring consistent validation behavior across
both preview and persistence paths, preventing potential issues where filters
might be cached without proper validation. The reviewer's concern regarding the
removal of `preview_or_error` is valid, as it was previously used to pass the
merged form data for validation.
**superset/mcp_service/chart/tool/update_chart.py**
```
with event_logger.log_context(action="mcp.update_chart.validation"):
validation_error = _validate_update_against_target(
None,
preview_or_error
if DatasourceType.SEMANTIC_VIEW.value
in (rebind_type, _chart_datasource_type(chart))
else {},
```
--
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]
