mikebridge commented on code in PR #45543:
URL: https://github.com/apache/superset/pull/45543#discussion_r4363735857
##########
superset/daos/dashboard.py:
##########
@@ -646,21 +678,31 @@ def set_dash_metadata( # noqa: C901
# documented restore-reattach contract) and writing
# ``uuid: None`` into its position slot via ``uuid_map`` below.
#
- # The bypass must be session-scoped and cover the ASSIGNMENT,
- # not just the resolution query: assigning to
+ # The bypass must also cover the ASSIGNMENT: assigning to
# ``dashboard.slices`` makes the unit of work diff the new
- # collection against the existing one, which it lazy-loads at
- # that moment. A filtered baseline load would exclude the
- # trashed member, so the diff would treat it as net-new and
- # INSERT a ``dashboard_slices`` row that still exists (soft
- # delete never removes junction rows) — an IntegrityError on
- # the composite primary key on every save of a dashboard
- # containing a trashed chart.
+ # collection against the existing one. A baseline missing the
+ # trashed member makes the diff treat it as net-new and INSERT a
+ # ``dashboard_slices`` row that still exists (soft delete never
+ # removes junction rows), an IntegrityError on the composite
+ # primary key on every save of a dashboard containing a trashed
+ # chart. Lazy-loading the baseline inside the bypass is not
+ # enough: a dashboard loaded through a query carries the
+ # visibility filter into its own lazy loads. Read the baseline
+ # from the association table instead and install it as the
+ # committed state.
with skip_visibility_filter(db.session, Slice):
current_slices = (
db.session.query(Slice).filter(Slice.id.in_(slice_ids)).all()
)
- dashboard.slices = current_slices
+ _install_membership_baseline(dashboard)
+ # 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
Review 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.
##########
superset/daos/dashboard.py:
##########
@@ -646,20 +678,23 @@ def set_dash_metadata( # noqa: C901
# documented restore-reattach contract) and writing
# ``uuid: None`` into its position slot via ``uuid_map`` below.
#
- # The bypass must be session-scoped and cover the ASSIGNMENT,
- # not just the resolution query: assigning to
+ # The bypass must also cover the ASSIGNMENT: assigning to
# ``dashboard.slices`` makes the unit of work diff the new
- # collection against the existing one, which it lazy-loads at
- # that moment. A filtered baseline load would exclude the
- # trashed member, so the diff would treat it as net-new and
- # INSERT a ``dashboard_slices`` row that still exists (soft
- # delete never removes junction rows) — an IntegrityError on
- # the composite primary key on every save of a dashboard
- # containing a trashed chart.
+ # collection against the existing one. A baseline missing the
+ # trashed member makes the diff treat it as net-new and INSERT a
+ # ``dashboard_slices`` row that still exists (soft delete never
+ # removes junction rows), an IntegrityError on the composite
+ # primary key on every save of a dashboard containing a trashed
+ # chart. Lazy-loading the baseline inside the bypass is not
+ # enough: a dashboard loaded through a query carries the
+ # visibility filter into its own lazy loads. Read the baseline
+ # from the association table instead and install it as the
+ # committed state.
with skip_visibility_filter(db.session, Slice):
current_slices = (
db.session.query(Slice).filter(Slice.id.in_(slice_ids)).all()
)
+ _install_membership_baseline(dashboard)
dashboard.slices = current_slices
Review 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.
--
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]
