DanRoscigno opened a new pull request, #42841:
URL: https://github.com/apache/superset/pull/42841
### SUMMARY
`XYChartConfig` and `MixedTimeseriesChartConfig` accept `x_axis` as a
validation alias for the x column — native `form_data` names the column that
way — while also declaring a separate `x_axis: AxisConfig` styling field. One
input key fed both fields.
That was survivable while nested models ignored unknown keys: a column dict
sent as `x_axis` set `x` and quietly built an unused empty `AxisConfig`. Since
#42732 made the nested models reject unknown fields, whichever of the two
models the payload does not fit now rejects it. Probing current master,
`XYChartConfig` with no explicit `x`:
| payload | master | with this PR |
| --- | --- | --- |
| `x_axis: {"name": "category"}` | `Unknown field 'name'. Valid fields:
format, scale, title` | sets `x` |
| `x_axis: "category"` | `Input should be a valid dictionary or instance of
AxisConfig` | sets `x` |
| `x_axis: {"title": "State"}` | `Unknown field 'title'. Valid fields:
aggregate, column_name, dtype, ...` | sets the axis |
The first is a regression from #42732 — that payload worked before it
merged. The other two predate it. The common thread is that every spelling of
`x_axis` is now rejected on these two chart types, and each error names the
model the caller *wasn't* thinking about, which is the opposite of what the
unknown-field checking is for.
#42732 met this same collision in
`test_known_aliases_not_flagged_as_unknown` and worked around it by switching
that test to `x_column`, documenting the ambiguity in the docstring. This PR
removes the ambiguity instead.
`x_axis` is now routed by shape in a `model_validator(mode="before")`, and
dropped from `x`'s `AliasChoices` so it no longer feeds both fields. A bare
string, or a mapping naming no `AxisConfig` field, is a column reference and
moves to `x`; anything else configures the axis. Supplying the column twice
(`x` plus a column-shaped `x_axis`) is now an explicit error rather than a
silent pick.
Keying that check off the *styling* names rather than the column names is
deliberate — it keeps a mistyped styling key reported against the model the
caller meant:
```python
{"x_axis": {"title": "State", "sort_by": "metric"}}
# -> Unknown field 'sort_by'. Valid fields: format, scale, title
{"x_axis": {"name": "category", "aggregat": "SUM"}}
# -> Unknown field 'aggregat' — did you mean 'aggregate'?
```
I checked every model in `schemas.py` for the same field-name/alias overlap;
`x_axis` on these two classes is the only instance.
Worth a reviewer's eye:
`test_mixed_timeseries_pre_validate_accepts_x_axis_alias` on master asserts the
`x_axis` alias is accepted, and passes today only because `pre_validate`
inspects the raw dict and never constructs the model. It keeps passing here,
and now the model behind it agrees.
### TESTING INSTRUCTIONS
New `TestXAxisRouting` covers both chart types: column-shaped `x_axis`
(dict, `column_name` alias, bare string), styling-shaped `x_axis`, both
supplied together, the column-named-twice rejection, and that a mistyped key is
reported against whichever model the payload was meant for.
```bash
pytest tests/unit_tests/mcp_service/chart/test_chart_schemas.py -k
XAxisRouting
```
Verified locally against this branch:
- `pytest tests/unit_tests/mcp_service/chart` — 1182 passed
- `pytest tests/unit_tests/mcp_service` — 3326 passed, 1 failed:
`test_mcp_e2e_smoke.py::test_tools_call_health_check_over_real_asgi_transport`,
which fails identically on clean master and is unrelated to this change
- `pre-commit run --files superset/mcp_service/chart/schemas.py
tests/unit_tests/mcp_service/chart/test_chart_schemas.py` — all hooks pass,
including mypy, ruff and pylint
To see the current behaviour before the fix, on master:
```python
XYChartConfig.model_validate({
"chart_type": "xy",
"y": [{"name": "sales", "aggregate": "SUM"}],
"x_axis": {"name": "category"},
})
# ValidationError: Unknown field 'name'. Valid fields: format, scale, title
```
### ADDITIONAL INFORMATION
- [x] Has associated issue: follow-up to #42626 / #42732
- [ ] 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
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
