mikebridge commented on code in PR #45545:
URL: https://github.com/apache/superset/pull/45545#discussion_r4351254449
##########
superset/commands/semantic_layer/delete.py:
##########
@@ -116,47 +119,54 @@ def _dashboard_target_ids(view_ids: sa.Select) ->
set[int]:
def _dependent_assets(
view_ids: sa.Select,
-) -> tuple[int, list[dict[str, str | int]], int]:
+) -> tuple[int, list[dict[str, str | int]], int, int]:
"""Count dependents, naming only assets visible through their list APIs."""
- chart_ids: sa.Select = sa.select(Slice.id).where(
- Slice.datasource_type == "semantic_view",
- Slice.datasource_id.in_(view_ids),
- Slice.deleted_at.is_(None),
+ # Core table selects bypass the ORM do_orm_execute soft-delete filter:
+ # archived charts and dashboards must still block semantic-source deletion.
+ chart_ids: sa.Select = sa.select(Slice.__table__.c.id).where(
+ Slice.__table__.c.datasource_type == "semantic_view",
+ Slice.__table__.c.datasource_id.in_(view_ids),
)
chart_dashboard_ids: sa.Select = (
sa.select(dashboard_slices.c.dashboard_id)
- .join(Slice, dashboard_slices.c.slice_id == Slice.id)
- .join(Dashboard, dashboard_slices.c.dashboard_id == Dashboard.id)
+ .join(Slice.__table__, dashboard_slices.c.slice_id ==
Slice.__table__.c.id)
+ .join(
+ Dashboard.__table__,
+ dashboard_slices.c.dashboard_id == Dashboard.__table__.c.id,
+ )
.where(
- Slice.id.in_(chart_ids),
- Dashboard.deleted_at.is_(None),
+ Slice.__table__.c.id.in_(chart_ids),
)
.distinct()
)
dashboard_target_ids: set[int] = _dashboard_target_ids(view_ids)
Review Comment:
Good catch: this is a real race in the existing best-effort guard. The
target scan at `delete.py:99-117` is materialized at line 142, so under READ
COMMITTED a dashboard target committed afterwards is missing from the count at
line 229. I reproduced a dangling target with SQLite's legacy transaction mode;
an explicit WAL transaction rejects the delete instead. The dashboard survives,
but its filter's datasource lookup returns 404.
Repeating the count would only narrow the window. A complete fix needs
coordination with the target writers, and the same limitation predates this PR,
so I'm leaving it for a focused concurrency follow-up rather than widening this
change.
--
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]
