mikebridge opened a new pull request, #40130: URL: https://github.com/apache/superset/pull/40130
### SUMMARY Wire **datasets** to the soft-delete infrastructure shipped in #39977 (sc-106188). One of three entity rollouts (charts / dashboards / datasets) decomposing the soft-delete work per [SIP-208](https://github.com/apache/superset/issues/39464) (passed 2026-05-08). **Depends on #39977 — do not merge until that PR is in master.** This PR consumes the abstractions (`SoftDeleteMixin`, `BaseRestoreCommand`, `BaseDeletedStateFilter`, `SoftDeleteApiMixin`, `find_existing_for_import` / `clear_soft_deleted_for_import`) and wires them to `SqlaTable`. #### What this PR ships **Migration** (`superset/migrations/versions/2026-05-08_12-10_3a8e6f2c1b95_add_deleted_at_to_tables.py`) - `revision = 3a8e6f2c1b95`, `down_revision = 33d7e0e21daa`. Adds a nullable `deleted_at` column and `ix_tables_deleted_at` index to `tables`. Reversible. Uses `superset.migrations.shared.utils` helpers so the migration is idempotent. - **Note**: this PR shares `down_revision = 33d7e0e21daa` with the charts (sc-106189) and dashboards (sc-106190 / #40128) sibling PRs. Whichever lands second/third will need a rebase or merge migration. **Model + DAO** - `SqlaTable` inherits `SoftDeleteMixin` (`superset/connectors/sqla/models.py`). The `do_orm_execute` listener from the infrastructure PR begins filtering `SqlaTable` queries (including relationship lazy loads, via `with_loader_criteria(..., propagate_to_loaders=True)`). - `DeleteDatasetCommand` routes through `BaseDAO.delete()` — which now detects the mixin and dispatches to `soft_delete()` — so no source change to the delete command is needed. **API** - `RestoreDatasetCommand` (`superset/commands/dataset/restore.py`) — 4 ClassVar declarations subclassing `BaseRestoreCommand[SqlaTable]`. No method overrides; the base class owns validation, ownership checks, and the transactional wrapper. - `POST /api/v1/dataset/<uuid>/restore` endpoint (`superset/datasets/api.py`) with the standard decorator stack (`@protect`, `@safe`, `@statsd_metrics`, `@event_logger.log_this_with_context`). Permission model mirrors delete exactly: endpoint-level `can_write`, resource-level `raise_for_ownership`. UUIDs in the path per the new-endpoints convention. - `DatasetDeletedStateFilter` (`superset/datasets/filters.py`) — 2-line subclass of `BaseDeletedStateFilter` setting `arg_name = "dataset_deleted_state"` and `model = SqlaTable`. Wired into `search_filters` on `DatasetRestApi`. - `DatasetRestApi` gains `SoftDeleteApiMixin` and `"restore"` is added to `include_route_methods`. - `DatasetRestoreFailedError` exception type added (`superset/commands/dataset/exceptions.py`). **Import pipeline** (`superset/commands/dataset/importers/v1/utils.py`) - Updates `import_dataset` to use the infrastructure's `find_existing_for_import` (pure lookup) + `clear_soft_deleted_for_import` (explicit destructive cleanup). The destructive call happens **after** the overwrite + permission check passes, so a soft-deleted match can't be quietly hard-deleted before the import command decides whether it has the right to proceed. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF No UI changes in this PR — REST endpoints only. The frontend "Restore" button is a follow-up PR. | Endpoint | Before | After | |---|---|---| | `DELETE /api/v1/dataset/<pk>` | Hard-deletes the row | Sets `deleted_at`; row remains but is hidden from default queries | | `GET /api/v1/dataset/?...&dataset_deleted_state=only` | Filter doesn't exist (400) | Returns soft-deleted datasets with `deleted_at` populated | | `GET /api/v1/dataset/?...&dataset_deleted_state=include` | Filter doesn't exist (400) | Returns live + soft-deleted; `deleted_at` populated on deleted rows | | `POST /api/v1/dataset/<uuid>/restore` | 404 (no such endpoint) | Clears `deleted_at` on the soft-deleted row | ### TESTING INSTRUCTIONS ```bash # Unit tests pytest tests/unit_tests/commands/dataset/restore_test.py -v # Integration tests pytest tests/integration_tests/datasets/soft_delete_tests.py -v ``` The integration suite covers: - DELETE sets `deleted_at` instead of removing the row - Soft-deleted datasets excluded from default list and from individual GET (404) - `dataset_deleted_state=only` returns soft-deleted with `deleted_at` populated - `dataset_deleted_state=include` returns both, response augmented - POST restore clears `deleted_at` (admin and owner paths) - Restore returns 403 for non-owners (via `raise_for_ownership`) - Restore returns 404 for non-existent or already-live datasets - Re-import of a soft-deleted UUID via the v1 importer succeeds (and only after permission check) - Charts referencing a soft-deleted dataset render an error at chart-load time (V1 documented behaviour) ### ADDITIONAL INFORMATION - [X] Has associated issue: [sc-106191](https://app.shortcut.com/preset/story/106191), [SIP-208 #39464](https://github.com/apache/superset/issues/39464) - [ ] Required feature flags: - [ ] Changes UI - [X] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [X] Migration is atomic, supports rollback & is backwards-compatible (nullable column + index; downgrade reverses both) - [X] Confirm DB migration upgrade and downgrade tested - [X] Runtime estimates and downtime expectations provided — *additive only: nullable column with no default + a single-column index on the `tables` table. Instant on Postgres/MySQL/SQLite for any realistic deployment size; no lock-holding.* - [X] Introduces new feature or API — soft-delete behaviour, `dataset_deleted_state` rison filter, `POST /api/v1/dataset/<uuid>/restore` endpoint, `deleted_at` field on list responses when augmentation is opted into - [ ] Removes existing feature or API #### Behaviour changes worth flagging - **`DELETE /api/v1/dataset/<pk>` is now soft.** Existing API consumers that called DELETE expecting permanent removal will see the dataset reappear if it's restored. The row is no longer findable via the default API after DELETE (the listener hides it), so most downstream behaviour is preserved — but anyone doing direct DB queries on the `tables` table will see the row. - **Cascade behaviour (V1, per SIP-208):** soft-delete does **not** cascade from a dataset to its charts. Charts that reference a soft-deleted dataset render an error at chart-load time — the existing behaviour for any unfound dataset. A future ticket may introduce a "degraded chart" state; that's out of scope here. The integration tests cover this case explicitly so the behaviour can't regress unnoticed. #### Depends on - #39977 (infrastructure) #### Coordination with sibling PRs This PR's migration shares `down_revision = 33d7e0e21daa` with: - sc-106189 charts — `7c4a8d09ca37` - #40128 (sc-106190 dashboards) — `9e1f3b8c4d2a` The first of the three to merge keeps its `down_revision`; the others will need a rebase or a merge migration. -- 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]
