aminghadersohi opened a new pull request, #44405:
URL: https://github.com/apache/superset/pull/44405
## TL;DR
- MCP-created dashboards could end up with `position_json` where a
component's `parents` array held only its immediate parent instead of the full
ancestor chain from `ROOT_ID`.
- Server-side native-filter scope derivation
(`superset/dashboards/filter_scope.py`) trusts stored `parents` and tests it
against `scope.rootPath`, so a truncated chain makes `chartsInScope` derive to
`[]` for every dashboard-wide filter — no filter values get applied to any
chart's queries, and in some layouts the filter doesn't render at all.
- Fixes this by rebuilding every component's `parents` as the true ancestor
chain (from the layout's actual `children` edges) in every MCP tool that
persists `position_json`, so the stored layout always matches what a UI save
would have produced.
- Related to #40704 (same user-visible symptom via a different path).
## Why
`superset/mcp_service/dashboard/layout_validation.py` documents an
assumption that predates dashboard filter-scope derivation moving server-side:
"the frontend treats `parents` as derived metadata and recomputes it during
hydration... the validated child edges are authoritative." Accordingly, the MCP
layout tools validate a layout's `children` graph but never touch `parents` —
they persist whatever the caller supplied (or, for incremental edits, whatever
chain an existing node already had) verbatim.
That was safe when only the client read `parents`. It stopped being safe
once `get_chart_ids_in_scope` started deriving `chartsInScope` server-side by
testing each chart's stored `parents` against `scope.rootPath` (`["ROOT_ID"]`)
— there's no server-side equivalent of the frontend's
`updateComponentParentsList` hydration repair. A layout whose `parents` arrays
hold only an immediate parent (e.g. `["ROW-1"]` instead of
`["ROOT_ID","GRID_ID","TABS-1","TAB-1","ROW-1"]`) has zero overlap with
`rootPath`, so every native filter's `chartsInScope` derives to an empty list
on `GET /api/v1/dashboard/{id}` — dropping default filter values from every
chart's first query and, depending on filter-bar orientation, hiding the filter
from the bar entirely.
Two ways to fix this were considered:
1. **Fix it at the write path** — make the MCP always persist a `parents`
chain the client would also compute, so stored data matches what a UI save
produces. This is what this PR does.
2. **Fix it at the read path** — have `filter_scope.py` rebuild the parent
chain from `children` before deriving scope, mirroring the frontend's hydration
repair, so any programmatically-written `position_json` self-heals regardless
of source.
Option 2 is a reasonable follow-up for defense in depth (it would also
protect against non-MCP writers), but it's a larger change to a shared code
path used by every dashboard read, not just MCP-authored ones, and it doesn't
fix the underlying data — the stored layout would remain wrong for any other
consumer of `position_json` (export/import, other tooling). Option 1 fixes the
data at rest and keeps the blast radius contained to the MCP tools that produce
it, so it's the one implemented here.
## What
Adds `rebuild_parent_chains` to `layout_validation.py`: given a layout dict,
it walks `children` edges outward from `ROOT_ID` — never the existing `parents`
field, since that's exactly the data being repaired — and rewrites every
reachable component's `parents` as the full ancestor chain. It's defensive by
construction (skips malformed entries, tolerates cycles without raising) so
it's safe to run even on a layout that hasn't been through
`validate_dashboard_layout`.
Wired into every MCP tool that persists `position_json`:
- `generate_dashboard` — both the auto-generated grid (already correct, so
this is a no-op there) and a caller-supplied custom layout (previously written
completely unvalidated and untouched).
- `update_dashboard` — a caller-supplied replacement layout.
- `add_chart_to_existing_dashboard` — the newly inserted chart's chain was
previously extended from the target container's own (possibly
already-truncated) `parents`; now the whole layout is rebuilt after insertion.
- `remove_chart_from_dashboard` — doesn't itself introduce truncation, but
rebuilding here means a dashboard with pre-existing truncated `parents` (e.g.
from before this fix) self-heals the next time it's touched through the MCP.
Also checked for a related read-modify-write hazard (GET returning derived
scope values that get persisted back as if authoritative on the next write):
the MCP's native-filter tool builds `scope` from scratch on each call and never
round-trips a derived `chartsInScope`/`tabsInScope` value back into storage, so
it isn't affected.
## Blast radius
`superset/mcp_service/dashboard/` only — the four tools listed above. No
change to the REST API, the frontend, or `superset/dashboards/filter_scope.py`.
`rebuild_parent_chains` only ever narrows/corrects the `parents` field already
being written by these tools; it can't introduce a topology change since it
derives strictly from the `children` edges the tool itself just constructed or
the caller supplied.
## How to test
Added unit tests in `tests/unit_tests/mcp_service/dashboard/`:
- `test_layout_validation.py` — `rebuild_parent_chains` against a TABS
layout with truncated `parents` (reproducing the reported shape), a no-op case
on an already-correct layout, missing-`parents` handling, the detached empty
`GRID_ID` Superset retains alongside top-level TABS, cycles, and malformed
input.
- `test_dashboard_generation.py`, `test_update_dashboard.py`,
`test_add_chart_to_existing_dashboard.py`,
`test_remove_chart_from_dashboard.py` — one regression test per tool asserting
the persisted `position_json` carries full ancestor chains for a TABS layout
that started out truncated.
Manually verified the acceptance condition end to end against
`superset/dashboards/filter_scope.get_chart_ids_in_scope`: feeding it a
truncated TABS layout (mirroring the reported repro) returns `chartsInScope:
[]` for a dashboard-wide filter; the same layout after `rebuild_parent_chains`
returns every chart id, and `scope.excluded` is still honored when present.
## Risk & rollback
Pure addition to MCP-owned code; no schema or migration changes.
`rebuild_parent_chains` never raises, so a malformed layout that previously
round-tripped unchanged still does (it's simply left untouched when `ROOT_ID`
is missing/malformed). Revert is a plain `git revert` if needed.
## Review guidance
Start with `rebuild_parent_chains` in `layout_validation.py` — the four call
sites are mechanical once that's reviewed. `test_layout_validation.py`'s new
tests are the most direct spec for its behavior.
--
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]
