mikebridge opened a new pull request, #39977: URL: https://github.com/apache/superset/pull/39977
### SUMMARY First of four PRs decomposing the soft-delete work for charts, dashboards, and datasets ([sc-103157](https://app.shortcut.com/preset/story/103157), [SIP-208](https://github.com/apache/superset/issues/39464) — passed 2026-05-08). This PR ships the **infrastructure** with **zero behaviour change**. No model uses the mixin yet, no migration runs, no API surface changes, no permission migrations. Everything in this PR is dormant until the entity-rollout PRs (linked below) opt their concrete entities in. Tracked at [sc-106188](https://app.shortcut.com/preset/story/106188). #### Entity rollouts (forthcoming, depend on this PR) - [sc-106189](https://app.shortcut.com/preset/story/106189) — Soft-delete and restore for **charts** - [sc-106190](https://app.shortcut.com/preset/story/106190) — Soft-delete and restore for **dashboards** - [sc-106191](https://app.shortcut.com/preset/story/106191) — Soft-delete and restore for **datasets** Each entity-rollout PR adds `SoftDeleteMixin` to its model, ships a one-table migration adding the `deleted_at` column + index, wires the entity-specific concrete subclasses of the abstractions in this PR, and adds entity-level integration tests. #### What this PR ships **Runtime mechanism** (`superset/models/helpers.py`, `superset/initialization/__init__.py`) - `SoftDeleteMixin` — adds a nullable `deleted_at` column, `is_deleted` hybrid property, `not_deleted()` filter clause, `soft_delete()` and `restore()` methods. - `_add_soft_delete_filter` — `do_orm_execute` listener using SQLAlchemy's officially-recommended pattern ([Mike Bayer / sqlalchemy#7973](https://github.com/sqlalchemy/sqlalchemy/issues/7973#issuecomment-1112561295)). Globally excludes soft-deleted rows from every ORM SELECT (including relationship lazy loads) for any class inheriting `SoftDeleteMixin`. **No-op until something inherits the mixin.** - Two opt-out paths: - **Per-query** — `execution_options(skip_visibility_filter=True)`. The narrow tool, for non-user-facing code paths (admin tooling, import re-lookups). - **Per-request** — `flask.g.skip_visibility_filter = True`. The broad tool, reserved for user-facing list-endpoint filters that explicitly ask to surface soft-deleted rows for the rest of the request. **BaseDAO surface** (`superset/daos/base.py`, `superset/daos/database.py`) - `BaseDAO.soft_delete(items)` — calls `item.soft_delete()` on each. - `BaseDAO.hard_delete(items)` — calls `db.session.delete(item)` on each (the original `BaseDAO.delete` behaviour, renamed and preserved). - `BaseDAO.delete(items)` — routes to soft for `SoftDeleteMixin`-inheriting models, hard otherwise. **Backwards-compatible**: until any model inherits the mixin, `delete()` continues to call `hard_delete()` exactly as before. - `find_by_id`, `find_by_uuid`, `find_by_ids`, `_find_by_column` gain a `skip_visibility_filter: bool = False` parameter, mirroring the existing `skip_base_filter` parameter. - `DatabaseDAO.find_by_id` is updated for signature compatibility. **Restore abstractions** (`superset/commands/restore.py`, `superset/views/filters.py`, `superset/commands/importers/v1/utils.py`, `superset/constants.py`) - `BaseRestoreCommand[T]` — `T` bound to `SoftDeleteMixin`. Subclasses provide `dao`, `not_found_exc`, `forbidden_exc`. Validates ownership via `security_manager.raise_for_ownership` (identical to existing delete commands; admins always pass). - `BaseDeletedStateFilter` — base class for per-entity rison filters (`*_deleted_state` with values `include` / `only`). Sets `g.skip_visibility_filter = True` to opt the request out of the global filter. - `find_existing_for_import(model_cls, uuid)` — helper for v1 importer pipelines that bypasses the visibility filter on UUID lookup and hard-deletes any soft-deleted match before returning so re-imports don't hit a unique-constraint violation on `uuid`. - `MODEL_API_RW_METHOD_PERMISSION_MAP["restore"] = "write"` — so future per-entity `/restore` endpoints inherit `can_write` (the same permission as `delete`). **No new permission, no role migration**. - `BaseSupersetModelRestApi.pre_get_list` augments list responses with `deleted_at` when the request has opted into seeing soft-deleted rows. #### Design decisions documented in code - **No feature flag** — soft-delete is always-on once a model inherits the mixin. Per [SIP-208 discussion](https://github.com/apache/superset/issues/39464) and the sc-103157 thread, a global flag risks confusing partial states. - **No cascade in V1** — each entity is soft-deleted independently; cascade is out of scope and tracked separately. - **Permission model mirrors delete exactly** — endpoint-level `can_write`, resource-level `raise_for_ownership`. No new permission plumbing. - **`datetime.now()` (naive)** for `deleted_at` to match `AuditMixinNullable.changed_on` precedent ([PR #33693 reverted UTC](https://github.com/apache/superset/pull/33693) on those columns; if/when audit columns move to UTC-aware, soft-delete should follow). - **Compatible with future SQLAlchemy-Continuum adoption** for full entity versioning (confirmed compatible with SQLAlchemy 1.4.54). ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF N/A — backend-only infrastructure with no runtime behaviour change. ### TESTING INSTRUCTIONS ```bash pytest tests/unit_tests/models/test_soft_delete_mixin.py tests/unit_tests/daos/test_base_dao_soft_delete.py -v ``` Expected: 10 passed. The tests use synthetic in-memory models (`_SoftDeletable`) that inherit `SoftDeleteMixin` directly, exercising the listener, mixin methods, and BaseDAO routing without any real Superset entity. Real entities acquire the mixin in the entity-rollout PRs above. Optional sanity check that nothing broke in master behaviour (since `BaseDAO.delete` is now routed): ```bash pytest tests/unit_tests/daos/ tests/unit_tests/models/ -q ``` Expected: all green. ### ADDITIONAL INFORMATION - [X] Has associated issue: [sc-106188](https://app.shortcut.com/preset/story/106188), [SIP-208 #39464](https://github.com/apache/superset/issues/39464) - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration — *(no, migrations land in the entity-rollout PRs)* - [X] Introduces new feature or API — *(adds BaseDAO methods, BaseRestoreCommand, BaseDeletedStateFilter, find_existing_for_import, and `restore` -> "write" mapping; no user-facing API surface yet)* - [ ] Removes existing feature or API #### Reviewer notes A reviewer may reasonably ask: *"why ship abstractions with no callers in master?"* Defenses: 1. **The callers exist** — the three entity-rollout PRs (sc-106189 / sc-106190 / sc-106191) are queued behind this one and each consumes the abstractions. The original mono-PR (#39286) demonstrated all three callers in a single diff; the decomposition just spreads them across reviewable PRs while keeping the abstraction as a single, focused unit. 2. **The unit tests prove the contract.** Synthetic `_SoftDeletable` models exercise the mixin, listener, and BaseDAO routing. The tests would fail if the abstractions were broken. 3. **The git history of the original branch** shows the abstractions emerged from concrete duplication that was reviewed (Bob Martin–style clean-code review of the mono-PR extracted them). This PR is the *reviewed* form, not a speculative one. -- 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]
