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: [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]
