aminghadersohi commented on code in PR #40128:
URL: https://github.com/apache/superset/pull/40128#discussion_r3342298127
##########
superset/commands/dashboard/importers/v1/utils.py:
##########
@@ -248,23 +248,93 @@ def import_dashboard( # noqa: C901
overwrite: bool = False,
ignore_permissions: bool = False,
) -> Dashboard:
+ """Import a dashboard from a config dict, handling existing matches.
+
+ Permission model for an existing UUID match:
+
+ +--------------+---------------+---------------------+-----------------+
+ | Existing row | overwrite arg | Caller has perms? | Outcome |
+ +==============+===============+=====================+=================+
+ | alive | False | (n/a) | return existing |
+ +--------------+---------------+---------------------+-----------------+
+ | alive | True | can_write + owner | UPDATE in place |
+ +--------------+---------------+---------------------+-----------------+
+ | alive | True | can_write, | raise |
+ | | | not owner/admin | |
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | can_write + owner | restore + UPDATE|
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | can_write, | raise |
+ | | | not owner/admin | |
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | not can_write | raise (Case B) |
+ +--------------+---------------+---------------------+-----------------+
+
+ "owner" in the matrix above means the caller is in ``existing.owners``
+ OR is an admin (the ownership check is bypassed for admins). The
+ mutation path also requires ``security_manager.can_access_dashboard
+ (existing)`` to pass — a per-row RBAC check distinct from the
+ ``can_write`` model-level grant.
+
+ Re-importing a soft-deleted UUID is implicitly a restore-with-update:
+ the user is bringing the dashboard back by uploading it again. We apply
+ the same ownership check as the explicit overwrite path so non-owners
+ cannot resurrect via re-import, and we raise rather than silently
+ returning a soft-deleted row to callers without write permission.
+ """
can_write = ignore_permissions or security_manager.can_access(
"can_write",
"Dashboard",
)
- existing =
db.session.query(Dashboard).filter_by(uuid=config["uuid"]).first()
+ from superset.commands.importers.v1.utils import find_existing_for_import
Review Comment:
**BLOCKER — Scan 1 (inline import without circular-import justification)**
```python
from superset.commands.importers.v1.utils import find_existing_for_import
```
This import is inside the `import_dashboard` function body.
`superset/commands/importers/v1/utils.py` does **not** import from
`superset/commands/dashboard/importers/v1/utils.py` — no circular dependency
exists. Move this to the module-level imports at the top of the file.
Per Superset convention, inline imports in production code require a `#
avoid circular import: <reason>` comment when they genuinely cannot be
top-level. The absence of that comment here confirms there is no justified
reason for deferral.
##########
superset/migrations/versions/2026-05-08_12-05_9e1f3b8c4d2a_add_deleted_at_to_dashboards.py:
##########
@@ -0,0 +1,159 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Add deleted_at + partial unique slug index for soft-delete.
+
+Adds to the ``dashboards`` table:
+- a nullable ``deleted_at`` column for soft-delete state
+- an index ``ix_dashboards_deleted_at`` for the visibility-filter listener
+- a partial unique index ``ix_dashboards_active_slug`` enforcing slug
+ uniqueness only among active (non-soft-deleted) rows
+
+Drops:
+- the existing full unique constraint on ``slug`` (named
+ ``idx_unique_slug``, created in migration 1a48a5411020)
+
+The constraint change makes the ``slug`` field reusable after soft-delete:
+soft-deleted rows no longer reserve their slug for the lifetime of the
+row. ``RestoreDashboardCommand`` handles the reverse case (restoring a
+dashboard whose slug has since been claimed by another active row) with
+an explicit conflict error. See UPDATING.md for the user-facing change.
+
+Dialect support for the partial index:
+- PostgreSQL: native ``WHERE deleted_at IS NULL`` partial index
+- MySQL 8.0+: functional index over
+ ``(CASE WHEN deleted_at IS NULL THEN slug END)``
+- MySQL <8.0: keeps the original full unique constraint (documented
+ limitation; functional indexes are not supported on these versions)
+- SQLite: keeps the original full unique constraint (column-level
+ ``UNIQUE`` cannot be dropped without recreating the table, which is
+ not worth the migration complexity for a test-only dialect). Tests
+ that need to verify the partial-index behaviour run only on
+ PostgreSQL and MySQL 8+.
+
+Revision ID: 9e1f3b8c4d2a
+Revises: 33d7e0e21daa
+Create Date: 2026-05-08 12:05:00.000000
+"""
+
+from alembic import op
+from sqlalchemy import Column, DateTime
+
+from superset.migrations.shared.utils import (
+ add_columns,
+ create_index,
+ drop_columns,
+ drop_index,
+)
+
+# revision identifiers, used by Alembic.
+revision = "9e1f3b8c4d2a"
+down_revision = "33d7e0e21daa"
+
+TABLE_NAME = "dashboards"
+DELETED_AT_INDEX_NAME = f"ix_{TABLE_NAME}_deleted_at"
+PARTIAL_SLUG_INDEX_NAME = f"ix_{TABLE_NAME}_active_slug"
+# The original full unique constraint on ``slug`` was created with an
+# explicit name in migration 1a48a5411020 (2015-12-04). Same name on
+# PostgreSQL (constraint) and MySQL (index).
+LEGACY_SLUG_INDEX_NAME = "idx_unique_slug"
+
+
+def _mysql_supports_functional_index(bind) -> bool:
+ """Return True iff the connected MySQL is 8.0+ (supports functional
indexes).
+
+ Excludes MariaDB even at server version ``>= (10, x)`` because MariaDB
+ reports through the same ``server_version_info`` attribute but uses
+ different functional-index semantics around ``CASE`` expressions.
+ Uses SQLAlchemy's parsed ``server_version_info`` rather than ``SELECT
+ VERSION()`` to avoid an extra round-trip and brittle string parsing.
+ """
+ if getattr(bind.dialect, "is_mariadb", False):
+ return False
+ return (bind.dialect.server_version_info or ()) >= (8, 0)
+
+
+def upgrade() -> None:
+ bind = op.get_bind()
+ _add_deleted_at_column()
+ _replace_slug_constraint_with_partial_index(bind)
+
+
+def downgrade() -> None:
+ bind = op.get_bind()
+ _restore_slug_constraint(bind)
+ _drop_deleted_at_column()
+
+
+def _add_deleted_at_column() -> None:
+ add_columns(TABLE_NAME, Column("deleted_at", DateTime(), nullable=True))
+ create_index(TABLE_NAME, DELETED_AT_INDEX_NAME, ["deleted_at"])
+
+
+def _drop_deleted_at_column() -> None:
+ drop_index(TABLE_NAME, DELETED_AT_INDEX_NAME)
+ drop_columns(TABLE_NAME, "deleted_at")
+
+
+def _replace_slug_constraint_with_partial_index(bind) -> None:
+ """Swap the full UNIQUE on ``slug`` for a partial index where supported.
+
+ The original constraint is named ``idx_unique_slug`` from migration
+ 1a48a5411020 — same name on PostgreSQL (constraint) and MySQL (index).
+
+ SQLite and MySQL <8.0 are no-ops here: they keep the original full
+ unique constraint. See the module docstring for the rationale.
+ """
+ dialect = bind.dialect.name
+ if dialect == "postgresql":
+ op.execute(
+ f"ALTER TABLE {TABLE_NAME} "
+ f"DROP CONSTRAINT IF EXISTS {LEGACY_SLUG_INDEX_NAME}"
+ )
+ # Some installations may have the unique enforced as a plain
+ # index rather than a constraint. Both DROPs are IF EXISTS, so
+ # whichever path applies cleans up.
+ op.execute(f"DROP INDEX IF EXISTS {LEGACY_SLUG_INDEX_NAME}")
+ op.execute(
+ f"CREATE UNIQUE INDEX {PARTIAL_SLUG_INDEX_NAME} "
+ f"ON {TABLE_NAME} (slug) WHERE deleted_at IS NULL"
+ )
+ elif dialect == "mysql" and _mysql_supports_functional_index(bind):
+ op.execute(f"ALTER TABLE {TABLE_NAME} DROP INDEX
{LEGACY_SLUG_INDEX_NAME}")
+ op.execute(
+ f"CREATE UNIQUE INDEX {PARTIAL_SLUG_INDEX_NAME} "
+ f"ON {TABLE_NAME} ((CASE WHEN deleted_at IS NULL THEN slug END))"
+ )
+
+
+def _restore_slug_constraint(bind) -> None:
Review Comment:
**MEDIUM — Downgrade `_restore_slug_constraint` can fail with a constraint
violation if slug conflicts were accumulated**
After the partial-index migration, a soft-deleted row and an active row can
share the same `slug` (that is the point). If an operator downgrades at that
point, `ALTER TABLE dashboards ADD CONSTRAINT idx_unique_slug UNIQUE (slug)`
(Postgres) / `ADD UNIQUE INDEX` (MySQL) will fail with a unique-constraint
violation.
The downgrade has no pre-flight check or documentation of this precondition.
Suggested docstring addition:
```
Pre-condition: no two rows in ``dashboards`` share the same ``slug``.
If the partial-index window allowed slug reuse (one active row and one
soft-deleted row with the same slug), hard-delete the soft-deleted
duplicates before running this downgrade, or the ADD CONSTRAINT step
will abort with a unique-violation.
```
##########
superset/commands/dashboard/importers/v1/utils.py:
##########
@@ -248,23 +248,93 @@ def import_dashboard( # noqa: C901
overwrite: bool = False,
ignore_permissions: bool = False,
) -> Dashboard:
+ """Import a dashboard from a config dict, handling existing matches.
+
+ Permission model for an existing UUID match:
+
+ +--------------+---------------+---------------------+-----------------+
+ | Existing row | overwrite arg | Caller has perms? | Outcome |
+ +==============+===============+=====================+=================+
+ | alive | False | (n/a) | return existing |
+ +--------------+---------------+---------------------+-----------------+
+ | alive | True | can_write + owner | UPDATE in place |
+ +--------------+---------------+---------------------+-----------------+
+ | alive | True | can_write, | raise |
+ | | | not owner/admin | |
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | can_write + owner | restore + UPDATE|
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | can_write, | raise |
+ | | | not owner/admin | |
+ +--------------+---------------+---------------------+-----------------+
+ | soft-deleted | False or True | not can_write | raise (Case B) |
+ +--------------+---------------+---------------------+-----------------+
+
+ "owner" in the matrix above means the caller is in ``existing.owners``
+ OR is an admin (the ownership check is bypassed for admins). The
+ mutation path also requires ``security_manager.can_access_dashboard
+ (existing)`` to pass — a per-row RBAC check distinct from the
+ ``can_write`` model-level grant.
+
+ Re-importing a soft-deleted UUID is implicitly a restore-with-update:
+ the user is bringing the dashboard back by uploading it again. We apply
+ the same ownership check as the explicit overwrite path so non-owners
+ cannot resurrect via re-import, and we raise rather than silently
+ returning a soft-deleted row to callers without write permission.
+ """
can_write = ignore_permissions or security_manager.can_access(
"can_write",
"Dashboard",
)
- existing =
db.session.query(Dashboard).filter_by(uuid=config["uuid"]).first()
+ from superset.commands.importers.v1.utils import find_existing_for_import
+
user = get_user()
- if existing:
- if overwrite and can_write and user:
+
+ if existing := find_existing_for_import(Dashboard, config["uuid"]):
+ is_soft_deleted = existing.deleted_at is not None
+ needs_mutation = overwrite or is_soft_deleted
Review Comment:
**MEDIUM — Implicit ownership bypass when `can_write=True` and `user=None`**
```python
needs_mutation = overwrite or is_soft_deleted # always True for
soft-deleted
if needs_mutation and can_write and user: # ownership check silently
skipped when user is None
```
This was pre-existing for the `overwrite=True` path, but this PR extends
`needs_mutation=True` to **all soft-deleted UUID imports** regardless of
`overwrite`. A programmatic caller with `can_write` but no Flask user context
(service account, background task) will skip the ownership check and silently
restore any soft-deleted dashboard.
The `ignore_permissions=True` path is the correct bypass for the example
loader. Other `can_write=True, user=None` contexts are an unintended bypass
that is now broader than before this PR.
Suggested fix: add a note to the permission matrix in the docstring, or add
an explicit guard:
```python
# When user is None and ignore_permissions is False, ownership cannot be
# checked. This matches the pre-existing overwrite=True behaviour but
# now also applies to soft-deleted matches (needs_mutation=True implicit).
```
##########
superset/migrations/versions/2026-05-08_12-05_9e1f3b8c4d2a_add_deleted_at_to_dashboards.py:
##########
@@ -0,0 +1,159 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Add deleted_at + partial unique slug index for soft-delete.
+
+Adds to the ``dashboards`` table:
+- a nullable ``deleted_at`` column for soft-delete state
+- an index ``ix_dashboards_deleted_at`` for the visibility-filter listener
+- a partial unique index ``ix_dashboards_active_slug`` enforcing slug
+ uniqueness only among active (non-soft-deleted) rows
+
+Drops:
+- the existing full unique constraint on ``slug`` (named
+ ``idx_unique_slug``, created in migration 1a48a5411020)
+
+The constraint change makes the ``slug`` field reusable after soft-delete:
+soft-deleted rows no longer reserve their slug for the lifetime of the
+row. ``RestoreDashboardCommand`` handles the reverse case (restoring a
+dashboard whose slug has since been claimed by another active row) with
+an explicit conflict error. See UPDATING.md for the user-facing change.
+
+Dialect support for the partial index:
+- PostgreSQL: native ``WHERE deleted_at IS NULL`` partial index
+- MySQL 8.0+: functional index over
+ ``(CASE WHEN deleted_at IS NULL THEN slug END)``
+- MySQL <8.0: keeps the original full unique constraint (documented
+ limitation; functional indexes are not supported on these versions)
+- SQLite: keeps the original full unique constraint (column-level
+ ``UNIQUE`` cannot be dropped without recreating the table, which is
+ not worth the migration complexity for a test-only dialect). Tests
+ that need to verify the partial-index behaviour run only on
+ PostgreSQL and MySQL 8+.
+
+Revision ID: 9e1f3b8c4d2a
+Revises: 33d7e0e21daa
+Create Date: 2026-05-08 12:05:00.000000
+"""
+
+from alembic import op
+from sqlalchemy import Column, DateTime
+
+from superset.migrations.shared.utils import (
+ add_columns,
+ create_index,
+ drop_columns,
+ drop_index,
+)
+
+# revision identifiers, used by Alembic.
+revision = "9e1f3b8c4d2a"
+down_revision = "33d7e0e21daa"
+
+TABLE_NAME = "dashboards"
+DELETED_AT_INDEX_NAME = f"ix_{TABLE_NAME}_deleted_at"
+PARTIAL_SLUG_INDEX_NAME = f"ix_{TABLE_NAME}_active_slug"
+# The original full unique constraint on ``slug`` was created with an
+# explicit name in migration 1a48a5411020 (2015-12-04). Same name on
+# PostgreSQL (constraint) and MySQL (index).
+LEGACY_SLUG_INDEX_NAME = "idx_unique_slug"
+
+
+def _mysql_supports_functional_index(bind) -> bool:
Review Comment:
**NIT — Missing type hint on `bind` parameter**
`_replace_slug_constraint_with_partial_index` (line 111) and
`_restore_slug_constraint` (line 142) have the same gap. `op.get_bind()`
returns `sqlalchemy.engine.Connection`:
```python
from sqlalchemy.engine import Connection
def _mysql_supports_functional_index(bind: Connection) -> bool:
def _replace_slug_constraint_with_partial_index(bind: Connection) -> None:
def _restore_slug_constraint(bind: Connection) -> None:
```
All new Python code in this codebase requires type hints.
##########
superset/dashboards/filters.py:
##########
@@ -255,3 +256,10 @@ def apply(self, query: Query, value: Any) -> Query:
if value is False:
return query.filter(and_(Dashboard.created_by_fk.is_(None)))
return query
+
+
+class DashboardDeletedStateFilter(BaseDeletedStateFilter): # pylint:
disable=too-few-public-methods
Review Comment:
**NIT — Line too long (102 chars, limit 88)**
```python
class DashboardDeletedStateFilter(BaseDeletedStateFilter): # pylint:
disable=too-few-public-methods
```
Ruff will flag this as E501. Either split the class definition:
```python
class DashboardDeletedStateFilter( # pylint: disable=too-few-public-methods
BaseDeletedStateFilter
):
```
or add `# noqa: E501` (consistent with the pylint-disable pattern used for
similar classes in this file).
##########
tests/integration_tests/dashboards/soft_delete_tests.py:
##########
@@ -0,0 +1,405 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Integration tests for dashboard soft-delete and restore."""
+
+from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES
+from superset.extensions import db
+from superset.models.dashboard import Dashboard
+from superset.utils import json
+from tests.integration_tests.base_tests import SupersetTestCase
+from tests.integration_tests.constants import ADMIN_USERNAME
+
+
+def _hard_delete_dashboard(dashboard_id: int) -> None:
+ """Hard-delete a dashboard row regardless of soft-delete state."""
+ row = (
+ db.session.query(Dashboard)
+ .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Dashboard}})
+ .filter(Dashboard.id == dashboard_id)
+ .one_or_none()
+ )
+ if row:
+ db.session.delete(row)
+ db.session.commit()
+
+
+class TestDashboardSoftDelete(SupersetTestCase):
+ """Tests for dashboard soft-delete behaviour (T014, T017)."""
+
+ def _create_dashboard(self, title: str = "soft_delete_test") -> Dashboard:
+ admin = self.get_user("admin")
+ dashboard = Dashboard(
+ dashboard_title=title,
+ slug=f"slug_{title}",
+ owners=[admin],
+ published=True,
+ )
+ db.session.add(dashboard)
+ db.session.commit()
+ return dashboard
+
+ def test_delete_dashboard_soft_deletes(self):
+ """DELETE should set deleted_at instead of removing the row."""
+ dashboard = self._create_dashboard("sd_test_1")
+ dashboard_id = dashboard.id
+ self.login(ADMIN_USERNAME)
+
+ rv = self.client.delete(f"/api/v1/dashboard/{dashboard_id}")
+ assert rv.status_code == 200
+
+ row = (
+ db.session.query(Dashboard)
+ .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Dashboard}})
+ .filter(Dashboard.id == dashboard_id)
+ .one_or_none()
+ )
+ assert row is not None
+ assert row.deleted_at is not None
+
+ # Cleanup
+ _hard_delete_dashboard(dashboard_id)
+
+ def test_soft_deleted_dashboard_excluded_from_list(self):
+ """GET /api/v1/dashboard/ should not include soft-deleted."""
+ dashboard = self._create_dashboard("sd_list_test")
+ dashboard_id = dashboard.id
+ self.login(ADMIN_USERNAME)
+
+ self.client.delete(f"/api/v1/dashboard/{dashboard_id}")
+
+ rv = self.client.get("/api/v1/dashboard/")
+ data = json.loads(rv.data)
+ ids = [d["id"] for d in data["result"]]
+ assert dashboard_id not in ids
+
+ # Cleanup
+ _hard_delete_dashboard(dashboard_id)
+
+ def test_soft_deleted_dashboard_included_in_list_when_requested(self):
+ """GET /api/v1/dashboard/ with dashboard_deleted_state=include returns
deleted dashboards.""" # noqa: E501
+ dashboard = self._create_dashboard("sd_list_with_deleted")
+ dashboard_id = dashboard.id
+ self.login(ADMIN_USERNAME)
+
+ self.client.delete(f"/api/v1/dashboard/{dashboard_id}")
+
+ rison_query =
"(filters:!((col:id,opr:dashboard_deleted_state,value:include)))"
+ rv = self.client.get(f"/api/v1/dashboard/?q={rison_query}")
+ assert rv.status_code == 200
+
+ data = json.loads(rv.data)
+ deleted_row = next(
+ (row for row in data["result"] if row["id"] == dashboard_id),
+ None,
+ )
+ assert deleted_row is not None
+ assert deleted_row["deleted_at"] is not None
+
+ # Cleanup
+ _hard_delete_dashboard(dashboard_id)
+
+ def test_only_filter_returns_only_soft_deleted_dashboards(self):
+ """dashboard_deleted_state=only excludes live rows and returns only
deleted ones.""" # noqa: E501
+ live_dashboard = self._create_dashboard("only_live_dash")
+ deleted_dashboard = self._create_dashboard("only_deleted_dash")
+ live_id = live_dashboard.id
+ deleted_id = deleted_dashboard.id
+ self.login(ADMIN_USERNAME)
+
+ self.client.delete(f"/api/v1/dashboard/{deleted_id}")
+
+ rison_query =
"(filters:!((col:id,opr:dashboard_deleted_state,value:only)))"
+ rv = self.client.get(f"/api/v1/dashboard/?q={rison_query}")
+ assert rv.status_code == 200
+
+ data = json.loads(rv.data)
+ returned_ids = {row["id"] for row in data["result"]}
+ assert deleted_id in returned_ids
+ assert live_id not in returned_ids
+
+ # Cleanup
+ _hard_delete_dashboard(live_id)
+ _hard_delete_dashboard(deleted_id)
+
+ def test_embedded_dashboard_with_soft_deleted_parent(self):
+ """Embedded URL keeps loading after the parent dashboard is
soft-deleted.
+
+ The embedded view (`embedded/view.py:embedded`) only reads
+ `embedded.allowed_domains` and `embedded.dashboard_id` (FK column,
+ not relationship), so it never dereferences the soft-deleted
+ Dashboard via `embedded.dashboard`. Iframe still returns 200; the
+ frontend's subsequent `/api/v1/dashboard/<id>` fetch returns 404
+ cleanly via the visibility filter, and the user sees the standard
+ "dashboard not found" UI rather than a 500.
+
+ This pins down the contract documented in pr-readiness.md #8 and
+ prevents future changes to either the embedded view or the schema
+ from regressing it into a 500.
+ """
+ from unittest import mock
+
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+
+ dashboard = self._create_dashboard("embedded_soft_delete_test")
+ dashboard_id = dashboard.id
+ embedded = EmbeddedDashboardDAO.upsert(dashboard, [])
+ db.session.flush()
+ embedded_uuid = str(embedded.uuid)
+ self.login(ADMIN_USERNAME)
+
+ # Soft-delete the parent
+ self.client.delete(f"/api/v1/dashboard/{dashboard_id}")
+
+ # The dashboard fetch returns 404 cleanly (visibility filter applies).
+ rv = self.client.get(f"/api/v1/dashboard/{dashboard_id}")
+ assert rv.status_code == 404, (
+ "Soft-deleted dashboard should fetch 404, not 500; got "
+ f"{rv.status_code}. Body: {rv.data[:200]!r}"
+ )
+
+ # The embedded iframe URL still loads (200) — embedded.dashboard is
+ # never dereferenced by the view. Done last because the embedded
+ # handler clears the session in CI, which would 401 any follow-up
+ # API call.
+ with mock.patch.dict(
+ "superset.extensions.feature_flag_manager._feature_flags",
+ EMBEDDED_SUPERSET=True,
+ ):
+ rv = self.client.get(f"/embedded/{embedded_uuid}")
+ assert rv.status_code == 200, (
+ "Embedded view should still load 200 with a soft-deleted parent; "
+ f"got {rv.status_code}. Body: {rv.data[:200]!r}"
+ )
+
+ # Cleanup: hard-deleting the dashboard cascades to the embedded
+ # row via the FK ondelete=CASCADE.
+ _hard_delete_dashboard(dashboard_id)
+
+
+class TestDashboardRestore(SupersetTestCase):
+ """Tests for dashboard restore behaviour (T026, T028)."""
+
+ def _create_dashboard(self, title: str = "restore_test") -> Dashboard:
+ admin = self.get_user("admin")
+ dashboard = Dashboard(
+ dashboard_title=title,
+ slug=f"slug_{title}",
+ owners=[admin],
+ published=True,
+ )
+ db.session.add(dashboard)
+ db.session.commit()
+ return dashboard
+
+ def test_restore_soft_deleted_dashboard(self):
+ """POST /api/v1/dashboard/<uuid>/restore makes it visible again."""
+ dashboard = self._create_dashboard("restore_sd_test")
+ dashboard_id = dashboard.id
+ dashboard_uuid = str(dashboard.uuid)
+ self.login(ADMIN_USERNAME)
+
+ self.client.delete(f"/api/v1/dashboard/{dashboard_id}")
+ rv = self.client.post(f"/api/v1/dashboard/{dashboard_uuid}/restore")
+ assert rv.status_code == 200
+
+ rv = self.client.get(f"/api/v1/dashboard/{dashboard_id}")
+ assert rv.status_code == 200
+
+ # Cleanup
+ _hard_delete_dashboard(dashboard_id)
+
+ def test_restore_preserves_chart_associations(self):
+ """Restoring a dashboard reconnects to its charts (T028)."""
+ from superset.models.slice import Slice
+
+ admin = self.get_user("admin")
+ dashboard = self._create_dashboard("assoc_test")
+
+ chart = Slice(
+ slice_name="assoc_chart",
+ viz_type="table",
+ datasource_type="table",
+ datasource_id=1,
+ owners=[admin],
+ )
+ db.session.add(chart)
+ db.session.commit()
+ dashboard.slices.append(chart)
+ db.session.commit()
+
+ dashboard_id = dashboard.id
+ dashboard_uuid = str(dashboard.uuid)
+ chart_id = chart.id
+ self.login(ADMIN_USERNAME)
+
+ self.client.delete(f"/api/v1/dashboard/{dashboard_id}")
+ rv = self.client.post(f"/api/v1/dashboard/{dashboard_uuid}/restore")
+ assert rv.status_code == 200
+
+ restored = (
+ db.session.query(Dashboard).filter(Dashboard.id ==
dashboard_id).one()
+ )
+ chart_ids = [s.id for s in restored.slices]
+ assert chart_id in chart_ids
+
+ # Cleanup
+ db.session.delete(chart)
+ _hard_delete_dashboard(dashboard_id)
+
+ def test_restore_blocked_by_active_slug_twin(self):
+ """Restore returns 422 when another active dashboard now owns the slug.
+
+ With the partial unique index allowing slug reuse during the
+ soft-deleted window, a new active dashboard can claim the slug
+ of a soft-deleted one. Restoring the original then has to fail
+ cleanly rather than producing a unique-index violation at flush
+ time. This pins the API contract end-to-end (command → endpoint
+ → 422 response).
+
+ The conflict guard runs in application code, so the test exercises
+ every dialect — not just those with a partial index.
+ """
+ shared_slug = "slug_conflict_test"
+ admin = self.get_user("admin")
+
+ # First dashboard: created, soft-deleted, awaiting restore.
+ first = Dashboard(
+ dashboard_title="conflict_first",
+ slug=shared_slug,
+ owners=[admin],
+ published=True,
+ )
+ db.session.add(first)
+ db.session.commit()
+ first_id = first.id
+ first_uuid = str(first.uuid)
+
+ self.login(ADMIN_USERNAME)
+ self.client.delete(f"/api/v1/dashboard/{first_id}")
+
+ # Second dashboard claims the same slug while the first is
soft-deleted.
+ # This only succeeds on Postgres / MySQL 8+ where the partial index
+ # frees the slug; on SQLite / MySQL <8 the full unique constraint
+ # would still block this insert, so skip the rest of the assertion
+ # path on those dialects.
+ dialect = db.session.bind.dialect.name
Review Comment:
**NIT — Duplicate dialect-skip block (DRY violation)**
This exact 10-line dialect-detection block appears verbatim here and again
at line 349 in
`test_partial_index_allows_multiple_soft_deleted_with_same_slug`. Extract to a
`_skip_if_no_partial_index(self)` helper method on the test class to avoid the
duplication.
##########
superset/commands/dashboard/restore.py:
##########
@@ -0,0 +1,80 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Command to restore a soft-deleted dashboard."""
+
+from superset.commands.dashboard.exceptions import (
+ DashboardForbiddenError,
+ DashboardNotFoundError,
+ DashboardRestoreFailedError,
+ DashboardSlugConflictError,
+)
+from superset.commands.restore import BaseRestoreCommand
+from superset.daos.dashboard import DashboardDAO
+from superset.extensions import db
+from superset.models.dashboard import Dashboard
+
+
+class RestoreDashboardCommand(BaseRestoreCommand[Dashboard]):
+ """Restore a soft-deleted dashboard by clearing its ``deleted_at`` field.
+
+ Most behaviour is inherited from ``BaseRestoreCommand``. The override
+ here adds the slug-conflict check: with the partial unique index on
+ ``slug WHERE deleted_at IS NULL``, slug reuse during the soft-deleted
+ window is allowed, so a restore may now collide with an active row
+ that claimed the slug while this one was deleted. Raise a clean
+ domain error in that case instead of letting the unique-index
+ violation surface as an opaque ``IntegrityError`` at flush time.
+ """
+
+ dao = DashboardDAO
+ not_found_exc = DashboardNotFoundError
+ forbidden_exc = DashboardForbiddenError
+ restore_failed_exc = DashboardRestoreFailedError
+
+ def validate(self) -> Dashboard: # type: ignore[override]
Review Comment:
**NIT — `validate()` override has no docstring**
The override adds a slug-conflict pre-check absent from
`BaseRestoreCommand.validate()`. A one-liner makes the contract visible without
requiring readers to diff against the base:
```python
def validate(self) -> Dashboard: # type: ignore[override]
"""Extend base validation with a slug-conflict pre-check."""
```
##########
tests/integration_tests/base_tests.py:
##########
@@ -594,10 +594,21 @@ def insert_dashboard(
role_obj = db.session.query(security_manager.role_model).get(role)
obj_roles.append(role_obj)
- # Defensive cleanup: remove any existing dashboard with the same slug
+ # Defensive cleanup: remove any existing dashboard with the same slug.
+ # Bypass the soft-delete visibility filter so a soft-deleted row from
+ # a prior test still gets cleared — without the bypass the lookup
+ # returns ``None`` and the INSERT below trips the unique constraint
+ # on ``slug`` against the soft-deleted (but hidden) row.
if slug:
+ from superset.constants import ( # noqa: PLC0415
Review Comment:
**NIT — Inline import without justification**
```python
from superset.constants import ( # noqa: PLC0415
SKIP_VISIBILITY_FILTER_CLASSES,
)
```
`SKIP_VISIBILITY_FILTER_CLASSES` is a plain string constant with no
circular-import concern. The `# noqa: PLC0415` silences ruff but gives no
reader explanation for why this cannot be a module-level import. Either move it
to the top of the file (removing the noqa), or add a comment explaining the
deferral reason.
##########
UPDATING.md:
##########
@@ -30,6 +30,22 @@ Importing a dataset now validates the `catalog` field
against the target databas
If you relied on importing datasets with a non-default catalog, enable "Allow
changing catalogs" on the target connection, or set the dataset's catalog to
the connection's default before importing.
+### Soft delete and restore for dashboards
+
+`DELETE /api/v1/dashboard/<id>` no longer hard-deletes the dashboard. The row
is marked with a `deleted_at` timestamp and hidden from all list, detail, and
lookup endpoints (including the embedded-dashboard iframe path, which now
returns 404 for soft-deleted parents).
+
+**New endpoint** — `POST /api/v1/dashboard/<uuid>/restore` clears `deleted_at`
and returns the dashboard to active state. Requires `can_write on Dashboard`
and ownership of the row (or admin). Soft-deleted dashboards can also be
surfaced in the list endpoint via the new `dashboard_deleted_state` rison
filter: `include` returns both live and soft-deleted rows, `only` returns just
the soft-deleted ones. Any other value is ignored.
Review Comment:
**MEDIUM — Lock description understates what `DROP CONSTRAINT` takes on
Postgres**
> "index builds run inline (no `CONCURRENTLY`) and may briefly block
**reads** on the `dashboards` table"
`ALTER TABLE dashboards DROP CONSTRAINT idx_unique_slug` acquires `ACCESS
EXCLUSIVE LOCK` on Postgres, which blocks **reads and writes** (not just
reads). The `CREATE UNIQUE INDEX` that follows acquires a lighter `ShareLock`
(blocks writes only). Suggest:
> "The constraint swap briefly blocks reads and writes on the `dashboards`
table on Postgres (ACCESS EXCLUSIVE during DROP CONSTRAINT, then writes-only
during CREATE INDEX). MySQL InnoDB builds the functional index online."
For most deployments the `dashboards` table is small and the window is
sub-second, but the lock semantics should be stated accurately.
--
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]
