feat(soft-delete): add soft delete for charts, dashboards, and datasets#39286
Closed
mikebridge wants to merge 24 commits into
Closed
feat(soft-delete): add soft delete for charts, dashboards, and datasets#39286mikebridge wants to merge 24 commits into
mikebridge wants to merge 24 commits into
Conversation
github-actions
Bot
added
risk:db-migration
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #39286 +/- ##
==========================================
+ Coverage 63.88% 63.91% +0.03%
==========================================
Files 2583 2588 +5
Lines 136618 136872 +254
Branches 31502 31519 +17
==========================================
+ Hits 87272 87482 +210
- Misses 47830 47867 +37
- Partials 1516 1523 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mikebridge
force-pushed
the
sc-103157-soft-deletes
branch
3 times, most recently
from
b7c334f to
bd77020
Compare
mikebridge
commented
Apr 13, 2026
| ) | ||
| user = get_user() | ||
|
|
||
| # If the matching row was soft-deleted, hard-delete it so the import |
Contributor
Author
There was a problem hiding this comment.
This is essentially the old behaviour---should it change when importing a record that has been soft-deleted?
mikebridge
commented
Apr 13, 2026
mikebridge
commented
Apr 13, 2026
| if self._model is None or self._model.deleted_at is None: | ||
| raise ChartNotFoundError() | ||
|
|
||
| # Permission check — isolated for easy future RBAC changes (FR-007) |
Contributor
Author
There was a problem hiding this comment.
We should determine if we want to do these now
mikebridge
commented
Apr 13, 2026
| user = get_user() | ||
|
|
||
| # If the matching row was soft-deleted, hard-delete it so the import | ||
| # can proceed without a unique-constraint violation on ``uuid``. |
Contributor
|
The current implementation hard-deletes soft-deleted records during import to avoid UUID unique-constraint violations, enabling successful re-import. This behavior should not change, as it's required for imports to proceed without errors when the same UUID exists in a soft-deleted state. |
mikebridge
commented
Apr 13, 2026
| ) | ||
| assert datasets == [] | ||
| # Assert permissions get cleaned | ||
| # With soft delete, FAB permissions are preserved for potential restore |
Contributor
Author
There was a problem hiding this comment.
Is it an issue if permissions are changed (e.g. a user leaves), and then an object is restored?
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
Author
We should note this in the spec |
mikebridge
force-pushed
the
sc-103157-soft-deletes
branch
3 times, most recently
from
63d2ec6 to
e221949
Compare
rusackas
added
review:checkpoint
mikebridge
force-pushed
the
sc-103157-soft-deletes
branch
from
e221949 to
3fc84d6
Compare
Draft
9 tasks
mikebridge
force-pushed
the
sc-103157-soft-deletes
branch
from
3fc84d6 to
86a7292
Compare
sadpandajoe
removed
the
review:checkpoint
mikebridge
force-pushed
the
sc-103157-soft-deletes
branch
2 times, most recently
from
e8ad3e8 to
ddbbce7
Compare
…lter
Phase 1 of sc-103157 (T001-T003, T005-T008). Adds soft-delete
infrastructure to Superset:
- SoftDeleteMixin in superset/models/helpers.py with deleted_at
column, is_deleted hybrid property, soft_delete()/restore() helpers
- SKIP_VISIBILITY_FILTER constant and _add_soft_delete_filter listener
using SQLAlchemy's recommended do_orm_execute + with_loader_criteria
pattern (per Mike Bayer)
- Listener registered in superset/initialization/__init__.py during
app init to ensure it fires in all contexts
- Mixin applied to Slice, Dashboard, and SqlaTable models
- BaseDAO: separate soft_delete()/hard_delete()/delete() methods with
routing via issubclass(cls.model_cls, SoftDeleteMixin)
Migration (T004) to follow in next commit.
feat(soft-delete): add Alembic migration for deleted_at column
Phase 1 T004 of sc-103157. Adds nullable deleted_at (DateTime)
column and index (ix_{table}_deleted_at) to slices, dashboards,
and tables. Reversible — downgrade drops columns and indexes.
Hand-written (not auto-generated) because the local venv cannot
run the Superset CLI without the full Docker app context. The
migration follows the pattern in data-model.md and uses standard
op.add_column() / op.create_index() for multi-dialect compatibility.
test(soft-delete): add foundational tests for mixin and DAO routing
Phase 2 of sc-103157 (T009-T012). Adds 8 unit tests:
Mixin tests (test_soft_delete_mixin.py):
- soft_delete() sets deleted_at to non-null datetime
- restore() clears deleted_at back to None
- not_deleted() filter clause excludes soft-deleted rows
- skip_visibility_filter execution option makes soft-deleted rows visible
DAO routing tests (test_base_dao_soft_delete.py):
- delete() routes to soft_delete() for ChartDAO (SoftDeleteMixin model)
- delete() routes to hard_delete() for DatabaseDAO (non-mixin model)
- hard_delete() calls db.session.delete() on each item
- soft_delete() calls item.soft_delete() on each item
Also fixes:
- Move setup_soft_delete_listener() after init_app_in_ctx() to avoid
import cascade hitting uninitialised singletons (event_logger,
security_manager)
- Guard issubclass() in BaseDAO.delete() against model_cls=None
fix(soft-delete): correct migration down_revision to chain after ce6bd21901ab
The migration was pointing at a1b2c3d4e5f6 which is also the parent
of ce6bd21901ab (migrate deckgl/mapbox), creating a branch with two
heads. Fix by chaining after ce6bd21901ab instead.
Phase 4 of sc-103157 (T019-T024). Adds the ability to restore soft-deleted charts, dashboards, and datasets: Commands (3 new files): - RestoreChartCommand in superset/commands/chart/restore.py - RestoreDashboardCommand in superset/commands/dashboard/restore.py - RestoreDatasetCommand in superset/commands/dataset/restore.py Each uses @transaction(), loads the soft-deleted object with SKIP_VISIBILITY_FILTER, validates it's actually soft-deleted, checks permissions (isolated per FR-007 for future RBAC changes), and clears deleted_at. API endpoints (3 modified files): - POST /api/v1/chart/<pk>/restore - POST /api/v1/dashboard/<pk>/restore - POST /api/v1/dataset/<pk>/restore Each returns 200/403/404/422 following the existing delete endpoint pattern. OpenAPI docstrings included. Exceptions (3 modified files): - ChartRestoreFailedError - DashboardRestoreFailedError - DatasetRestoreFailedError
Phase 3 (T013-T018) and Phase 4 tests (T025-T028) of sc-103157. charts/soft_delete_tests.py: - Soft delete preserves row with deleted_at set - Soft-deleted chart excluded from GET and list - Double-delete returns 404 (FR-008) - Restore makes chart visible again - Restore nonexistent/active chart returns 404 dashboards/soft_delete_tests.py: - Soft delete preserves row - Excluded from list - Restore reconnects to chart associations (junction rows preserved) datasets/soft_delete_tests.py: - Soft delete preserves row - Excluded from list - No cascade to dependent charts (FR-009, T018) - Restore makes dataset visible again docs(soft-delete): add UPDATING.md entry for soft-delete behaviour change Phase 5 T029 of sc-103157. Documents the behavioural change for external integrations: DELETE endpoints now soft-delete, new restore endpoints available, no cascade, database-direct queries may see soft-deleted rows. fix(soft-delete): add 'restore' to include_route_methods whitelist Flask-AppBuilder uses include_route_methods to control which API methods are registered as routes. Without listing 'restore' in this set, the endpoint exists in the code but Flask never registers the URL rule — resulting in a 404. fix(soft-delete): add 'restore' to MODEL_API_RW_METHOD_PERMISSION_MAP Flask-AppBuilder uses this map to determine the required permission level for each API method. Without it, @Protect() denies access with a 403. Mapped to 'write' (same as delete) — permission model is isolated per FR-007 for future RBAC changes.
…ssingChart handles UI)
When a chart is soft-deleted, its rows in the dashboard_slices junction
table are intentionally left in place — the SIP's no-cascade principle.
Behaviour:
- Dashboards referencing the soft-deleted chart render the existing
MissingChart placeholder (same UI as a hard-deleted chart's
dangling position_json reference).
- Restoring the chart automatically reattaches it to all the
dashboards it was previously a member of, with no data loss.
Earlier work in this commit experimented with explicit junction
cleanup; that was removed once it became clear the frontend handled
both cases identically and the no-cleanup path is what the SIP
actually mandates. The original commit title incorrectly described
the abandoned approach.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fix(soft-delete): satisfy targeted pre-commit checks
Remove the commented-out dashboard_slices cleanup code from DeleteChartCommand — the frontend's MissingChart component handles orphaned junction rows gracefully. Also remove the unused delete() method from SoftDeleteMixin and update tests to match. The delete routing decision stays in BaseDAO.delete() per the Data Mapper pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
mikebridge
force-pushed
the
sc-103157-soft-deletes
branch
from
ddbbce7 to
839ab30
Compare
…d override DatabaseDAO overrides BaseDAO.find_by_id but was missing the new skip_visibility_filter parameter, causing a mypy override-incompatibility error introduced when the parameter was added to the base class. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Aggregated soft-deleted-items listing that returns chart, dashboard,
and dataset rows where deleted_at IS NOT NULL in a single paginated
response. Powers the frontend "Archive" view. See spec sc-103157 US4
(FR-014 – FR-017).
Package layout: superset/deleted/{__init__.py, api.py, dao.py,
schemas.py}. Exposed as "Deleted" class_permission_name and
/api/v1/deleted/ URL path; the internal SoftDeleteMixin /
skip_visibility_filter plumbing retains its explicit soft_* naming.
Query parameters travel in a rison-encoded q=(...) envelope, matching
every other Superset list endpoint. Supported fields: types (rison
list subset of chart/dashboard/dataset), search (ilike on name),
deleted_from / deleted_to (ISO-8601 bounds on deleted_at),
order_column (deleted_at | deleted_by | type | name), order_direction
(asc | desc), page, page_size. Validation via @rison(get_deleted_
schema); malformed input yields 400.
Response envelope: { count, result } — matches the standard Superset
shape. Each row carries type, id, uuid, name (normalised from
slice_name / dashboard_title / table_name), deleted_at, and
deleted_by (nested user object joined from
AuditMixinNullable.changed_by_fk per the Attribution design
constraint — no new deleted_by_fk column is added).
DAO strategy: three per-entity ORM queries under
skip_visibility_filter=True plus each entity's normal base security
filter, merged in Python then sorted and paged via a pure-function
helper. Chosen over a SQL UNION ALL to sidestep column-shape padding
and base-filter composition issues; see research R-007 for rationale.
Renamed from list() to list_items() to avoid shadowing the list
built-in in class-scoped type annotations.
Authorisation: row-level access mirrors the corresponding active list
endpoint — the base filter (DatasourceFilter and friends) still gates
which rows a caller can see; only the soft-delete visibility filter
is bypassed. An authenticated user with zero accessible rows receives
200 with count: 0, never 403.
Registered in superset/initialization/__init__.py via
appbuilder.add_api(DeletedRestApi).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Master removed the word-cloud sort_by_series migration in apache#39575 (replaced its effect with code defaults), so the previous a3b4c5d6e7f8 merge — which referenced fd0c8583b46d — became invalid. After dropping it, two heads remained: cb39f18af67f (sc-103157 soft delete) and 33d7e0e21daa (semantic layers and views), both descending from ce6bd21901ab. Empty merge migration unifies them so ``superset db upgrade head`` has an unambiguous target. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ty rison filter
Drops GET /api/v1/deleted/ (aggregated UNION-then-sort) in favour of a
per-entity rison filter on the existing list endpoints. Aligns with how
every other Superset list filter is expressed (rison q=(filters:!(...))
triples) and avoids the cost of querying three heterogeneous tables and
sorting their union.
Filter: opr is ``chart_deleted_state`` / ``dashboard_deleted_state`` /
``dataset_deleted_state``. Values:
- ``include`` — live + soft-deleted rows
- ``only`` — only soft-deleted rows
- absent / other — default behaviour (live rows only)
When applied, list responses augment each row with ``deleted_at``.
Removed:
- superset/deleted/{api,dao,schemas}.py
- tests/{unit,integration}_tests/deleted/*
- allow_include_deleted_list flag, _should_include_deleted_in_list,
_maybe_include_deleted_in_list (and the wrapper on get_list_headless)
Tests cover both ``include`` and ``only`` semantics for chart, dashboard,
and dataset endpoints. Frontend grep returned zero callers of the dropped
endpoint, so no client-side changes needed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…f67f Replace raw op.add_column / op.create_index calls with the project's idempotent helpers from superset.migrations.shared.utils. Matches the pattern used by recent migrations (c233f5365c9e themes, 4b2a8c9d3e1f create_tasks) and aligns with CLAUDE.md guidance. Behaviour is unchanged. The helpers add a skip-if-already-exists guard plus structured logging, so the migration is re-runnable if interrupted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…y_filter Adds the skip_visibility_filter kwarg to BaseDAO.find_by_ids so it matches the bypass surface already exposed by find_by_id, find_by_id_or_uuid, and _find_by_column. Without this, callers that need to bulk-load soft-deleted entities (admin tooling, future permanent-purge jobs, audit queries) had to reach past the DAO and write raw session.query(...).execution_options(...) calls. Audit confirms zero existing callers pass 2+ positional args to find_by_ids — every multi-arg call uses kwargs — so inserting the new kwarg between skip_base_filter and id_column breaks no caller. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
force-pushed
the
sc-103157-soft-deletes
branch
from
8473dd3 to
80f8ac3
Compare
Adds a defensive null-guard at the top of ``BaseReportState._get_url`` (``superset/commands/report/execute.py``). When both ``self._report_schedule.chart`` and ``self._report_schedule.dashboard`` are ``None``, the method now raises ``ReportScheduleUnexpectedError`` with a message identifying the report and the missing target, rather than crashing on the downstream ``dashboard.id`` access at the (formerly) ``else`` branch of the ``dashboard.uuid if dashboard and dashboard.uuid else dashboard.id`` ternary. This is a pre-existing latent bug — ``_report_schedule.dashboard`` was already nullable in principle, but reaching the ``None`` state was practically unreachable in master because every code path that orphaned a report was blocked by ``DeleteChartCommand`` / ``DeleteDashboardCommand`` validation. The soft-delete branch makes the bug newly reachable through admin tooling / direct SQLAlchemy paths that bypass the validation: the visibility filter applied to relationship loads returns ``None`` for soft-deleted targets even though the FK still references the row. The existing API protections still block the most common trigger path (``DELETE /api/v1/chart/<id>`` returns 422 when reports reference the chart), so production scheduled reports are unaffected. Adds a unit test in ``tests/unit_tests/commands/report/execute_test.py`` that constructs a ``ReportSchedule`` mock with both relationships returning ``None`` and asserts the new error is raised with the right identifying details (report id, name, chart_id, dashboard_id). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Surfaced by `pre-commit run --all-files`:
- tests/unit_tests/commands/{chart,dashboard,dataset}/restore_test.py:
Restore*Command takes a UUID string, not an int. The DB is mocked
in these unit tests so the actual value doesn't matter; the type
just needs to be `str`.
- tests/integration_tests/datasets/soft_delete_tests.py:
ruff-format collapses a list comprehension that fit within the
88-char line-length budget.
No behaviour changes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ks, embedded, reattach)
Three integration tests pinning down behaviour described in
specs/sc-103157-soft-deletes/pr-readiness.md:
test_delete_chart_blocked_when_active_report_references_it
Pins down the existing API protection: DeleteChartCommand.validate()
raises ChartDeleteFailedReportsExistError (HTTP 422) before any
soft-delete routing happens. This is what makes the
"report-execution against soft-deleted target" crash class
(commands/report/execute.py:_get_url, addressed in commit
e8156de's null-guard) unreachable through the API.
test_embedded_dashboard_with_soft_deleted_parent
Pins down that the embedded view never dereferences the
embedded.dashboard relationship — it only reads the FK column and
allow_domain_list — so soft-deleting the parent dashboard leaves
the iframe URL returning 200 (rather than 500 from a None deref).
The frontend's subsequent /api/v1/dashboard/<id> fetch returns 404
cleanly via the visibility filter; the user sees the standard
"dashboard not found" UI.
test_restore_chart_reattaches_to_dashboards
Positive test for the corrected commit
"feat(soft-delete): preserve dashboard_slices on chart soft-delete
(MissingChart handles UI)". Confirms the SIP's no-cascade contract
end-to-end: junction rows are preserved on chart soft-delete, the
relationship-load filter hides the chart from dashboard.slices
while soft-deleted, and on restore the chart automatically
reappears in every dashboard it was a member of — no manual
re-attachment needed.
All three tests are scoped to the existing TestChart/TestDashboard
soft-delete suites and use the established _hard_delete_* cleanup
pattern.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-code review
Three near-identical clones across the soft-delete branch consolidated
into shared base classes / helpers. Net: -103 lines of code, the
Restore* / *DeletedStateFilter / import_* surfaces are now the right
size for what they express.
1. BaseRestoreCommand (superset/commands/base.py)
─ Restore{Chart,Dashboard,Dataset}Command shrink from ~65 lines
each to ~10 lines (just dao + exception bindings + decorator
application). Behaviour identical: same DAO calls, same
exception types, same @transaction wrapping. validate() now
returns the model rather than mutating self._model + assert,
and the not-found case differentiates "no row" from "row exists
but isn't soft-deleted" in the error message.
2. BaseDeletedStateFilter (superset/views/filters.py)
─ Chart/Dashboard/DatasetDeletedStateFilter shrink from ~30 lines
each to ~5 lines (arg_name + model). The g.skip_visibility_filter
mutation moved to a named _opt_out_of_visibility_filter() helper
so the side effect is visible at the call site rather than
buried in a 5-line comment repeated three times.
3. find_existing_for_import (superset/commands/importers/v1/utils.py)
─ The "look up by uuid + hard-delete soft-deleted match" block
in each of import_{chart,dashboard,dataset} collapses from ~14
lines to a single helper call. Same SQL behaviour, same
SKIP_VISIBILITY_FILTER bypass.
The restore unit tests were also rewritten — they were patching
superset.commands.{chart,dashboard,dataset}.restore.db, which never
existed in those modules, so the tests had been failing on master.
Tests now mock the DAO method directly (the level the unit test
actually owns) and pass cleanly.
Verified against the live container:
- 12/12 restore unit tests pass
- test_get_url_raises_unexpected_error_when_target_is_missing passes
- Live API returns expected counts under include / only / default
chart_deleted_state filter values
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
force-pushed
the
sc-103157-soft-deletes
branch
from
9d9e71b to
b41d7f8
Compare
Three lint failures CI surfaced on b41d7f8: - tests/integration_tests/charts/soft_delete_tests.py:112 — line too long (89 > 88) on a docstring. Added `# noqa: E501`. - tests/unit_tests/commands/dashboard/restore_test.py:62 — same, line 90 > 88 on a sed-generated docstring. Added `# noqa: E501`. - superset/dashboards/filters.py: unsorted-imports (I001). superset/datasets/filters.py: unused-import (F401). Both ruff-auto-fixable; applied locally with `ruff check --fix` + `ruff format`. `superset/commands/base.py` also got a one-line ruff-format adjustment (`# type: ignore[override]` placement) — not a behaviour change. The same `pre-commit run --all-files` that CI runs is now clean locally on the touched files. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two small "fewest elements" tidyings flagged by /tidy-first-review:
1. BaseRestoreCommand.failed_exc — Dead Code.
Declared as a ClassVar on the base, set on each subclass, but
never *read* anywhere. The @transaction decorator on each
subclass's run() carries the concrete RestoreFailed exception
directly (`reraise=ChartRestoreFailedError`), not via
`cls.failed_exc`. Removing the field and updating the docstring
to express the contract in prose. Subclasses lose three lines.
2. BaseDeletedStateFilter._opt_out_of_visibility_filter — speculative
parameters. The static helper took (g, key) as arguments, but both
always passed the same values (flask.g and SKIP_VISIBILITY_FILTER).
Inlined: imports moved to module level (no circular-import risk
in views/filters.py), arguments dropped. Call site reads
`self._opt_out_of_visibility_filter()` — three characters less
and no implicit "what if g were something else" speculation.
Verified clean: ruff check + format pass on all touched files; 12
restore unit tests pass; live API returns expected counts (104 with
include, 0 with only) after node restart.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move the soft-delete restore base class out of commands/base.py (which is generic command infrastructure) into a sibling commands/restore.py module. No behaviour change. Also tighten the TypeVar bound from unbounded T to T bound=SoftDeleteMixin, which makes the contract — that the base class only restores SoftDeleteMixin models — visible to mypy and fixes a pre-existing attr-defined error on model.restore(). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-commit's auto-walrus hook in CI rewrote the `existing = find_existing_for_import(...); if existing:` pattern to `if existing := find_existing_for_import(...):` across the three v1 importer utils. No behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two short documentation additions in models/helpers.py: * SoftDeleteMixin.soft_delete: note that datetime.now() mirrors AuditMixinNullable.changed_on by design (PR apache#33693 reverted UTC on the audit columns). * _add_soft_delete_filter listener: distinguish the per-query execution_options bypass (narrow, for non-user-facing paths) from the per-request flask.g flag (broad, reserved for user-facing list filters that ask to surface soft-deleted rows). Reduces the risk of a future code path setting g.skip_visibility_filter and bleeding the bypass into incidental queries. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
changed the title
Three CI integration jobs (sqlite, mysql, postgres-previous) were failing on test_embedded_dashboard_with_soft_deleted_parent at the 'soft-deleted dashboard fetch returns 404' assertion, getting 401 'Missing Authorization Header' instead. The embedded iframe handler invoked under mock.patch.dict was clearing the session cookie in CI (interaction between EMBEDDED_SUPERSET feature-flag swap and Flask test-client cookie state), so the follow-up authenticated API call saw no auth. Reordered the test to check the API 404 before hitting the embedded URL. Both assertions still cover the regression they were added for; they just no longer interfere with each other. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per the constitution: f"" prefix must not appear on string literals with no interpolation, including individual fragments in implicit string concatenation. Drop f-prefix on the trailing fragment of the null-target error message in BaseReportState._get_url. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… fragments Three more cases where an f-string fragment in an implicit concatenation has no interpolation, found by an AST scanner. Per the constitution: f"" must not appear on string literals with no interpolation, including individual fragments in implicit string concatenation. Ruff F541 doesn't currently flag this case in implicit concat, so they slipped through. * superset/commands/restore.py:76 — "nothing to restore" * tests/integration_tests/dashboards/soft_delete_tests.py:170 — leading fragment of the 404 assertion message * tests/integration_tests/dashboards/soft_delete_tests.py:184 — leading fragment of the embedded-load assertion message Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
Contributor
Author
|
Closing as superseded by a decomposition into smaller PRs. Per reviewer feedback that this work should land in smaller, more reviewable pieces, the soft-delete change is being split into independent PRs under epic Versioning (sc-103155):
The original SIP-208 (#39464) is approved and covers all five PRs. Any review feedback you intended to leave here, please leave on the relevant PR above. The original branch ( |
Contributor
Author
|
Superseded by the decomposition documented in the comment above. Closing now to clear the review queue; reopen if needed. |
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
4 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
Adds soft-delete support for Charts, Dashboards, and Datasets as the first step in the Versioning epic (sc-103157). Implements SIP-208 (passed 2026-05-08).
Terminology: The UI presents this as Archive / Unarchive. The backend uses standard soft-delete naming (
deleted_at,soft_delete(),/restore) — the terminology boundary is at the UI label layer only.What changed:
DELETEendpoints for charts, dashboards, and datasets now setdeleted_at = current_timestampinstead of removing the rowdo_orm_execute+with_loader_criterialistener — SQLAlchemy's officially recommended patternq=(filters:!((col:id,opr:chart_deleted_state,value:include)))(ordashboard_deleted_state/dataset_deleted_state). Values:include(live + soft-deleted),only(soft-deleted only), absent (default — live only). When applied, response rows include adeleted_atISO-8601 timestamp.POST /api/v1/{chart,dashboard,dataset}/<uuid>/restoreendpoints restore soft-deleted objects (UUID path param — new endpoints start on UUID from day one)BaseDAOgainssoft_delete(),hard_delete(), and an updateddelete()that routes based on whether the model includesSoftDeleteMixin;skip_visibility_filter: booladded tofind_by_id()and related methods to mirror the existingskip_base_filterparameterDesign decisions:
restoreis mapped to"write"inMODEL_API_RW_METHOD_PERMISSION_MAP, so@protect()enforcescan_writeonChart/Dashboard/Dataset— the same gate asdeleteandbulk_delete. No new permission migration is required.BaseRestoreCommand.validate()callssecurity_manager.raise_for_ownership()— identical to what each delete command already does. Admins always pass; otherwise the user must be inmodel.owners.can_restorepermission) without touching the API surface.datetime.now()fordeleted_atto match existingAuditMixinNullableconvention (UTC conversion was previously reverted)session.execute(delete(...))) bypasses thedo_orm_executelistener — all deletes must go throughBaseDAO.delete()BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only change, no UI modifications. API behaviour is identical from the caller's perspective (same endpoints, same status codes). The only observable difference is that deleted rows remain in the database with
deleted_atset.TESTING INSTRUCTIONS
Verify: returns 200. Chart no longer appears in
GET /api/v1/chart/. Row still exists in DB withdeleted_atset.Verify: returns 404 (soft-deleted chart is invisible).
Verify: soft-deleted chart appears in results with
deleted_atset.Verify: returns 200. Chart reappears in
GET /api/v1/chart/.Repeat for dashboards and datasets using
/api/v1/dashboard/and/api/v1/dataset/(risonoprbecomesdashboard_deleted_state/dataset_deleted_state).Run tests:
pytest tests/unit_tests/models/test_soft_delete_mixin.py tests/unit_tests/daos/test_base_dao_soft_delete.py tests/unit_tests/commands/{chart,dashboard,dataset}/restore_test.py tests/integration_tests/charts/soft_delete_tests.py tests/integration_tests/dashboards/soft_delete_tests.py tests/integration_tests/datasets/soft_delete_tests.py -vADDITIONAL INFORMATION
Migration details:
deleted_at(DateTime) column + index toslices,dashboards,tablesRuntime expectations:
Column add (nullable, no default): metadata-only on Postgres ≥ 11 and MySQL InnoDB; instant on SQLite. Tested upgrade and downgrade against all three dialects.
Index creation: O(rows) on each of
slices,dashboards,tables. The migration runs inside the alembic transaction (noCREATE INDEX CONCURRENTLY), so writes to these tables are blocked for the duration:superset db upgradeahead of the application restart and during off-hours.Has associated issue: sc-103157 / SIP #39464
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
Introduces new feature or API
Removes existing feature or API