EnxDev commented on code in PR #44303:
URL: https://github.com/apache/superset/pull/44303#discussion_r4063689754
##########
superset/daos/dashboard.py:
##########
@@ -384,6 +385,12 @@ def set_dash_metadata(
chart_id = obj["meta"]["chartId"]
obj["meta"]["uuid"] = uuid_map.get(chart_id)
+ # Repair the layout before it is persisted. Charts are resolved
+ # from the incoming positions above, so a chart freed from a
+ # detached subtree keeps its ``dashboard_slices`` row and the
+ # frontend places it back into the layout on the next load.
+ positions = repair_position(positions, dashboard.id)
Review Comment:
Could we preserve the recovered chart's cross-filter settings through this
save/load cycle? Keeping its `dashboard_slices` row doesn't quite cover that:
on the next dashboard GET, `derive_json_metadata()` calls
`_derive_cross_filter_scopes()`, which drops `chart_configuration` entries for
charts absent from `position_json`. Hydration then re-adds the chart with the
default global scope.
I reproduced this with a trapped chart whose scope excludes chart 1: after
backend repair and hydration, its scope becomes `"global"`, so it starts
filtering chart 1. Reattaching the chart node before persisting, or retaining
its configuration while it's still a dashboard member, would avoid the reset. A
save/load regression test with an explicit exclusion would catch this.
##########
superset-frontend/src/dashboard/actions/hydrate.ts:
##########
@@ -133,11 +134,14 @@ export const hydrateDashboard =
// new dash: position_json could be {} or null
// getEmptyLayout() includes a version string entry plus BasicLayoutItem
entries
// which lack the `meta` field; layout is mutated below to add full
LayoutItem entries
- const layout = (
- positionData && Object.keys(positionData).length > 0
+ // Detached components are dropped before anything indexes the layout: they
+ // never render, but a detached cycle crashes the filter scope modal, and a
+ // chart trapped in one is neither visible nor eligible for re-adding
below.
+ const layout = removeUnreachableComponents(
+ (positionData && Object.keys(positionData).length > 0
? positionData
- : getEmptyLayout()
- ) as Record<string, LayoutItem | DashboardEntity>;
+ : getEmptyLayout()) as Record<string, LayoutItem | DashboardEntity>,
+ );
Review Comment:
Could recovery keep a chart placeholder when the trapped chart isn't present
in `charts`? With `SOFT_DELETE` enabled, the dashboard charts endpoint omits
archived charts. This removes their layout entries, and the loop below can't
re-add them. On the next save, `set_dash_metadata()` rebuilds
`dashboard.slices` from the remaining positions, dropping the archived chart's
association. Restoring it then won't bring it back to this dashboard.
I reproduced the missing entry through hydration and the membership loss
through `set_dash_metadata()` with a mocked database. Reparenting the existing
chart entry into a reachable container would preserve the placeholder and its
ID. A regression test covering an archived chart in the detached subtree,
followed by load/save/restore, would help protect that 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]
