feat(dashboards): soft-delete and restore#40128
Merged
rusackas merged 45 commits intoJul 2, 2026
Merged
Conversation
github-actions
Bot
added
risk:db-migration
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This was referenced May 14, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40128 +/- ##
==========================================
- Coverage 64.52% 59.32% -5.20%
==========================================
Files 2673 811 -1862
Lines 147639 69655 -77984
Branches 34093 8727 -25366
==========================================
- Hits 95260 41324 -53936
+ Misses 50650 26592 -24058
- Partials 1729 1739 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mikebridge
force-pushed
the
sc-106190-dashboards-soft-delete
branch
from
May 18, 2026 17:03
267f238 to
a248dc0
Compare
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 20, 2026
Eight items from the 2026-05-20 review on the infrastructure PR: * **H1 — subclass walk cached via ``__init_subclass__``.** The previous ``_all_soft_delete_subclasses()`` walked ``SoftDeleteMixin.__subclasses__()`` on every primary ORM SELECT. With one adopter today the cost is negligible, but the listener fires on every query in the app and the walk grows linearly with each adopted entity. ``SoftDeleteMixin`` now maintains a sorted ``_registered_subclasses`` list updated by ``__init_subclass__`` at class-definition time; the listener reads the cached list with no walk. Sort happens once per registration (rare event) rather than per query. * **M3 — ``SoftDeleteMixin`` import in ``BaseDAO.delete`` moved to top.** ``SKIP_VISIBILITY_FILTER_CLASSES`` from the same module was already top-level; ``SoftDeleteMixin`` had no real reason to be inline. Cleaner. * **M4 — pylint-disable in ``raise_for_ownership`` documented.** A top-level import here actually does cause a circular (security ↔ models.core ↔ superset's lazy ``feature_flag_manager``). Kept the inline import but added a comment naming the cycle so future maintainers don't try to "fix" it and re-trip the issue. * **M5 — ``BaseDeletedStateFilter.model`` typed as ``ClassVar[type[SoftDeleteMixin]]``.** Previously ``Any``, which meant a subclass accidentally binding ``model`` to a non-soft-deletable entity would crash at runtime on ``.deleted_at`` rather than failing mypy. * **M6 — listener placement documented.** ``setup_soft_delete_listener`` is called outside the ``with app_context()`` block because the ``do_orm_execute`` hook attaches to the ``Session`` class (process-wide), not to a Session instance, so it doesn't need Flask app context. Added an explaining comment in ``init_app_in_ctx``'s caller; ``setup_db()`` already ran earlier so the Session import is initialised. * **M7 — unit tests for ``BaseDeletedStateFilter`` + ``SoftDeleteApiMixin``.** New ``tests/unit_tests/views/test_soft_delete_filter.py`` (10 tests): filter no-op / include / only / case-insensitivity / unknown value; mixin no-op-without-flag / inject-with-flag / consume-flag-on-call; ``_serialize_deleted_at`` None / datetime handling. * **Nit — explicit ``orig_resource is None`` guard in ``raise_for_ownership``.** Previously fell through to ``hasattr(None, "owners")`` returning ``False`` → ``owners = []`` → generic ownership error. Now raises the proper "resource removed before ownership could be verified" error so the cause is clear in the rare race-with-hard-delete case. * **Nit — unit tests for ``find_existing_for_import`` / ``clear_soft_deleted_for_import``.** New ``tests/unit_tests/commands/importers/v1/test_find_existing_for_import.py`` (5 tests): no-match-returns-None, live-row-as-is, soft-deleted-row-as-is (pure lookup, no side effect), ``clear`` hard-deletes via ORM (listener-firing path), and the composed find-then-clear contract that's the documented caller sequence. Deferred with explanation in the PR-comment reply (not in this commit): * **H2 — non-restorable / GDPR hook**: V2 concern. Designing the interface without a concrete use case risks getting it wrong; follow-up ticket if compliance work is ever scoped. * **M8 — ``deleted_at`` schema mutation**: documented in the entity- rollout PR bodies (apache#40128/apache#40129/apache#40130) rather than blocking infrastructure. 40 unit tests pass (was 26; added 14 covering filter, mixin, importer helpers). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
force-pushed
the
sc-106190-dashboards-soft-delete
branch
5 times, most recently
from
May 20, 2026 22:36
c54a6eb to
d26afed
Compare
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 20, 2026
The listener's loader_criteria was passing a concrete SQL expression (``cls.where_not_deleted()``) which rendered as the raw table name (``slices.deleted_at``) regardless of how the table was aliased in the statement. When the same soft-deletable model appeared under an alias in a JOIN — e.g., FAB's outer/inner reconstruction or ``report_schedule.chart`` aliasing ``slices AS chart`` — the JOIN's ON clause produced ``Unknown column 'slices.deleted_at' in 'on clause'`` and broke the entire query. Surfaced as CI failures on the entity-rollout PRs (apache#40128 / apache#40129 / apache#40130) — specifically pre-existing report-schedule tests that join to ``slices AS chart`` started failing once ``Slice`` inherited ``SoftDeleteMixin``. The infrastructure PR itself doesn't exercise this because no entity has the mixin yet. Fix: switch the criteria to a lambda (the form Bayer's canonical pattern uses). SQLAlchemy invokes the lambda per occurrence and adapts the column reference to the alias at each site. The lambda's body is trivial (``c.deleted_at.is_(None)``) so the ``DeferredLambdaElement`` parser handles it without issue — we originally moved AWAY from a lambda because of an ``if cls in bypass_classes`` conditional that DeferredLambdaElement mis-parsed as a SQL ``IN`` operator, but the conditional now lives outside the lambda (in the per-class iteration), so the lambda body itself is clean. Test added: test_listener_adapts_criteria_to_aliased_table_in_joins — reproduces the aliased-join shape using the synthetic ``_SoftDeletableChild`` model. Without the fix, this query raises ``OperationalError: no such column: ..._soft_deletable_child.deleted_at`` because the criteria renders as the raw table name. With the fix, the query runs cleanly. 45 unit tests pass (was 44 + 1 regression test). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
force-pushed
the
sc-106190-dashboards-soft-delete
branch
3 times, most recently
from
May 21, 2026 17:59
32317fd to
59e8a76
Compare
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 25, 2026
Eight items from the 2026-05-20 review on the infrastructure PR: * **H1 — subclass walk cached via ``__init_subclass__``.** The previous ``_all_soft_delete_subclasses()`` walked ``SoftDeleteMixin.__subclasses__()`` on every primary ORM SELECT. With one adopter today the cost is negligible, but the listener fires on every query in the app and the walk grows linearly with each adopted entity. ``SoftDeleteMixin`` now maintains a sorted ``_registered_subclasses`` list updated by ``__init_subclass__`` at class-definition time; the listener reads the cached list with no walk. Sort happens once per registration (rare event) rather than per query. * **M3 — ``SoftDeleteMixin`` import in ``BaseDAO.delete`` moved to top.** ``SKIP_VISIBILITY_FILTER_CLASSES`` from the same module was already top-level; ``SoftDeleteMixin`` had no real reason to be inline. Cleaner. * **M4 — pylint-disable in ``raise_for_ownership`` documented.** A top-level import here actually does cause a circular (security ↔ models.core ↔ superset's lazy ``feature_flag_manager``). Kept the inline import but added a comment naming the cycle so future maintainers don't try to "fix" it and re-trip the issue. * **M5 — ``BaseDeletedStateFilter.model`` typed as ``ClassVar[type[SoftDeleteMixin]]``.** Previously ``Any``, which meant a subclass accidentally binding ``model`` to a non-soft-deletable entity would crash at runtime on ``.deleted_at`` rather than failing mypy. * **M6 — listener placement documented.** ``setup_soft_delete_listener`` is called outside the ``with app_context()`` block because the ``do_orm_execute`` hook attaches to the ``Session`` class (process-wide), not to a Session instance, so it doesn't need Flask app context. Added an explaining comment in ``init_app_in_ctx``'s caller; ``setup_db()`` already ran earlier so the Session import is initialised. * **M7 — unit tests for ``BaseDeletedStateFilter`` + ``SoftDeleteApiMixin``.** New ``tests/unit_tests/views/test_soft_delete_filter.py`` (10 tests): filter no-op / include / only / case-insensitivity / unknown value; mixin no-op-without-flag / inject-with-flag / consume-flag-on-call; ``_serialize_deleted_at`` None / datetime handling. * **Nit — explicit ``orig_resource is None`` guard in ``raise_for_ownership``.** Previously fell through to ``hasattr(None, "owners")`` returning ``False`` → ``owners = []`` → generic ownership error. Now raises the proper "resource removed before ownership could be verified" error so the cause is clear in the rare race-with-hard-delete case. * **Nit — unit tests for ``find_existing_for_import`` / ``clear_soft_deleted_for_import``.** New ``tests/unit_tests/commands/importers/v1/test_find_existing_for_import.py`` (5 tests): no-match-returns-None, live-row-as-is, soft-deleted-row-as-is (pure lookup, no side effect), ``clear`` hard-deletes via ORM (listener-firing path), and the composed find-then-clear contract that's the documented caller sequence. Deferred with explanation in the PR-comment reply (not in this commit): * **H2 — non-restorable / GDPR hook**: V2 concern. Designing the interface without a concrete use case risks getting it wrong; follow-up ticket if compliance work is ever scoped. * **M8 — ``deleted_at`` schema mutation**: documented in the entity- rollout PR bodies (apache#40128/apache#40129/apache#40130) rather than blocking infrastructure. 40 unit tests pass (was 26; added 14 covering filter, mixin, importer helpers). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 25, 2026
The listener's loader_criteria was passing a concrete SQL expression (``cls.where_not_deleted()``) which rendered as the raw table name (``slices.deleted_at``) regardless of how the table was aliased in the statement. When the same soft-deletable model appeared under an alias in a JOIN — e.g., FAB's outer/inner reconstruction or ``report_schedule.chart`` aliasing ``slices AS chart`` — the JOIN's ON clause produced ``Unknown column 'slices.deleted_at' in 'on clause'`` and broke the entire query. Surfaced as CI failures on the entity-rollout PRs (apache#40128 / apache#40129 / apache#40130) — specifically pre-existing report-schedule tests that join to ``slices AS chart`` started failing once ``Slice`` inherited ``SoftDeleteMixin``. The infrastructure PR itself doesn't exercise this because no entity has the mixin yet. Fix: switch the criteria to a lambda (the form Bayer's canonical pattern uses). SQLAlchemy invokes the lambda per occurrence and adapts the column reference to the alias at each site. The lambda's body is trivial (``c.deleted_at.is_(None)``) so the ``DeferredLambdaElement`` parser handles it without issue — we originally moved AWAY from a lambda because of an ``if cls in bypass_classes`` conditional that DeferredLambdaElement mis-parsed as a SQL ``IN`` operator, but the conditional now lives outside the lambda (in the per-class iteration), so the lambda body itself is clean. Test added: test_listener_adapts_criteria_to_aliased_table_in_joins — reproduces the aliased-join shape using the synthetic ``_SoftDeletableChild`` model. Without the fix, this query raises ``OperationalError: no such column: ..._soft_deletable_child.deleted_at`` because the criteria renders as the raw table name. With the fix, the query runs cleanly. 45 unit tests pass (was 44 + 1 regression test). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
force-pushed
the
sc-106190-dashboards-soft-delete
branch
2 times, most recently
from
May 25, 2026 19:41
07b7d3f to
cfcc5a9
Compare
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 25, 2026
…tore (Option C) Same shape as the charts and datasets importer fixes. The dashboard importer on this branch had three bugs from the soft-delete refactor: 1. **Null-user `config["id"]` fallthrough was lost.** The `if overwrite and can_write and user:` gate excluded the example-loader case (ignore_permissions=True, user=None) from id-preservation, so re-import over an existing live row would collide with the UUID unique index. 2. **Hard-delete-and-replace on soft-deleted overwrite.** The code called `clear_soft_deleted_for_import` for soft-deleted matches, cascading through dashboard_slices junctions and role/owner/tag associations. The import then had to reconstruct everything. 3. **Silent return of soft-deleted rows on non-overwrite.** The agent's "silent resurrection" concern — direct re-import returned the soft-deleted dashboard without restoring it, leaving the row hidden in the list. Adopts the Option-C shape from f4768c4 (charts) and 7b2aa39 (datasets): - Any mutation path (overwrite OR soft-delete restore) applies the overwrite path's ownership check (`can_access_dashboard` + owner-or- admin), so non-owners cannot resurrect via re-import. - Soft-deleted match + can_write: restore in place + UPDATE via `config["id"] = existing.id`. Preserves the PK and all relationship rows (slices junctions, owners, roles, tags). - Alive match + non-overwrite: returns existing unchanged (master behavior). - Null user path correctly falls through to the mutation path when `needs_mutation` is true, preserving the example-loader semantics. Four new tests pin the contracts: - test_import_soft_deleted_dashboard_overwrite_restores_in_place - test_import_soft_deleted_dashboard_non_overwrite_restores_for_owner - test_import_soft_deleted_dashboard_non_overwrite_raises_for_non_owner - test_import_soft_deleted_dashboard_ignore_permissions_restores_in_place This should clear the dashboard zip-import playwright failure on apache#40128. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 28, 2026
…tore (Option C) Same shape as the charts and datasets importer fixes. The dashboard importer on this branch had three bugs from the soft-delete refactor: 1. **Null-user `config["id"]` fallthrough was lost.** The `if overwrite and can_write and user:` gate excluded the example-loader case (ignore_permissions=True, user=None) from id-preservation, so re-import over an existing live row would collide with the UUID unique index. 2. **Hard-delete-and-replace on soft-deleted overwrite.** The code called `clear_soft_deleted_for_import` for soft-deleted matches, cascading through dashboard_slices junctions and role/owner/tag associations. The import then had to reconstruct everything. 3. **Silent return of soft-deleted rows on non-overwrite.** The agent's "silent resurrection" concern — direct re-import returned the soft-deleted dashboard without restoring it, leaving the row hidden in the list. Adopts the Option-C shape from f4768c4 (charts) and 7b2aa39 (datasets): - Any mutation path (overwrite OR soft-delete restore) applies the overwrite path's ownership check (`can_access_dashboard` + owner-or- admin), so non-owners cannot resurrect via re-import. - Soft-deleted match + can_write: restore in place + UPDATE via `config["id"] = existing.id`. Preserves the PK and all relationship rows (slices junctions, owners, roles, tags). - Alive match + non-overwrite: returns existing unchanged (master behavior). - Null user path correctly falls through to the mutation path when `needs_mutation` is true, preserving the example-loader semantics. Four new tests pin the contracts: - test_import_soft_deleted_dashboard_overwrite_restores_in_place - test_import_soft_deleted_dashboard_non_overwrite_restores_for_owner - test_import_soft_deleted_dashboard_non_overwrite_raises_for_non_owner - test_import_soft_deleted_dashboard_ignore_permissions_restores_in_place This should clear the dashboard zip-import playwright failure on apache#40128. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
force-pushed
the
sc-106190-dashboards-soft-delete
branch
from
May 28, 2026 19:59
a34575b to
8c147e5
Compare
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 28, 2026
Eight items from the 2026-05-20 review on the infrastructure PR: * **H1 — subclass walk cached via ``__init_subclass__``.** The previous ``_all_soft_delete_subclasses()`` walked ``SoftDeleteMixin.__subclasses__()`` on every primary ORM SELECT. With one adopter today the cost is negligible, but the listener fires on every query in the app and the walk grows linearly with each adopted entity. ``SoftDeleteMixin`` now maintains a sorted ``_registered_subclasses`` list updated by ``__init_subclass__`` at class-definition time; the listener reads the cached list with no walk. Sort happens once per registration (rare event) rather than per query. * **M3 — ``SoftDeleteMixin`` import in ``BaseDAO.delete`` moved to top.** ``SKIP_VISIBILITY_FILTER_CLASSES`` from the same module was already top-level; ``SoftDeleteMixin`` had no real reason to be inline. Cleaner. * **M4 — pylint-disable in ``raise_for_ownership`` documented.** A top-level import here actually does cause a circular (security ↔ models.core ↔ superset's lazy ``feature_flag_manager``). Kept the inline import but added a comment naming the cycle so future maintainers don't try to "fix" it and re-trip the issue. * **M5 — ``BaseDeletedStateFilter.model`` typed as ``ClassVar[type[SoftDeleteMixin]]``.** Previously ``Any``, which meant a subclass accidentally binding ``model`` to a non-soft-deletable entity would crash at runtime on ``.deleted_at`` rather than failing mypy. * **M6 — listener placement documented.** ``setup_soft_delete_listener`` is called outside the ``with app_context()`` block because the ``do_orm_execute`` hook attaches to the ``Session`` class (process-wide), not to a Session instance, so it doesn't need Flask app context. Added an explaining comment in ``init_app_in_ctx``'s caller; ``setup_db()`` already ran earlier so the Session import is initialised. * **M7 — unit tests for ``BaseDeletedStateFilter`` + ``SoftDeleteApiMixin``.** New ``tests/unit_tests/views/test_soft_delete_filter.py`` (10 tests): filter no-op / include / only / case-insensitivity / unknown value; mixin no-op-without-flag / inject-with-flag / consume-flag-on-call; ``_serialize_deleted_at`` None / datetime handling. * **Nit — explicit ``orig_resource is None`` guard in ``raise_for_ownership``.** Previously fell through to ``hasattr(None, "owners")`` returning ``False`` → ``owners = []`` → generic ownership error. Now raises the proper "resource removed before ownership could be verified" error so the cause is clear in the rare race-with-hard-delete case. * **Nit — unit tests for ``find_existing_for_import`` / ``clear_soft_deleted_for_import``.** New ``tests/unit_tests/commands/importers/v1/test_find_existing_for_import.py`` (5 tests): no-match-returns-None, live-row-as-is, soft-deleted-row-as-is (pure lookup, no side effect), ``clear`` hard-deletes via ORM (listener-firing path), and the composed find-then-clear contract that's the documented caller sequence. Deferred with explanation in the PR-comment reply (not in this commit): * **H2 — non-restorable / GDPR hook**: V2 concern. Designing the interface without a concrete use case risks getting it wrong; follow-up ticket if compliance work is ever scoped. * **M8 — ``deleted_at`` schema mutation**: documented in the entity- rollout PR bodies (apache#40128/apache#40129/apache#40130) rather than blocking infrastructure. 40 unit tests pass (was 26; added 14 covering filter, mixin, importer helpers). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 28, 2026
The listener's loader_criteria was passing a concrete SQL expression (``cls.where_not_deleted()``) which rendered as the raw table name (``slices.deleted_at``) regardless of how the table was aliased in the statement. When the same soft-deletable model appeared under an alias in a JOIN — e.g., FAB's outer/inner reconstruction or ``report_schedule.chart`` aliasing ``slices AS chart`` — the JOIN's ON clause produced ``Unknown column 'slices.deleted_at' in 'on clause'`` and broke the entire query. Surfaced as CI failures on the entity-rollout PRs (apache#40128 / apache#40129 / apache#40130) — specifically pre-existing report-schedule tests that join to ``slices AS chart`` started failing once ``Slice`` inherited ``SoftDeleteMixin``. The infrastructure PR itself doesn't exercise this because no entity has the mixin yet. Fix: switch the criteria to a lambda (the form Bayer's canonical pattern uses). SQLAlchemy invokes the lambda per occurrence and adapts the column reference to the alias at each site. The lambda's body is trivial (``c.deleted_at.is_(None)``) so the ``DeferredLambdaElement`` parser handles it without issue — we originally moved AWAY from a lambda because of an ``if cls in bypass_classes`` conditional that DeferredLambdaElement mis-parsed as a SQL ``IN`` operator, but the conditional now lives outside the lambda (in the per-class iteration), so the lambda body itself is clean. Test added: test_listener_adapts_criteria_to_aliased_table_in_joins — reproduces the aliased-join shape using the synthetic ``_SoftDeletableChild`` model. Without the fix, this query raises ``OperationalError: no such column: ..._soft_deletable_child.deleted_at`` because the criteria renders as the raw table name. With the fix, the query runs cleanly. 45 unit tests pass (was 44 + 1 regression test). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
pushed a commit
to mikebridge/superset
that referenced
this pull request
May 28, 2026
…tore (Option C) Same shape as the charts and datasets importer fixes. The dashboard importer on this branch had three bugs from the soft-delete refactor: 1. **Null-user `config["id"]` fallthrough was lost.** The `if overwrite and can_write and user:` gate excluded the example-loader case (ignore_permissions=True, user=None) from id-preservation, so re-import over an existing live row would collide with the UUID unique index. 2. **Hard-delete-and-replace on soft-deleted overwrite.** The code called `clear_soft_deleted_for_import` for soft-deleted matches, cascading through dashboard_slices junctions and role/owner/tag associations. The import then had to reconstruct everything. 3. **Silent return of soft-deleted rows on non-overwrite.** The agent's "silent resurrection" concern — direct re-import returned the soft-deleted dashboard without restoring it, leaving the row hidden in the list. Adopts the Option-C shape from f4768c4 (charts) and 7b2aa39 (datasets): - Any mutation path (overwrite OR soft-delete restore) applies the overwrite path's ownership check (`can_access_dashboard` + owner-or- admin), so non-owners cannot resurrect via re-import. - Soft-deleted match + can_write: restore in place + UPDATE via `config["id"] = existing.id`. Preserves the PK and all relationship rows (slices junctions, owners, roles, tags). - Alive match + non-overwrite: returns existing unchanged (master behavior). - Null user path correctly falls through to the mutation path when `needs_mutation` is true, preserving the example-loader semantics. Four new tests pin the contracts: - test_import_soft_deleted_dashboard_overwrite_restores_in_place - test_import_soft_deleted_dashboard_non_overwrite_restores_for_owner - test_import_soft_deleted_dashboard_non_overwrite_raises_for_non_owner - test_import_soft_deleted_dashboard_ignore_permissions_restores_in_place This should clear the dashboard zip-import playwright failure on apache#40128. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
force-pushed
the
sc-106190-dashboards-soft-delete
branch
from
May 28, 2026 20:08
8c147e5 to
86a6c5b
Compare
…test Adds ``test_restore_via_import_with_slug_rename`` to ``TestDashboardRestore``. Captures the scenario Richard called out on apache#40128 review: 1. Create dashboard A with slug "old". 2. Soft-delete A (via the API). 3. Create dashboard B with slug "old" (only succeeds on partial- index dialects, where the slug is free during the soft-deleted window). 4. Re-import A's UUID with a v1 config that changes the slug to "new". 5. Assert A is restored with slug "new" and ``deleted_at = None``; B is unchanged. Without the fix at ``commands/dashboard/importers/v1/utils.py`` that applies ``config["slug"]`` to ``existing`` before ``db.session.flush()``, step 4 would fail with a partial-index unique-constraint violation at flush time — even though the operator was explicitly trying to dodge the collision by uploading a YAML with a different slug. The test would catch that regression before it shipped. Postgres / MySQL 8.0.13+ only (gated via ``_skip_if_no_partial_index()``); the fallback dialects cannot reach the conflict state because the full unique constraint blocks step 3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…red." in all .po `pybabel update` fuzzy-matches the new exception string against the existing "Dashboard could not be created."/"updated." entries in every catalog, which trips the translation-regression CI check. Pre-seeding an empty `msgstr ""` for the new msgid keeps pybabel from inventing a fuzzy match — translators can fill in the real string later through the normal flow. Affects all 26 active catalogs.
… present Adds first test coverage for migration ``9e1f3b8c4d2a_add_deleted_at_to_dashboards``: * End-to-end exercise of ``upgrade()`` and ``downgrade()`` against an in-memory SQLite engine with a real Alembic ``Operations`` context. * Downgrade-with-soft-deleted-rows-present is pinned: rows that had ``deleted_at`` set before downgrade survive the migration and become indistinguishable from live rows. Matches the "Rollback note" in ``UPDATING.md``; makes the contract regression-proof. * SQLite's no-op branch on the slug-constraint swap is pinned — ``upgrade()`` and ``downgrade()`` must leave the legacy full unique constraint in place (the partial-index path is PostgreSQL / MySQL 8.0.13+ only). Integration tests against those backends cover the partial-index path. * Idempotency on both directions is covered.
…n test Superset is on SQLAlchemy 1.4; ``Connection`` doesn't have a ``commit()`` method until 2.0. The implicit transaction over the single ``with engine.connect() as conn:`` block keeps reads-after-writes visible within the same connection, so the explicit commit was redundant even on 2.0.
…onstraint Production schema seeds the legacy slug uniqueness as a named UNIQUE INDEX (``idx_unique_slug``), and the migration drops it by that name. The test fixture seeded it as a SQLAlchemy ``UniqueConstraint``, which SQLite materializes as a column-level ``UNIQUE`` clause — that does not surface in ``inspect().get_indexes()``, so the pre-condition assertion in ``test_sqlite_slug_constraint_swap_is_a_no_op`` was fooling itself. Replace the ``UniqueConstraint`` with ``Index(..., unique=True)`` so the fixture matches the production shape and the inspector reports it through the same path the no-op assertion checks.
…ect gate - RestoreDashboardCommand: guard the slug-conflict check on 'slug is not None' rather than truthiness so an empty-string slug is also caught instead of falling through to an opaque IntegrityError (codeant). - Make the deleted_at timestamps in the importer and migration tests timezone-aware (tzinfo=timezone.utc) (amin NIT). - _skip_if_no_partial_index: gate MySQL at 8.0.13+ to match the migration (functional key parts land in 8.0.13), not 8.0 (codeant). - Move the skip above row creation in test_restore_blocked_by_active_slug_twin so an unsupported dialect can't strand a soft-deleted dashboard (codeant). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adding tzinfo=timezone.utc pushed the dict literal past the line limit; ruff-format wraps the datetime(...) call. Formatting only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Add a unit test asserting the slug-conflict check runs for an empty-string slug (the 'is not None' guard), per bito's suggestion that the fix lacked a test. - Add 'Dashboard could not be restored.' to messages.pot (the .po placeholders were already seeded in all 26 languages; the template was missing it). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…er Case B Three review fixes (apache#40128): - importer: key Case B on is_soft_deleted, not the fused needs_mutation, so an active dashboard re-imported with overwrite=True but no can_write returns the existing row instead of raising the restore error. Adds regression test. - DashboardDeletedStateFilter: restrict soft-deleted rows to the restore audience (owners/admins), mirroring RestoreDashboardCommand.raise_for_ownership. A read-access non-owner can no longer enumerate soft-deleted dashboards they could never restore; live rows keep normal DashboardAccessFilter visibility. Adds integration coverage (owner sees own deleted; gamma with datasource access does not). - UPDATING.md: correct the slug partial-index dialect note to MySQL 8.0.13+ and add MariaDB to the backends that keep the full unique constraint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Richard's follow-up: MySQL autocommits each DDL statement, so the drop-then-create ordering left the table with no slug uniqueness if the CREATE failed. Reorder both upgrade and downgrade to create the replacement unique index first and drop the previous one only after it succeeds, keeping the stricter existing uniqueness if the DDL fails. Postgres is unaffected (transactional DDL makes its drop+create atomic). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…+ format The rebase landed the dashboard soft-delete migration on down_revision 33d7e0e21daa, but master has since added 31dae2559c05 on top of it, so the branch had two alembic heads -> `superset db upgrade` failed (Multiple head revisions), breaking docker-build(dev) and every dev-image-based test. Re-point down_revision to 31dae2559c05 (master's current head) for a single linear head. Also apply ruff-format to filters.py and the soft-delete integration tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
No-op commit to re-run the workflow; the prior test-load-examples failure was a transient `docker pull postgres:17-alpine` registry timeout during container setup, not a code issue. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Richard's review: dropping column-level unique=True from Dashboard.slug (the
full unique constraint became a partial active-only index for soft-delete) also
removed slug from ImportExportMixin._unique_constraints, so a re-import whose
UUID differs but whose slug matches an existing active dashboard no longer
matched-and-updated that row by slug — it fell through to an insert and collided
on the partial active-slug index at flush (DB error instead of a clean update).
Override Dashboard._unique_constraints to re-add {"slug"} as an import lookup
key while keeping DB-level uniqueness partial. NULL slugs are skipped by the
importer's filter builder, so this only adds a real lookup when the config
carries a slug. Adds regression tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…write import_dashboard fused two distinct operations behind needs_mutation = overwrite or is_soft_deleted. Split into explicit branches: a soft-deleted match is a RESTORE (own can_write gate, ownership check, in-place deleted_at clear + slug pre-apply); an alive match is an OVERWRITE (return existing unless overwrite+can_write, then ownership check). Behaviour unchanged (importer matrix tests still pass). Also adds a restore-failure-path test pinning DashboardRestoreFailedError -> 422. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n check Multi-lens review-panel findings (sqlalchemy + committer + DDD lenses): - migration 9e1f3b8c4d2a: guard the MySQL slug-swap DDL (upgrade and downgrade) with table_has_index. MySQL has no IF [NOT] EXISTS for indexes and autocommits DDL, so an unguarded run on a table missing the legacy index (created inside try/except in 2015's 1a48a5411020) wedged the migration after the partial index had committed. Guards make both directions re-runnable from any partial state. - importer: enforce the active-slug-twin rule at the second restore door. Re-importing an export whose slug was claimed during the soft-deleted window now raises a readable ImportFailedError naming the slug, instead of the flush hitting the partial unique index as an opaque IntegrityError. The predicate now has one home: RestoreDashboardCommand._has_active_slug_twin delegates to DashboardDAO.validate_update_slug_uniqueness, and the importer calls the same DAO method. New unit test pins the collision case. - api: DELETE/bulk-delete OpenAPI docstrings now describe soft-delete semantics and the restore path. - UPDATING.md: note bulk delete is also soft; note the deleted-state filter is owner-scoped for non-admins. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ent master head Master advanced past 31dae2559c05 (now 78a40c08b4be); point the soft-delete migration at the current head so it forms a single linear head against master. Branch/merge sequence unchanged.
- exceptions: drop the inaccurate "RestoreFailedError lives in apache#39977" claim from the DashboardRestoreFailedError comment (it doesn't exist in commands/exceptions.py) — extraction noted as a follow-up (per @aminghadersohi). - daos: validate_slug_uniqueness uses `slug is None` (not `not slug`) so an empty-string slug still runs the uniqueness check, matching validate_update_slug_uniqueness; widen the param to `str | None`. - filters: add a `_normalize` staticmethod on BaseDeletedStateFilter and reuse it in DashboardDeletedStateFilter so the normalization isn't duplicated (and can't drift). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
add_deleted_at_to_dashboards chained off 78a40c08b4be, which was master's head when this branch was cut. Master has since advanced (78a40c08b4be -> a7d3f1b9c2e4 -> 2bee73611e32 via apache#39859), leaving 78a40c08b4be with two children — a forked migration head that failed the DB-conflict check and every DB-dependent CI job. Re-point onto the current single head.
The model comment and migration docstring said "MySQL 8.0+", but the functional index requires MySQL 8.0.13+ (functional key parts landed in 8.0.13) and is disabled on MariaDB — matching _mysql_supports_functional_index. Align both comments with the actual rule.
…tests The soft-delete behaviour is gated behind the temporary SOFT_DELETE rollout flag (superset/daos/base.py, superset/models/helpers.py), which apache#41166 landed on master as off-by-default. These tests were written before the gate and did not enable it, so once rebased onto the gated master every soft-delete assertion failed (rows hard-deleted, deleted-state filters empty, embedded view 404). Decorate each test with @with_feature_flags(SOFT_DELETE=True) so they exercise the gated behaviour.
…check ordering Two fixes from pre-merge review: 1. CreateDashboardCommand defaulted an absent slug to "" before passing it to validate_slug_uniqueness, whose guard deliberately only skips None. Once any dashboard row carried an empty-string slug, every slugless create returned 422 DashboardSlugExistsValidationError — with SOFT_DELETE off, breaking the dark-ship guarantee. Pass the absent slug through as None (matching the update path); an explicit "" is still checked. 2. The importer's restore-via-import branch mutated first (restore() + slug assignment) and validated second — the validation query's autoflush emitted the restoring UPDATE into the partial unique index mid-check, so on Postgres/MySQL 8.0.13+ the readable slug-conflict error was unreachable (opaque IntegrityError instead). Check the effective post-restore slug before mutating, mirroring RestoreDashboardCommand; a failed import now leaves the row untouched, pinned by test.
betodealmeida
approved these changes
Jul 1, 2026
Comment on lines
+86
to
+87
| "Dashboard cannot be restored because its slug is now used by " | ||
| "another active dashboard. Rename one of the dashboards and retry." |
Member
There was a problem hiding this comment.
Love that this is actionable!
Review findings (launch-reviews aggregate, 4 lenses flagged the flag-accuracy gap, 7 flagged the import contract): - UPDATING.md and the delete/bulk_delete OpenAPI docstrings described soft-delete as unconditional, but SOFT_DELETE defaults to False — on a default deployment DELETE hard-deletes permanently. Lead the UPDATING.md section with the flag gate + default, document the flag-toggle caveat (soft-deleted rows reappear when the flag is turned off; restore and the deleted-state filter deliberately stay live), and qualify both endpoint docstrings. - Document the import restore-with-update asymmetry where callers see it: a soft-deleted UUID match is restored and updated even without overwrite=true (deliberate, ownership-gated), unlike active rows which are never mutated without overwrite. Noted in the import endpoint's OpenAPI docstring and expanded in UPDATING.md. - Declare restore-in-place as the canonical soft-deleted re-import pattern in find_existing_for_import's docstring (the shared helper previously prescribed hard-delete-and-replace via clear_soft_deleted_for_import, which has no callers), so charts/datasets adopt uniform semantics. - Add the missing blank line before the UPDATING.md section heading.
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #6e7d54
Actionable Suggestions - 3
-
superset/translations/lv/LC_MESSAGES/messages.po - 1
- Missing Latvian translation · Line 4432-4433
-
superset/migrations/versions/2026-05-08_12-05_9e1f3b8c4d2a_add_deleted_at_to_dashboards.py - 1
- PostgreSQL downgrade not idempotent · Line 185-187
-
tests/integration_tests/dashboards/soft_delete_tests.py - 1
- Timezone-naive datetime call · Line 241-241
Additional Suggestions - 1
-
superset/dashboards/api.py - 1
-
Missing unit tests for restore endpoint · Line 1250-1306The `restore` API endpoint has no unit tests in `tests/unit_tests/dashboards/api_test.py`. Only command-level integration tests exist in `soft_delete_tests.py`. API-level unit tests are needed to verify the endpoint's error handling, response codes, and permission enforcement in isolation.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/daos/dashboard.py - 2
- Empty-string slug now validated · Line 262-262
- Type signature updated to Optional · Line 247-247
-
superset/dashboards/api.py - 1
- Missing permission decorator on restore · Line 1250-1306
-
tests/integration_tests/dashboards/soft_delete_tests.py - 3
- Missing soft-delete before restore call · Line 415-456
- Incomplete test helper stub · Line 328-340
- Test bypasses API layer it claims to test · Line 241-242
Review Details
-
Files reviewed - 44 · Commit Range:
2eeaad0..bf642f3- superset/commands/dashboard/create.py
- superset/commands/dashboard/exceptions.py
- superset/commands/dashboard/importers/v1/utils.py
- superset/commands/dashboard/restore.py
- superset/daos/dashboard.py
- superset/dashboards/api.py
- superset/dashboards/filters.py
- superset/migrations/versions/2026-05-08_12-05_9e1f3b8c4d2a_add_deleted_at_to_dashboards.py
- superset/models/dashboard.py
- superset/translations/ar/LC_MESSAGES/messages.po
- superset/translations/ca/LC_MESSAGES/messages.po
- superset/translations/cs/LC_MESSAGES/messages.po
- superset/translations/de/LC_MESSAGES/messages.po
- superset/translations/en/LC_MESSAGES/messages.po
- superset/translations/es/LC_MESSAGES/messages.po
- superset/translations/fa/LC_MESSAGES/messages.po
- superset/translations/fi/LC_MESSAGES/messages.po
- superset/translations/fr/LC_MESSAGES/messages.po
- superset/translations/it/LC_MESSAGES/messages.po
- superset/translations/ja/LC_MESSAGES/messages.po
- superset/translations/ko/LC_MESSAGES/messages.po
- superset/translations/lv/LC_MESSAGES/messages.po
- superset/translations/messages.pot
- superset/translations/mi/LC_MESSAGES/messages.po
- superset/translations/nl/LC_MESSAGES/messages.po
- superset/translations/pl/LC_MESSAGES/messages.po
- superset/translations/pt/LC_MESSAGES/messages.po
- superset/translations/pt_BR/LC_MESSAGES/messages.po
- superset/translations/ru/LC_MESSAGES/messages.po
- superset/translations/sk/LC_MESSAGES/messages.po
- superset/translations/sl/LC_MESSAGES/messages.po
- superset/translations/th/LC_MESSAGES/messages.po
- superset/translations/tr/LC_MESSAGES/messages.po
- superset/translations/uk/LC_MESSAGES/messages.po
- superset/translations/zh/LC_MESSAGES/messages.po
- superset/translations/zh_TW/LC_MESSAGES/messages.po
- superset/views/filters.py
- tests/integration_tests/base_tests.py
- tests/integration_tests/dashboards/soft_delete_tests.py
- tests/integration_tests/dashboards/superset_factory_util.py
- tests/unit_tests/commands/dashboard/create_test.py
- tests/unit_tests/commands/dashboard/restore_test.py
- tests/unit_tests/dashboards/commands/importers/v1/import_test.py
- tests/unit_tests/migrations/test_add_deleted_at_to_dashboards.py
-
Files skipped - 1
- UPDATING.md - Reason: Filter setting
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Contributor
|
Bito Automatic Review Skipped – PR Already Merged |
This was referenced Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Wire dashboards into the soft-delete + restore infrastructure (from #39977), per SIP-208.
Dashboardnow inheritsSoftDeleteMixin, soDELETEmarks the row with adeleted_attimestamp and hides it from the API instead of removing it, and a new restore endpoint brings it back.What this PR ships:
DashboardinheritsSoftDeleteMixin(superset/models/dashboard.py). The globaldo_orm_executelistener filters soft-deletedDashboardrows out of all SELECTs, including relationship lazy-loads.DeleteDashboardCommandneeds no change —BaseDAO.delete()detects the mixin and routes tosoft_delete()....9e1f3b8c4d2a_add_deleted_at_to_dashboards.py,down_revision = 2bee73611e32) — adds a nullabledeleted_atcolumn +ix_dashboards_deleted_atindex, and replaces the full unique constraintidx_unique_slugwith a partial unique index scoped to active (non-soft-deleted) rows. Fully reversible.RestoreDashboardCommand+POST /api/v1/dashboard/<uuid>/restore(see API CHANGES).DashboardRestoreFailedErrorandDashboardSlugConflictError(422) added.DashboardDeletedStateFilter(arg_name = "dashboard_deleted_state") lets the list endpoint surface soft-deleted rows; opted-in responses are augmented with adeleted_atfield.deleted_atis cleared and the upload's contents are applied in place, preserving the PK and all relationship rows (dashboard_slicesjunctions, role grants, owners, tags). Non-owners and callers withoutcan_writegetImportFailedError.Slug uniqueness across soft-delete
By default a soft-deleted row would keep reserving its
slug, blocking a new dashboard from taking it. The migration's partial unique index frees the slug of soft-deleted rows on PostgreSQL (nativeWHERE deleted_at IS NULLindex) and MySQL 8.0.13+ (functional index). MySQL <8.0.13, MariaDB, and SQLite keep the original full unique constraint, so on those backends a soft-deleted dashboard continues to reserve its slug (documented inUPDATING.md). Restoring a dashboard whose slug has since been claimed returns a clean 422 (DashboardSlugConflictError) instead of an opaque IntegrityError.API CHANGES
POST /api/v1/dashboard/<uuid>/restore— restore a soft-deleted dashboard.can_writeonDashboard(route-level, viamethod_permission_name["restore"] = "write") plus ownership of the row enforced byraise_for_ownership(admins bypass ownership). Same model asDELETE/bulk_delete. Existingcan_writegrants cover it — no role migration.uuid(string, uuid format). The dashboard is located by UUID, skipping the visibility filter but keeping the RBAC base filter.200—{"message": "OK"}(the dashboard object is not returned).403— caller is not an owner/admin (DashboardForbiddenError).404— no such UUID, or the row exists but is not soft-deleted (nothing to restore).422— the dashboard's slug has been claimed by another active dashboard (DashboardSlugConflictError), or the restore failed at flush (DashboardRestoreFailedError).401— unauthenticated.Deleted-state list filter — rison key
dashboard_deleted_stateonGET /api/v1/dashboard/:include— live + soft-deleted rows.only— soft-deleted rows only.Opted-in responses add a
deleted_atfield to each result row. Visibility still respects the existing RBAC base filter, so non-admins only see soft-deleted dashboards they could otherwise see (the same audience that can restore them).DELETE /api/v1/dashboard/<id>andbulk_delete— now soft-delete: the row is markeddeleted_atand hidden from list/detail/lookup (which return 404), recoverable via the restore endpoint. Direct DB queries ondashboardsstill see the row.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI changes — REST endpoints only.
DELETE /api/v1/dashboard/<id>deleted_at; row hidden, recoverableGET /api/v1/dashboard/?...(dashboard_deleted_state=only)deleted_atpopulatedGET /api/v1/dashboard/?...(dashboard_deleted_state=include)deleted_atpopulated on deleted rowsPOST /api/v1/dashboard/<uuid>/restoredeleted_at(200), or 422 if the slug is now takenTESTING INSTRUCTIONS
Automated
QA / manual — enable the
SOFT_DELETEfeature flag first (it is onmastervia #41166, default off); then:DELETE /api/v1/dashboard/<id>→ 200. Confirm it no longer appears inGET /api/v1/dashboard/,GET /api/v1/dashboard/<id>→ 404, and anything that lazy-loads the dashboard no longer dereferences it.GET /api/v1/dashboard/?q=(filters:!((col:id,opr:dashboard_deleted_state,value:only)))→ only soft-deleted rows withdeleted_atpopulated;value:include→ both. As a non-admin, confirm you only see soft-deleted dashboards you own.POST /api/v1/dashboard/<uuid>/restore→200 {"message":"OK"}. Confirm it reappears in the default list andGET /api/v1/dashboard/<id>→ 200.foo). Create dashboard B with slugfoo(succeeds — partial index freed it).POST .../A/restore→ 422DashboardSlugConflictError. Rename B (or A) and retry → 200.dashboard_slicesjunctions / roles / owners / tags preserved, and the importer does not add itself as an owner. As a non-owner, re-import →ImportFailedError. As a caller withoutcan_write, import →ImportFailedError.deleted_at+ix_dashboards_deleted_at+ the partial unique slug index exist andidx_unique_slugis gone (Postgres / MySQL 8.0.13+). Downgrade: confirm the full unique constraint is restored. Before downgrading, ensure no slug is shared between an active and a soft-deleted row, or the constraint re-add aborts.SOFT_DELETEand confirmDELETEhard-deletes as before and the restore endpoint / filter are inert.ADDITIONAL INFORMATION
SOFT_DELETE(merged in feat(soft-delete): gate soft delete behind a temporary SOFT_DELETE release toggle #41166, default off — this PR ships dark)idx_unique_slug, create partial/functional unique index) is fully reversible by the downgrade. Downgrade caveat: if slug reuse occurred during the partial-index window, hard-delete/rename the duplicates before downgrading, or the constraint re-add aborts.dashboardstable. Postgres briefly takesACCESS EXCLUSIVEduring the constraint drop, then blocks writes only during the index build; MySQL 8.0.13+ builds the functional index online. MySQL <8.0.13 / MariaDB / SQLite skip the swap.POST /api/v1/dashboard/<uuid>/restore, thedashboard_deleted_staterison filter,deleted_aton opted-in list responses, and the partial unique slug index (Postgres / MySQL 8.0.13+).Behavior changes worth flagging
DELETEis now soft (whenSOFT_DELETEis on). Consumers expecting permanent removal will see the row reappear if restored; it is hidden from the default API but visible to direct DB queries.Depends on
SOFT_DELETErelease toggle) — merged 2026-07-01 ✅