apache / apache/superset

fix(mcp): update_chart resets omitted color_scheme and row_limit to schema defaults

Open
#44,176 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
74.8k
Forks
18.3k
Avg merge
2d 5h
Merged PRs (30d)
685

Description

### Bug description

`update_chart` preserves controls the caller omits — except for any control whose mapper materializes a default. Those silently reset to the schema default instead of keeping the saved value.

`merge_chart_form_data` merges with `merged = {**existing_form_data, **new_form_data}`, and `new_form_data` comes from the `map_*_config` mapper, which always emits a value for fields like `color_scheme` (`config.color_scheme or "supersetColors"`) and `row_limit` (which carries a per-type schema default). Those keys are therefore present in the patch whether or not the caller set them, so they overwrite the saved value on every update. The merge has no way to tell "omitted" from "explicitly set to the default" once the mapper has run.

Reproduction, editing only the `size` metric of a saved bubble chart:

```python
saved = map_bubble_config(BubbleChartConfig(**base, color_scheme="lyftColors", row_limit=50))
# caller omits color_scheme and row_limit
cfg = BubbleChartConfig(**dict(base, size={"name": "gdp_total", "aggregate": "SUM"}))
merged = merge_chart_form_data(saved, map_bubble_config(cfg), cfg)
```

```
saved: color_scheme=lyftColors row_limit=50
merged: color_scheme=supersetColors row_limit=10000
```

Not specific to one chart type — the same edit on a saved pie chart:

```
pie: color_scheme lyftColors -> supersetColors | row_limit 25 -> 100
```

Expected: a control the caller did not mention keeps its saved value, matching how the same merge already treats filters, group-bys and axis options.

Actual: the user's color scheme and row limit are replaced with defaults, so an LLM client editing one metric quietly restyles the chart and changes how much data it queries.

### A fix already exists for one type

Gauge does this correctly. `merge_chart_form_data` takes the gauge branch and drops patch keys whose config field was never set:

```python
fields_set = config.model_fields_set
for config_field, form_data_field in _GAUGE_FORM_DATA_FIELD_MAP.items():
if config_field not in fields_set:
patch.pop(form_data_field, None)
```

Generalizing that `model_fields_set`-driven patch to every registered chart type looks like the shape of the fix — either a per-plugin config-field-to-form-data map, or having the mappers stop materializing defaults for optional presentation controls so the merge can distinguish an omission.

Worth deciding deliberately rather than per type, since "omitted means keep" vs "omitted means reset to default" is a contract every MCP chart tool shares, and `update_chart` already documents that it requires the full config.

Surfaced while reviewing the bubble chart plugin (#43572); filing separately because the behavior predates it and affects all ten registered chart types.

### Screenshots/recordings

N/A — MCP/backend behavior, reproduced through the Python API above.

### Superset version

master / latest-dev

### Python version

3.11

### Node version

I don't know

### Browser

Not applicable

### Additional context

Affected code: `superset/mcp_service/chart/chart_utils.py::merge_chart_form_data`, plus the per-type `map_*_config` mappers that emit default values.

### Checklist

- [x] I have searched Superset docs and Slack and didn't find a solution to my problem.
- [x] I have searched the GitHub issue tracker and didn't find a similar bug report.
- [x] I have checked Superset's logs for errors and if I found a relevant Python stacktrace, I included it here as text in the "additional context" section.

Contributor guide

Open the contributing guide

Research direction

Start in superset/mcp_service/chart/chart_utils.py::merge_chart_form_data and trace the per-type map_*_config mappers, comparing their behavior with the existing Gauge branch. Run the bubble and pie reproductions from the issue, then verify that omitted controls preserve saved values while explicitly supplied defaults still apply across the registered chart types.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend-api-design, data-visualization
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.