This is an automated email from the ASF dual-hosted git repository.
msyavuz pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 6323b37f50a fix(dashboard): prevent and repair self-nested layout
components (#44303)
6323b37f50a is described below
commit 6323b37f50a0f289e50167ce7b364ff0e0a0f609
Author: Mehmet Salih Yavuz <[email protected]>
AuthorDate: Wed Sep 23 16:35:34 2026 +0300
fix(dashboard): prevent and repair self-nested layout components (#44303)
---
.../src/dashboard/actions/hydrate.test.ts | 85 +++++-
superset-frontend/src/dashboard/actions/hydrate.ts | 28 +-
.../src/dashboard/reducers/dashboardLayout.test.ts | 62 ++++
.../src/dashboard/reducers/dashboardLayout.ts | 26 ++
.../src/dashboard/util/getDropPosition.test.ts | 83 ++++++
.../src/dashboard/util/getDropPosition.ts | 13 +
.../util/removeUnreachableComponents.test.ts | 283 ++++++++++++++++++
.../dashboard/util/removeUnreachableComponents.ts | 225 ++++++++++++++
.../util/updateComponentParentsList.test.ts | 17 ++
.../dashboard/util/updateComponentParentsList.ts | 7 +-
superset/commands/dashboard/update.py | 7 +-
superset/daos/dashboard.py | 5 +
superset/dashboards/layout.py | 269 +++++++++++++++++
.../dashboard/tool/generate_dashboard.py | 3 +-
tests/unit_tests/commands/dashboard/update_test.py | 46 +++
tests/unit_tests/dao/dashboard_test.py | 139 +++++++++
tests/unit_tests/dashboards/layout_test.py | 325 +++++++++++++++++++++
.../dashboard/tool/test_dashboard_generation.py | 63 ++++
18 files changed, 1666 insertions(+), 20 deletions(-)
diff --git a/superset-frontend/src/dashboard/actions/hydrate.test.ts
b/superset-frontend/src/dashboard/actions/hydrate.test.ts
index 797df346abe..8da88098cd8 100644
--- a/superset-frontend/src/dashboard/actions/hydrate.test.ts
+++ b/superset-frontend/src/dashboard/actions/hydrate.test.ts
@@ -18,11 +18,15 @@
*/
import { HYDRATE_DASHBOARD, hydrateDashboard } from './hydrate';
import {
+ CHART_TYPE,
+ COLUMN_TYPE,
+ DASHBOARD_GRID_TYPE,
DASHBOARD_ROOT_TYPE,
+ ROW_TYPE,
TABS_TYPE,
TAB_TYPE,
} from '../util/componentTypes';
-import { DASHBOARD_ROOT_ID } from '../util/constants';
+import { DASHBOARD_GRID_ID, DASHBOARD_ROOT_ID } from '../util/constants';
/**
* Regression guard for the follow-up to PR #39417 / PR #41832: the default
@@ -274,3 +278,82 @@ test('a permalink activeTabs: [] (empty but present) wins
and seeds []', () => {
expect(action.data.dashboardState.activeTabs).toEqual([]);
});
+
+test('keeps a trapped chart missing from the charts payload in the layout', ()
=> {
+ // e.g. an archived chart with SOFT_DELETE: the charts endpoint omits it, so
+ // hydration cannot re-add it and only its layout entry keeps its membership
+ const positionData = {
+ [DASHBOARD_ROOT_ID]: layoutItem(
+ DASHBOARD_ROOT_ID,
+ DASHBOARD_ROOT_TYPE,
+ [DASHBOARD_GRID_ID],
+ [],
+ ),
+ [DASHBOARD_GRID_ID]: layoutItem(
+ DASHBOARD_GRID_ID,
+ DASHBOARD_GRID_TYPE,
+ [],
+ [DASHBOARD_ROOT_ID],
+ ),
+ 'COLUMN-orphan': layoutItem(
+ 'COLUMN-orphan',
+ COLUMN_TYPE,
+ ['CHART-archived', 'ROW-orphan'],
+ [DASHBOARD_ROOT_ID, DASHBOARD_GRID_ID],
+ ),
+ 'ROW-orphan': layoutItem(
+ 'ROW-orphan',
+ ROW_TYPE,
+ ['COLUMN-orphan'],
+ [DASHBOARD_ROOT_ID, DASHBOARD_GRID_ID, 'COLUMN-orphan'],
+ ),
+ 'CHART-archived': {
+ ...layoutItem('CHART-archived', CHART_TYPE, [], []),
+ meta: { chartId: 42, width: 4, height: 50 },
+ },
+ };
+
+ const layout = hydrate(positionData).data.dashboardLayout.present;
+
+ expect(layout['COLUMN-orphan']).toBeUndefined();
+ expect(layout['ROW-orphan']).toBeUndefined();
+ const [rowId] = layout[DASHBOARD_GRID_ID].children;
+ expect(layout[rowId].children).toEqual(['CHART-archived']);
+ expect(layout['CHART-archived'].meta.chartId).toBe(42);
+});
+
+test('rebuilds stale parents from the layout children', () => {
+ const positionData = {
+ [DASHBOARD_ROOT_ID]: layoutItem(
+ DASHBOARD_ROOT_ID,
+ DASHBOARD_ROOT_TYPE,
+ [DASHBOARD_GRID_ID],
+ [],
+ ),
+ [DASHBOARD_GRID_ID]: layoutItem(
+ DASHBOARD_GRID_ID,
+ DASHBOARD_GRID_TYPE,
+ ['ROW-a'],
+ [DASHBOARD_ROOT_ID],
+ ),
+ 'ROW-a': layoutItem('ROW-a', ROW_TYPE, ['COLUMN-a'], [DASHBOARD_ROOT_ID]),
+ 'COLUMN-a': layoutItem(
+ 'COLUMN-a',
+ COLUMN_TYPE,
+ [],
+ [DASHBOARD_ROOT_ID, DASHBOARD_GRID_ID, 'ROW-stale'],
+ ),
+ };
+
+ const layout = hydrate(positionData).data.dashboardLayout.present;
+
+ expect(layout['ROW-a'].parents).toEqual([
+ DASHBOARD_ROOT_ID,
+ DASHBOARD_GRID_ID,
+ ]);
+ expect(layout['COLUMN-a'].parents).toEqual([
+ DASHBOARD_ROOT_ID,
+ DASHBOARD_GRID_ID,
+ 'ROW-a',
+ ]);
+});
diff --git a/superset-frontend/src/dashboard/actions/hydrate.ts
b/superset-frontend/src/dashboard/actions/hydrate.ts
index 3619f8c920b..64038c2026c 100644
--- a/superset-frontend/src/dashboard/actions/hydrate.ts
+++ b/superset-frontend/src/dashboard/actions/hydrate.ts
@@ -55,6 +55,7 @@ import getLocationHash from
'src/dashboard/util/getLocationHash';
import newComponentFactory, {
DashboardEntity,
} from 'src/dashboard/util/newComponentFactory';
+import removeUnreachableComponents from
'src/dashboard/util/removeUnreachableComponents';
import { URL_PARAMS } from 'src/constants';
import { getUrlParam } from 'src/utils/urlUtils';
import { ResourceStatus } from 'src/hooks/apiResources/apiResources';
@@ -133,11 +134,13 @@ 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
+ // Repaired before anything indexes the layout: a detached cycle crashes
the
+ // filter scope modal, and a chart trapped in one would never render.
+ const layout = removeUnreachableComponents(
+ (positionData && Object.keys(positionData).length > 0
? positionData
- : getEmptyLayout()
- ) as Record<string, LayoutItem | DashboardEntity>;
+ : getEmptyLayout()) as Record<string, LayoutItem | DashboardEntity>,
+ );
// create a lookup to sync layout names with slice names
const chartIdToLayoutId: Record<number, string> = {};
@@ -241,17 +244,12 @@ export const hydrateDashboard =
}
});
- // make sure that parents tree is built
- if (
- Object.values(layout).some(
- element => element.id !== DASHBOARD_ROOT_ID && !element.parents,
- )
- ) {
- updateComponentParentsList({
- currentComponent: layout[DASHBOARD_ROOT_ID] as LayoutItem,
- layout: layout as Record<string, LayoutItem>,
- });
- }
+ // buildActiveFilters reads `parents` for filter scopes before the layout
+ // reducer rebuilds them, and the repair above may have moved components
+ updateComponentParentsList({
+ currentComponent: layout[DASHBOARD_ROOT_ID] as LayoutItem,
+ layout: layout as Record<string, LayoutItem>,
+ });
buildActiveFilters({
dashboardFilters: dashboardFilters as Parameters<
diff --git a/superset-frontend/src/dashboard/reducers/dashboardLayout.test.ts
b/superset-frontend/src/dashboard/reducers/dashboardLayout.test.ts
index 011acfdc87a..28463ac65d3 100644
--- a/superset-frontend/src/dashboard/reducers/dashboardLayout.test.ts
+++ b/superset-frontend/src/dashboard/reducers/dashboardLayout.test.ts
@@ -33,6 +33,7 @@ import type { DashboardLayout } from 'src/dashboard/types';
import {
CHART_TYPE,
+ COLUMN_TYPE,
DASHBOARD_GRID_TYPE,
DASHBOARD_ROOT_TYPE,
ROW_TYPE,
@@ -231,6 +232,67 @@ describe('dashboardLayout reducer', () => {
});
});
+ test('should not move a component into its own descendant', () => {
+ const layout = {
+ parent: {
+ id: 'parent',
+ type: ROW_TYPE,
+ children: ['column'],
+ },
+ column: {
+ id: 'column',
+ type: COLUMN_TYPE,
+ children: ['nestedRow'],
+ parents: ['parent'],
+ },
+ nestedRow: {
+ id: 'nestedRow',
+ type: ROW_TYPE,
+ children: [],
+ parents: ['parent', 'column'],
+ },
+ };
+
+ const dropResult = {
+ source: { id: 'parent', type: ROW_TYPE, index: 0 },
+ destination: { id: 'nestedRow', type: ROW_TYPE, index: 0 },
+ dragging: { id: 'column', type: COLUMN_TYPE },
+ };
+
+ expect(
+ testReducer(layout, {
+ type: MOVE_COMPONENT,
+ payload: { dropResult },
+ }),
+ ).toBe(layout);
+ });
+
+ test('should not move a component into a descendant with stale parents', ()
=> {
+ const layout = {
+ parent: { id: 'parent', type: ROW_TYPE, children: ['column'] },
+ column: { id: 'column', type: COLUMN_TYPE, children: ['nestedRow'] },
+ nestedRow: {
+ id: 'nestedRow',
+ type: ROW_TYPE,
+ children: [],
+ parents: ['somewhere-else'],
+ },
+ };
+
+ const dropResult = {
+ source: { id: 'parent', type: ROW_TYPE, index: 0 },
+ destination: { id: 'nestedRow', type: ROW_TYPE, index: 0 },
+ dragging: { id: 'column', type: COLUMN_TYPE },
+ };
+
+ expect(
+ testReducer(layout, {
+ type: MOVE_COMPONENT,
+ payload: { dropResult },
+ }),
+ ).toBe(layout);
+ });
+
test('should wrap a moved component in a row if need be', () => {
const layout = {
source: {
diff --git a/superset-frontend/src/dashboard/reducers/dashboardLayout.ts
b/superset-frontend/src/dashboard/reducers/dashboardLayout.ts
index aab6cd25a32..56b0ddb7c90 100644
--- a/superset-frontend/src/dashboard/reducers/dashboardLayout.ts
+++ b/superset-frontend/src/dashboard/reducers/dashboardLayout.ts
@@ -47,6 +47,25 @@ import { HYDRATE_DASHBOARD } from '../actions/hydrate';
import { DashboardLayout } from '../types';
import { DropResult } from '../components/dnd/dragDroppableConfig';
+// Walks `children` rather than trusting `parents`, which can be stale.
+function isSelfOrDescendant(
+ layout: DashboardLayout,
+ ancestorId: string,
+ targetId: string,
+): boolean {
+ const seen = new Set<string>();
+ const stack = [ancestorId];
+ while (stack.length) {
+ const id = stack.pop() as string;
+ if (id === targetId) return true;
+ if (!seen.has(id)) {
+ seen.add(id);
+ stack.push(...(layout[id]?.children ?? []));
+ }
+ }
+ return false;
+}
+
interface DashboardLayoutAction {
type: string;
payload?: {
@@ -177,6 +196,13 @@ const actionHandlers: Record<
if (!source || !destination || !dragging) return state;
+ // Defense in depth against the drop guard in getDropPosition: moving a
+ // component into itself or into one of its own descendants detaches the
+ // subtree and leaves a cycle that no root walk can reach or repair.
+ if (isSelfOrDescendant(state, dragging.id, destination.id)) {
+ return state;
+ }
+
const nextEntities = reorderItem({
entitiesMap: state,
source,
diff --git a/superset-frontend/src/dashboard/util/getDropPosition.test.ts
b/superset-frontend/src/dashboard/util/getDropPosition.test.ts
index 46f12c25322..0950e6b4fa2 100644
--- a/superset-frontend/src/dashboard/util/getDropPosition.test.ts
+++ b/superset-frontend/src/dashboard/util/getDropPosition.test.ts
@@ -26,6 +26,7 @@ import getDropPositionOriginal, {
import {
CHART_TYPE,
+ COLUMN_TYPE,
DASHBOARD_GRID_TYPE,
DASHBOARD_ROOT_TYPE,
HEADER_TYPE,
@@ -457,3 +458,85 @@ describe('getDropPosition', () => {
});
});
});
+
+// The self-nesting guard needs ids and a parents chain, which getMocks above
+// intentionally omits, so these build their own minimal mocks.
+const selfNestingMocks = ({
+ componentId,
+ componentParents = [],
+ parentComponentId,
+ parentComponentParents = [],
+}: {
+ componentId: string;
+ componentParents?: string[];
+ parentComponentId?: string;
+ parentComponentParents?: string[];
+}) => [
+ {
+ getItem: () => ({ id: 'COLUMN-dragged', type: COLUMN_TYPE }),
+ getClientOffset: () => ({ x: 0, y: 0 }),
+ },
+ {
+ props: {
+ depth: 2,
+ component: {
+ id: componentId,
+ type: ROW_TYPE,
+ children: [],
+ parents: componentParents,
+ },
+ parentComponent: parentComponentId
+ ? {
+ id: parentComponentId,
+ type: TAB_TYPE,
+ children: [],
+ parents: parentComponentParents,
+ }
+ : undefined,
+ orientation: 'column',
+ isDraggingOverShallow: true,
+ },
+ ref: {
+ getBoundingClientRect: () => ({
+ top: 0,
+ right: 100,
+ bottom: 100,
+ left: 0,
+ }),
+ },
+ },
+];
+
+test('returns DROP_FORBIDDEN when dropping a component into its own
descendant', () => {
+ expect(
+ getDropPosition(
+ ...selfNestingMocks({
+ componentId: 'ROW-nested-in-dragged',
+ componentParents: ['ROOT_ID', 'GRID_ID', 'COLUMN-dragged'],
+ }),
+ ),
+ ).toBe(DROP_FORBIDDEN);
+});
+
+test('returns DROP_FORBIDDEN when dropping a component as a sibling inside
itself', () => {
+ expect(
+ getDropPosition(
+ ...selfNestingMocks({
+ componentId: 'ROW-nested-in-dragged-sibling',
+ componentParents: ['ROOT_ID', 'GRID_ID', 'COLUMN-dragged'],
+ parentComponentId: 'COLUMN-dragged',
+ }),
+ ),
+ ).toBe(DROP_FORBIDDEN);
+});
+
+test('allows a drop on an unrelated component', () => {
+ expect(
+ getDropPosition(
+ ...selfNestingMocks({
+ componentId: 'ROW-unrelated',
+ componentParents: ['ROOT_ID', 'GRID_ID'],
+ }),
+ ),
+ ).not.toBe(DROP_FORBIDDEN);
+});
diff --git a/superset-frontend/src/dashboard/util/getDropPosition.ts
b/superset-frontend/src/dashboard/util/getDropPosition.ts
index 341aa1edde8..ee14097a16b 100644
--- a/superset-frontend/src/dashboard/util/getDropPosition.ts
+++ b/superset-frontend/src/dashboard/util/getDropPosition.ts
@@ -88,6 +88,19 @@ export default function getDropPosition(
return null;
}
+ // A container cannot receive itself. Dropping it into one of its own
+ // descendants (as a child, or as a sibling of one) makes reorderItem detach
+ // the subtree from its real parent and close a cycle between the two, which
+ // is unreachable from the root and so never repaired downstream.
+ const isSelfOrDescendant = (target?: LayoutItem) =>
+ !!target &&
+ (target.id === draggingItem.id ||
+ (target.parents || []).includes(draggingItem.id));
+
+ if (isSelfOrDescendant(component) || isSelfOrDescendant(parentComponent)) {
+ return DROP_FORBIDDEN;
+ }
+
const validChild = isValidChild({
parentType: component.type,
parentDepth: componentDepth,
diff --git
a/superset-frontend/src/dashboard/util/removeUnreachableComponents.test.ts
b/superset-frontend/src/dashboard/util/removeUnreachableComponents.test.ts
new file mode 100644
index 00000000000..5291ba17ab5
--- /dev/null
+++ b/superset-frontend/src/dashboard/util/removeUnreachableComponents.test.ts
@@ -0,0 +1,283 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import removeUnreachableComponents from
'src/dashboard/util/removeUnreachableComponents';
+import type { DashboardComponent } from 'src/dashboard/types';
+
+type Layout = Record<string, DashboardComponent>;
+
+const component = (
+ id: string,
+ type: string,
+ children: string[] = [],
+ extra: Partial<DashboardComponent> = {},
+): DashboardComponent => ({ id, type, children, meta: {}, ...extra });
+
+// DASHBOARD_VERSION_KEY is a plain string in real position data
+const withVersionKey = (layout: Layout): Layout =>
+ ({ DASHBOARD_VERSION_KEY: 'v2', ...layout }) as unknown as Layout;
+
+const reachableLayout = (): Layout =>
+ withVersionKey({
+ ROOT_ID: component('ROOT_ID', 'ROOT', ['GRID_ID']),
+ GRID_ID: component('GRID_ID', 'GRID', ['ROW-a']),
+ 'ROW-a': component('ROW-a', 'ROW', ['CHART-a']),
+ 'CHART-a': component('CHART-a', 'CHART', [], { meta: { chartId: 1 } }),
+ HEADER_ID: component('HEADER_ID', 'HEADER'),
+ });
+
+// the shape of a real corrupted dashboard: a column moved into a row nested
+// inside itself, leaving the pair pointing at each other and detached
+const trappedComponents = (chartMeta: DashboardComponent['meta']): Layout => ({
+ 'COLUMN-orphan': component(
+ 'COLUMN-orphan',
+ 'COLUMN',
+ ['CHART-trapped', 'ROW-orphan'],
+ { parents: ['ROOT_ID', 'GRID_ID', 'ROW-a'] },
+ ),
+ 'ROW-orphan': component('ROW-orphan', 'ROW', ['COLUMN-orphan'], {
+ parents: ['ROOT_ID', 'GRID_ID', 'ROW-a', 'COLUMN-orphan'],
+ }),
+ 'CHART-trapped': component('CHART-trapped', 'CHART', [], {
+ parents: ['ROOT_ID', 'GRID_ID', 'ROW-a', 'COLUMN-orphan'],
+ meta: chartMeta,
+ }),
+});
+
+test('returns the same layout when everything is reachable', () => {
+ const layout = reachableLayout();
+ expect(removeUnreachableComponents(layout)).toBe(layout);
+});
+
+test('drops a detached subtree, including one that contains a cycle', () => {
+ const layout = { ...reachableLayout(), ...trappedComponents({}) };
+
+ expect(Object.keys(removeUnreachableComponents(layout)).sort()).toEqual(
+ Object.keys(reachableLayout()).sort(),
+ );
+});
+
+test('reattaches a detached chart to the grid, keeping its id and meta', () =>
{
+ const layout = {
+ ...reachableLayout(),
+ ...trappedComponents({ chartId: 2, width: 4 }),
+ };
+
+ const repaired = removeUnreachableComponents(layout);
+
+ expect(repaired['COLUMN-orphan']).toBeUndefined();
+ expect(repaired['ROW-orphan']).toBeUndefined();
+ const [, newRowId] = repaired.GRID_ID.children;
+ expect(repaired[newRowId]).toMatchObject({
+ type: 'ROW',
+ children: ['CHART-trapped'],
+ parents: ['ROOT_ID', 'GRID_ID'],
+ });
+ expect(repaired['CHART-trapped']).toMatchObject({
+ parents: ['ROOT_ID', 'GRID_ID', newRowId],
+ meta: { chartId: 2, width: 4 },
+ });
+ expect(layout.GRID_ID.children).toEqual(['ROW-a']);
+});
+
+test('reattaches detached markdown to a row and a header to the grid', () => {
+ const layout = {
+ ...reachableLayout(),
+ 'COLUMN-orphan': component('COLUMN-orphan', 'COLUMN', [
+ 'HEADER-trapped',
+ 'MARKDOWN-trapped',
+ 'ROW-orphan',
+ ]),
+ 'ROW-orphan': component('ROW-orphan', 'ROW', ['COLUMN-orphan']),
+ 'HEADER-trapped': component('HEADER-trapped', 'HEADER', [], {
+ meta: { text: 'Section title' },
+ }),
+ 'MARKDOWN-trapped': component('MARKDOWN-trapped', 'MARKDOWN', [], {
+ meta: { code: '# Notes', width: 4 },
+ }),
+ };
+
+ const repaired = removeUnreachableComponents(layout);
+
+ expect(repaired['COLUMN-orphan']).toBeUndefined();
+ expect(repaired['ROW-orphan']).toBeUndefined();
+ const newRowId = repaired.GRID_ID.children[2];
+ // a header is not a valid row child, so it sits directly in the grid
+ expect(repaired.GRID_ID.children).toEqual([
+ 'ROW-a',
+ 'HEADER-trapped',
+ newRowId,
+ ]);
+ expect(repaired['HEADER-trapped']).toMatchObject({
+ parents: ['ROOT_ID', 'GRID_ID'],
+ meta: { text: 'Section title' },
+ });
+ expect(repaired[newRowId]).toMatchObject({
+ type: 'ROW',
+ children: ['MARKDOWN-trapped'],
+ });
+ expect(repaired['MARKDOWN-trapped']).toMatchObject({
+ parents: ['ROOT_ID', 'GRID_ID', newRowId],
+ meta: { code: '# Notes', width: 4 },
+ });
+});
+
+test('reattaches a detached chart to the first tab of top-level tabs', () => {
+ const layout = withVersionKey({
+ ROOT_ID: component('ROOT_ID', 'ROOT', ['TABS-t']),
+ GRID_ID: component('GRID_ID', 'GRID'),
+ 'TABS-t': component('TABS-t', 'TABS', ['TAB-1', 'TAB-2']),
+ 'TAB-1': component('TAB-1', 'TAB'),
+ 'TAB-2': component('TAB-2', 'TAB'),
+ 'CHART-trapped': component('CHART-trapped', 'CHART', [], {
+ meta: { chartId: 2 },
+ }),
+ });
+
+ const repaired = removeUnreachableComponents(layout);
+
+ const [newRowId] = repaired['TAB-1'].children;
+ expect(repaired['CHART-trapped'].parents).toEqual([
+ 'ROOT_ID',
+ 'TABS-t',
+ 'TAB-1',
+ newRowId,
+ ]);
+});
+
+test('reattaches a detached chart to the tab recorded in its stale parents',
() => {
+ const layout = withVersionKey({
+ ROOT_ID: component('ROOT_ID', 'ROOT', ['TABS-t']),
+ GRID_ID: component('GRID_ID', 'GRID'),
+ 'TABS-t': component('TABS-t', 'TABS', ['TAB-1', 'TAB-2']),
+ 'TAB-1': component('TAB-1', 'TAB'),
+ 'TAB-2': component('TAB-2', 'TAB'),
+ 'CHART-trapped': component('CHART-trapped', 'CHART', [], {
+ parents: ['ROOT_ID', 'TABS-t', 'TAB-2', 'ROW-gone', 'COLUMN-gone'],
+ meta: { chartId: 2 },
+ }),
+ });
+
+ const repaired = removeUnreachableComponents(layout);
+
+ expect(repaired['TAB-1'].children).toEqual([]);
+ const [newRowId] = repaired['TAB-2'].children;
+ expect(repaired['CHART-trapped'].parents).toEqual([
+ 'ROOT_ID',
+ 'TABS-t',
+ 'TAB-2',
+ newRowId,
+ ]);
+});
+
+test('returns detached headers and markdown to their tab in order', () => {
+ const staleParents = ['ROOT_ID', 'TABS-t', 'TAB-2', 'ROW-gone'];
+ const layout = withVersionKey({
+ ROOT_ID: component('ROOT_ID', 'ROOT', ['TABS-t']),
+ GRID_ID: component('GRID_ID', 'GRID'),
+ 'TABS-t': component('TABS-t', 'TABS', ['TAB-1', 'TAB-2']),
+ 'TAB-1': component('TAB-1', 'TAB'),
+ 'TAB-2': component('TAB-2', 'TAB'),
+ 'CHART-trapped': component('CHART-trapped', 'CHART', [], {
+ parents: staleParents,
+ meta: { chartId: 2 },
+ }),
+ 'HEADER-trapped': component('HEADER-trapped', 'HEADER', [], {
+ parents: ['ROOT_ID', 'TABS-t', 'TAB-2'],
+ }),
+ 'MARKDOWN-trapped': component('MARKDOWN-trapped', 'MARKDOWN', [], {
+ parents: staleParents,
+ }),
+ });
+
+ const repaired = removeUnreachableComponents(layout);
+
+ expect(repaired['TAB-1'].children).toEqual([]);
+ const [chartRow, headerId, markdownRow] = repaired['TAB-2'].children;
+ expect(headerId).toBe('HEADER-trapped');
+ expect(repaired['HEADER-trapped'].parents).toEqual([
+ 'ROOT_ID',
+ 'TABS-t',
+ 'TAB-2',
+ ]);
+ expect(repaired[chartRow].children).toEqual(['CHART-trapped']);
+ expect(repaired[markdownRow].children).toEqual(['MARKDOWN-trapped']);
+});
+
+test('does not duplicate a detached chart that is also placed reachably', ()
=> {
+ const layout = { ...reachableLayout(), ...trappedComponents({ chartId: 1 })
};
+
+ const repaired = removeUnreachableComponents(layout);
+
+ expect(repaired['CHART-trapped']).toBeUndefined();
+ expect(repaired.GRID_ID.children).toEqual(['ROW-a']);
+});
+
+test('keeps the detached empty grid of a dashboard with top-level tabs', () =>
{
+ const layout = withVersionKey({
+ ROOT_ID: component('ROOT_ID', 'ROOT', ['TABS-t']),
+ GRID_ID: component('GRID_ID', 'GRID'),
+ 'TABS-t': component('TABS-t', 'TABS', ['TAB-1']),
+ 'TAB-1': component('TAB-1', 'TAB'),
+ HEADER_ID: component('HEADER_ID', 'HEADER'),
+ });
+
+ expect(removeUnreachableComponents(layout)).toBe(layout);
+});
+
+test('clears the children of a detached grid when dropping them', () => {
+ const layout = withVersionKey({
+ ROOT_ID: component('ROOT_ID', 'ROOT', ['TABS-t']),
+ GRID_ID: component('GRID_ID', 'GRID', ['ROW-stale']),
+ 'ROW-stale': component('ROW-stale', 'ROW'),
+ 'TABS-t': component('TABS-t', 'TABS', ['TAB-1']),
+ 'TAB-1': component('TAB-1', 'TAB'),
+ });
+
+ const repaired = removeUnreachableComponents(layout);
+
+ expect(repaired['ROW-stale']).toBeUndefined();
+ expect(repaired.GRID_ID.children).toEqual([]);
+ expect(layout.GRID_ID.children).toEqual(['ROW-stale']);
+});
+
+test('returns the layout untouched when there is no root to walk from', () => {
+ const layout: Layout = { 'ROW-a': component('ROW-a', 'ROW') };
+
+ expect(removeUnreachableComponents(layout)).toBe(layout);
+});
+
+test('leaves the layout untouched when root children is not an array', () => {
+ const layout = reachableLayout();
+ layout.ROOT_ID = {
+ ...layout.ROOT_ID,
+ children: { 0: 'GRID_ID' } as unknown as string[],
+ };
+
+ expect(removeUnreachableComponents(layout)).toBe(layout);
+});
+
+test('tolerates non-array children below the root', () => {
+ const layout = reachableLayout();
+ layout['ROW-a'] = {
+ ...layout['ROW-a'],
+ children: 'CHART-a' as unknown as string[],
+ };
+
+ expect(() => removeUnreachableComponents(layout)).not.toThrow();
+});
diff --git
a/superset-frontend/src/dashboard/util/removeUnreachableComponents.ts
b/superset-frontend/src/dashboard/util/removeUnreachableComponents.ts
new file mode 100644
index 00000000000..81e93dda922
--- /dev/null
+++ b/superset-frontend/src/dashboard/util/removeUnreachableComponents.ts
@@ -0,0 +1,225 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { DashboardComponent } from '../types';
+import {
+ CHART_TYPE,
+ DASHBOARD_GRID_TYPE,
+ HEADER_TYPE,
+ MARKDOWN_TYPE,
+ ROW_TYPE,
+ TAB_TYPE,
+ TABS_TYPE,
+} from './componentTypes';
+import {
+ DASHBOARD_GRID_ID,
+ DASHBOARD_HEADER_ID,
+ DASHBOARD_ROOT_ID,
+ DASHBOARD_VERSION_KEY,
+ GRID_COLUMN_COUNT,
+ GRID_DEFAULT_CHART_WIDTH,
+} from './constants';
+import newComponentFactory, { DashboardEntity } from './newComponentFactory';
+
+// HEADER_ID is dashboard metadata rather than a rendered child, GRID_ID is
+// retained (emptied) and detached when a dashboard uses top-level tabs, and
+// DASHBOARD_VERSION_KEY is a version string rather than a component.
+const RESERVED_IDS = new Set<string>([
+ DASHBOARD_GRID_ID,
+ DASHBOARD_HEADER_ID,
+ DASHBOARD_ROOT_ID,
+ DASHBOARD_VERSION_KEY,
+]);
+
+// containers that accept both new rows and headers
+const REATTACH_CONTAINER_TYPES = new Set<string>([
+ DASHBOARD_GRID_TYPE,
+ TAB_TYPE,
+]);
+
+const childrenOf = (component: unknown): string[] => {
+ const children = (component as DashboardComponent | undefined)?.children;
+ return Array.isArray(children)
+ ? children.filter((id): id is string => typeof id === 'string')
+ : [];
+};
+
+const isComponent = (value: unknown): value is DashboardComponent =>
+ typeof value === 'object' && value !== null;
+
+/**
+ * Drop components that cannot be reached from ROOT_ID, reattaching detached
+ * charts, markdown and headers instead of dropping them.
+ *
+ * Detached components never render, but they survive in position_json and stay
+ * visible to code that walks every layout entry or trusts the stale `parents`
+ * of a node. A detached subtree containing a cycle is what crashes the filter
+ * scope modal with "Maximum call stack size exceeded".
+ *
+ * A detached chart keeps its layout id and is moved into new rows in the
deepest
+ * container of its stored `parents` that is still reachable, falling back to
+ * the first top-level container. Only the tail of a stale chain is gone, so
its
+ * reachable prefix still says e.g. which tab the chart came from. Dropping it
would lose the chart's cross-filter
+ * configuration, and a chart missing from the charts payload (e.g. archived
+ * with SOFT_DELETE) could not be re-added, so the next save would drop its
+ * dashboard membership. A chart that is also placed reachably is not
duplicated.
+ *
+ * Detached markdown and headers are reattached the same way, since their text
+ * lives nowhere but position_json. Markdown goes into the new rows; a header,
+ * which cannot be a row child, goes directly into the container.
+ * Mirrors `superset/dashboards/layout.py`.
+ */
+export default function removeUnreachableComponents<
+ T extends DashboardComponent,
+>(layout: Record<string, T>): Record<string, T | DashboardEntity> {
+ const root = layout[DASHBOARD_ROOT_ID];
+ if (!isComponent(root) || !Array.isArray(root.children)) {
+ return layout;
+ }
+
+ // path from ROOT_ID to each reachable id
+ const paths = new Map<string, string[]>();
+ const stack: [string, string[]][] = [[DASHBOARD_ROOT_ID, []]];
+
+ while (stack.length) {
+ const [id, parentPath] = stack.pop() as [string, string[]];
+ // doubles as the cycle guard: an id already seen is never expanded twice
+ if (!paths.has(id)) {
+ const path = [...parentPath, id];
+ paths.set(id, path);
+ childrenOf(layout[id]).forEach(childId => {
+ if (isComponent(layout[childId])) {
+ stack.push([childId, path]);
+ }
+ });
+ }
+ }
+ const reachable = new Set(paths.keys());
+
+ const unreachable = Object.keys(layout).filter(
+ id =>
+ isComponent(layout[id]) && !reachable.has(id) && !RESERVED_IDS.has(id),
+ );
+
+ if (!unreachable.length) {
+ return layout;
+ }
+
+ const placedChartIds = new Set<number | undefined>(
+ [...reachable]
+ .filter(id => layout[id].type === CHART_TYPE)
+ .map(id => layout[id].meta?.chartId),
+ );
+ const rescued: [string, T][] = [];
+ unreachable.forEach(id => {
+ const component = layout[id];
+ if (component.type === MARKDOWN_TYPE || component.type === HEADER_TYPE) {
+ rescued.push([id, component]);
+ return;
+ }
+ const chartId = component.meta?.chartId;
+ if (
+ component.type === CHART_TYPE &&
+ chartId !== undefined &&
+ !placedChartIds.has(chartId)
+ ) {
+ placedChartIds.add(chartId);
+ rescued.push([id, component]);
+ }
+ });
+
+ const next: Record<string, T | DashboardEntity> = { ...layout };
+ unreachable.forEach(id => {
+ delete next[id];
+ });
+ // a detached reserved id is kept, but must not reference the dropped nodes
+ RESERVED_IDS.forEach(id => {
+ const component = next[id];
+ if (
+ !reachable.has(id) &&
+ isComponent(component) &&
+ component.children?.length
+ ) {
+ next[id] = { ...component, children: [] };
+ }
+ });
+
+ // mirrors findFirstParentContainerId; the path is built here rather than
+ // read from `parents`, which may be stale or missing
+ const [firstId] = childrenOf(layout[DASHBOARD_ROOT_ID]);
+ const fallbackPath =
+ layout[firstId]?.type === TABS_TYPE
+ ? [DASHBOARD_ROOT_ID, firstId, childrenOf(layout[firstId])[0]]
+ : [DASHBOARD_ROOT_ID, firstId];
+
+ const homePath = (component: T): string[] => {
+ const parents = Array.isArray(component.parents) ? component.parents : [];
+ for (let i = parents.length - 1; i >= 0; i -= 1) {
+ const path = paths.get(parents[i]);
+ if (path && REATTACH_CONTAINER_TYPES.has(layout[parents[i]].type)) {
+ return path;
+ }
+ }
+ return fallbackPath;
+ };
+
+ const groups = new Map<string, [string[], [string, T][]]>();
+ rescued.forEach(([componentKey, component]) => {
+ const path = homePath(component);
+ const key = path.join('/');
+ const group = groups.get(key) ?? [path, []];
+ group[1].push([componentKey, component]);
+ groups.set(key, group);
+ });
+
+ groups.forEach(([rowParents, components]) => {
+ const containerId = rowParents[rowParents.length - 1];
+ const container = containerId ? next[containerId] : undefined;
+ if (!isComponent(container)) {
+ return;
+ }
+ const containerChildren = childrenOf(container);
+ let row: DashboardEntity | undefined;
+ let rowWidth = 0;
+ components.forEach(([componentKey, component]) => {
+ if (component.type === HEADER_TYPE) {
+ containerChildren.push(componentKey);
+ next[componentKey] = { ...component, parents: rowParents.slice() };
+ row = undefined;
+ return;
+ }
+ const { width: rawWidth } = component.meta ?? {};
+ const width =
+ typeof rawWidth === 'number' && rawWidth > 0
+ ? rawWidth
+ : GRID_DEFAULT_CHART_WIDTH;
+ if (!row || rowWidth + width > GRID_COLUMN_COUNT) {
+ row = newComponentFactory(ROW_TYPE, undefined, rowParents.slice());
+ next[row.id] = row;
+ containerChildren.push(row.id);
+ rowWidth = 0;
+ }
+ row.children.push(componentKey);
+ next[componentKey] = { ...component, parents: [...rowParents, row.id] };
+ rowWidth += width;
+ });
+ next[containerId] = { ...container, children: containerChildren };
+ });
+
+ return next;
+}
diff --git
a/superset-frontend/src/dashboard/util/updateComponentParentsList.test.ts
b/superset-frontend/src/dashboard/util/updateComponentParentsList.test.ts
index f187b41bf2c..dc4001d06d4 100644
--- a/superset-frontend/src/dashboard/util/updateComponentParentsList.test.ts
+++ b/superset-frontend/src/dashboard/util/updateComponentParentsList.test.ts
@@ -168,3 +168,20 @@ describe('updateComponentParentsList with bad inputs', ()
=> {
).not.toThrow();
});
});
+
+test('skips a back-edge to an ancestor instead of recursing forever', () => {
+ const layout: Record<
+ string,
+ { id: string; children?: string[]; parents?: string[] }
+ > = {
+ root: { id: 'root', children: ['row'] },
+ row: { id: 'row', children: ['col'] },
+ col: { id: 'col', children: ['row'] },
+ };
+
+ expect(() =>
+ updateComponentParentsList({ currentComponent: layout.root, layout }),
+ ).not.toThrow();
+ expect(layout.row.parents).toEqual(['root']);
+ expect(layout.col.parents).toEqual(['root', 'row']);
+});
diff --git a/superset-frontend/src/dashboard/util/updateComponentParentsList.ts
b/superset-frontend/src/dashboard/util/updateComponentParentsList.ts
index 67499d642c6..d0bbad7329a 100644
--- a/superset-frontend/src/dashboard/util/updateComponentParentsList.ts
+++ b/superset-frontend/src/dashboard/util/updateComponentParentsList.ts
@@ -44,7 +44,12 @@ export default function updateComponentParentsList({
if (Array.isArray(currentComponent.children)) {
currentComponent.children.forEach(childId => {
- if (layout[childId]) {
+ if (parentsList.includes(childId)) {
+ // A back-edge to an ancestor would recurse forever.
+ logging.warn(
+ `The component ${childId} is its own ancestor in the current
layout. Skipping this component`,
+ );
+ } else if (layout[childId]) {
// eslint-disable-next-line no-param-reassign
layout[childId] = {
...layout[childId],
diff --git a/superset/commands/dashboard/update.py
b/superset/commands/dashboard/update.py
index 56e87d66827..f0eddf2c554 100644
--- a/superset/commands/dashboard/update.py
+++ b/superset/commands/dashboard/update.py
@@ -48,6 +48,7 @@ from superset.commands.utils import (
)
from superset.daos.dashboard import DashboardDAO
from superset.daos.report import ReportScheduleDAO
+from superset.dashboards.layout import repair_position
from superset.exceptions import SupersetSecurityException
from superset.models.dashboard import Dashboard
from superset.reports.models import ReportSchedule
@@ -97,10 +98,12 @@ class UpdateDashboardCommand(UpdateMixin, BaseCommand):
ObjectType.dashboard, self._model.id, self._model.tags,
tags
)
- # Re-serialize position_json to escape 4-byte Unicode characters
+ # Re-serialize position_json to escape 4-byte Unicode characters,
+ # repairing a layout that carries detached components on the way
+ # through.
if position_json := self._properties.get("position_json"):
self._properties["position_json"] = json.dumps(
- json.loads(position_json)
+ repair_position(json.loads(position_json), self._model_id)
)
# ``set_dash_metadata`` merges the incoming metadata against
diff --git a/superset/daos/dashboard.py b/superset/daos/dashboard.py
index cb04587213a..6d323cd0717 100644
--- a/superset/daos/dashboard.py
+++ b/superset/daos/dashboard.py
@@ -36,6 +36,7 @@ from superset.commands.dashboard.exceptions import (
from superset.daos.base import BaseDAO, ColumnOperator, ColumnOperatorEnum
from superset.dashboards.filter_scope import derive_metadata_scopes
from superset.dashboards.filters import DashboardAccessFilter
+from superset.dashboards.layout import repair_position
from superset.exceptions import SupersetSecurityException
from superset.extensions import db
from superset.models.core import FavStar, FavStarClassName
@@ -384,6 +385,10 @@ class DashboardDAO(BaseDAO[Dashboard]):
chart_id = obj["meta"]["chartId"]
obj["meta"]["uuid"] = uuid_map.get(chart_id)
+ # Repair the layout before it is persisted; detached charts are
+ # reattached so their membership and cross-filter config survive.
+ positions = repair_position(positions, dashboard.id)
+
# remove leading and trailing white spaces in the dumped json
dashboard.position_json = json.dumps(
positions, indent=None, separators=(",", ":"), sort_keys=True
diff --git a/superset/dashboards/layout.py b/superset/dashboards/layout.py
new file mode 100644
index 00000000000..7f64ca24489
--- /dev/null
+++ b/superset/dashboards/layout.py
@@ -0,0 +1,269 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Structural repair for a dashboard's ``position_json``."""
+
+from __future__ import annotations
+
+import logging
+from typing import Any
+from uuid import uuid4
+
+logger = logging.getLogger(__name__)
+
+ROOT_ID = "ROOT_ID"
+CHART_TYPE = "CHART"
+HEADER_TYPE = "HEADER"
+MARKDOWN_TYPE = "MARKDOWN"
+ROW_TYPE = "ROW"
+TABS_TYPE = "TABS"
+# containers that accept both new rows and headers
+REATTACH_CONTAINER_TYPES = frozenset({"GRID", "TAB"})
+GRID_COLUMN_COUNT = 12
+GRID_DEFAULT_CHART_WIDTH = 4
+# ``HEADER_ID`` is dashboard metadata rather than a rendered child, and a
+# dashboard with top-level tabs keeps an empty, detached ``GRID_ID``. Both are
+# unreachable by design. Mirrors the reserved ids in the frontend's
+# ``removeUnreachableComponents``.
+RESERVED_IDS = frozenset({ROOT_ID, "GRID_ID", "HEADER_ID"})
+
+
+def _children(component: Any) -> list[str]:
+ children = component.get("children") if isinstance(component, dict) else
None
+ if not isinstance(children, list):
+ return []
+ return [child_id for child_id in children if isinstance(child_id, str)]
+
+
+def _chart_id(component: dict[str, Any]) -> Any:
+ meta = component.get("meta")
+ return meta.get("chartId") if isinstance(meta, dict) else None
+
+
+def _first_container_path(position: dict[str, Any]) -> list[str] | None:
+ """Path from ``ROOT_ID`` to where new charts go, mirroring the frontend's
+ ``findFirstParentContainerId``."""
+ top_level = _children(position.get(ROOT_ID))
+ if not top_level or not isinstance(position.get(top_level[0]), dict):
+ return None
+ if position[top_level[0]].get("type") == TABS_TYPE:
+ tabs = _children(position[top_level[0]])
+ if not tabs or not isinstance(position.get(tabs[0]), dict):
+ return None
+ return [ROOT_ID, top_level[0], tabs[0]]
+ return [ROOT_ID, top_level[0]]
+
+
+def _reattach_components(
+ position: dict[str, Any],
+ components: list[tuple[str, dict[str, Any]]],
+ container_path: list[str],
+) -> None:
+ container_id = container_path[-1]
+ container = position[container_id]
+ row_parents = list(container_path)
+ children = _children(container)
+ row: dict[str, Any] | None = None
+ row_width = 0
+ for component_key, component in components:
+ # a header is not a valid ROW child, so it goes directly in the
container
+ if component.get("type") == HEADER_TYPE:
+ children.append(component_key)
+ position[component_key] = {**component, "parents":
list(row_parents)}
+ row = None
+ continue
+ width = (component.get("meta") or {}).get("width")
+ if not isinstance(width, int) or width <= 0:
+ width = GRID_DEFAULT_CHART_WIDTH
+ if row is None or row_width + width > GRID_COLUMN_COUNT:
+ row_id = f"{ROW_TYPE}-{uuid4().hex[:10]}"
+ row = {
+ "id": row_id,
+ "type": ROW_TYPE,
+ "children": [],
+ "parents": list(row_parents),
+ "meta": {"background": "BACKGROUND_TRANSPARENT"},
+ }
+ position[row_id] = row
+ children.append(row_id)
+ row_width = 0
+ row["children"].append(component_key)
+ position[component_key] = {
+ **component,
+ "parents": [*row_parents, row["id"]],
+ }
+ row_width += width
+ position[container_id] = {**container, "children": children}
+
+
+def _reachable_paths(position: dict[str, Any]) -> dict[str, list[str]]:
+ """Map each id reachable from ``ROOT_ID`` to its path from ``ROOT_ID``."""
+ paths: dict[str, list[str]] = {}
+ stack: list[tuple[str, list[str]]] = [(ROOT_ID, [])]
+ while stack:
+ component_id, parent_path = stack.pop()
+ # doubles as the cycle guard: an id already seen is never expanded
twice
+ if component_id in paths:
+ continue
+ path = [*parent_path, component_id]
+ paths[component_id] = path
+ for child_id in _children(position.get(component_id)):
+ if isinstance(position.get(child_id), dict):
+ stack.append((child_id, path))
+ return paths
+
+
+def _home_container_path(
+ component: dict[str, Any],
+ position: dict[str, Any],
+ paths: dict[str, list[str]],
+) -> list[str] | None:
+ """Path to the deepest still-reachable container in the component's stored
+ ``parents``, so a rescued component returns to e.g. the tab it came from.
+ Only the tail of a stale chain is gone; its reachable prefix is trusted."""
+ parents = component.get("parents")
+ if not isinstance(parents, list):
+ return None
+ for parent_id in reversed(parents):
+ if (
+ isinstance(parent_id, str)
+ and parent_id in paths
+ and position.get(parent_id, {}).get("type") in
REATTACH_CONTAINER_TYPES
+ ):
+ return paths[parent_id]
+ return None
+
+
+def _reattach_rescued(
+ position: dict[str, Any],
+ rescued: list[tuple[str, dict[str, Any]]],
+ paths: dict[str, list[str]],
+) -> None:
+ """Reattach each rescued component to its home container, falling back to
+ the first top-level container."""
+ fallback_path = _first_container_path(position)
+ groups: dict[tuple[str, ...], list[tuple[str, dict[str, Any]]]] = {}
+ for component_id, component in rescued:
+ container_path = (
+ _home_container_path(component, position, paths) or fallback_path
+ )
+ if container_path:
+ groups.setdefault(tuple(container_path), []).append(
+ (component_id, component)
+ )
+ for group_path, components in groups.items():
+ _reattach_components(position, components, list(group_path))
+
+
+def remove_unreachable_components(
+ position: dict[str, Any],
+) -> tuple[dict[str, Any], list[str]]:
+ """Drop layout components that cannot be reached from ``ROOT_ID``.
+
+ Superset renders only what hangs off ``ROOT_ID``, so a detached component
is
+ already invisible. It still survives in ``position_json``, where code that
+ walks every entry or trusts a node's stale ``parents`` can find it — a
+ detached subtree holding a cycle is what crashes the filter scope modal
with
+ "Maximum call stack size exceeded".
+
+ A detached chart is reattached in new rows of the deepest container in its
+ stored ``parents`` that is still reachable (falling back to the first
+ top-level container) instead of dropped, keeping its layout id. That
preserves its
+ ``chart_configuration`` (derived scopes drop entries for charts absent from
+ the layout) and its dashboard membership even when the chart is archived
and
+ so absent from the charts the frontend loads. A chart that is also placed
+ reachably is not duplicated.
+
+ Detached markdown and headers are reattached the same way, since their text
+ lives nowhere but ``position_json``. Markdown goes into the new rows; a
+ header, which cannot be a row child, goes directly into the container.
+
+ Returns the (possibly unchanged) position and the ids that were detached;
+ reattached components are among them. Non-dict entries such as
+ ``DASHBOARD_VERSION_KEY`` are never removed.
+ """
+ root = position.get(ROOT_ID) if isinstance(position, dict) else None
+ # a malformed root would make everything look detached; leave it alone
+ if not isinstance(root, dict) or not isinstance(root.get("children"),
list):
+ return position, []
+
+ paths = _reachable_paths(position)
+ reachable = set(paths)
+
+ removed = [
+ component_id
+ for component_id, component in position.items()
+ if isinstance(component, dict)
+ and component_id not in reachable
+ and component_id not in RESERVED_IDS
+ ]
+ if not removed:
+ return position, []
+
+ placed_chart_ids = {
+ _chart_id(position[component_id])
+ for component_id in reachable
+ if position[component_id].get("type") == CHART_TYPE
+ }
+ rescued: list[tuple[str, dict[str, Any]]] = []
+ for component_id in removed:
+ component = position[component_id]
+ component_type = component.get("type")
+ if component_type in (MARKDOWN_TYPE, HEADER_TYPE):
+ rescued.append((component_id, component))
+ continue
+ chart_id = _chart_id(component)
+ if (
+ component_type == CHART_TYPE
+ and chart_id is not None
+ and chart_id not in placed_chart_ids
+ ):
+ placed_chart_ids.add(chart_id)
+ rescued.append((component_id, component))
+
+ repaired = {
+ component_id: component
+ for component_id, component in position.items()
+ if component_id not in removed
+ }
+ # a detached reserved id is kept, but must not reference the dropped nodes
+ for component_id in RESERVED_IDS - reachable:
+ component = repaired.get(component_id)
+ if isinstance(component, dict) and component.get("children"):
+ repaired[component_id] = {**component, "children": []}
+ _reattach_rescued(repaired, rescued, paths)
+ return repaired, removed
+
+
+def repair_position(
+ position: dict[str, Any], dashboard_id: int | None = None
+) -> dict[str, Any]:
+ """Return *position* with detached components repaired, logging which.
+
+ Thin wrapper over ``remove_unreachable_components`` for the write paths,
+ which care about the repaired layout rather than the detached ids.
+ """
+ repaired, removed = remove_unreachable_components(position)
+ if removed:
+ logger.warning(
+ "Dashboard %s: repaired %d layout component(s) unreachable from "
+ "ROOT_ID: %s",
+ dashboard_id,
+ len(removed),
+ ", ".join(sorted(removed)),
+ )
+ return repaired
diff --git a/superset/mcp_service/dashboard/tool/generate_dashboard.py
b/superset/mcp_service/dashboard/tool/generate_dashboard.py
index 4cac9d3214c..624a04d9268 100644
--- a/superset/mcp_service/dashboard/tool/generate_dashboard.py
+++ b/superset/mcp_service/dashboard/tool/generate_dashboard.py
@@ -30,6 +30,7 @@ from pydantic import ValidationError
from sqlalchemy.exc import IntegrityError, SQLAlchemyError
from superset_core.mcp.decorators import tool, ToolAnnotations
+from superset.dashboards.layout import repair_position
from superset.extensions import db, event_logger
from superset.mcp_service.dashboard.constants import (
generate_id,
@@ -277,7 +278,7 @@ def generate_dashboard( # noqa: C901
# 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)
+ layout =
rebuild_parent_chains(repair_position(request.position_json))
else:
layout = _create_dashboard_layout(chart_objects)
diff --git a/tests/unit_tests/commands/dashboard/update_test.py
b/tests/unit_tests/commands/dashboard/update_test.py
index 1f425a394e4..0105f63e4ca 100644
--- a/tests/unit_tests/commands/dashboard/update_test.py
+++ b/tests/unit_tests/commands/dashboard/update_test.py
@@ -17,6 +17,8 @@
from unittest.mock import MagicMock, patch, PropertyMock
+from pytest_mock import MockerFixture
+
from superset.commands.dashboard.update import UpdateDashboardCommand
from superset.utils import json
@@ -166,3 +168,47 @@ def
test_process_native_filter_diff_only_touches_reports_on_the_updated_dashboar
# NATIVE_FILTER-2 is the only one dropped from the new metadata.
report_dao.find_by_native_filter_id.assert_called_once_with("NATIVE_FILTER-2")
report_dao.update.assert_called_once_with(own_report, {"active": False})
+
+
+def test_run_repairs_a_position_json_with_a_detached_cycle(
+ mocker: MockerFixture, app_context: None
+) -> None:
+ """A PUT carrying a corrupted layout is repaired before it is persisted."""
+ position = {
+ "ROOT_ID": {"id": "ROOT_ID", "type": "ROOT", "children": ["GRID_ID"]},
+ "GRID_ID": {"id": "GRID_ID", "type": "GRID", "children": []},
+ # a column dropped into a row nested inside itself: a detached cycle
+ "COLUMN-orphan": {
+ "id": "COLUMN-orphan",
+ "type": "COLUMN",
+ "children": ["CHART-trapped", "ROW-orphan"],
+ },
+ "ROW-orphan": {
+ "id": "ROW-orphan",
+ "type": "ROW",
+ "children": ["COLUMN-orphan"],
+ },
+ "CHART-trapped": {
+ "id": "CHART-trapped",
+ "type": "CHART",
+ "children": [],
+ "meta": {"chartId": 2, "width": 4},
+ },
+ }
+ command = UpdateDashboardCommand(1, {"position_json":
json.dumps(position)})
+ command._model = MagicMock() # noqa: SLF001
+ mocker.patch.object(command, "validate")
+ mocker.patch.object(command, "process_tab_diff")
+ mocker.patch.object(command, "process_native_filter_diff")
+ mocker.patch("superset.db")
+ mocker.patch("superset.commands.dashboard.update.db")
+ dao = mocker.patch("superset.commands.dashboard.update.DashboardDAO")
+
+ command.run()
+
+ saved = json.loads(dao.update.call_args.args[1]["position_json"])
+ assert "COLUMN-orphan" not in saved
+ assert "ROW-orphan" not in saved
+ [new_row_id] = saved["GRID_ID"]["children"]
+ assert saved[new_row_id]["children"] == ["CHART-trapped"]
+ assert saved["CHART-trapped"]["parents"] == ["ROOT_ID", "GRID_ID",
new_row_id]
diff --git a/tests/unit_tests/dao/dashboard_test.py
b/tests/unit_tests/dao/dashboard_test.py
index a263e8807e2..830cb2feef8 100644
--- a/tests/unit_tests/dao/dashboard_test.py
+++ b/tests/unit_tests/dao/dashboard_test.py
@@ -22,7 +22,9 @@ from sqlalchemy.orm.session import Session
from superset import db
from superset.connectors.sqla.models import Database, SqlaTable
from superset.daos.dashboard import DashboardDAO
+from superset.dashboards.filter_scope import derive_metadata_scopes
from superset.models.dashboard import Dashboard
+from superset.models.helpers import skip_visibility_filter
from superset.models.slice import Slice
from superset.utils import json
from tests.unit_tests.conftest import with_feature_flags
@@ -168,3 +170,140 @@ def
test_set_dash_metadata_updates_refresh_frequency_when_present(
assert md["refresh_frequency"] == 0, (
"refresh_frequency should be updated when present in data"
)
+
+
+def _position_with_trapped_chart(
+ placed_chart_id: int, trapped_chart_id: int
+) -> dict[str, Any]:
+ """A layout where a column was dropped into a row nested inside itself."""
+ return {
+ "DASHBOARD_VERSION_KEY": "v2",
+ "ROOT_ID": {"id": "ROOT_ID", "type": "ROOT", "children": ["GRID_ID"]},
+ "GRID_ID": {
+ "id": "GRID_ID",
+ "type": "GRID",
+ "children": ["ROW-a"],
+ "parents": ["ROOT_ID"],
+ },
+ "ROW-a": {
+ "id": "ROW-a",
+ "type": "ROW",
+ "children": ["CHART-placed"],
+ "parents": ["ROOT_ID", "GRID_ID"],
+ "meta": {},
+ },
+ "CHART-placed": {
+ "id": "CHART-placed",
+ "type": "CHART",
+ "children": [],
+ "parents": ["ROOT_ID", "GRID_ID", "ROW-a"],
+ "meta": {"chartId": placed_chart_id, "width": 4, "height": 50},
+ },
+ "COLUMN-orphan": {
+ "id": "COLUMN-orphan",
+ "type": "COLUMN",
+ "children": ["CHART-trapped", "ROW-orphan"],
+ "parents": ["ROOT_ID", "GRID_ID", "ROW-a"],
+ "meta": {},
+ },
+ "ROW-orphan": {
+ "id": "ROW-orphan",
+ "type": "ROW",
+ "children": ["COLUMN-orphan"],
+ "parents": ["ROOT_ID", "GRID_ID", "ROW-a", "COLUMN-orphan"],
+ "meta": {},
+ },
+ "CHART-trapped": {
+ "id": "CHART-trapped",
+ "type": "CHART",
+ "children": [],
+ "parents": ["ROOT_ID", "GRID_ID", "ROW-a", "COLUMN-orphan"],
+ "meta": {"chartId": trapped_chart_id, "width": 4, "height": 50},
+ },
+ }
+
+
+def _make_charts(
+ session: Session, trapped_deleted: bool = False
+) -> tuple[Slice, Slice]:
+ Dashboard.metadata.create_all(session.get_bind())
+ dataset = SqlaTable(
+ table_name="trapped_table",
+ database=Database(database_name="trapped_db",
sqlalchemy_uri="sqlite://"),
+ )
+ db.session.add(dataset)
+ db.session.flush()
+ placed = Slice(
+ slice_name="placed", datasource_id=dataset.id, datasource_type="table"
+ )
+ trapped = Slice(
+ slice_name="trapped",
+ datasource_id=dataset.id,
+ datasource_type="table",
+ deleted_at=datetime(2026, 1, 1, tzinfo=timezone.utc)
+ if trapped_deleted
+ else None,
+ )
+ db.session.add_all([placed, trapped])
+ db.session.flush()
+ return placed, trapped
+
+
+def test_set_dash_metadata_keeps_cross_filter_config_of_trapped_chart(
+ session: Session,
+) -> None:
+ placed, trapped = _make_charts(session)
+ dashboard = Dashboard(dashboard_title="trapped", slices=[placed, trapped])
+ db.session.add(dashboard)
+ db.session.flush()
+
+ scope = {"rootPath": ["ROOT_ID"], "excluded": [trapped.id, placed.id]}
+ DashboardDAO.set_dash_metadata(
+ dashboard,
+ {
+ "positions": _position_with_trapped_chart(placed.id, trapped.id),
+ "chart_configuration": {
+ str(trapped.id): {
+ "id": trapped.id,
+ "crossFilters": {"scope": scope, "chartsInScope": []},
+ }
+ },
+ },
+ )
+
+ derived = derive_metadata_scopes(dashboard, dashboard.params_dict)
+
+ cross_filters =
derived["chart_configuration"][str(trapped.id)]["crossFilters"]
+ assert cross_filters["scope"] == scope
+ assert cross_filters["chartsInScope"] == []
+
+
+@with_feature_flags(SOFT_DELETE=True)
+def test_set_dash_metadata_keeps_archived_trapped_chart_through_resave(
+ session: Session,
+) -> None:
+ placed, trapped = _make_charts(session, trapped_deleted=True)
+ dashboard = Dashboard(dashboard_title="trapped", slices=[placed, trapped])
+ db.session.add(dashboard)
+ db.session.flush()
+
+ DashboardDAO.set_dash_metadata(
+ dashboard,
+ {"positions": _position_with_trapped_chart(placed.id, trapped.id)},
+ )
+ db.session.flush()
+
+ # The client re-saves what it loaded; the archived chart is absent from the
+ # charts payload, so only its layout entry can carry its membership.
+ saved = json.loads(dashboard.position_json)
+ assert "COLUMN-orphan" not in saved
+ assert "ROW-orphan" not in saved
+ assert saved["CHART-trapped"]["parents"][:2] == ["ROOT_ID", "GRID_ID"]
+
+ db.session.expire(dashboard, ["slices"])
+ DashboardDAO.set_dash_metadata(dashboard, {"positions": saved})
+ db.session.flush()
+
+ with skip_visibility_filter(db.session, Slice):
+ db.session.expire(dashboard, ["slices"])
+ assert {chart.id for chart in dashboard.slices} == {placed.id,
trapped.id}
diff --git a/tests/unit_tests/dashboards/layout_test.py
b/tests/unit_tests/dashboards/layout_test.py
new file mode 100644
index 00000000000..894f4718f4a
--- /dev/null
+++ b/tests/unit_tests/dashboards/layout_test.py
@@ -0,0 +1,325 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from typing import Any
+
+from superset.dashboards.layout import remove_unreachable_components
+
+
+def reachable_position() -> dict[str, Any]:
+ return {
+ "DASHBOARD_VERSION_KEY": "v2",
+ "ROOT_ID": {"id": "ROOT_ID", "type": "ROOT", "children": ["GRID_ID"]},
+ "GRID_ID": {"id": "GRID_ID", "type": "GRID", "children": ["ROW-a"]},
+ "ROW-a": {"id": "ROW-a", "type": "ROW", "children": ["CHART-a"]},
+ "CHART-a": {"id": "CHART-a", "type": "CHART", "children": []},
+ "HEADER_ID": {"id": "HEADER_ID", "type": "HEADER"},
+ }
+
+
+def test_intact_layout_is_returned_unchanged() -> None:
+ position = reachable_position()
+
+ assert remove_unreachable_components(position) == (position, [])
+
+
+def trapped_components(chart_meta: dict[str, Any]) -> dict[str, Any]:
+ # the shape of the reported corruption: a column moved into a row nested
+ # inside itself, leaving the pair pointing at each other and detached
+ return {
+ "COLUMN-orphan": {
+ "id": "COLUMN-orphan",
+ "type": "COLUMN",
+ "children": ["CHART-trapped", "ROW-orphan"],
+ "parents": ["ROOT_ID", "GRID_ID", "ROW-a"],
+ },
+ "ROW-orphan": {
+ "id": "ROW-orphan",
+ "type": "ROW",
+ "children": ["COLUMN-orphan"],
+ "parents": ["ROOT_ID", "GRID_ID", "ROW-a", "COLUMN-orphan"],
+ },
+ "CHART-trapped": {
+ "id": "CHART-trapped",
+ "type": "CHART",
+ "children": [],
+ "parents": ["ROOT_ID", "GRID_ID", "ROW-a", "COLUMN-orphan"],
+ "meta": chart_meta,
+ },
+ }
+
+
+def test_detached_subtree_with_a_cycle_is_removed() -> None:
+ position = reachable_position() | trapped_components({})
+
+ cleaned, removed = remove_unreachable_components(position)
+
+ assert cleaned == reachable_position()
+ assert sorted(removed) == ["CHART-trapped", "COLUMN-orphan", "ROW-orphan"]
+
+
+def test_detached_chart_is_reattached_to_the_grid() -> None:
+ position = reachable_position() | trapped_components({"chartId": 2,
"width": 4})
+
+ cleaned, removed = remove_unreachable_components(position)
+
+ assert sorted(removed) == ["CHART-trapped", "COLUMN-orphan", "ROW-orphan"]
+ assert "COLUMN-orphan" not in cleaned
+ assert "ROW-orphan" not in cleaned
+ new_row_id = cleaned["GRID_ID"]["children"][-1]
+ assert cleaned["GRID_ID"]["children"] == ["ROW-a", new_row_id]
+ assert cleaned[new_row_id]["type"] == "ROW"
+ assert cleaned[new_row_id]["children"] == ["CHART-trapped"]
+ assert cleaned["CHART-trapped"]["parents"] == ["ROOT_ID", "GRID_ID",
new_row_id]
+ assert cleaned["CHART-trapped"]["meta"] == {"chartId": 2, "width": 4}
+ # the caller's position is not mutated
+ assert position["GRID_ID"]["children"] == ["ROW-a"]
+
+
+def test_detached_markdown_and_header_are_reattached() -> None:
+ position = reachable_position() | {
+ "COLUMN-orphan": {
+ "id": "COLUMN-orphan",
+ "type": "COLUMN",
+ "children": ["HEADER-trapped", "MARKDOWN-trapped", "ROW-orphan"],
+ },
+ "ROW-orphan": {
+ "id": "ROW-orphan",
+ "type": "ROW",
+ "children": ["COLUMN-orphan"],
+ },
+ "HEADER-trapped": {
+ "id": "HEADER-trapped",
+ "type": "HEADER",
+ "children": [],
+ "meta": {"text": "Section title"},
+ },
+ "MARKDOWN-trapped": {
+ "id": "MARKDOWN-trapped",
+ "type": "MARKDOWN",
+ "children": [],
+ "meta": {"code": "# Notes", "width": 4},
+ },
+ }
+
+ cleaned, removed = remove_unreachable_components(position)
+
+ assert sorted(removed) == [
+ "COLUMN-orphan",
+ "HEADER-trapped",
+ "MARKDOWN-trapped",
+ "ROW-orphan",
+ ]
+ new_row_id = cleaned["GRID_ID"]["children"][-1]
+ # a header is not a valid row child, so it sits directly in the grid
+ assert cleaned["GRID_ID"]["children"] == ["ROW-a", "HEADER-trapped",
new_row_id]
+ assert cleaned["HEADER-trapped"]["parents"] == ["ROOT_ID", "GRID_ID"]
+ assert cleaned["HEADER-trapped"]["meta"] == {"text": "Section title"}
+ assert cleaned[new_row_id]["children"] == ["MARKDOWN-trapped"]
+ assert cleaned["MARKDOWN-trapped"]["parents"] == ["ROOT_ID", "GRID_ID",
new_row_id]
+ assert cleaned["MARKDOWN-trapped"]["meta"] == {"code": "# Notes", "width":
4}
+
+
+def test_detached_chart_is_reattached_to_the_first_tab() -> None:
+ position = {
+ "DASHBOARD_VERSION_KEY": "v2",
+ "ROOT_ID": {"id": "ROOT_ID", "type": "ROOT", "children": ["TABS-t"]},
+ "GRID_ID": {"id": "GRID_ID", "type": "GRID", "children": []},
+ "TABS-t": {
+ "id": "TABS-t",
+ "type": "TABS",
+ "children": ["TAB-1", "TAB-2"],
+ "parents": ["ROOT_ID"],
+ },
+ "TAB-1": {
+ "id": "TAB-1",
+ "type": "TAB",
+ "children": [],
+ "parents": ["ROOT_ID", "TABS-t"],
+ },
+ "TAB-2": {"id": "TAB-2", "type": "TAB", "children": []},
+ "CHART-trapped": {
+ "id": "CHART-trapped",
+ "type": "CHART",
+ "children": [],
+ "meta": {"chartId": 2},
+ },
+ }
+
+ cleaned, _ = remove_unreachable_components(position)
+
+ [new_row_id] = cleaned["TAB-1"]["children"]
+ assert cleaned["CHART-trapped"]["parents"] == [
+ "ROOT_ID",
+ "TABS-t",
+ "TAB-1",
+ new_row_id,
+ ]
+
+
+def test_detached_chart_returns_to_the_tab_in_its_stale_parents() -> None:
+ position = {
+ "ROOT_ID": {"id": "ROOT_ID", "type": "ROOT", "children": ["TABS-t"]},
+ "TABS-t": {"id": "TABS-t", "type": "TABS", "children": ["TAB-1",
"TAB-2"]},
+ "TAB-1": {"id": "TAB-1", "type": "TAB", "children": []},
+ "TAB-2": {"id": "TAB-2", "type": "TAB", "children": []},
+ "CHART-trapped": {
+ "id": "CHART-trapped",
+ "type": "CHART",
+ "children": [],
+ "parents": ["ROOT_ID", "TABS-t", "TAB-2", "ROW-gone",
"COLUMN-gone"],
+ "meta": {"chartId": 2},
+ },
+ }
+
+ cleaned, _ = remove_unreachable_components(position)
+
+ assert cleaned["TAB-1"]["children"] == []
+ [new_row_id] = cleaned["TAB-2"]["children"]
+ assert cleaned["CHART-trapped"]["parents"] == [
+ "ROOT_ID",
+ "TABS-t",
+ "TAB-2",
+ new_row_id,
+ ]
+
+
+def test_detached_header_and_markdown_return_to_their_tab_in_order() -> None:
+ stale_parents = ["ROOT_ID", "TABS-t", "TAB-2", "ROW-gone"]
+ position = {
+ "ROOT_ID": {"id": "ROOT_ID", "type": "ROOT", "children": ["TABS-t"]},
+ "TABS-t": {"id": "TABS-t", "type": "TABS", "children": ["TAB-1",
"TAB-2"]},
+ "TAB-1": {"id": "TAB-1", "type": "TAB", "children": []},
+ "TAB-2": {"id": "TAB-2", "type": "TAB", "children": []},
+ "CHART-trapped": {
+ "id": "CHART-trapped",
+ "type": "CHART",
+ "children": [],
+ "parents": stale_parents,
+ "meta": {"chartId": 2},
+ },
+ "HEADER-trapped": {
+ "id": "HEADER-trapped",
+ "type": "HEADER",
+ "children": [],
+ "parents": ["ROOT_ID", "TABS-t", "TAB-2"],
+ },
+ "MARKDOWN-trapped": {
+ "id": "MARKDOWN-trapped",
+ "type": "MARKDOWN",
+ "children": [],
+ "parents": stale_parents,
+ },
+ }
+
+ cleaned, _ = remove_unreachable_components(position)
+
+ assert cleaned["TAB-1"]["children"] == []
+ chart_row, header_id, markdown_row = cleaned["TAB-2"]["children"]
+ assert header_id == "HEADER-trapped"
+ assert cleaned["HEADER-trapped"]["parents"] == ["ROOT_ID", "TABS-t",
"TAB-2"]
+ assert cleaned[chart_row]["children"] == ["CHART-trapped"]
+ assert cleaned[markdown_row]["children"] == ["MARKDOWN-trapped"]
+
+
+def test_detached_chart_already_placed_is_not_duplicated() -> None:
+ position = reachable_position()
+ position["CHART-a"]["meta"] = {"chartId": 2}
+ position |= trapped_components({"chartId": 2})
+
+ cleaned, _ = remove_unreachable_components(position)
+
+ assert "CHART-trapped" not in cleaned
+ assert cleaned["GRID_ID"]["children"] == ["ROW-a"]
+
+
+def test_detached_charts_wrap_into_rows_by_width() -> None:
+ position = reachable_position()
+ for index, width in enumerate([6, 6, 4]):
+ chart_key = f"CHART-orphan-{index}"
+ position[chart_key] = {
+ "id": chart_key,
+ "type": "CHART",
+ "children": [],
+ "meta": {"chartId": 10 + index, "width": width},
+ }
+
+ cleaned, _ = remove_unreachable_components(position)
+
+ new_rows = cleaned["GRID_ID"]["children"][1:]
+ assert [cleaned[row_id]["children"] for row_id in new_rows] == [
+ ["CHART-orphan-0", "CHART-orphan-1"],
+ ["CHART-orphan-2"],
+ ]
+
+
+def test_malformed_children_do_not_raise() -> None:
+ position = reachable_position()
+ position["ROW-a"]["children"] = ["CHART-a", ["nested"], {"id": "x"}, 3]
+ position["CHART-a"]["children"] = {"not": "a list"}
+
+ assert remove_unreachable_components(position) == (position, [])
+
+
+def test_detached_grid_of_a_tabbed_dashboard_is_kept() -> None:
+ position = {
+ "DASHBOARD_VERSION_KEY": "v2",
+ "ROOT_ID": {"id": "ROOT_ID", "type": "ROOT", "children": ["TABS-t"]},
+ "GRID_ID": {"id": "GRID_ID", "type": "GRID", "children": []},
+ "TABS-t": {"id": "TABS-t", "type": "TABS", "children": ["TAB-1"]},
+ "TAB-1": {"id": "TAB-1", "type": "TAB", "children": []},
+ "HEADER_ID": {"id": "HEADER_ID", "type": "HEADER"},
+ }
+
+ assert remove_unreachable_components(position) == (position, [])
+
+
+def test_detached_grid_children_are_cleared() -> None:
+ position: dict[str, Any] = {
+ "DASHBOARD_VERSION_KEY": "v2",
+ "ROOT_ID": {"id": "ROOT_ID", "type": "ROOT", "children": ["TABS-t"]},
+ "GRID_ID": {"id": "GRID_ID", "type": "GRID", "children":
["ROW-stale"]},
+ "ROW-stale": {"id": "ROW-stale", "type": "ROW", "children": []},
+ "TABS-t": {"id": "TABS-t", "type": "TABS", "children": ["TAB-1"]},
+ "TAB-1": {"id": "TAB-1", "type": "TAB", "children": []},
+ }
+
+ cleaned, removed = remove_unreachable_components(position)
+
+ assert removed == ["ROW-stale"]
+ assert cleaned["GRID_ID"]["children"] == []
+ assert position["GRID_ID"]["children"] == ["ROW-stale"]
+
+
+def test_layout_without_a_root_is_left_alone() -> None:
+ position = {"ROW-a": {"id": "ROW-a", "type": "ROW", "children": []}}
+
+ assert remove_unreachable_components(position) == (position, [])
+
+
+def test_missing_child_reference_does_not_raise() -> None:
+ position = reachable_position()
+ position["ROW-a"]["children"] = ["CHART-a", "CHART-does-not-exist"]
+
+ assert remove_unreachable_components(position) == (position, [])
+
+
+def test_malformed_root_children_leave_the_layout_alone() -> None:
+ position = reachable_position()
+ position["ROOT_ID"]["children"] = {"0": "GRID_ID"}
+
+ assert remove_unreachable_components(position) == (position, [])
diff --git
a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
index 3d41f405ef8..f5eef0d86a6 100644
--- a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
+++ b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
@@ -888,6 +888,69 @@ class TestGenerateDashboard:
"TAB-out-quarter",
]
+ @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_repairs_detached_cycle(
+ self,
+ mock_db_session,
+ mock_find_by_id,
+ mock_dashboard_cls,
+ mcp_server,
+ ) -> None:
+ """A caller-supplied layout with a detached COLUMN/ROW cycle is
+ repaired like the other write paths: the cycle is dropped and the
+ chart trapped in it is reattached under the grid."""
+ from superset.utils import json
+
+ charts = [_mock_chart(id=1, slice_name="Sales")]
+ mock_dashboard = _mock_dashboard(id=73, title="Detached Cycle")
+ _setup_generate_dashboard_mocks(
+ mock_db_session,
+ mock_find_by_id,
+ mock_dashboard_cls,
+ charts,
+ mock_dashboard,
+ )
+
+ layout = {
+ "DASHBOARD_VERSION_KEY": "v2",
+ "ROOT_ID": {"id": "ROOT_ID", "type": "ROOT", "children":
["GRID_ID"]},
+ "GRID_ID": {"id": "GRID_ID", "type": "GRID", "children": []},
+ "COLUMN-orphan": {
+ "id": "COLUMN-orphan",
+ "type": "COLUMN",
+ "children": ["CHART-1", "ROW-orphan"],
+ },
+ "ROW-orphan": {
+ "id": "ROW-orphan",
+ "type": "ROW",
+ "children": ["COLUMN-orphan"],
+ },
+ "CHART-1": {
+ "id": "CHART-1",
+ "type": "CHART",
+ "children": [],
+ "meta": {"chartId": 1},
+ },
+ }
+ request = {
+ "chart_ids": [1],
+ "dashboard_title": "Detached Cycle",
+ "position_json": layout,
+ }
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool("generate_dashboard", {"request":
request})
+
+ assert result.structured_content["error"] is None
+ stored = json.loads(mock_dashboard_cls.return_value.position_json)
+ assert "COLUMN-orphan" not in stored
+ assert "ROW-orphan" not in stored
+ [new_row_id] = stored["GRID_ID"]["children"]
+ assert stored["CHART-1"]["parents"] == ["ROOT_ID", "GRID_ID",
new_row_id]
+
@patch("superset.models.dashboard.Dashboard")
@patch("superset.daos.dashboard.DashboardDAO.find_by_id")
@patch("superset.db.session")
