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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
