Repository navigation
fix(dashboard): keep archived charts' membership when restoring a version - #45482
aminghadersohi merged 7 commits into
Conversation
…sion Restoring a dashboard version sets its charts from that version. Archived (soft-deleted) charts were left out of that update: the lookup of the version's charts hid them, and so did the dashboard's current chart list the update compares against. - A version that had an archived chart did not link it again, so restoring the chart later did not bring it back to the dashboard, and the restore reported the chart as no longer existing. - A version without an archived chart kept it linked, so restoring the chart put it back on a dashboard whose layout had no slot for it. Read both chart sets with archived charts visible. Take the current set from the association table: the dashboard's own load hides archived charts from its chart list, even inside the visibility-filter bypass.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #45482 +/- ##
===========================================
+ Coverage 56.19% 83.66% +27.46%
===========================================
Files 3053 3054 +1
Lines 200807 200791 -16
Branches 46867 46858 -9
===========================================
+ Hits 112842 167983 +55141
+ Misses 86880 29559 -57321
- Partials 1085 3249 +2164
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:
|
…rchived-membership
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| live_slices: list[Slice] = ( | ||
| db.session.query(Slice).filter(Slice.id.in_(member_ids)).all() | ||
| if member_ids | ||
| else [] | ||
| ) |
There was a problem hiding this comment.
Suggestion: A concurrent purge can delete a matched Slice after this unlocked read; reattaching it then violates the association foreign key and aborts the dashboard restore.
Assessment: 🟠 Major · 🔁 Occurrence: Rarely · 🏷️ Race condition
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/versioning/restore.py
**Line:** 764:768
**Comment:**
*Race Condition: A concurrent purge can delete a matched `Slice` after this unlocked read; reattaching it then violates the association foreign key and aborts the dashboard restore.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
This race predates the PR (live members were read the same way), and if it happens the restore fails and rolls back cleanly, so a retry works. Not changing it here.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Do not flag the unlocked live-member read in restore.py as a race-condition issue; a failed restore rolls back cleanly and can be retried.
Applied to:
superset/versioning/restore.py
💡 To manage or update this instruction, visit: CodeAnt AI Settings
CodeAnt PR Risk: Low Risk
Assessed commit: |
fitzee
left a comment
fitzee
left a comment
There was a problem hiding this comment.
LGTM with nits. No blocking issues.
The root cause is correct: the Dashboard load carries the Slice soft-delete criteria into the dashboard.slices lazy load even inside skip_visibility_filter, so the old diff could neither add nor remove archived members. Reading the baseline from dashboard_slices under the bypass fixes both directions. Hard-deleted charts are still skipped (their rows are gone via the ON DELETE CASCADE FK, so nothing dangles). The composite PK on dashboard_slices is what makes the baseline necessary.
Verified on SQLite locally:
version_restore_tests.py: 12/12 pass on the PR head.- With
restore.pyreverted to master, all three new cases fail (the "no longer exist" message, and stale membership for the not-in-version case). - With only the
set_committed_valueline removed, cases 1 and 3 fail withUNIQUE constraint failed: dashboard_slices.dashboard_id, dashboard_slices.slice_id. The tests do guard the baseline. - The restore writes no
slices_versionrow for the archived chart in any of the three cases.
CI is green at 778fab1 (test-sqlite, test-postgres, test-mysql, unit-tests all pass).
The purge race CodeAnt raised is not repeated here. Inline: one non-blocking hardening suggestion, two nits.
| # Installing the baseline drops any unflushed change to the collection. | ||
| # There is none here: the dashboard revert restores no relationships, and | ||
| # single_flush_scope holds back autoflush until the restore is done. | ||
| set_committed_value(dashboard, "slices", current_slices) |
There was a problem hiding this comment.
Non-blocking. set_committed_value silently throws away any pending change to dashboard.slices. Correctness here depends on two things nothing enforces: _RESTORE_RELATIONS["Dashboard"] staying [], and nothing between the command's lock and this point touching the collection. The model comment at superset/models/dashboard.py:185-189 still describes Reverter(relations=["slices"]) as the intended mechanism, so adding slices to the list is a plausible future change. If that happens, this line discards the reverter's work without any error.
A cheap guard would turn that into a loud failure:
if sa.inspect(dashboard).attrs.slices.history.has_changes():
raise RuntimeError("dashboard.slices has unflushed changes; baseline would drop them")There was a problem hiding this comment.
Added the guard as suggested, with a test that a pending change to dashboard.slices makes the restore refuse instead of dropping it, in 1f54dce.
| live_slices: list[Slice] = ( | ||
| db.session.query(Slice).filter(Slice.id.in_(member_ids)).all() | ||
| if member_ids | ||
| else [] |
There was a problem hiding this comment.
Nit (tests). The empty-snapshot path used to early-return with dashboard.slices = []. That had the same bug: it left an archived member's row in place. It now goes through the baseline, but none of the three new cases cover it, because original_ids >= 2 keeps the version's membership non-empty. A case where the version has zero charts and the only current member is archived would pin it.
There was a problem hiding this comment.
Added a case where the version has no charts and the only member is archived, in 1f54dce. It fails on master's restore and passes here.
| have since been permanently deleted, the layout is restored without them and | ||
| Superset tells you how many were not reattached. Archived charts are not | ||
| skipped: they are linked to the dashboard as in the version, so restoring | ||
| one from the archive later puts it back on that dashboard. |
There was a problem hiding this comment.
Nit. This only describes the re-link direction. The more surprising user-visible change is the other one: an archived chart that wasn't in the version is now unlinked, so restoring it from the archive later won't put it back on this dashboard. Worth one sentence.
There was a problem hiding this comment.
Added a sentence on the unlink direction in 1f54dce.
…rchived-membership
…changes Guard the dashboard_slices baseline so a future relationship revert fails loudly instead of being silently discarded, pin the empty-version restore of an archived member, and document that a restore unlinks archived charts the version did not have. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rchived-membership # Conflicts: # tests/integration_tests/dashboards/version_restore_tests.py
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks @mikebridge, good catch. I walked the four combinations (chart archived or live, in the version or not) through the full lifecycle on both SQLite and Postgres: archive the chart, restore the version, then restore the chart from the archive. Membership and layout agree on every one of them here, and disagree in both directions on master. Approving.
Two non-blocking notes inline, one of them about a sibling path rather than this diff. CI is green on the head.
| # Archived charts must be visible both to the lookup and to the | ||
| # assignment, which diffs against the current membership. A membership | ||
| # loaded through ``dashboard.slices`` misses archived members, even inside | ||
| # the bypass, because the dashboard's own load carried the filter, so the | ||
| # flush would keep or re-insert their rows. Read it from the association | ||
| # table instead and diff against that. |
There was a problem hiding this comment.
Not a blocker and not this PR's job, but this comment is load-bearing somewhere else too.
DashboardDAO.set_dash_metadata (superset/daos/dashboard.py:659) relies on exactly the baseline load you are saying here cannot be trusted: it assigns inside the bypass and lets the unit of work lazy-load dashboard.slices as the old collection. The comment above it says that is what prevents "an IntegrityError on the composite primary key on every save of a dashboard containing a trashed chart", and tests/unit_tests/dao/dashboard_test.py::test_set_dash_metadata_preserves_soft_deleted_members is meant to pin it.
On master that save is broken. Archive a chart that sits on a dashboard, GET /api/v1/dashboard/<id>, then PUT the same layout straight back, and it returns 422 with duplicate key value violates unique constraint "pk_dashboard_slices". With nothing archived the same round trip returns 200. That unit test passes because its Dashboard is constructed in Python and never loaded through a query, so no with_loader_criteria ever propagates into its lazy baseline. A query-loaded dashboard behaves exactly the way you describe here.
Swapping that baseline for the association-table read plus set_committed_value you use here turns the 422 into a 200 and leaves dashboard_test.py green. Happy to open it as a follow-up if you would rather keep this PR focused, or it is yours if you want it while the mechanism is fresh.
There was a problem hiding this comment.
Thanks, that's a precise repro, and it explains why I couldn't reproduce it earlier: my test built the dashboard in the same session, so slices was already loaded with the archived chart in it. I'll take it with the association-table baseline as you suggested, plus an API round-trip test that loads the dashboard fresh. Separate PR to follow; I'll keep this one focused.
There was a problem hiding this comment.
PR is up: #45543. I credited your diagnosis there.
| - **A dashboard restore may be partial.** If the version referenced charts that | ||
| have since been deleted, the layout is restored without them and Superset | ||
| tells you how many were not reattached. | ||
| have since been permanently deleted, the layout is restored without them and |
There was a problem hiding this comment.
Just a small NIT on the sentence you rewrapped: I do not think the layout comes back without them. The revert restores position_json verbatim from the snapshot and nothing on this path reconciles it (reconcile_position_json runs on the PUT path), so a permanently deleted chart's CHART-<id> node is still in the layout afterwards and GET /api/v1/dashboard/<id> serves it. Only the membership omits it.
- **A dashboard restore may be partial.** If the version referenced charts that
have since been permanently deleted, they are not reattached and Superset
tells you how many. Their layout slots come back as the version had them.
Or am I reading "without them" more literally than you meant it?
There was a problem hiding this comment.
You're right, I was reading my own sentence too loosely: the snapshot's position_json comes back verbatim, and only membership leaves the deleted charts out. I took your wording in 6ef3eff.
…rchived-membership
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SUMMARY
TL;DR: Restoring a dashboard version now gets archived charts' membership right. Before this fix, a restored version that included an archived chart left it off the dashboard. A restored version that didn't include one left it on.
What users saw. Say a dashboard had a chart that was later archived (soft-deleted), and someone used "Restore this version" in version history.
Live charts and hard-deleted charts were not affected.
Why, in plain terms. An archived chart is hidden from normal lookups but still belongs to its dashboards, so restoring it from the archive puts it back where it was. The version restore set the dashboard's charts using normal lookups. It couldn't see archived charts, either in the version it was restoring or in the dashboard's current chart list, so it never added or removed them.
What this changes. The restore reads both chart lists with archived charts visible. An archived chart's membership now follows the restored version, and the restore no longer reports archived charts as missing. Permanently deleted charts are still skipped and reported. The version-history docs now say this.
Details.
_restore_dashboard_membershipnow does the following:skip_visibility_filter(db.session, Slice).dashboard_slicesunder the same bypass.The current set can't come from
dashboard.slices. The soft-delete loader criteria the dashboard was loaded with also apply to its relationship loads, even inside the bypass, so a lazy load or refresh still leaves archived members out. The flush would then keep a row it should delete, or insert one that already exists.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (API behavior).
Before:
POST /api/v1/dashboard/<uuid>/versions/<version_uuid>/restorereturns 200. An archived chart in the version is not linked, the message says "1 chart(s) referenced by the snapshot no longer exist and were not reattached", and an archived chart not in the version stays linked. After: the archived chart's link matches the restored version, and the message is "OK". Restoring the chart afterwards brings it back only where the version had it.TESTING INSTRUCTIONS
pytest tests/integration_tests/dashboards/version_restore_tests.py -k archived_chart_membership, on PostgreSQL and SQLite.ADDITIONAL INFORMATION
SOFT_DELETE(on by default) andVERSION_HISTORY