msyavuz opened a new pull request, #44235:
URL: https://github.com/apache/superset/pull/44235
### SUMMARY
A dashboard's stored `position_json` is not guaranteed to be a tree. It can
name a component that is
also one of its own ancestors. Nothing on the write path rejects that:
`DashboardPutSchema.position_json`
is validated with `validate_json`, which only checks that the string parses.
When such a layout is loaded, two things go wrong in sequence:
1. `updateComponentParentsList` walks the layout's children recursively with
no record of what it has
already visited. Following a self-reference recurses until the stack
overflows with
`RangeError: Maximum call stack size exceeded`. It runs during hydration
(`actions/hydrate.ts`) and again after every layout action
(`reducers/dashboardLayout.ts`), so the
dashboard fails to render and every subsequent edit throws too.
2. With that fixed, hydration succeeds and the cyclic layout is handed to
the React tree, where the
components render each other without bound until the renderer dies — so
the dashboard still never
appears.
This fixes both:
- `updateComponentParentsList` now tracks visited components and skips one
the layout reaches more than
once, logging a warning. This also stops a component reachable from two
parents being re-walked once
per path, which was exponential on a deep layout.
- `DashboardComponent` is the single container every nested component
renders through, so the repeat is
guarded there: a component cannot be its own ancestor, so when the layout
reaches one from its own
descendant it renders nothing and logs a warning.
Both mirror how the backend already degrades on the same data condition —
`Dashboard.tabs` in
`models/dashboard.py` carries a `visited` set and warns rather than raising
(#43575).
The guard suppresses the cyclic subtree rather than repairing it, so an
affected dashboard opens with
those components missing. That is deliberate and is enough to reach edit
mode and fix the layout by hand,
which is not possible today. Adding server-side structural validation of
`position_json` would stop new
corruption being written, but it turns payloads that are accepted today into
400s, so it is left out of
this change — #43574 called that out as needing its own discussion.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before — dashboard never renders, `Maximum call stack size exceeded`, no way
into edit mode:
https://media.app.shortcut.com/api/attachments/files/clubhouse-assets/5d8baaab-e1a8-4512-a9c7-12979c8cd1c9/6aa7a0cd-e8f7-42ac-83d2-d54506368134/tc2-before.webm
After — dashboard renders, no console errors, edit mode works:
https://media.app.shortcut.com/api/attachments/files/clubhouse-assets/5d8baaab-e1a8-4512-a9c7-12979c8cd1c9/6aa7a0cd-5a2c-4b57-8f9d-3260cf529d68/tc2-after.webm
A healthy, acyclic dashboard is unaffected:
https://media.app.shortcut.com/api/attachments/files/clubhouse-assets/5d8baaab-e1a8-4512-a9c7-12979c8cd1c9/6aa7a0cf-9bc1-4a22-9342-a6ee833f9f8e/tc3-after.webm
### TESTING INSTRUCTIONS
Automated:
```bash
cd superset-frontend
npm run test -- src/dashboard/util/updateComponentParentsList.test.ts
src/dashboard/containers/DashboardComponent.test.tsx
```
Both suites fail on `master` (the first with `RangeError: Maximum call stack
size exceeded`) and pass with
this change.
Manual:
1. Create a dashboard, then `PUT /api/v1/dashboard/{id}` a `position_json`
where a component is reachable
from its own descendant, e.g. `GRID_ID -> ROW-a -> ROW-b -> ROW-a`. The
API accepts it.
2. Open that dashboard. On `master` the page stays blank and the console
shows
`Maximum call stack size exceeded`; with this change the dashboard
renders, the console is clean, and
"Edit dashboard" works so the layout can be repaired.
3. Open any normal dashboard and confirm it is unchanged.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] 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]
