DanRoscigno opened a new pull request, #42628:
URL: https://github.com/apache/superset/pull/42628
### SUMMARY
`UnknownFieldCheckMixin` in `superset/mcp_service/chart/schemas.py` exists
to stop unknown fields being silently dropped:
> Catches fields that would be silently dropped by `extra='ignore'` and
provides actionable error messages to help LLMs self-correct parameter names.
All ten top-level chart config models inherit it. The nested models they are
composed of do not — `ColumnRef`, `AxisConfig`, `LegendConfig`,
`CurrencyFormat`, `FilterConfig` and `SortByConfig` are plain `BaseModel`, so
pydantic's default `extra="ignore"` applies one level down.
The result is that unknown-field protection stops at exactly one level of
nesting:
```python
# rejected — "Unknown field 'sort_by'. Valid fields: ..."
parse_chart_config({**base, "sort_by": "metric"})
# accepted, silently discarded
parse_chart_config({**base, "x_axis": {"title": "State", "sort_by":
"metric"}})
# -> AxisConfig(title='State', scale='linear', format=None)
```
Through the tools this is worse than a plain rejection: `update_chart`
returns `success: true` with a chart URL and no warnings, while the stored
`form_data` is unchanged. A client — human or agent — reasonably concludes the
setting was applied, and confirming otherwise requires a follow-up
`get_chart_info` plus knowledge of which native `form_data` key the config
field maps to. An agent that self-corrects on error has nothing to correct
against.
This PR makes the six nested models inherit the mixin so they produce the
same "did you mean?" errors as top-level configs.
`_check_unknown_fields` is already generic over `model_class` and resolves
aliases through `_get_known_fields`, so no other change was needed and existing
aliases (`column_name`, `col`, `opr`, `val`) keep working.
**Behaviour change worth a reviewer's attention:** payloads carrying extra
keys inside nested objects now fail instead of being silently accepted. That is
the mixin's stated intent and matches existing top-level behaviour, but it is a
change for any client currently sending extras. I checked that nothing internal
relies on the lax behaviour — there are no `Model(**kwargs)` constructions, no
direct `model_validate` calls, and no subclasses of these six models anywhere
in `superset/`.
### TESTING INSTRUCTIONS
Added to
`tests/unit_tests/mcp_service/chart/test_chart_schemas.py::TestUnknownFieldDetection`:
- `test_nested_models_reject_unknown_fields` — parametrized over all six
models
- `test_unknown_field_nested_in_axis_config_rejected` — through the real
nesting path via `XYChartConfig`
- `test_nested_aliases_still_accepted` — guards the alias behaviour that is
the main regression risk
```bash
pytest tests/unit_tests/mcp_service/chart/test_chart_schemas.py -k
UnknownField
```
Manual check against a running instance (Superset 6.1.0, StarRocks-backed
virtual dataset):
1. `generate_chart` an `xy` bar chart and note `x_axis_sort_series_type:
"name"` in the saved `form_data`
2. `update_chart` with `config.x_axis = {"title": "...", "sort_by":
"metric"}`
3. Before: returns `success: true`; `get_chart_info` shows `form_data`
unchanged
4. After: rejected with `Unknown field 'sort_by'. Valid fields: format,
scale, title`
I verified the patched classes against the 6.1.0 runtime by loading a
patched copy of its own `schemas.py` — nested unknown fields are rejected,
aliases (`column_name` → `name`, `col` → `column`) still resolve, and valid
nested configs are still accepted. `CurrencyFormat` and `SortByConfig` do not
exist in 6.1.0, so those two were exercised only by the new unit tests. `ruff
check` and `ruff format` are clean on both files. I did not have a full
Superset dev environment available to run the whole unit suite locally, so I'm
relying on CI for that.
### ADDITIONAL INFORMATION
- [x] Has associated issue: #42626
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
