gabotorresruiz commented on code in PR #44405:
URL: https://github.com/apache/superset/pull/44405#discussion_r4048917024
##########
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:
Not a blocker, but `rebuild_parent_chains` can raise on the one input path
that reaches it unvalidated.
This line does `reversed(root.get("children") or [])` without the
`isinstance(..., list)` guard the child branch applies at line 357, so a
non-reversible `ROOT_ID["children"]` raises `TypeError: 'int' object is not
reversible`. That contradicts the docstring ("malformed entries and cycles are
skipped rather than raised").
`update_dashboard` is safe because `_validate_update_request` runs
`validate_dashboard_layout` first, and that rejects exactly this with `"Layout
component ROOT_ID.children must be a list."`. `generate_dashboard` has no
pre-flight and `Dict[str, Any]` does not constrain nested values, so the
caller's layout goes straight in. I reproduced it end to end through
`fastmcp.Client` on this branch, calling `generate_dashboard` with
`position_json={"DASHBOARD_VERSION_KEY": "v2", "ROOT_ID": {"id": "ROOT_ID",
"type": "ROOT", "children": 5}, ...}`:
```
ToolError: Error calling tool 'generate_dashboard': 'int' object is not
reversible
```
It escapes because the handler at `generate_dashboard.py:557` catches
`(SQLAlchemyError, ValueError, AttributeError, ValidationError)`.
`add_chart_to_existing_dashboard.py:661` and
`remove_chart_from_dashboard.py:534` do not catch `TypeError` either, so a
stored layout in that shape would escape there too.
Mirroring the guard you already have below fixes it:
```python
root_children = root.get("children")
if not isinstance(root_children, list):
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_children)
if isinstance(child_id, str)
]
```
I applied that locally: `int`, `float`, `str` and `None` all return the
layout untouched, `ruff` is clean, and the 505 tests in
`tests/unit_tests/mcp_service/dashboard/` still pass. A case in
`test_rebuild_parent_chains_skips_malformed_entries` would lock it in, since
that test only covers bad entries inside a valid list today.
This is distinct from the missing `ROOT_ID` point you already answered in
the other thread. Or am I misunderstanding something here?
##########
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:
Just a small NIT, easy to sweep: this docstring carries an internal tracker
id (`SC-121314`) and a customer reference ("the shape the customer's dashboard
export carried"), neither of which means anything to a reader of this repo.
Same id in `test_add_chart_to_existing_dashboard.py:312`,
`test_duplicate_dashboard.py:253`, `test_remove_chart_from_dashboard.py:516`
and `test_update_dashboard.py:161`, plus "the story's repro" in
`test_layout_validation.py:408` and at line 830 here. The docstrings describe
the shape well enough on their own.
--
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]
