aminghadersohi commented on code in PR #44405:
URL: https://github.com/apache/superset/pull/44405#discussion_r4049811024
##########
superset/mcp_service/dashboard/layout_validation.py:
##########
@@ -310,3 +310,53 @@ def validate_dashboard_layout( # noqa: C901
return f"Layout references charts not associated with the dashboard:
{unknown}."
return None
+
+
+def rebuild_parent_chains(layout: dict[str, Any]) -> dict[str, Any]:
+ """Return ``layout`` with every reachable component's ``parents`` rebuilt.
+
+ ``parents`` is supposed to hold the full ancestor chain from ``ROOT_ID``,
+ but an MCP-authored layout may carry only the immediate parent (or omit
+ the field). The frontend repairs this on every load via
+ ``updateComponentParentsList`` during hydration; ``superset.dashboards.
+ filter_scope`` trusts the stored value instead, so a truncated chain
+ silently empties every native filter's ``chartsInScope`` on read. This is
+ the server-side equivalent of that client-side repair, run before a
+ layout is persisted.
+
+ Walks ``children`` edges outward from ``ROOT_ID`` rather than trusting
+ the existing ``parents`` field, which may be exactly the stale data being
+ repaired. A component unreachable from ``ROOT_ID`` — a malformed layout,
+ or the detached empty ``GRID_ID`` Superset retains alongside top-level
+ TABS — is left untouched, as is ``ROOT_ID`` itself, which never carries a
+ ``parents`` key. Safe to call on layouts that have not been through
+ ``validate_dashboard_layout``: malformed entries and cycles are skipped
+ rather than raised.
+ """
+ root = layout.get(_ROOT_ID)
+ if not isinstance(root, dict):
+ return layout
+
+ rebuilt = dict(layout)
+ visited: set[str] = {_ROOT_ID}
+ stack: list[tuple[str, list[str]]] = [
+ (child_id, [_ROOT_ID])
+ for child_id in reversed(root.get("children") or [])
Review Comment:
Good catch — valid, and it does raise. `reversed()` on a non-list truthy
value blows up before any of the per-child `isinstance` filtering runs:
```
TypeError: 'int' object is not reversible
```
As you note, `update_dashboard` is shielded by `_validate_update_request`,
but `generate_dashboard` hands a caller-supplied `position_json` straight to
`rebuild_parent_chains` with no pre-flight, so the docstring's "malformed
entries and cycles are skipped rather than raised" promise was reachable-false
there.
Fixed in bdffabf780: `ROOT_ID`'s `children` now gets the same
`isinstance(..., list)` guard the child branch applies, falling back to `[]` so
nothing is reachable from a malformed root and every component keeps the
`parents` it came in with.
Added `test_generate_dashboard_position_json_root_children_not_a_list`,
which drives the `generate_dashboard` path with `"ROOT_ID": {"type": "ROOT",
"children": 1}`. Confirmed it reproduces the `TypeError` with the guard
reverted and passes with it in place.
##########
tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py:
##########
@@ -773,11 +775,118 @@ async def test_generate_dashboard_position_json_override(
# and verify the caller's layout — not the auto-generated 2-col
# grid — was written.
stored = json.loads(created.position_json)
- assert stored == custom_layout
# The auto-generated layout's HEADER/ROW ids wouldn't match
# `ROW-custom`; this sanity-check guards against regressions
# where the override silently merges with the default.
assert "ROW-custom" in stored
+ # children/meta pass through unchanged; only `parents` is
+ # (re)computed from the children edges.
+ assert stored["ROW-custom"]["children"] == ["CHART-1"]
+ assert stored["ROW-custom"]["meta"] == {
+ "background": "BACKGROUND_TRANSPARENT"
+ }
+ assert stored["CHART-1"]["meta"] == {
+ "chartId": 1,
+ "width": 12,
+ "height": 100,
+ }
+ assert "parents" not in stored["ROOT_ID"]
+ assert stored["GRID_ID"]["parents"] == ["ROOT_ID"]
+ assert stored["ROW-custom"]["parents"] == ["ROOT_ID", "GRID_ID"]
+ assert stored["CHART-1"]["parents"] == [
+ "ROOT_ID",
+ "GRID_ID",
+ "ROW-custom",
+ ]
+
+ @patch("superset.models.dashboard.Dashboard")
+ @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
+ @patch("superset.db.session")
+ @pytest.mark.asyncio
+ async def
test_generate_dashboard_position_json_override_repairs_truncated_tabs(
+ self,
+ mock_db_session,
+ mock_find_by_id,
+ mock_dashboard_cls,
+ mcp_server,
+ ) -> None:
+ """Regression test for SC-121314: a caller-supplied TABS layout whose
Review Comment:
You're right — those don't belong in a public repo. Scrubbed in bdffabf780.
Grepped the whole branch diff for internal references (tracker ids, customer
names, internal URLs) rather than just the sites you flagged, which turned up
two more beyond your list: `test_layout_validation.py:407` and
`test_dashboard_generation.py:831` both said "Mirrors the story's repro".
Everything cleaned:
- `test_dashboard_generation.py` — dropped the tracker id and reworded the
customer reference to "the shape a dashboard export can carry"; dropped "the
story's repro" from the inline comment
- `test_layout_validation.py` — dropped "Mirrors the story's repro" from
`_tabs_layout_with_truncated_parents`
- `test_add_chart_to_existing_dashboard.py`, `test_duplicate_dashboard.py`,
`test_remove_chart_from_dashboard.py`, `test_update_dashboard.py` — dropped the
tracker id prefix
Each docstring was reworded rather than truncated, so they still describe
the layout shape under test. A re-grep over the diff for tracker ids,
"shortcut", "the story", internal hosts and customer references comes back
empty.
--
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]
