mikebridge commented on code in PR #44393:
URL: https://github.com/apache/superset/pull/44393#discussion_r4050968887
##########
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:
Addressed in
https://github.com/apache/superset/commit/7cdd538995955741071aaebba270856e363ef28e
— restored table-only source-rebind preview/save parity without weakening
semantic retained-role validation. The new retained-legacy-filter entrypoint
test failed with invalid_column in preview mode before the fix; both modes pass
afterward, preserving filters and the source-only save payload. Full MCP suite:
4,376 passed, 1 skipped under UTC; scoped hooks/MyPy and Pylint pass. Thanks
for catching this.
##########
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:
Addressed in
https://github.com/apache/superset/commit/7cdd538995955741071aaebba270856e363ef28e
— saved semantic previews use resolve_semantic_view and its existing
authorization check, never a colliding table lookup. Added save_chart=True +
generate_preview=True coverage and allowed/denied/missing preview cases;
denied/missing views cannot render. All new controls failed on the previous
code for the forbidden table lookup and pass with the fix. Independent
authorization review approved the change; the table guest path is unchanged.
Thanks.
--
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]
