You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mikebridge (via GitHub)" <gi...@apache.org> on 2026/10/19 19:36:50 UTC
[PR] fix(dashboard): keep archived charts' membership when restoring a version [superset]
mikebridge opened a new pull request, #45482:
URL: https://github.com/apache/superset/pull/45482
### 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.
- **The version had the chart:** the layout came back with the chart's slot, but the dashboard no longer counted the chart as one of its charts, and the restore reported it as a chart that "no longer exist[s]". Restoring the chart from the archive later didn't bring it back onto the dashboard.
- **The version didn't have the chart:** the dashboard kept the chart. Restoring the chart later put it back on a dashboard whose layout had no slot for it.
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_membership` now does the following:
- It resolves the version's charts inside `skip_visibility_filter(db.session, Slice)`.
- It takes the dashboard's current charts from `dashboard_slices` under the same bypass.
- It installs those current charts as the collection's committed value before assigning.
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>/restore` returns 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.
- All three cases fail without the fix. The third case is a chart archived while it is a member and present in the version. Without the fix it fails on the "no longer exist" message, and it also guards against inserting that chart's existing row again.
- Each case checks the live membership, the chart's newest membership-history row and the restore message. It then restores the chart from the archive.
- Manually, with soft delete and version history on:
1. Add a chart to a dashboard and save.
2. Remove the chart and save again.
3. Archive the chart.
4. Restore the version saved in step 1.
5. Restore the chart from the archive. It is back on the dashboard.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [x] Required feature flags: `SOFT_DELETE` (on by default) and `VERSION_HISTORY`
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): keep archived charts' membership when restoring a version [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #45482:
URL: https://github.com/apache/superset/pull/45482#issuecomment-6275682316
### <span aria-hidden="true">โ
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">๐จ</span> Latest commit | 45634d9b8bca06274e939da587591abb8dbb3d51 |
|<span aria-hidden="true">๐</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ad67153582fbe436a0229c5 |
|<span aria-hidden="true">๐</span> Deploy Preview | [https://deploy-preview-45482--superset-docs-preview.netlify.app](https://deploy-preview-45482--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">๐ฑ</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">๐ค</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctLCoAgEADQu8w6Iac06wYdw8-EQaaMEUR099y9zXuhcE7lggWgA8f29LG5eiHRYK8EU70yk7Ds435TEImSI65xLy0EKkd-1tCKtkFPUg3K4OZoHLTtEWev4PsB) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-45482--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): keep archived charts' membership when restoring a version [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #45482:
URL: https://github.com/apache/superset/pull/45482#discussion_r4327816575
##########
superset/versioning/restore.py:
##########
@@ -742,11 +748,29 @@ def _restore_dashboard_membership(dashboard: Any, transaction_id: int) -> list[i
if window.contains(transaction_id)
}
)
- if not member_ids:
- dashboard.slices = []
- return []
-
- live_slices = db.session.query(Slice).filter(Slice.id.in_(member_ids)).all()
+ # 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.
+ with skip_visibility_filter(db.session, Slice):
+ current_slices: list[Slice] = (
+ db.session.query(Slice)
+ .join(dashboard_slices, dashboard_slices.c.slice_id == Slice.id)
+ .filter(dashboard_slices.c.dashboard_id == dashboard.id)
+ .all()
+ )
+ live_slices: list[Slice] = (
+ db.session.query(Slice).filter(Slice.id.in_(member_ids)).all()
+ if member_ids
+ else []
+ )
+ # 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)
Review 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 1f54dced2b.
##########
superset/versioning/restore.py:
##########
@@ -742,11 +748,29 @@ def _restore_dashboard_membership(dashboard: Any, transaction_id: int) -> list[i
if window.contains(transaction_id)
}
)
- if not member_ids:
- dashboard.slices = []
- return []
-
- live_slices = db.session.query(Slice).filter(Slice.id.in_(member_ids)).all()
+ # 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.
+ with skip_visibility_filter(db.session, Slice):
+ current_slices: list[Slice] = (
+ db.session.query(Slice)
+ .join(dashboard_slices, dashboard_slices.c.slice_id == Slice.id)
+ .filter(dashboard_slices.c.dashboard_id == dashboard.id)
+ .all()
+ )
+ live_slices: list[Slice] = (
+ db.session.query(Slice).filter(Slice.id.in_(member_ids)).all()
+ if member_ids
+ else []
Review Comment:
Added a case where the version has no charts and the only member is archived, in 1f54dced2b. It fails on master's restore and passes here.
##########
docs/docs/using-superset/version-history.mdx:
##########
@@ -282,8 +282,10 @@ worth knowing:
you moved away from. Returning to one of those states depends on its history
still being available; retention and permanent deletion can remove it.
- **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
+ 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.
Review Comment:
Added a sentence on the unlink direction in 1f54dced2b.
##########
superset/versioning/restore.py:
##########
@@ -742,11 +748,29 @@ def _restore_dashboard_membership(dashboard: Any, transaction_id: int) -> list[i
if window.contains(transaction_id)
}
)
- if not member_ids:
- dashboard.slices = []
- return []
-
- live_slices = db.session.query(Slice).filter(Slice.id.in_(member_ids)).all()
+ # 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.
+ with skip_visibility_filter(db.session, Slice):
+ current_slices: list[Slice] = (
+ db.session.query(Slice)
+ .join(dashboard_slices, dashboard_slices.c.slice_id == Slice.id)
+ .filter(dashboard_slices.c.dashboard_id == dashboard.id)
+ .all()
+ )
+ live_slices: list[Slice] = (
+ db.session.query(Slice).filter(Slice.id.in_(member_ids)).all()
+ if member_ids
+ else []
+ )
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): keep archived charts' membership when restoring a version [superset]
Posted by "fitzee (via GitHub)" <gi...@apache.org>.
fitzee commented on code in PR #45482:
URL: https://github.com/apache/superset/pull/45482#discussion_r4321921192
##########
superset/versioning/restore.py:
##########
@@ -742,11 +748,29 @@ def _restore_dashboard_membership(dashboard: Any, transaction_id: int) -> list[i
if window.contains(transaction_id)
}
)
- if not member_ids:
- dashboard.slices = []
- return []
-
- live_slices = db.session.query(Slice).filter(Slice.id.in_(member_ids)).all()
+ # 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.
+ with skip_visibility_filter(db.session, Slice):
+ current_slices: list[Slice] = (
+ db.session.query(Slice)
+ .join(dashboard_slices, dashboard_slices.c.slice_id == Slice.id)
+ .filter(dashboard_slices.c.dashboard_id == dashboard.id)
+ .all()
+ )
+ live_slices: list[Slice] = (
+ db.session.query(Slice).filter(Slice.id.in_(member_ids)).all()
+ if member_ids
+ else []
Review 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.
##########
superset/versioning/restore.py:
##########
@@ -742,11 +748,29 @@ def _restore_dashboard_membership(dashboard: Any, transaction_id: int) -> list[i
if window.contains(transaction_id)
}
)
- if not member_ids:
- dashboard.slices = []
- return []
-
- live_slices = db.session.query(Slice).filter(Slice.id.in_(member_ids)).all()
+ # 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.
+ with skip_visibility_filter(db.session, Slice):
+ current_slices: list[Slice] = (
+ db.session.query(Slice)
+ .join(dashboard_slices, dashboard_slices.c.slice_id == Slice.id)
+ .filter(dashboard_slices.c.dashboard_id == dashboard.id)
+ .all()
+ )
+ live_slices: list[Slice] = (
+ db.session.query(Slice).filter(Slice.id.in_(member_ids)).all()
+ if member_ids
+ else []
+ )
+ # 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)
Review 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:
```python
if sa.inspect(dashboard).attrs.slices.history.has_changes():
raise RuntimeError("dashboard.slices has unflushed changes; baseline would drop them")
```
##########
docs/docs/using-superset/version-history.mdx:
##########
@@ -282,8 +282,10 @@ worth knowing:
you moved away from. Returning to one of those states depends on its history
still being available; retention and permanent deletion can remove it.
- **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
+ 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.
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): keep archived charts' membership when restoring a version [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45482:
URL: https://github.com/apache/superset/pull/45482#discussion_r4327822918
##########
superset/versioning/restore.py:
##########
@@ -742,11 +748,29 @@ def _restore_dashboard_membership(dashboard: Any, transaction_id: int) -> list[i
if window.contains(transaction_id)
}
)
- if not member_ids:
- dashboard.slices = []
- return []
-
- live_slices = db.session.query(Slice).filter(Slice.id.in_(member_ids)).all()
+ # 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.
+ with skip_visibility_filter(db.session, Slice):
+ current_slices: list[Slice] = (
+ db.session.query(Slice)
+ .join(dashboard_slices, dashboard_slices.c.slice_id == Slice.id)
+ .filter(dashboard_slices.c.dashboard_id == dashboard.id)
+ .all()
+ )
+ live_slices: list[Slice] = (
+ db.session.query(Slice).filter(Slice.id.in_(member_ids)).all()
+ if member_ids
+ else []
+ )
Review 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](https://app.codeant.ai/org/settings/learnings)*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): keep archived charts' membership when restoring a version [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #45482:
URL: https://github.com/apache/superset/pull/45482#discussion_r4329170692
##########
docs/docs/using-superset/version-history.mdx:
##########
@@ -282,8 +282,12 @@ worth knowing:
you moved away from. Returning to one of those states depends on its history
still being available; retention and permanent deletion can remove it.
- **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
Review 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 6ef3eff663.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): keep archived charts' membership when restoring a version [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi merged PR #45482:
URL: https://github.com/apache/superset/pull/45482
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): keep archived charts' membership when restoring a version [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #45482:
URL: https://github.com/apache/superset/pull/45482#discussion_r4330826663
##########
superset/versioning/restore.py:
##########
@@ -742,11 +748,35 @@ def _restore_dashboard_membership(dashboard: Any, transaction_id: int) -> list[i
if window.contains(transaction_id)
}
)
- if not member_ids:
- dashboard.slices = []
- return []
-
- live_slices = db.session.query(Slice).filter(Slice.id.in_(member_ids)).all()
+ # 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.
Review Comment:
PR is up: #45543. I credited your diagnosis there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): keep archived charts' membership when restoring a version [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #45482:
URL: https://github.com/apache/superset/pull/45482#issuecomment-6276102601
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/45482?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 64.89%. Comparing base ([`99ecfe5`](https://app.codecov.io/gh/apache/superset/commit/99ecfe5801c4cbb79eedd475003b134b6ba7ff28?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`45634d9`](https://app.codecov.io/gh/apache/superset/commit/45634d9b8bca06274e939da587591abb8dbb3d51?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (99ecfe5) and HEAD (45634d9). Click for more details.
>
> <details><summary>HEAD has 4 uploads less than BASE</summary>
>
>| Flag | BASE (99ecfe5) | HEAD (45634d9) |
>|------|------|------|
>|python|7|3|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #45482 +/- ##
===========================================
- Coverage 83.56% 64.89% -18.67%
===========================================
Files 3048 3049 +1
Lines 199836 199988 +152
Branches 46613 46649 +36
===========================================
- Hits 166984 129788 -37196
- Misses 29610 67260 +37650
+ Partials 3242 2940 -302
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/45482/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage ฮ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/45482/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.92% <12.50%> (-0.03%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/45482/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/45482/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/45482/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.68% <12.50%> (-0.03%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/45482/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.80% <100.00%> (-34.29%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/45482/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.69% <100.00%> (-0.05%)` | :arrow_down: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/45482/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/45482?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): keep archived charts' membership when restoring a version [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on code in PR #45482:
URL: https://github.com/apache/superset/pull/45482#discussion_r4328666886
##########
superset/versioning/restore.py:
##########
@@ -742,11 +748,35 @@ def _restore_dashboard_membership(dashboard: Any, transaction_id: int) -> list[i
if window.contains(transaction_id)
}
)
- if not member_ids:
- dashboard.slices = []
- return []
-
- live_slices = db.session.query(Slice).filter(Slice.id.in_(member_ids)).all()
+ # 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.
Review 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.
##########
docs/docs/using-superset/version-history.mdx:
##########
@@ -282,8 +282,12 @@ worth knowing:
you moved away from. Returning to one of those states depends on its history
still being available; retention and permanent deletion can remove it.
- **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
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): keep archived charts' membership when restoring a version [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #45482:
URL: https://github.com/apache/superset/pull/45482#discussion_r4329424753
##########
superset/versioning/restore.py:
##########
@@ -742,11 +748,35 @@ def _restore_dashboard_membership(dashboard: Any, transaction_id: int) -> list[i
if window.contains(transaction_id)
}
)
- if not member_ids:
- dashboard.slices = []
- return []
-
- live_slices = db.session.query(Slice).filter(Slice.id.in_(member_ids)).all()
+ # 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.
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): keep archived charts' membership when restoring a version [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45482:
URL: https://github.com/apache/superset/pull/45482#discussion_r4320985456
##########
superset/versioning/restore.py:
##########
@@ -742,11 +748,29 @@ def _restore_dashboard_membership(dashboard: Any, transaction_id: int) -> list[i
if window.contains(transaction_id)
}
)
- if not member_ids:
- dashboard.slices = []
- return []
-
- live_slices = db.session.query(Slice).filter(Slice.id.in_(member_ids)).all()
+ # 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.
+ with skip_visibility_filter(db.session, Slice):
+ current_slices: list[Slice] = (
+ db.session.query(Slice)
+ .join(dashboard_slices, dashboard_slices.c.slice_id == Slice.id)
+ .filter(dashboard_slices.c.dashboard_id == dashboard.id)
+ .all()
+ )
+ live_slices: list[Slice] = (
+ db.session.query(Slice).filter(Slice.id.in_(member_ids)).all()
+ if member_ids
+ else []
+ )
Review 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`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7014574179ed47799cd29208b0df4764&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7014574179ed47799cd29208b0df4764&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
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 fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45482&comment_hash=a5b6e384bcc8e61c8b9be64e24fcf515543bbe064e2c07de9db677b8a2fff5fa&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45482&comment_hash=a5b6e384bcc8e61c8b9be64e24fcf515543bbe064e2c07de9db677b8a2fff5fa&reaction=dislike'>๐</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): keep archived charts' membership when restoring a version [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #45482:
URL: https://github.com/apache/superset/pull/45482#issuecomment-6282208821
## CodeAnt PR Risk: Medium Risk
- The PR needs attention before merging because dashboard restoration can race with a concurrent chart purge.
- The unlocked chart lookup can become stale before membership is flushed, causing the restore to fail if the chart is purged in between.
**Assessed commit:** `778fab16b609`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org