mikebridge opened a new pull request, #44184:
URL: https://github.com/apache/superset/pull/44184
### SUMMARY
Soft delete keeps rows in the database, so unique slots held by deleted rows
can block new work with errors that never mention the invisible holder
(sc-107581, part 1):
- **Dashboards (slug)**: migration `9e1f3b8c4d2a` gave PostgreSQL and MySQL
8.0+ a partial unique index scoped to live rows, so a deleted dashboard frees
its slug there. On the full-constraint dialects (SQLite, MariaDB, MySQL <
8.0.13) the insert still dies at flush with a raw `IntegrityError` 422 that
names nobody.
- **Datasets (table reference)**: the create path already refuses with
`DatasetSoftDeletedTwinExistsError` naming the hidden twin and its restore
endpoint, but the **update** path still says only "already exists" about a row
the caller cannot see in any listing.
This PR adds a small shared abstraction,
`superset/commands/soft_delete_collisions.py`:
- `find_soft_deleted_slot_holder(model_cls, *criteria)` — looks up the
deleted holder, bypassing the soft-delete visibility filter with the
session-scoped context manager (a per-query execution option never reaches the
`do_orm_execute` listener) and matching **deleted rows only**, so a live-row
conflict is never misreported as restorable.
- `raise_for_soft_deleted_slug_collision(slug, cause)` — translates a
dashboard-slug collision into a 422 naming the deleted holder's uuid and `POST
/api/v1/dashboard/<uuid>/restore`; returns (so the caller re-raises the
original error unmasked) when there is no slug in play or no deleted holder.
Wired in:
- **Dashboard create/update commands** flush inside `run()` (the DAO only
`session.add`s; without the explicit flush the `IntegrityError` fires at the
`@transaction` decorator's commit, outside any command-level `except`) and
translate the collision. Any integrity failure not caused by a soft-deleted row
re-raises unchanged.
- **Dataset update command** gains the same hidden-twin branch the create
path already has, raising the shared `DatasetSoftDeletedTwinExistsError`; the
dataset PUT handler maps it to a 422 exactly like POST already does.
- **Dataset PUT complexity budget**: the `DatasetRefreshFailedError` /
`DatasetUpdateFailedError` handlers were behaviorally identical (same 422 body;
`logger.exception` ≡ `logger.error(..., exc_info=True)`) and are merged into
one, keeping `put` under the C901 limit with the new except added.
Deliberately **not** changed (already covered on master): dashboard/dataset
importers resolve or restore soft-deleted collisions themselves; dataset
**create** twin guidance; the dashboard partial-index dialects, where a deleted
slug is intentionally reclaimable and no error exists to translate. Part 2 of
the ticket (restore UX surfacing) is out of scope per the dispatch.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
API-only change. Before (full-constraint dialects):
```json
{"message": "Dashboard could not be created."}
```
After:
```json
{"message": {"slug": ["Slug q1-report belongs to the soft-deleted dashboard
<uuid>. Restore it via POST /api/v1/dashboard/<uuid>/restore, or use a
different slug."]}}
```
### TESTING INSTRUCTIONS
- `pytest tests/unit_tests/commands/dashboard/slug_collision_test.py` —
translation with the deleted holder's uuid + restore endpoint and `__cause__`
chained; no-holder returns so the caller re-raises (a conflict NOT caused by a
soft-deleted row is never masked); no-slug skips the probe entirely.
- `pytest tests/unit_tests/commands/dataset/update_test.py` — the update
twin branch raises the shared guidance error; the plain exists case still
raises `DatasetExistsValidationError`.
- `pytest
"tests/integration_tests/dashboards/api_tests.py::TestDashboardApi::test_create_dashboard_slug_held_by_soft_deleted"`
— end-to-end: create, soft-delete, re-create the same slug. The test
introspects whether the schema reserves the slug for deleted rows
(partial-index schemas → 201, slot freed; full-constraint schemas → 422 with
guidance) and, on a `create_all`-built SQLite database (which has neither,
since the model no longer declares the column unique), creates the full unique
index in-test so the collision path is exercised the way a migration-built
database behaves.
- Dataset PUT twin control is unit-level only: the API except mirrors the
POST handler's existing, tested mapping verbatim.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
--
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]
