Repository navigation
fix(dashboard): saving a dashboard that contains an archived chart no longer fails - #45543
Conversation
A dashboard loaded through a query carries the soft-delete filter into the lazy load of its chart list, so the baseline that set_dash_metadata diffs against omitted archived members even inside the visibility bypass. Saving the layout back then re-inserted the archived chart's dashboard_slices row and failed on the composite primary key. Read the baseline from the association table and install it as the committed state. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #45543 +/- ##
==========================================
+ Coverage 83.98% 84.10% +0.11%
==========================================
Files 3055 3054 -1
Lines 201048 201635 +587
Branches 46967 47112 +145
==========================================
+ Hits 168855 169587 +732
+ Misses 28948 28804 -144
+ Partials 3245 3244 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
CodeAnt PR Risk: Low Risk
Assessed commit: |
fitzee
left a comment
fitzee
left a comment
There was a problem hiding this comment.
No blocking issues; LGTM. The fix stays inside the existing soft-delete bypass, changes only the baseline the membership diff runs against, and does not widen which chart IDs a layout can link.
What I checked
- Root cause and fix. A dashboard loaded through a query carries the
deleted_at IS NULLloader criteria into its lazy loads, sodashboard.slicescame back without archived members. Assigning the rebuilt list then INSERTed adashboard_slicesrow that already existed. Reading the baseline straight fromdashboard_slicesand installing it withset_committed_value(superset/daos/dashboard.py:328-355, called at:697) gives the unit of work the real stored membership. The archived chart keeps its row and its slot, and restoring it brings the tile back, which the integration test asserts end to end. The frontend already keeps archived chart nodes in the layout (removeUnreachableComponents.ts), so a normal UI save sends the node back and goes through this path. - Behaviour change, intended. Now that the baseline includes archived members, a save that deliberately removes an archived chart's tile deletes its
dashboard_slicesrow. Before this PR that save left the row in place. Removing the tile now detaches the chart, so restoring it later does not put it back. That's the right semantics. - No access bypass. The PR does not change chart-ID resolution. The
skip_visibility_filteraroundSlice.id.in_(slice_ids)was already on master, so archived non-member IDs in a layout already resolved and linked before this change. Once #45538 lands,_member_chart_idsexempts only this dashboard's own junction rows, and every other ID, archived or not, still goes throughcan_access_chart. A user who can't read another user's archived chart can't attach it, so it can't later reach them through dashboard-based access when it's restored. This PR doesn't depend on #45538. #45538 is still open, and the two only conflict textually. - Other callers. The pending-change early return can't trigger from the PUT path:
UpdateDashboardCommand.runflushes explicitly beforeset_dash_metadata. Create and copy call it on dashboards that are either just flushed (empty or correct baseline) or not yet persistent (skipped). - Archived chart whose datasource was purged. The baseline query never touches the datasource, so the slot and membership survive. Detached charts (junction row removed, node still in the layout) behave as before: they are absent from the baseline and get re-linked. Whether that 422s for a non-reader depends on #45538, not on this PR.
Nits (non-blocking)
tests/unit_tests/dao/dashboard_test.py:140:test_install_membership_baseline_skips_transient_dashboarddoesn't exercise thestate.persistentguard atdashboard.py:340. Deleting the guard still passes, because the transient dashboard'sslices=[chart]is a pending change and thehas_changes()early return at:348catches it first. A transient dashboard with no chart-list changes would leave only the persistent guard in play. That case is harmless anyway (empty baseline), so the guard could also be dropped as redundant.tests/unit_tests/dao/dashboard_test.py:46: the unit test has a single dashboard, so it still passes with thedashboard_slices.c.dashboard_id == dashboard.idfilter (dashboard.py:345) removed. A second dashboard sharing one of the charts would pin it. The integration test runs against a populated DB, so it probably covers this already.- After #45538 merges,
set_dash_metadatawill readdashboard_slicesfor the same dashboard twice in a row (_member_chart_idsand_install_membership_baseline). Consider deriving the member IDs from the installed baseline in whichever PR lands second.
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Thanks for tracking this down. The root cause and the set_committed_value approach look right, and the new tests fail on master with the exact PK error. One blocker inline.
| db.session.query(Slice).filter(Slice.id.in_(slice_ids)).all() | ||
| ) | ||
| _install_membership_baseline(dashboard) | ||
| dashboard.slices = current_slices |
There was a problem hiding this comment.
🔴 Blocker: saving the dashboard severs an archived chart that isn't in the layout
With the baseline now including archived members, any archived chart missing from positions gets its dashboard_slices row deleted on save. One way to get there: a chart is saved from Explore with "add to dashboard" (commands/chart/create.py adds the junction row but leaves position_json alone), then archived before the dashboard is next saved. The charts payload omits archived charts, so hydrate never adds it to the layout, and the next save drops the link. Restoring the chart no longer re-attaches it, which breaks the UPDATING.md contract: "saving a dashboard does not sever a trashed member".
Repro (unit-level, dashboard loaded through a query, one live chart in the layout plus one archived chart that is only in dashboard_slices): members after save are {1, 2} on the merge base and {1} on this branch.
Suggested fix: keep archived members that the layout doesn't carry. With this applied, the repro passes, as do tests/unit_tests/dao/dashboard_test.py and tests/unit_tests/commands/dashboard (209 passed). A regression test for the "archived member not in the layout" case would be good too.
| dashboard.slices = current_slices | |
| # Never sever an archived member the layout does not carry. | |
| layout_ids = {slc.id for slc in current_slices} | |
| kept_archived = [ | |
| slc | |
| for slc in dashboard.slices | |
| if slc.deleted_at is not None and slc.id not in layout_ids | |
| ] | |
| dashboard.slices = current_slices + kept_archived |
Side effect: removing an archived chart's placeholder from the layout would also keep its link again (the merge-base behaviour, and what UPDATING.md describes). If you'd rather keep severing in that case, an alternative is to keep only archived members that were also absent from the previously stored position_json.
There was a problem hiding this comment.
Thanks for the repro. Saving now keeps existing archived members even when they have no slot in the submitted layout; omitted live charts are still detached. The regression test test_set_dash_metadata_preserves_archived_member_missing_from_layout query-loads the dashboard and covers archived members absent from the stored layout as well as removal of an existing archived slot. Both cases fail before the fix and pass with it.
There was a problem hiding this comment.
Confirmed closed, and good catch @rebenitez1802. I rebuilt your repro independently (query-loaded dashboard, one live chart in the layout, one archived chart only in dashboard_slices) and landed on your exact numbers: members {1, 2} on the merge base, {1} on c927483eb, {1, 2} again on e30773a10.
On the design question that was left open, this implements the first of your two options. kept_archived is subtracted against layout_ids, so an archived member survives however it came to be missing from the submitted layout, including a user explicitly deleting its placeholder tile. I measured that shape separately (stored layout carrying the archived slot, submitted layout without it): {1, 2} here, {1} on c927483eb. That is the merge-base behaviour and it matches UPDATING.md, "saving a dashboard does not sever a trashed member".
Worth saying out loud because @fitzee's review on c927483eb describes the opposite as intended ("a save that deliberately removes an archived chart's tile deletes its dashboard_slices row ... That's the right semantics"), so I would rather the three of us agree which one we are shipping than have it settled by whoever reads the diff last. My own read is that the UPDATING.md line decides it and this branch is right.
There was a problem hiding this comment.
AI-assisted reply on Mike's behalf.
I think this is settled. @fitzee's re-review on e30773a1 withdraws the earlier point ("Withdrawn: 'removing an archived tile detaches the chart is the right semantics'") and agrees this branch is right, so the three of us are on option 1, which matches UPDATING.md: saving a dashboard does not sever a trashed member.
- If a user deletes an archived chart's placeholder tile, the chart stays a member.
- When it's restored, it comes back in the container for newly added charts rather than its old slot.
- To remove it for good, restore it and then remove it.
There was a problem hiding this comment.
Agreed, settled. @fitzee withdrawing his earlier point at e30773a1 and @rebenitez1802 approving is the three of us on option 1, and it is the reading UPDATING.md already describes, so no doc change needed either. Thanks for writing the restore behaviour out, that is the part a user would otherwise be surprised by: the chart comes back in the container for newly added charts rather than the slot it used to hold.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
@mikebridge LGTM. Nice pickup from a comment on an unrelated PR, and thanks for the credit.
I checked out e30773a109 and ran it: the merge base answers the PUT with a 422 and IntegrityError: UNIQUE constraint failed: dashboard_slices.dashboard_id, dashboard_slices.slice_id, this branch answers 200, and I rebuilt @rebenitez1802's sever repro independently and it is closed (numbers on his thread). One non-blocking nit inline.
CI is green on e30773a109: 58 checks passing, 14 skipped, none failing or pending, test-postgres, test-mysql, test-sqlite and unit-tests included. @rebenitez1802's change request still needs his own dismissal, so my approval alone does not unblock the merge.
| kept_archived: list[Slice] = [ | ||
| chart | ||
| for chart in dashboard.slices | ||
| if chart.deleted_at is not None and chart.id not in layout_ids |
There was a problem hiding this comment.
Just a small NIT, not a blocker: nothing pins the chart.id not in layout_ids clause. I dropped it and all 66 tests in tests/unit_tests/dao/dashboard_test.py still pass, because every assertion reads dashboard_slices rows or a set of ids, while the symptom is in the list: dashboard.slices comes back as [live, trashed, trashed] from a save where the archived chart does have a layout slot. One line in test_set_dash_metadata_preserves_soft_deleted_members catches it:
member_ids = [chart.id for chart in dashboard.slices]
assert member_ids == [live_chart.id, trashed_chart.id]That passes as is and gives assert [1, 2, 2] == [1, 2] with the clause removed.
There was a problem hiding this comment.
AI-assisted reply on Mike's behalf.
Added, thanks. test_set_dash_metadata_preserves_soft_deleted_members now also checks the member list, not just the set of ids. With the chart.id not in layout_ids clause removed it fails as [1, 2, 2] == [1, 2], and it passes with the clause in place.
There was a problem hiding this comment.
That does it, thanks. test_set_dash_metadata_preserves_soft_deleted_members now asserts the ordered member list at line 137 rather than only the set, which is the part the mutant slipped through, and the clause is pinned.
fitzee
left a comment
fitzee
left a comment
There was a problem hiding this comment.
Re-reviewed at e30773a1: Still LGTM. Rafael's blocker is fixed, the delta adds no access path, and one point from my earlier review was wrong and is withdrawn below.
Rafael's blocker: fixed
superset/daos/dashboard.py:698-705now keeps every stored archived member that the submitted layout doesn't carry (kept_archived). It reads only from the baseline that_install_membership_baselineinstalled, which is this dashboard's owndashboard_slicesrows.- The new
test_set_dash_metadata_preserves_archived_member_missing_from_layout(tests/unit_tests/dao/dashboard_test.py:142) covers both shapes:[False]: the archived chart sits only indashboard_slices. This is Rafael's "added from Explore, then archived" repro.[True]: the stored layout had the archived slot and the submitted layout drops it.
- Both cases fail with the line reverted to
dashboard.slices = current_slices, and both pass on the head. Omitted live charts are still detached (theremovedchart in the same test).
Prior items
- Withdrawn: "removing an archived tile detaches the chart is the right semantics". UPDATING.md's soft-delete notes say "saving a dashboard does not sever a trashed member". The head follows that, and so did the merge base, so I agree with @gabotorresruiz and @rebenitez1802 that this branch is right.
- Visible consequence: if a user deletes an archived chart's placeholder tile, the chart stays a member. When it's restored, hydrate places it back on the dashboard in the container for newly added charts (
superset-frontend/src/dashboard/actions/hydrate.ts:160), not in its old slot. - To drop it for good, remove it after restoring.
- Visible consequence: if a user deletes an archived chart's placeholder tile, the chart stays a member. When it's restored, hydrate places it back on the dashboard in the container for newly added charts (
- Nit 1, still open. The
state.persistentguard (dashboard.py:340) is still unpinned. Deleting it leaves all 210 tests intests/unit_tests/dao/dashboard_test.pyandtests/unit_tests/commands/dashboardpassing. It's harmless and can be dropped. - Nit 2, closed. Removing the per-dashboard filter (
dashboard.py:345) now fails 5 tests, includingtest_copy_dashboard_does_not_double_remap_overlapping_ids(dashboard_test.py:1682). - Nit 3, still open. After the merge with #45538 there are still two reads of the same
dashboard_slicesrows in a row (_member_chart_idsand the baseline). This is cosmetic.
New findings
- No blockers.
- Agree with @gabotorresruiz's nit. Nothing pins
chart.id not in layout_idsatdashboard.py:703. Dropping it still passes all 210 tests. His one-line list assertion intest_set_dash_metadata_preserves_soft_deleted_memberswould catch it. - No access bypass.
kept_archivedcan only hold charts that are already this dashboard's members. Chart IDs referenced from the layout are still resolved only throughcurrent_slices. The bypass also does not reach other callers:- Create: the baseline holds only rows the create just wrote.
- Copy with charts: the copy's members are live clones, and archived originals are excluded upstream.
- Plain copy with a layout: the collection is never populated.
- Trial merge with #45538 (
ebb6ca1c). Only textual conflicts, indashboard.pyand theapi_tests.pyimports.- Resolved by running
_raise_for_unreadable_new_charts(current_slices, ...)before the baseline install and thekept_archivedmerge. This PR doesn't change the set of IDs that check sees, so there is no semantic conflict. - 772 passed on the merged tree across
tests/unit_tests/dao,commands/dashboard,commands/chartanddashboards. - A layout that references another user's archived non-member chart still 422s through #45538's check.
- Resolved by running
- CI is green on
e30773a1: 58 passed, 14 skipped, includingtest-sqlite,test-postgres,test-mysqlandunit-tests. That covers the integration testtest_update_dashboard_keeps_archived_chart_slot, including restoring the chart and getting it back in its slot.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. e30773a fixes the blocker: an archived member missing from the layout keeps its link, live charts left out of the layout are still detached, and the new test fails on c927483 and passes here. LGTM. I left one non-blocking suggestion inline for the SOFT_DELETE-off case.
| from sqlalchemy.orm.attributes import set_committed_value | ||
| from sqlalchemy.orm.state import InstanceState | ||
|
|
||
| from superset import security_manager |
There was a problem hiding this comment.
Import needed by the SOFT_DELETE suggestion on kept_archived below.
| from superset import security_manager | |
| from superset import is_feature_enabled, security_manager |
| # Preserve archived membership even when no layout slot represents it. | ||
| layout_ids: set[int] = {chart.id for chart in current_slices} | ||
| kept_archived: list[Slice] = [ | ||
| chart | ||
| for chart in dashboard.slices | ||
| if chart.deleted_at is not None and chart.id not in layout_ids | ||
| ] |
There was a problem hiding this comment.
🟡 Non-blocking: with SOFT_DELETE off, users can't remove a previously archived chart
kept_archived only checks deleted_at is not None. UPDATING.md documents turning SOFT_DELETE off as the emergency rollback, and doing so makes previously archived charts show up as live again while their deleted_at stays set. If a user then deletes such a chart from the layout and saves, its dashboard_slices row is kept, and on the next load hydrate.ts adds it back at the bottom, so the chart can't be removed.
Repro (flag off, chart 1 live, chart 2 with deleted_at set and dropped from the submitted layout): members after save are [1, 2] on this branch and [1] on master and on c927483ebf.
Suggested fix: only retain archived members while the flag is on (it needs the is_feature_enabled import suggested on line 34). With both suggestions applied, the flag-off repro gives [1], the flag-on cases still keep archived members (your new test passes), and tests/unit_tests/dao, commands/dashboard, soft_delete, versioning and mcp_service/dashboard pass. A flag-off case in test_set_dash_metadata_preserves_archived_member_missing_from_layout would pin it.
| # Preserve archived membership even when no layout slot represents it. | |
| layout_ids: set[int] = {chart.id for chart in current_slices} | |
| kept_archived: list[Slice] = [ | |
| chart | |
| for chart in dashboard.slices | |
| if chart.deleted_at is not None and chart.id not in layout_ids | |
| ] | |
| # Preserve archived membership even when no layout slot represents it. | |
| # With SOFT_DELETE off, archived charts render as live, so leaving | |
| # one out of the layout is a deliberate removal and must detach it. | |
| layout_ids: set[int] = {chart.id for chart in current_slices} | |
| kept_archived: list[Slice] = ( | |
| [ | |
| chart | |
| for chart in dashboard.slices | |
| if chart.deleted_at is not None and chart.id not in layout_ids | |
| ] | |
| if is_feature_enabled("SOFT_DELETE") | |
| else [] | |
| ) |
|
AI-assisted reply on Mike's behalf. Thanks for the re-review. Gabriel's assertion is in. I've left the unpinned |
SUMMARY
Saving a dashboard that has an archived chart on it fails, and this fixes it.
What users saw. After a chart on a dashboard was archived, every later save of that dashboard failed with "Dashboard could not be updated." (a 422), even when nothing about the archived chart changed. Opening the dashboard and saving it straight back was enough to trigger it, so archiving any chart on a dashboard blocked further edits to it.
Why. Saving rebuilds the dashboard's chart list from the layout and compares it with the stored one. The layout still holds the archived chart, as intended (it comes back when the chart is restored), but the stored chart list was loaded with archived charts filtered out. To the save, the archived chart looked newly added, so it tried to insert a
dashboard_slicesrow that already exists and failed on the primary key. The existing unit test didn't catch this because its dashboard was built in memory, not loaded through a query, so the filter never applied.@gabotorresruiz found and diagnosed this, with an exact repro, in #45482 (comment). Thanks!
What this changes.
DashboardDAO.set_dash_metadatareads the stored chart list, archived charts included, directly fromdashboard_slices, and compares the new list against that. The save then keeps the archived chart's slot and link, so restoring the chart puts it back on the dashboard. If the chart list has unsaved changes that can't be flushed first, the save leaves it alone rather than discard them, and logs a warning.This touches the same function as #45538 (server-side validation of layout chart references), a few lines apart. Whichever merges second needs a small textual merge; there is no behavioural overlap.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (API behaviour only).
TESTING INSTRUCTIONS
pytest tests/integration_tests/dashboards/api_tests.py -k keeps_archived_chart_slotpytest tests/unit_tests/dao/dashboard_test.py -k "soft_deleted_members or install_membership_baseline"GET /api/v1/dashboard/<id>andPUTthe same layout back. Before this change it returns 422. After, it returns 200, and restoring the archived chart puts it back in its slot.ADDITIONAL INFORMATION