gabotorresruiz commented on code in PR #43737:
URL: https://github.com/apache/superset/pull/43737#discussion_r3961106798
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -389,6 +420,22 @@ def _validate_update_against_dataset(
}
)
+ try:
+ merged_gantt_config = validate_gantt_form_data(
Review Comment:
Hey Amin, thanks for the follow-up rounds. I verified two of the things I
had queued up are now fixed on this head: the Vega-Lite preview correctly skips
a NULL `end_dt` row and returns the renderable rows with trimmed fields, and an
update that omits `filters` now preserves the saved SIMPLE filters through
`preserve_previous_adhoc_filters`. Both confirmed live against a real MCP
server on 1dfd27f629.
One blocker remains though: `update_chart` with a config fails for every
saved Gantt chart when `generate_preview` is left at its default of True.
`_build_preview_form_data` injects `slice_name` into the merged preview state
(lines 368-370, alongside `slice_id` and `datasource`), and that merged dict is
what this gate feeds into `validate_gantt_form_data`. The transport allowlist
in `GanttChartConfig.adapt_native_form_data` pops `slice_id` and `datasource`
but not `slice_name`, so `UnknownFieldCheckMixin` raises and the tool returns
`ValidationError: Merged Gantt form data is invalid: Unknown field
'slice_name'`.
I verified it live on this head: the default flow fails with exactly that
error on a freshly generated Gantt chart, while the same request with
`generate_preview=False` succeeds. Since every saved chart has a `slice_name`,
this breaks the default update flow for the new chart type entirely.
The suite misses it because
`test_gantt_update_preview_matches_would_be_persisted_state` strips
`{"datasource", "slice_id", "slice_name"}` from the preview dict before
asserting, and nothing runs the preview-first output through
`validate_gantt_form_data`.
Fix looks like a one-liner: add `"slice_name"` to the transport-metadata pop
tuple in `adapt_native_form_data`. It is exactly the "saved chart transport
metadata" that allowlist comment describes, and the repo's saved `Gantt.yaml`
example shows UI-produced params contain only the allowlisted keys plus real
controls, so typo protection stays intact. Please also add a test that asserts
`validate_gantt_form_data(_build_preview_form_data(request, chart, config))`
returns a config for a chart with `slice_name` set, so the default flow stays
pinned.
--
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]
