sadpandajoe commented on code in PR #45538:
URL: https://github.com/apache/superset/pull/45538#discussion_r4382758859
##########
tests/unit_tests/dao/dashboard_test.py:
##########
@@ -229,6 +232,108 @@ def
test_install_membership_baseline_keeps_unflushed_change(
assert inspect(dashboard).attrs.slices.history.added == [added]
+@with_feature_flags(SOFT_DELETE=True)
+def test_set_dash_metadata_keeps_only_stored_archived_members(
+ session: Session,
+) -> None:
+ """Archived charts kept outside the layout come from the stored rows, so
+ an unflushed archived addition is not linked past the access check."""
+ Dashboard.metadata.create_all(session.get_bind())
+ archived_at: datetime = datetime(2026, 1, 1, tzinfo=timezone.utc)
+ live: Slice = Slice(slice_name="stored_live", datasource_type="table")
+ stored: Slice = Slice(
+ slice_name="stored_archived", datasource_type="table",
deleted_at=archived_at
+ )
+ pending: Slice = Slice(
+ slice_name="pending_archived", datasource_type="table",
deleted_at=archived_at
+ )
+ dashboard: Dashboard = Dashboard(
+ dashboard_title="stored_archived_dash", slices=[live, stored]
+ )
+ db.session.add_all([live, stored, pending, dashboard])
+ db.session.commit()
+ positions: dict[str, dict[str, Any]] = {
+ "CHART-live": {"type": "CHART", "meta": {"chartId": live.id}}
+ }
+
+ with db.session.no_autoflush:
+ dashboard.slices.append(pending)
+ DashboardDAO.set_dash_metadata(dashboard, {"positions": positions})
+ db.session.flush()
+
+ member_ids: set[int] = set(
+ db.session.scalars(
+ select(dashboard_slices.c.slice_id).where(
+ dashboard_slices.c.dashboard_id == dashboard.id
+ )
+ )
+ )
+ assert member_ids == {live.id, stored.id}
+
+
+@with_feature_flags(SOFT_DELETE=True)
+def test_set_dash_metadata_checks_new_charts_beside_archived_member(
+ session: Session,
+) -> None:
+ """With an archived member outside the layout, a new unreadable chart is
+ refused against the stored rows, and a new readable chart is linked
+ alongside the kept archived member, which is never access-checked."""
+ Dashboard.metadata.create_all(session.get_bind())
+ live: Slice = Slice(slice_name="mixed_live", datasource_type="table")
+ archived: Slice = Slice(
+ slice_name="mixed_archived",
+ datasource_type="table",
+ deleted_at=datetime(2026, 1, 1, tzinfo=timezone.utc),
+ )
+ readable: Slice = Slice(slice_name="mixed_readable",
datasource_type="table")
+ unreadable: Slice = Slice(slice_name="mixed_unreadable",
datasource_type="table")
Review Comment:
The only rejected chart here is a live one, so nothing exercises a
soft-deleted chart that is in the layout but not already a member. If the check
ever skipped candidates with `deleted_at` set, a caller could attach an
archived chart they cannot read and every test in this file would still pass.
Should this add an archived, unreadable, non-member chart to the layout and
assert `DashboardLayoutInvalidError` with the stored membership unchanged?
--
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]
