DanRoscigno opened a new issue, #42626:
URL: https://github.com/apache/superset/issues/42626
### Bug
`superset/mcp_service/chart/schemas.py` defines `UnknownFieldCheckMixin` to
stop unknown fields from being silently dropped:
```python
def _check_unknown_fields(data: Any, model_class: type[BaseModel]) -> Any:
"""Raise ValueError for unrecognized fields with 'did you mean?'
suggestions.
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 — `PieChartConfig`,
`PivotTableChartConfig`, `MixedTimeseriesChartConfig`, `HandlebarsChartConfig`,
`BigNumberChartConfig`, `TableChartConfig`, `XYChartConfig`,
`HistogramChartConfig`, `BoxPlotChartConfig`, `WaterfallChartConfig`.
The nested models those configs are composed of do not:
| Model | Line (master @ `7b351d5`) | Base |
|---|---|---|
| `ColumnRef` | 774 | `BaseModel` |
| `AxisConfig` | 895 | `BaseModel` |
| `LegendConfig` | 901 | `BaseModel` |
| `CurrencyFormat` | 906 | `BaseModel` |
| `FilterConfig` | 929 | `BaseModel` |
| `SortByConfig` | 997 | `BaseModel` |
So pydantic's default `extra="ignore"` applies one level down, and
unknown-field protection is enforced at exactly one level of nesting.
### Reproduction
```python
from superset.mcp_service.chart.schemas import parse_chart_config
base = {"chart_type": "xy", "kind": "bar",
"x": {"name": "state"}, "y": [{"name": "orders", "aggregate":
"SUM"}]}
# unknown field NESTED inside x_axis
cfg = parse_chart_config({**base, "x_axis": {"title": "State", "sort_by":
"metric"}})
print(cfg.x_axis)
# the same unknown field at TOP level
parse_chart_config({**base, "sort_by": "metric"})
```
Output:
```
AxisConfig(title='State', scale='linear', format=None) # sort_by
silently gone
ValueError: 1 validation error for tagged-union[...]
xy
Value error, Unknown field 'sort_by'. Valid fields: breakdown, chart_type,
...
```
Same asymmetry through the MCP tools. Against a saved bar chart,
`update_chart` with an unknown field nested in `x_axis`, `x`, or `legend`
returns `success: true` and a chart URL:
```json
{"chart": {"id": 1, "slice_name": "Orders by Customer State", ...},
"success": true, "error": null, "warnings": []}
```
while a subsequent `get_chart_info` shows the stored `form_data` unchanged:
```json
"x_axis_sort_series_type": "name", "x_axis_sort_series_ascending": true
```
The same field at the top level of `config` is correctly rejected with the
"Valid fields:" list.
### Impact
The mixin exists specifically so that LLM clients get an actionable error
instead of a silent drop. Nested objects are where axis, legend, currency and
filter options live — precisely the options a client is most likely to guess
at, and the ones least likely to be memorised correctly.
The failure mode is worse than a plain rejection. The tool returns `success:
true` with a chart URL and no warnings, so the caller reasonably concludes the
setting was applied. Detecting otherwise requires a follow-up `get_chart_info`
*and* knowing which native `form_data` key the config field maps to. An agent
that self-corrects on error has nothing to correct against, so it moves on
believing the chart is configured.
Encountered while trying to sort a bar chart by value via `x_axis:
{"sort_by": "metric"}`: three successive `update_chart` calls each returned
success and changed nothing.
### Suggested fix
Have the nested models inherit `UnknownFieldCheckMixin` instead of
`BaseModel`:
```python
class AxisConfig(UnknownFieldCheckMixin):
...
```
`_check_unknown_fields` is already generic over `model_class` and resolves
aliases via `_get_known_fields`, so no other change is required. Models using
`validation_alias` (`ColumnRef`, `FilterConfig`, `SortByConfig`) keep working,
since `_get_known_fields` already collects `AliasChoices`.
One consideration for reviewers: this makes previously-accepted payloads
fail. That is the intent of the mixin, and it matches the existing top-level
behaviour, but it is a behaviour change for any client currently passing extra
nested keys.
I'm happy to open a PR with the change plus a regression test in
`tests/unit_tests/mcp_service/chart/test_chart_schemas.py`.
### Environment
- Apache Superset 6.1.0 (docker), pydantic 2.11.7, fastmcp 3.4.5, Python
3.10.20
- Class definitions confirmed unchanged on `master` @ `7b351d5`
--
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]
