rebenitez1802 commented on code in PR #45543:
URL: https://github.com/apache/superset/pull/45543#discussion_r4361273326
##########
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:
🟡 **Non-blocking: with `SOFT_DELETE` off, users can't remove a previously
archived chart**
`kept_archived` only checks `deleted_at is not None`. UPDATING.md documents
turning `SOFT_DELETE` off as the emergency rollback, and doing so makes
previously archived charts show up as live again while their `deleted_at` stays
set. If a user then deletes such a chart from the layout and saves, its
`dashboard_slices` row is kept, and on the next load `hydrate.ts` adds it back
at the bottom, so the chart can't be removed.
Repro (flag off, chart 1 live, chart 2 with `deleted_at` set and dropped
from the submitted layout): members after save are `[1, 2]` on this branch and
`[1]` on master and on `c927483ebf`.
Suggested fix: only retain archived members while the flag is on (it needs
the `is_feature_enabled` import suggested on line 34). With both suggestions
applied, the flag-off repro gives `[1]`, the flag-on cases still keep archived
members (your new test passes), and `tests/unit_tests/dao`,
`commands/dashboard`, `soft_delete`, `versioning` and `mcp_service/dashboard`
pass. A flag-off case in
`test_set_dash_metadata_preserves_archived_member_missing_from_layout` would
pin it.
```suggestion
# Preserve archived membership even when no layout slot
represents it.
# With SOFT_DELETE off, archived charts render as live, so
leaving
# one out of the layout is a deliberate removal and must
detach 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
]
if is_feature_enabled("SOFT_DELETE")
else []
)
```
##########
superset/daos/dashboard.py:
##########
@@ -25,9 +25,11 @@
from flask import g
from flask_appbuilder.models.sqla.interface import SQLAInterface
from marshmallow import ValidationError
-from sqlalchemy import or_, select
+from sqlalchemy import inspect as sa_inspect, or_, select
from sqlalchemy.engine import Row
from sqlalchemy.orm import lazyload, Query, selectinload
+from sqlalchemy.orm.attributes import set_committed_value
+from sqlalchemy.orm.state import InstanceState
from superset import security_manager
Review Comment:
Import needed by the `SOFT_DELETE` suggestion on `kept_archived` below.
```suggestion
from superset import is_feature_enabled, security_manager
```
--
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]
