mikebridge opened a new pull request, #40129: URL: https://github.com/apache/superset/pull/40129
### SUMMARY Wire **charts** 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 `Slice`. #### What this PR ships **Migration** (`superset/migrations/versions/2026-05-08_12-00_7c4a8d09ca37_add_deleted_at_to_slices.py`) - `revision = 7c4a8d09ca37`, `down_revision = 33d7e0e21daa`. Adds a nullable `deleted_at` column and `ix_slices_deleted_at` index to `slices`. Reversible. Uses `superset.migrations.shared.utils` helpers so the migration is idempotent. - **Note**: this PR shares `down_revision = 33d7e0e21daa` with the dashboards (sc-106190 / #40128) and datasets (sc-106191) sibling PRs. Whichever lands second/third will need a rebase or merge migration. **Model + DAO** - `Slice` inherits `SoftDeleteMixin` (`superset/models/slice.py`). The `do_orm_execute` listener from the infrastructure PR begins filtering `Slice` queries (including relationship lazy loads, via `with_loader_criteria(..., propagate_to_loaders=True)`). - `DeleteChartCommand` 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** - `RestoreChartCommand` (`superset/commands/chart/restore.py`) — 4 ClassVar declarations subclassing `BaseRestoreCommand[Slice]`. No method overrides; the base class owns validation, ownership checks, and the transactional wrapper. - `POST /api/v1/chart/<uuid>/restore` endpoint (`superset/charts/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. - `ChartDeletedStateFilter` (`superset/charts/filters.py`) — 2-line subclass of `BaseDeletedStateFilter` setting `arg_name = "chart_deleted_state"` and `model = Slice`. Wired into `search_filters` on `ChartRestApi`. - `ChartRestApi` gains `SoftDeleteApiMixin` and `"restore"` is added to `include_route_methods`. - `ChartRestoreFailedError` exception type added (`superset/commands/chart/exceptions.py`). **Import pipeline** (`superset/commands/chart/importers/v1/utils.py`) - Updates `import_chart` 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/chart/<pk>` | Hard-deletes the row | Sets `deleted_at`; row remains but is hidden from default queries | | `GET /api/v1/chart/?...&chart_deleted_state=only` | Filter doesn't exist (400) | Returns soft-deleted charts with `deleted_at` populated | | `GET /api/v1/chart/?...&chart_deleted_state=include` | Filter doesn't exist (400) | Returns live + soft-deleted; `deleted_at` populated on deleted rows | | `POST /api/v1/chart/<uuid>/restore` | 404 (no such endpoint) | Clears `deleted_at` on the soft-deleted row | ### TESTING INSTRUCTIONS ```bash # Unit tests pytest tests/unit_tests/commands/chart/restore_test.py -v # Integration tests pytest tests/integration_tests/charts/soft_delete_tests.py -v ``` The integration suite covers: - DELETE sets `deleted_at` instead of removing the row - Soft-deleted charts excluded from default list and from individual GET (404) - `chart_deleted_state=only` returns soft-deleted with `deleted_at` populated - `chart_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 charts - Re-import of a soft-deleted UUID via the v1 importer succeeds (and only after permission check) - Charts blocked by an active alert/report don't get soft-deleted (existing protection still applies) ### ADDITIONAL INFORMATION - [X] Has associated issue: [sc-106189](https://app.shortcut.com/preset/story/106189), [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 `slices` table. Even on large deployments, adding a nullable column with no default is instant on Postgres 11+ and MySQL 8+. The index build is the only meaningful cost; on a 1M-row `slices` table, expect a few seconds with `ALGORITHM=INPLACE, LOCK=NONE` (MySQL/InnoDB) or non-blocking on Postgres for a small column. No lock-holding for typical deployments.* - [X] Introduces new feature or API — soft-delete behaviour, `chart_deleted_state` rison filter, `POST /api/v1/chart/<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/chart/<pk>` is now soft.** Existing API consumers that called DELETE expecting permanent removal will see the chart 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 `slices` table will see the row. - **Dashboards referencing a soft-deleted chart** still render their chart slots; the slot shows a missing-chart placeholder (the existing behaviour for any unfound chart). Soft-delete does **not** cascade per SIP-208 V1. #### Depends on - #39977 (infrastructure) #### Coordination with sibling PRs This PR's migration shares `down_revision = 33d7e0e21daa` with: - #40128 (sc-106190 dashboards) — `9e1f3b8c4d2a` - sc-106191 datasets — `3a8e6f2c1b95` 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]
