rebenitez1802 commented on code in PR #45543:
URL: https://github.com/apache/superset/pull/45543#discussion_r4353268916
##########
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:
🔴 **Blocker: saving the dashboard severs an archived chart that isn't in the
layout**
With the baseline now including archived members, any archived chart missing
from `positions` gets its `dashboard_slices` row deleted on save. One way to
get there: a chart is saved from Explore with "add to dashboard"
(`commands/chart/create.py` adds the junction row but leaves `position_json`
alone), then archived before the dashboard is next saved. The charts payload
omits archived charts, so hydrate never adds it to the layout, and the next
save drops the link. Restoring the chart no longer re-attaches it, which breaks
the UPDATING.md contract: "saving a dashboard does not sever a trashed member".
Repro (unit-level, dashboard loaded through a query, one live chart in the
layout plus one archived chart that is only in `dashboard_slices`): members
after save are `{1, 2}` on the merge base and `{1}` on this branch.
Suggested fix: keep archived members that the layout doesn't carry. With
this applied, the repro passes, as do `tests/unit_tests/dao/dashboard_test.py`
and `tests/unit_tests/commands/dashboard` (209 passed). A regression test for
the "archived member not in the layout" case would be good too.
```suggestion
# Never sever an archived member the layout does not carry.
layout_ids = {slc.id for slc in current_slices}
kept_archived = [
slc
for slc in dashboard.slices
if slc.deleted_at is not None and slc.id not in
layout_ids
]
dashboard.slices = current_slices + kept_archived
```
Side effect: removing an archived chart's placeholder from the layout would
also keep its link again (the merge-base behaviour, and what UPDATING.md
describes). If you'd rather keep severing in that case, an alternative is to
keep only archived members that were also absent from the previously stored
`position_json`.
--
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]
