gabotorresruiz commented on code in PR #44188:
URL: https://github.com/apache/superset/pull/44188#discussion_r4020492781
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -694,6 +694,11 @@ def merge_chart_form_data( # noqa: C901
if "filters" not in fields_set:
preserve_previous_adhoc_filters(new_form_data, existing_form_data)
merged = {**existing_form_data, **new_form_data}
+ # Preserve the shared color/limit controls when omitted. Chart-specific
+ # presentation defaults retain their existing mapper behavior.
+ for field in ("color_scheme", "row_limit"):
+ if field not in fields_set and field in existing_form_data:
+ merged[field] = existing_form_data[field]
Review Comment:
Hi @dennisimoo, thanks for taking this on, and for the honest testing notes
and the deliberately bounded scope. The merge semantics change itself is right,
and your new tests correctly fail on unmodified master.
This block worries me a bit though: I believe it never fires on the real
`update_chart` path. Before the merge runs, `update_chart` replaces the config
with the output of `DatasetValidator.normalize_column_names`
(`update_chart.py`, `parsed_config = validation_config`), and every non-Gauge
plugin's `normalize_column_refs` round-trips the config through `model_dump()`
plus `model_validate()` (see `plugins/pie.py`). A plain `model_dump()`
materializes every default, so `model_validate` marks all fields as set, and
`"color_scheme" not in fields_set` is never true after that.
`update_chart_preview` normalizes before its merge too.
I verified it on this branch: a `PieChartConfig` built with only `dimension`
and `metric` has `model_fields_set == {"dimension", "metric"}`, but after
`DatasetValidator.normalize_column_names` (stubbed dataset context, no DB) it
has all 20 fields set, and merging against saved `color_scheme="lyftColors"`,
`row_limit=42` still produces `supersetColors` / `100`. Your tests pass because
they hand `merge_chart_form_data` the freshly constructed config, which is the
one state the live tool never sees.
Gauge is immune because its plugin already dumps with omission preserved
(`plugins/gauge.py`):
```python
config_dict = config.model_dump(exclude_unset=True)
```
Two possible paths forward:
- Switch the non-Gauge plugins' `normalize_column_refs` to
`model_dump(exclude_unset=True)`, matching Gauge. I tried this for the Pie
plugin on this branch: the normalized-path merge then preserves `lyftColors` /
`42`, and the full `tests/unit_tests/mcp_service/chart/` suite still passes
(1594 passed). Note it also revives the pre-existing `"filters" not in
fields_set` gate in this function, so each plugin's suite should be re-run when
flipping it.
- If touching normalization feels too broad here, capture `model_fields_set`
from the pre-normalization config and pass it into `merge_chart_form_data`
explicitly.
Either way, could you add a test that routes the config through
`DatasetValidator.normalize_column_names` (a stubbed `DatasetContext` is
enough) before calling `merge_chart_form_data`? That is the test that fails on
this branch today and would have caught the gap. Happy to dig in with you if
any of this is unclear.
--
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]
