tien238lnd opened a new pull request, #43575:
URL: https://github.com/apache/superset/pull/43575
### SUMMARY
`position_json` is validated for JSON parseability only, so a layout whose
`children` array names an id that is not a key in that layout is accepted by
`PUT /api/v1/dashboard/{id}` and stored. `Dashboard.tabs` then raises
`KeyError` walking it, and since `UpdateDashboardCommand.process_tab_diff`
reads the layout from the database rather than from the request body, every
subsequent update to that dashboard returns 500 — including the update that
would repair the layout. The dashboard is left unwritable through the API,
recoverable only by export → edit YAML → import with overwrite.
The write that causes it returns 200 and the failure surfaces on the *next*
request, so nothing points back at the request responsible.
Two changes:
**`superset/commands/dashboard/update.py`** — `find_deleted_tabs` computed
`self._model.tabs` before checking whether the payload carried a
`position_json`, so every `PUT` walked the whole stored layout, `{"published":
true}` included. It now returns early when there is no incoming layout. A tab
can only be deleted by an update that supplies one, so behaviour is unchanged
for every input that could produce a non-empty result.
**`superset/models/dashboard.py`** — `Dashboard.tabs` no longer raises on
stored layout data:
- `get_node` uses `.get()`; a `children` entry that is missing or is not an
object is skipped rather than dereferenced. Skipping is required, not just
tolerating: a `None` pushed onto the queue would fail on `node.get("children",
[])` in the next iteration with `AttributeError`, which is still a 500.
- A node with no `type` is skipped. A node whose `type` is present but not
one of `ROOT`/`GRID`/`TABS`/`TAB` is walked through without contributing to the
tree, exactly as before — that is how ordinary rows and charts have always been
handled, so it stays silent.
- A `TAB` whose title cannot be read gets an empty title. `meta` is checked
with `isinstance`, not just `.get("meta", {})`: a `meta` that is present but is
a string or a list would otherwise raise `AttributeError`, which is *not*
caught in `superset/dashboards/api.py`, turning a 400 into a 500.
- A layout with no usable `ROOT_ID`, or one that parses to something other
than an object (`null`, `[]`), returns `{}`.
The patch does not hide the problem: every node that is skipped or degraded
is logged at WARNING with the dashboard id and the node id, so a broken layout
stays visible to operators instead of quietly producing fewer tabs. Well-formed
layouts produce identical output.
No structural validation was added to `DashboardPutSchema`, on purpose. It
would turn payloads that are accepted today into 400s — a breaking change for
any client scripting dashboards — and doing it properly means formally
specifying the `position_json` invariants, which Superset has never specified.
That belongs in its own discussion.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable — backend-only change, no UI surface.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/models/dashboard_test.py \
tests/unit_tests/commands/dashboard/update_test.py
```
The pre-existing integration coverage for the function reworked in
`update.py` should also still pass:
```bash
pytest tests/integration_tests/dashboards/update_tabs_test.py
pytest tests/integration_tests/dashboards/api_tests.py -k
test_get_dashboard_tabs
```
Cases covered by the new tests:
`tests/unit_tests/models/dashboard_test.py` — `Dashboard.tabs`
- a well-formed layout, including a tab nested inside another tab, returns
the same `all_tabs` mapping and `tab_tree` shape as before (the regression
guard)
- a `children` entry naming an id that is not in the layout is skipped, and
a WARNING is logged
- a `children` entry resolving to something that is not an object is
skipped, and a WARNING is logged
- a node with no `type` is skipped, and a WARNING is logged
- a `TAB` with no `meta`, an empty `meta`, or a non-object `meta` gets an
empty title, and a WARNING is logged
- a `TAB` with no `id` is left out of `all_tabs`, and a WARNING is logged
- a layout with no `ROOT_ID`, an empty layout, and a layout that is not a
mapping all return `{}`
`tests/unit_tests/commands/dashboard/update_test.py` — `process_tab_diff`
- a payload with no `position_json` does not read `self._model.tabs` at all
- a payload that drops a tab still deactivates the reports using it
Verified as real regression guards: reverting only the two source files
fails 8 of the 17 new tests, and downgrading the `logger.warning` calls to
`debug` fails 7 of them.
Manual check, against an affected dashboard: `GET
/api/v1/dashboard/{id}/tabs` returns 200 with the tabs that resolve instead of
500, and `PUT /api/v1/dashboard/{id}` with a corrected `position_json` now
succeeds and repairs the dashboard.
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #43574
- [ ] 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
Not a breaking change: no request that succeeds today starts failing, and no
response shape changes for a well-formed layout.
--
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]
