rebenitez1802 commented on PR #40853:
URL: https://github.com/apache/superset/pull/40853#issuecomment-4663140045
From my agent reviewer:
Overall: solid fix, security model intact. One real bug to fix before merge.
π΄ High β Rename silently dropped
In _build_update_payload, the dataset-only branch returns {datasource_id,
datasource_type} and returns before the name branch β so dataset_id +
chart_name (no config) on the persist path loses the rename. The preview path
does apply it β preview/persist divergence. Test test_dataset_and_name_update
codifies the bug. Fix: add slice_name to the payload when chart_name is set;
update the test.
π‘ Medium β Dataset-only rebind skips validation
_validate_update_against_dataset runs only when config is present, so a
rebind keeps old params (old datasetβs columns/metrics) pointed at the new
dataset β chart can break silently with no warning.
π’ Low β datasource_type hardcoded to "table"
Fine (all MCP datasets are SqlaTable), but add a comment noting non-table
datasources arenβt supported.
π’ Low β DRY
effective_dataset_id block duplicated in _build_update_payload and
_build_preview_form_data; extract a helper.
π’ Low β Test gaps
No test for dataset_id+chart_name persist asserting rename; no negative test
for rebinding to an inaccessible dataset (the security-relevant case).
--
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]
