aminghadersohi commented on code in PR #44405:
URL: https://github.com/apache/superset/pull/44405#discussion_r4045974108
##########
superset/mcp_service/dashboard/tool/generate_dashboard.py:
##########
@@ -264,11 +267,17 @@ def generate_dashboard( # noqa: C901
)
# Create dashboard layout with chart objects.
- # If the caller provided an explicit position_json, use it verbatim;
- # otherwise auto-generate a packed-grid layout from the chart ids.
+ # If the caller provided an explicit position_json, use its
+ # children/meta as given; otherwise auto-generate a packed-grid
+ # layout from the chart ids (which already carries correct parents).
with event_logger.log_context(action="mcp.generate_dashboard.layout"):
if request.position_json:
- layout = request.position_json
+ # A caller-supplied layout may carry only an immediate
+ # parent (or omit `parents` altogether); rebuild the full
+ # ancestor chains so server-side filter-scope derivation
+ # sees the same tree the frontend would after hydration.
+ # See superset.dashboards.filter_scope.get_chart_ids_in_scope.
+ layout = rebuild_parent_chains(request.position_json)
Review Comment:
Traced this through the call path — it's the designed behavior, not an
incomplete implementation.
`rebuild_parent_chains` is the server-side mirror of the frontend's
`updateComponentParentsList`, and it is deliberately a no-op in exactly the
case the frontend is.
**1. The client repair is a no-op without `ROOT_ID` too.**
`superset-frontend/src/dashboard/actions/hydrate.ts:250` calls it as
`updateComponentParentsList({ currentComponent: layout[DASHBOARD_ROOT_ID],
layout })`. With no `ROOT_ID`, `currentComponent` is `undefined` and the `if
(currentComponent && layout)` guard in `updateComponentParentsList.ts` returns
without touching anything. So server and client derive the same (empty) scopes
for such a layout — which is precisely the invariant
`superset/dashboards/filter_scope.py` exists to preserve ("Deriving them on
read makes the API agree with the client").
**2. There is nothing to rebuild *to*.** `parents` is the ancestor chain
rooted at `ROOT_ID`, and `get_chart_ids_in_scope` in `filter_scope.py` tests a
chart's `parents` against `scope.rootPath` — `["ROOT_ID"]` for the default
scope. With no ROOT node there is no chain to derive and no scope entry a chain
could legitimately match; stamping one would be fabricating ancestry rather
than repairing it.
**3. The layout is unrenderable regardless.** `DashboardBuilder.tsx:578`
reads `dashboardLayout[DASHBOARD_ROOT_ID]`. A layout missing it doesn't render
at all, so "filter scopes remain empty" isn't the operative failure mode.
One clarification on "valid": `rebuild_parent_chains` only requires
`layout["ROOT_ID"]` to be a dict — it does not check `type == "ROOT"`. A
`ROOT_ID` with an unexpected type is still walked and its descendants still get
full chains, again matching the frontend, which doesn't type-check the root
either. The no-op is limited to `ROOT_ID` being absent or not a mapping.
This is stated in the function's docstring ("A component unreachable from
`ROOT_ID` ... is left untouched") and already covered by
`test_rebuild_parent_chains_returns_input_when_root_missing` in
`tests/unit_tests/mcp_service/dashboard/test_layout_validation.py`.
Adjacent but distinct: `generate_dashboard` doesn't run
`validate_dashboard_layout` on a caller-supplied `position_json` the way
`update_dashboard` does (`update_dashboard.py:253`). That's a real pre-existing
input-validation gap, but it's about rejecting malformed layouts up front, not
about parent chains, and is out of scope for this fix.
Resolving.
--
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]
