feat(core): SoftDeleteMixin and restore infrastructure#39977
feat(core): SoftDeleteMixin and restore infrastructure#39977mikebridge wants to merge 21 commits into
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #39977 +/- ##
==========================================
- Coverage 64.14% 64.03% -0.12%
==========================================
Files 2591 2593 +2
Lines 138247 138852 +605
Branches 32067 32154 +87
==========================================
+ Hits 88684 88914 +230
- Misses 48033 48399 +366
- Partials 1530 1539 +9
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:
|
|
The explanation accurately describes the code in superset/commands/importers/v1/utils.py, where the per-query execution_options(**{SKIP_VISIBILITY_FILTER: True}) bypasses the visibility filter only for the UUID lookup in find_existing_for_import, allowing access to soft-deleted rows while keeping other entity queries filtered. |
|
@mistercrunch @michael-s-molina @betodealmeida @eschutho @sadpandajoe @kgabryje: I lack the ability to add you as reviewers, so I'm tagging you instead. |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #096e88
Actionable Suggestions - 1
-
superset/models/helpers.py - 1
- datetime.now() called without timezone · Line 695-700
Additional Suggestions - 1
-
superset/models/helpers.py - 1
-
Missing Return Type Hint · Line 707-707The repository rule [7819] requires explicit return type hints for all functions and methods, including when they return nothing. The new _add_soft_delete_filter function lacks a return type hint, violating this policy.
Code suggestion
@@ -707,1 +707,1 @@ -def _add_soft_delete_filter(execute_state): # type: ignore +def _add_soft_delete_filter(execute_state) -> None: # type: ignore
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/daos/database.py - 1
- Missing skip_visibility_filter implementation · Line 72-72
Review Details
-
Files reviewed - 11 · Commit Range:
d54e7cc..d54e7cc- superset/commands/importers/v1/utils.py
- superset/commands/restore.py
- superset/constants.py
- superset/daos/base.py
- superset/daos/database.py
- superset/initialization/__init__.py
- superset/models/helpers.py
- superset/views/base_api.py
- superset/views/filters.py
- tests/unit_tests/daos/test_base_dao_soft_delete.py
- tests/unit_tests/models/test_soft_delete_mixin.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Code Review Agent Run #915a41Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Posting on Richard's behalf — this is his PR reviewer agent. Forward any pushback to him and he'll loop me back in.
Left a few notes below — the main things worth checking before merge are around the visibility opt-out boundaries and the restore lookup path. All line numbers verified against HEAD 9c83215.
Functional — worth checking before merge
-
superset/views/filters.py:164BaseDeletedStateFilteropts out by settingg.skip_visibility_filter, and the global listener reads that flag for every ORM SELECT later in the same request. Once more than one entity adoptsSoftDeleteMixin, a list request that asks for deleted dashboards/charts/datasets could also make incidental relationship loads, serializer lookups, or helper queries expose soft-deleted rows for other models in the rest of that request.WDYT — could this use the query-scoped execution option for the primary list query, and keep any response-annotation state separate from the visibility bypass?
-
superset/commands/restore.py:79BaseRestoreCommand.validate()correctly finds the soft-deleted row withskip_visibility_filter=True, butraise_for_ownership()re-loads the same model throughself.session.query(resource.__class__).get(resource.id)without that opt-out. The global listener will hide the soft-deleted row there, so non-admin owners can be denied restore even when they own the row; admins bypass this path, so it may be easy to miss without owner-level restore coverage.WDYT — would it be worth making the ownership check soft-delete-aware for this command, or adding a restore-specific ownership helper/test so owners can restore their own deleted resources?
-
superset/daos/database.py:82DatabaseDAO.find_by_id()now acceptsskip_visibility_filter, but this override builds the query without applying the execution option. Any restore/admin path that routes through the override will still be filtered even though it passedskip_visibility_filter=True.Small suggestion: could we mirror
BaseDAO.find_by_id()here and applyquery.execution_options(**{SKIP_VISIBILITY_FILTER: True})when the flag is set?
Compatibility / follow-up
-
superset/daos/base.py:299Adding
skip_visibility_filterbefore the existingid_columnandquery_optionsparameters changes the meaning of positional third/fourth arguments to a public DAO helper. I did not find an in-tree positional caller in the changed checkout, but Superset extensions and downstream code may still call this positionally.Totally optional if the team treats this helper as keyword-only in practice, but could we append the new flag after existing parameters or make it keyword-only to preserve the current positional ABI?
Praise
-
superset/models/helpers.py:739The core listener uses SQLAlchemy's
do_orm_executepluswith_loader_criteriapattern and already has a narrow per-query escape hatch, which seems like the right primitive for restore/import/admin flows once the broader request-level bypass is tightened.
Two real bugs in the infrastructure surface, both flagged by codeant-ai-for-open-source on PR apache#39977. Both are fixed in this commit with documentation and a new unit test. 1. raise_for_ownership re-query was filtered by the listener BaseRestoreCommand.validate calls security_manager.raise_for_ownership, which re-queries the row internally to fetch its current owners. That re-query goes through the global do_orm_execute listener; for a soft-deleted row the listener filters it out, raise_for_ownership sees no row, falls back to an empty owners list, and raises MISSING_OWNERSHIP_ERROR for any non-admin user. The only callers who could restore were admins. Fix: set g.skip_visibility_filter = True for the scope of the security check, restoring the previous value afterward. This is the documented per-request opt-out path for cases like this where the bypass must propagate to a function we don't directly own. 2. Listener missed the documented loader-path guards SQLAlchemy's recommended soft-delete pattern at github.com/sqlalchemy/sqlalchemy/issues/7973 guards the criteria application with not execute_state.is_column_load and not execute_state.is_relationship_load. Without these, every lazy/eager relationship load re-applies the criteria, stacking redundant deleted_at IS NULL clauses on the loader's query. Adds both guards. Tests * New tests/unit_tests/commands/test_restore.py exercises the BaseRestoreCommand contract directly: - flag is set to True during the security check - flag is restored to its previous value afterward - flag is restored even when the security check raises The previous entity-level tests mocked raise_for_ownership directly and never exercised the real re-query path, so they masked the bug. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three changes addressing items flagged in apache#39977 (review) 1. DatabaseDAO.find_by_id ignored the new flag The override accepted skip_visibility_filter for signature compatibility but never applied it to the query. Any caller of the override that passed skip_visibility_filter=True still got filtered results. Now applies execution_options when the flag is True, matching BaseDAO.find_by_id. 2. skip_visibility_filter moved to keyword-only The original signature inserted skip_visibility_filter at position 3 in BaseDAO.find_by_id / find_by_ids / find_by_id_or_uuid / _find_by_column, shifting id_column and query_options. Downstream positional callers (extensions, library consumers) would silently bind the new parameter to a different intent. Moved to the end and made keyword-only across all four methods plus the DatabaseDAO override; existing in-tree callers all use keyword form already. 3. BaseDeletedStateFilter scope tightened Previously set g.skip_visibility_filter = True (broad, per-request), which is fine in isolation but would leak soft-deleted rows of unrelated entities once multiple models adopt SoftDeleteMixin: a list request for soft-deleted dashboards would also bypass the filter for incidental Slice / SqlaTable relationship loads. Two changes decouple the concerns: - The visibility-filter bypass is now applied per-query on the primary list query via .execution_options(skip_visibility_filter= True). Affects only the list query for this entity. - Response augmentation (adding deleted_at to result rows) is signalled via a separate request-scoped flag, g._augment_response_with_deleted_at. Distinct from the bypass. pre_get_list now checks the augmentation flag instead of the bypass flag. Listener docstring updated to clarify the narrower per-request use case: the broad flag is now reserved for cases where a function we don't directly control performs an internal re-query that must see the soft-deleted row (e.g., security_manager.raise_for_ownership). The previously-claimed use case (list-endpoint rison filters) has moved to the per-query option. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@richardfogaca thanks for the careful review — all four addressed in
Listener docstring updated to reflect the narrower documented use case for the per-request flag: it's now reserved for cases like wrapping The "Praise" item also stays accurate — the |
|
After some discussion with AI about the maintainability of the official side-effecting pattern, I'd like to create a follow-up PR after this one to wrap the per-request bypass in a context manager. The issue here is that it's easy for a dev to forget to reset the flag in try/catch:
Both cases can be eliminated by a small context manager next to the @contextmanager
def skip_visibility_filter() -> Iterator[None]:
"""Opt the current request out of the soft-delete listener for the
duration of the block. Restores the previous value on exit,
including the exception path. Use only when the bypass must
propagate through a function whose internal queries are not under
your control; for queries you build directly, prefer the per-query
``execution_options(skip_visibility_filter=True)``.
"""
previous = getattr(g, SKIP_VISIBILITY_FILTER, False)
setattr(g, SKIP_VISIBILITY_FILTER, True)
try:
yield
finally:
setattr(g, SKIP_VISIBILITY_FILTER, previous)Call site in BaseRestoreCommand.validate becomes: with skip_visibility_filter():
try:
security_manager.raise_for_ownership(model)
except SupersetSecurityException as ex:
raise self.forbidden_exc() from exI also think there should be a primer document somewhere on how the two bypasses work and why they are there, because it is not obvious. |
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Posting on Richard's behalf — this is his PR reviewer agent. Forward any pushback to him and he'll loop me back in.
Left a few notes below — the import hard-delete cleanup path and the deleted-state visibility contract look like the main things to tighten before the entity rollout PRs depend on this infrastructure. All line numbers verified against HEAD c499e318.
Functional — worth addressing before entity rollout
-
superset/commands/importers/v1/utils.py:430The replacement path for a soft-deleted import match uses a Core table
DELETE, which bypasses the ORMafter_deletelisteners that currently own cleanup for the rollout entities. That matters because tags are cleaned up byObjectUpdater.after_delete(superset/tags/core.py:38,:42,:46;superset/tags/models.py:278-289), and the physicaltagged_object.object_idmigration is just an integer (superset/migrations/versions/2018-07-26_11-10_c82ee8a39623_add_implicit_tags.py:84-89), so DB cascades will not remove those rows. Datasets also route permission cleanup throughSqlaTable.after_delete(superset/connectors/sqla/models.py:2115-2123).WDYT — could this use an existing hard-delete path that still fires the relevant cleanup, or explicitly run the tag/permission cleanup before the direct delete?
-
superset/views/filters.py:162BaseDeletedStateFilteropts the whole ORM statement out of the soft-delete listener with a booleanskip_visibility_filter; theonlybranch does the same atsuperset/views/filters.py:165. The listener then skips criteria for everySoftDeleteMixinsubclass in that statement (superset/models/helpers.py:733-759), not justself.model, so a future list query that joins multiple soft-deletable entities could surface unrelated deleted rows while only asking for deleted state on one entity.Would it be worth making the bypass model-scoped, or adding an explicit non-deleted predicate back for other soft-deletable models participating in the query?
-
superset/views/filters.py:160The deleted-state filter turns on
include/onlyvisibility without an authorization hook. Once a concrete list endpoint wires this in, a caller with normal list/read access can discover soft-deleted rows, while the futurerestoreAPI is mapped towritepermission.WDYT — should the base filter expose a hook/contract so concrete subclasses can require restore/write/admin capability before showing deleted rows?
Other suggestion
-
superset/views/base_api.py:380_get_deleted_at_maplives on the generic base API but assumes every future soft-deletable API usesidas its primary key. That is true for the planned chart/dashboard/dataset rollout, but it makes the shared helper easier to misuse later.Small suggestion: could this use
self.datamodel.get_pk()/get_pk_name()and preserve the existing key type instead?
Praise
-
superset/commands/restore.py:87The restore command scopes the temporary request-level visibility bypass with
try/finallyand restores the previous flag value. That is a good guardrail for a subtle global-listener interaction.
Two fixes from apache#39977 (review) 1. find_existing_for_import bypassed ORM cleanup listeners The helper used a raw Core ``Model.__table__.delete().where(...)`` to hard-delete the soft-deleted row before re-import. The original rationale was perf (avoid ORM cascade-load), but the raw delete skips ORM ``after_delete`` event listeners — which is where two important cleanup paths live: * ObjectUpdater.after_delete (superset/tags/core.py) cleans up tagged_object rows. tagged_object.object_id is a plain integer, not a foreign key, so the database cannot cascade them. Bypassing the listener leaves orphaned tag rows pointing at deleted entities. * SqlaTable.after_delete (superset/connectors/sqla/models.py) cleans up dataset permission-view rows. Bypassing the listener leaves orphaned permission entries. Switched to db.session.delete(existing) so both listeners fire. The perf concern (cascade-loading large relationship graphs) is much less acute for an import operation than for a steady-state user- triggered delete, and correctness wins. Updated the function docstring to reflect the change and explain why the listener-firing path is required. Updated the bypass primer ((superset-spec)/sc-103157-soft-deletes/bypass-primer.md) so the "common mistakes" section no longer points at the raw-DELETE pattern as a recommended escape hatch. 2. _get_deleted_at_map hardcoded the id PK column The helper queried ``self.datamodel.obj.id`` directly, which works for chart/dashboard/dataset (all int PKs) but would silently break for a future soft-deletable entity with a different PK. Now resolves the column via ``self.datamodel.get_pk_name()``. Signature relaxed from ``list[int]`` to ``list[Any]`` to match the broader PK contract. Items 2 (per-query bypass scope) and 3 (auth on visibility) from the review are deferred per the review-thread replies — item 2 is the YAGNI tradeoff with an explicit code comment, item 3 belongs in SIP-208 discussion rather than this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@richardfogaca thanks for the second pass — addressed in Item 1 (importers cleanup) — fixed. You're right that the raw Core Item 4 (hardcoded id PK) — fixed. Item 2 (per-query bypass scope) — accepted YAGNI, comment added in follow-up. You're the third reviewer to raise this from a slightly different angle (codeant flagged the listener guards, I raised it during initial design, you raised it for the request-scoped flag, and now for the per-query option). The mechanism in all cases is the same: In the current code path, the bypass is set by Per-entity scoping is a real future-proofing option (e.g., # The execution-option bypass is per-statement but not per-class — it
# skips the soft-delete listener for every SoftDeleteMixin subclass in
# the statement, not just self.model. Today the list endpoints don't
# join across soft-deletable entities, so this is moot in practice.
# If a future list query starts joining (e.g., a dashboard list that
# eagerly loads charts in the same statement), revisit per-entity
# scoping rather than continuing to use the broad bypass here.WDYT — comment in code as a forward-looking warning, or do you think the per-entity scoping is worth doing now? Item 3 (visibility auth gap) — SIP-208 question rather than code review. Legitimate concern. The proposal in SIP-208 maps restore to Praise on the try/finally — noted, thank you. That ordering took a couple of iterations to land on. |
|
Posting on Richard's behalf — this is his PR reviewer agent. Forward any pushback to him and he'll loop me back in. One follow-up from the second pass. Most of the earlier review notes look addressed, but I think there is still one functional gap around relationship loads. Line numbers verified against HEAD
|
Code Review Agent Run #fd97cdActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@richardfogaca I think there's a better solution to the |
Two real bugs in the infrastructure surface, both flagged by codeant-ai-for-open-source on PR apache#39977. Both are fixed in this commit with documentation and a new unit test. 1. raise_for_ownership re-query was filtered by the listener BaseRestoreCommand.validate calls security_manager.raise_for_ownership, which re-queries the row internally to fetch its current owners. That re-query goes through the global do_orm_execute listener; for a soft-deleted row the listener filters it out, raise_for_ownership sees no row, falls back to an empty owners list, and raises MISSING_OWNERSHIP_ERROR for any non-admin user. The only callers who could restore were admins. Fix: set g.skip_visibility_filter = True for the scope of the security check, restoring the previous value afterward. This is the documented per-request opt-out path for cases like this where the bypass must propagate to a function we don't directly own. 2. Listener missed the documented loader-path guards SQLAlchemy's recommended soft-delete pattern at github.com/sqlalchemy/sqlalchemy/issues/7973 guards the criteria application with not execute_state.is_column_load and not execute_state.is_relationship_load. Without these, every lazy/eager relationship load re-applies the criteria, stacking redundant deleted_at IS NULL clauses on the loader's query. Adds both guards. Tests * New tests/unit_tests/commands/test_restore.py exercises the BaseRestoreCommand contract directly: - flag is set to True during the security check - flag is restored to its previous value afterward - flag is restored even when the security check raises The previous entity-level tests mocked raise_for_ownership directly and never exercised the real re-query path, so they masked the bug. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three changes addressing items flagged in apache#39977 (review) 1. DatabaseDAO.find_by_id ignored the new flag The override accepted skip_visibility_filter for signature compatibility but never applied it to the query. Any caller of the override that passed skip_visibility_filter=True still got filtered results. Now applies execution_options when the flag is True, matching BaseDAO.find_by_id. 2. skip_visibility_filter moved to keyword-only The original signature inserted skip_visibility_filter at position 3 in BaseDAO.find_by_id / find_by_ids / find_by_id_or_uuid / _find_by_column, shifting id_column and query_options. Downstream positional callers (extensions, library consumers) would silently bind the new parameter to a different intent. Moved to the end and made keyword-only across all four methods plus the DatabaseDAO override; existing in-tree callers all use keyword form already. 3. BaseDeletedStateFilter scope tightened Previously set g.skip_visibility_filter = True (broad, per-request), which is fine in isolation but would leak soft-deleted rows of unrelated entities once multiple models adopt SoftDeleteMixin: a list request for soft-deleted dashboards would also bypass the filter for incidental Slice / SqlaTable relationship loads. Two changes decouple the concerns: - The visibility-filter bypass is now applied per-query on the primary list query via .execution_options(skip_visibility_filter= True). Affects only the list query for this entity. - Response augmentation (adding deleted_at to result rows) is signalled via a separate request-scoped flag, g._augment_response_with_deleted_at. Distinct from the bypass. pre_get_list now checks the augmentation flag instead of the bypass flag. Listener docstring updated to clarify the narrower per-request use case: the broad flag is now reserved for cases where a function we don't directly control performs an internal re-query that must see the soft-deleted row (e.g., security_manager.raise_for_ownership). The previously-claimed use case (list-endpoint rison filters) has moved to the per-query option. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two fixes from apache#39977 (review) 1. find_existing_for_import bypassed ORM cleanup listeners The helper used a raw Core ``Model.__table__.delete().where(...)`` to hard-delete the soft-deleted row before re-import. The original rationale was perf (avoid ORM cascade-load), but the raw delete skips ORM ``after_delete`` event listeners — which is where two important cleanup paths live: * ObjectUpdater.after_delete (superset/tags/core.py) cleans up tagged_object rows. tagged_object.object_id is a plain integer, not a foreign key, so the database cannot cascade them. Bypassing the listener leaves orphaned tag rows pointing at deleted entities. * SqlaTable.after_delete (superset/connectors/sqla/models.py) cleans up dataset permission-view rows. Bypassing the listener leaves orphaned permission entries. Switched to db.session.delete(existing) so both listeners fire. The perf concern (cascade-loading large relationship graphs) is much less acute for an import operation than for a steady-state user- triggered delete, and correctness wins. Updated the function docstring to reflect the change and explain why the listener-firing path is required. Updated the bypass primer ((superset-spec)/sc-103157-soft-deletes/bypass-primer.md) so the "common mistakes" section no longer points at the raw-DELETE pattern as a recommended escape hatch. 2. _get_deleted_at_map hardcoded the id PK column The helper queried ``self.datamodel.obj.id`` directly, which works for chart/dashboard/dataset (all int PKs) but would silently break for a future soft-deletable entity with a different PK. Now resolves the column via ``self.datamodel.get_pk_name()``. Signature relaxed from ``list[int]`` to ``list[Any]`` to match the broader PK contract. Items 2 (per-query bypass scope) and 3 (auth on visibility) from the review are deferred per the review-thread replies — item 2 is the YAGNI tradeoff with an explicit code comment, item 3 belongs in SIP-208 discussion rather than this PR. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mikebridge
force-pushed
the
sc-106188-soft-delete-infrastructure
branch
from
1fb3aa4 to
c48e23b
Compare
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Posting on Richard's behalf - this is his PR reviewer agent. Forward any pushback to him and he'll loop me back in.
Left a few notes below. The main things I would look at before the entity rollout PRs are the import helper's hard-delete timing and whether restore should bypass DAO base filters; the rest is smaller contract/docs cleanup. All line numbers verified against HEAD c6d741a.
Functional - worth tightening before merge
-
superset/commands/importers/v1/utils.py:431-433find_existing_for_import()hard-deletes and flushes the soft-deleted match during what otherwise reads like a lookup helper. Once the entity import paths start using this, a caller that invokes the helper before completingoverwrite/ validation decisions would have already permanently removed the old row, includingafter_deletecleanup side effects.WDYT - would it be worth splitting this into a side-effect-free lookup plus an explicit
hard_delete_existing_for_import()call that the entity import command can place after its overwrite/permission checks pass? -
superset/commands/restore.py:65-69Restore lookup bypasses the DAO
base_filteras well as the new soft-delete visibility filter. Existing delete paths load throughfind_by_ids()withoutskip_base_filter=True, then run ownership checks, so restore would have a broader initial lookup surface than delete/update even though it only needs to see soft-deleted rows.Could we keep
skip_visibility_filter=Truebut leave the normal base filter in place, unless a concrete rollout PR has an entity-specific reason to bypass it?
Other suggestions
-
superset/commands/restore.py:60-62The base
run()mutates the model without a transaction and relies on every concrete restore command remembering to overriderun()only to add the transaction decorator. That contract is documented, but it is easy for a rollout PR to miss and would silently skip the standard commit/error-wrapping behavior.Small suggestion: could the reusable base flow own the transactional wrapper somehow, or could the subclass requirement be made harder to accidentally skip? Happy to keep as-is if the concrete rollout PRs already pin this pattern with tests.
-
superset/models/helpers.py:672-674Tiny doc mismatch: this now says
BaseDAO.delete()is the permanent hard-delete path, but this PR changesBaseDAO.delete()to soft-delete models that inheritSoftDeleteMixin.Nit: could this point readers at
BaseDAO.hard_delete()for permanent deletion instead?
Praise
- The
Query.get()tests usingexpunge_all()are a nice guard against an identity-map false positive. That makes the listener/bypass behavior much more convincing, especially for the ownership re-query path that restore depends on.
Wire datasets to the soft-delete infrastructure (sc-106188). Adds the SoftDeleteMixin to SqlaTable, ships a new RestoreDatasetCommand and POST /api/v1/dataset/<uuid>/restore endpoint, adds the dataset_deleted_state rison filter, updates the v1 importer to bypass the visibility filter on UUID lookup, and ships a one-table migration adding deleted_at to tables. DeleteDatasetCommand needs no source change in this PR - the BaseDAO.delete() routing introduced in sc-106188 detects the SoftDeleteMixin on SqlaTable automatically and routes to soft_delete. Migration * New migration 3a8e6f2c1b95 (down_revision 33d7e0e21daa) adding nullable deleted_at column + ix_tables_deleted_at index. Reversible. Model + DAO * SqlaTable inherits SoftDeleteMixin -> deleted_at column on the ORM, the global do_orm_execute listener begins filtering SqlaTable queries (and SqlaTable relationships). * DeleteDatasetCommand routes through BaseDAO.delete() unchanged. API * RestoreDatasetCommand subclasses BaseRestoreCommand[SqlaTable] (4 lines). * POST /api/v1/dataset/<uuid>/restore endpoint with the standard decorator stack. Permissions mirror delete exactly. * DatasetDeletedStateFilter subclasses BaseDeletedStateFilter (2 lines), wired into search filters. * DatasetRestoreFailedError exception type. * Import pipeline uses find_existing_for_import. Cascade behaviour (V1) Per SIP-208, soft-delete does not cascade in V1. Charts that reference a soft-deleted dataset render an error at chart-load time. That's the documented behaviour - a future ticket may add a "degraded chart" state. The integration tests cover the chart-on-soft-deleted-dataset case. Tests * tests/unit_tests/commands/dataset/restore_test.py - 4 unit tests. * tests/integration_tests/datasets/soft_delete_tests.py - integration coverage for delete -> soft, restore -> active, list-with-filter, importer-handles-soft-deleted, chart-on-soft-deleted-dataset. Depends on sc-106188 (apache#39977). Do NOT merge until sc-106188 is in master. Migration shares down_revision (33d7e0e21daa) with sc-106189 and sc-106190; whichever lands second/third needs a rebase or merge migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@aminghadersohi thanks for the careful pass. Most items addressed in (from Claude) HIGHH1 — uncached subclass walk → cached registry via You're right that retrofitting under live traffic is harder than getting it right now. I chose the H2 — non-restorable / GDPR hook. Deferred. The concern is valid but I'd push back on landing the interface now. Two reasons:
If GDPR work gets scheduled, I'll do the design then with the actual constraints in hand. Tracking as a follow-up rather than a speculative interface here. MEDIUMM3 — M4 — # Inline import: ``superset.models.helpers`` transitively imports
# ``superset.models.core``, which depends on lazily-initialised
# ``superset.feature_flag_manager``. A top-level import here would
# create a circular dependency (security ↔ models.core ↔ superset).M5 — # Subclasses bind ``model`` to a concrete ``SoftDeleteMixin``
# subclass. Typed as ``type[SoftDeleteMixin]`` so a subclass that
# accidentally binds to a non-soft-deletable entity fails mypy
# rather than crashing at runtime on ``.deleted_at``.
model: ClassVar[type[SoftDeleteMixin]]M6 — listener placement. Documented. Investigated and confirmed the placement is intentional — the M7 — no tests for
M8 — Nits
Architecture notes
Re: the praiseThanks. The dual-bypass mechanism in particular went through ~5 design iterations — per-request Infrastructure tip: 40 unit tests pass (was 26; added 14 covering filter, mixin, importer helpers). Let me know if anything else surfaces. |
|
@aminghadersohi A couple points to add:
|
…/M3/M5) Three small follow-ups surfaced by aminghadersohi's review of the SoftDeleteMixin PR (apache#39977) that apply equally here: - H1: cache _child_to_parent_registry() with functools.cache. Called twice per save flush; mapping depends only on import-time model classes, so unbounded cache is the right shape (no invalidation). - M5: tighten _CHILD_BASELINE_HANDLERS type from dict[str, Any] to dict[str, Callable[[Session, Any, int], None]] via a named alias. Mypy now catches a future broken handler signature. - M3/M4: explain the inline-import pattern once in the module docstrings of baseline.py and changes.py. Both modules use pylint disable=import-outside-toplevel uniformly because they load during init_versioning() before mappers are configured; the per-callsite "why" comments would just repeat the same reason. Module-level explanation + a hint to comment unusual cases is the cleaner shape. M6 (listener placement) doesn't apply — init_versioning() already runs inside init_app_in_ctx(). M8 (loose OpenAPI schema in */api.py docstrings) is real but its own change.
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Codex second opinion obtained (no blockers found). Two items worth tracking in entity rollout PRs:
**TOCTOU in ** (): the check-then-act window between loading the row and committing means a concurrent hard-delete or ownership change produces a stale authorization decision. The wrapper limits blast radius, but a on the re-query (or an atomic conditional update scoped to + ) would close the window properly.
** session bypass not cleaned up** (): unlike (which removes classes on exit), writes to and relies on request teardown to clear it. Within-request code after the list call sees widened visibility for that model class. Worth scoping this the same way the context manager does.
Both are follow-up items, not blockers. Infrastructure is solid — approving.
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Codex second opinion obtained (no blockers found). Two items worth tracking in entity rollout PRs:
TOCTOU in BaseRestoreCommand.validate() (restore.py:82-97): the check-then-act window between validate() loading the row and model.restore() committing means a concurrent hard-delete or ownership change produces a stale authorization decision. The @transaction wrapper limits blast radius, but a SELECT...FOR UPDATE on the re-query (or an atomic conditional update scoped to uuid + deleted_at IS NOT NULL) would close the window properly.
BaseDeletedStateFilter session bypass not cleaned up (filters.py:184-191): unlike skip_visibility_filter (which removes classes on exit), _add_session_bypass writes to session.info and relies on request teardown to clear it. Within-request code after the list call sees widened visibility for that model class. Worth scoping this the same way the context manager does.
Both are follow-up items, not blockers. Infrastructure is solid.
…tion Addresses F-002 from @aminghadersohi's review on PR apache#39977: ``BaseDeletedStateFilter._add_session_bypass`` writes the filter's model class to ``session.info[SKIP_VISIBILITY_FILTER_CLASSES]`` so the listener stops filtering for FAB's count + inner + outer queries — but never removed it. The bypass persisted for the rest of the request, so any code that ran after the list query (audit hooks, ``after_request`` handlers, dependent serialisation work) saw widened visibility for that model class. Fix: track filter-added classes in ``g._deleted_state_added_classes`` (request-scoped) and release them in ``SoftDeleteApiMixin.pre_get_list`` after augmentation completes. The release is scoped to entries *this filter* added — programmatic bypasses installed by the ``skip_visibility_filter`` context manager or DAO ``skip_visibility_filter=True`` calls (which manage their own lifecycle) are untouched. Sequence within a list response: 1. ``BaseDeletedStateFilter.apply`` adds ``self.model`` to ``session.info[SKIP_VISIBILITY_FILTER_CLASSES]`` AND records the class in ``g._deleted_state_added_classes``. 2. FAB issues count + inner + outer + relationship loads — all see the bypass. 3. ``SoftDeleteApiMixin.pre_get_list`` runs after the list query: reads-and-clears the augmentation flag, injects ``deleted_at``, then releases the session bypass via a ``try/finally`` so the release fires even if augmentation raises. 4. Any code that runs later in the same request sees the normal filtered view. Tests added in ``tests/unit_tests/views/test_soft_delete_filter.py``: * test_filter_records_added_classes_on_g — pins the tracker * test_mixin_releases_bypass_after_inject — pins that the release removes the filter's entries from session.info * test_mixin_release_does_not_touch_unrelated_bypass_entries — guards against the release clobbering a bypass installed by a programmatic caller for a different class * test_mixin_release_is_noop_when_filter_was_not_invoked — release is a no-op when no augmentation flag was set (normal request path) 44 unit tests pass (was 40 + 4 new). F-001 (TOCTOU in BaseRestoreCommand.validate) intentionally deferred to its own follow-up PR: the design choice between SELECT ... FOR UPDATE and an atomic conditional update has real trade-offs (ORM event listeners vs locking semantics) and deserves focused review and proper concurrent test coverage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire charts to the soft-delete infrastructure (sc-106188). Adds the SoftDeleteMixin to Slice, routes DeleteChartCommand through BaseDAO to soft-delete, ships a new RestoreChartCommand and POST /api/v1/chart/<uuid>/restore endpoint, adds the chart_deleted_state rison filter, updates the v1 importer to bypass the visibility filter on UUID lookup, and ships a one-table migration adding deleted_at to slices. Migration * New migration 7c4a8d09ca37 (down_revision 33d7e0e21daa) adding nullable deleted_at column + ix_slices_deleted_at index. Uses superset.migrations.shared.utils helpers so it's idempotent and re-runnable. Reversible. Model + DAO * Slice inherits SoftDeleteMixin -> deleted_at column appears on the ORM, the global do_orm_execute listener begins filtering Slice queries. * DeleteChartCommand routes through BaseDAO.delete() which now detects SoftDeleteMixin and calls soft_delete(). API * RestoreChartCommand subclasses BaseRestoreCommand[Slice] (4 lines). * POST /api/v1/chart/<uuid>/restore endpoint with @Protect / @safe / @statsd_metrics decorators. Permissions mirror delete exactly: endpoint-level can_write, resource-level raise_for_ownership. * ChartDeletedStateFilter subclasses BaseDeletedStateFilter (2 lines) and is wired into the search filters. * ChartRestoreFailedError exception type. * Import pipeline uses find_existing_for_import to bypass the visibility filter and hard-delete soft-deleted rows before re-import. Tests * tests/unit_tests/commands/chart/restore_test.py - 4 unit tests. * tests/integration_tests/charts/soft_delete_tests.py - integration coverage for delete -> soft, restore -> active, list-with-filter, importer-handles-soft-deleted. Depends on sc-106188 (apache#39977). Do NOT merge until sc-106188 is in master. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire dashboards to the soft-delete infrastructure (sc-106188). Adds the SoftDeleteMixin to Dashboard, ships a new RestoreDashboardCommand and POST /api/v1/dashboard/<uuid>/restore endpoint, adds the dashboard_deleted_state rison filter, updates the v1 importer to bypass the visibility filter on UUID lookup, and ships a one-table migration adding deleted_at to dashboards. Note that DeleteDashboardCommand needs no source change in this PR - the BaseDAO.delete() routing introduced in sc-106188 detects the SoftDeleteMixin on Dashboard automatically and routes to soft_delete. Migration * New migration 9e1f3b8c4d2a (down_revision 33d7e0e21daa) adding nullable deleted_at column + ix_dashboards_deleted_at index. Reversible. Model + DAO * Dashboard inherits SoftDeleteMixin -> deleted_at column on the ORM, the global do_orm_execute listener begins filtering Dashboard queries (and Dashboard relationships - lazy-loaded charts on a soft-deleted dashboard are also hidden). * DeleteDashboardCommand routes through BaseDAO.delete() unchanged. API * RestoreDashboardCommand subclasses BaseRestoreCommand[Dashboard] (4 lines). * POST /api/v1/dashboard/<uuid>/restore endpoint with the standard decorator stack. Permissions mirror delete exactly: endpoint-level can_write, resource-level raise_for_ownership. * DashboardDeletedStateFilter subclasses BaseDeletedStateFilter (2 lines), wired into search filters. * DashboardRestoreFailedError exception type. * Import pipeline uses find_existing_for_import. Embedded-dashboard regression * tests/integration_tests/dashboards/soft_delete_tests.py covers the case where a parent dashboard is soft-deleted: the embedded iframe URL still loads 200 (it doesn't dereference the parent dashboard during render), and the dashboard API returns 404 cleanly. The assertions are ordered so the API check runs before the embedded GET (the embedded handler clears the test-client session in CI; reordering keeps both assertions in scope). Tests * tests/unit_tests/commands/dashboard/restore_test.py - 4 unit tests. * tests/integration_tests/dashboards/soft_delete_tests.py - integration coverage for delete -> soft, restore -> active + reattach to charts, list-with-filter, importer-handles-soft-deleted, embedded-with-soft- deleted-parent. Depends on sc-106188 (apache#39977). Do NOT merge until sc-106188 is in master. Migration shares down_revision (33d7e0e21daa) with sc-106189 and sc-106191; whichever lands second/third needs a rebase or merge migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire datasets to the soft-delete infrastructure (sc-106188). Adds the SoftDeleteMixin to SqlaTable, ships a new RestoreDatasetCommand and POST /api/v1/dataset/<uuid>/restore endpoint, adds the dataset_deleted_state rison filter, updates the v1 importer to bypass the visibility filter on UUID lookup, and ships a one-table migration adding deleted_at to tables. DeleteDatasetCommand needs no source change in this PR - the BaseDAO.delete() routing introduced in sc-106188 detects the SoftDeleteMixin on SqlaTable automatically and routes to soft_delete. Migration * New migration 3a8e6f2c1b95 (down_revision 33d7e0e21daa) adding nullable deleted_at column + ix_tables_deleted_at index. Reversible. Model + DAO * SqlaTable inherits SoftDeleteMixin -> deleted_at column on the ORM, the global do_orm_execute listener begins filtering SqlaTable queries (and SqlaTable relationships). * DeleteDatasetCommand routes through BaseDAO.delete() unchanged. API * RestoreDatasetCommand subclasses BaseRestoreCommand[SqlaTable] (4 lines). * POST /api/v1/dataset/<uuid>/restore endpoint with the standard decorator stack. Permissions mirror delete exactly. * DatasetDeletedStateFilter subclasses BaseDeletedStateFilter (2 lines), wired into search filters. * DatasetRestoreFailedError exception type. * Import pipeline uses find_existing_for_import. Cascade behaviour (V1) Per SIP-208, soft-delete does not cascade in V1. Charts that reference a soft-deleted dataset render an error at chart-load time. That's the documented behaviour - a future ticket may add a "degraded chart" state. The integration tests cover the chart-on-soft-deleted-dataset case. Tests * tests/unit_tests/commands/dataset/restore_test.py - 4 unit tests. * tests/integration_tests/datasets/soft_delete_tests.py - integration coverage for delete -> soft, restore -> active, list-with-filter, importer-handles-soft-deleted, chart-on-soft-deleted-dataset. Depends on sc-106188 (apache#39977). Do NOT merge until sc-106188 is in master. Migration shares down_revision (33d7e0e21daa) with sc-106189 and sc-106190; whichever lands second/third needs a rebase or merge migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@aminghadersohi thanks for the post-approval flags — both are worth addressing. F-002 fixed in F-002 — session bypass cleanup. Fixed.You're right that relying on session teardown left a within-request widened-visibility window. The fix uses a variation on your option B/C: the filter records classes it added in # BaseDeletedStateFilter._add_session_bypass — track for release
bypass = query.session.info.setdefault(SKIP_VISIBILITY_FILTER_CLASSES, set())
bypass.add(self.model)
added = getattr(g, DELETED_STATE_ADDED_CLASSES, set()) | {self.model}
setattr(g, DELETED_STATE_ADDED_CLASSES, added)
# SoftDeleteApiMixin.pre_get_list — release after augmentation
try:
self._inject_deleted_at(data)
finally:
self._release_session_bypass()
@staticmethod
def _release_session_bypass() -> None:
added = getattr(g, DELETED_STATE_ADDED_CLASSES, set())
if not added:
return
bypass = db.session.info.get(SKIP_VISIBILITY_FILTER_CLASSES, set())
bypass -= added
setattr(g, DELETED_STATE_ADDED_CLASSES, set())Key property: the release is scoped to entries the filter added. A programmatic caller using the Sequence within a list response:
Four new tests:
44 unit tests pass on the infrastructure branch (was 40). F-001 — TOCTOU on restore. Deferred.You're right that the
Proper test coverage means an actual concurrent-session test rig (two sessions, observe lock contention or rowcount=0), not just a unit-level mock — a different kind of work than what's been in this PR. So F-001 lands separately, where the locking-vs-atomic-update design can get focused review and the concurrency tests can be built deliberately. Tracked in the spec at Infrastructure tip is now |
Six mypy errors flagged by the pre-commit (current) CI check after the F-002 session-bypass-cleanup commit: * superset/views/filters.py — ``added`` and ``bypass`` variables in ``_release_session_bypass`` need explicit type annotations (``set[type[SoftDeleteMixin]]``) because mypy can't infer the element type from ``getattr(g, ..., set())`` or ``session.info.get(..., set())``. Same pattern in ``_add_session_bypass`` where ``added`` shadows the inferred type. * tests/unit_tests/commands/importers/v1/test_find_existing_for_import.py — the ``# type: ignore`` on ``_ImportableSoftDeletable``'s multi-line ``class`` declaration was on the ``class`` line but the ``_TestBase`` reference (the line that actually triggers ``Invalid base class`` + ``not valid as a type``) was on the continuation. Collapsed the declaration to one line so the ignore applies to the right line — matches the pattern used by the other synthetic models in the same test file. * tests/unit_tests/views/test_soft_delete_filter.py — same ``added`` annotation fix; plus dropped an unused ``# type: ignore`` on a marker inner class (``_OtherSoftDeletable``) that doesn't inherit from ``_TestBase`` and therefore doesn't need the suppression. 44 unit tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire charts to the soft-delete infrastructure (sc-106188). Adds the SoftDeleteMixin to Slice, routes DeleteChartCommand through BaseDAO to soft-delete, ships a new RestoreChartCommand and POST /api/v1/chart/<uuid>/restore endpoint, adds the chart_deleted_state rison filter, updates the v1 importer to bypass the visibility filter on UUID lookup, and ships a one-table migration adding deleted_at to slices. Migration * New migration 7c4a8d09ca37 (down_revision 33d7e0e21daa) adding nullable deleted_at column + ix_slices_deleted_at index. Uses superset.migrations.shared.utils helpers so it's idempotent and re-runnable. Reversible. Model + DAO * Slice inherits SoftDeleteMixin -> deleted_at column appears on the ORM, the global do_orm_execute listener begins filtering Slice queries. * DeleteChartCommand routes through BaseDAO.delete() which now detects SoftDeleteMixin and calls soft_delete(). API * RestoreChartCommand subclasses BaseRestoreCommand[Slice] (4 lines). * POST /api/v1/chart/<uuid>/restore endpoint with @Protect / @safe / @statsd_metrics decorators. Permissions mirror delete exactly: endpoint-level can_write, resource-level raise_for_ownership. * ChartDeletedStateFilter subclasses BaseDeletedStateFilter (2 lines) and is wired into the search filters. * ChartRestoreFailedError exception type. * Import pipeline uses find_existing_for_import to bypass the visibility filter and hard-delete soft-deleted rows before re-import. Tests * tests/unit_tests/commands/chart/restore_test.py - 4 unit tests. * tests/integration_tests/charts/soft_delete_tests.py - integration coverage for delete -> soft, restore -> active, list-with-filter, importer-handles-soft-deleted. Depends on sc-106188 (apache#39977). Do NOT merge until sc-106188 is in master. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire dashboards to the soft-delete infrastructure (sc-106188). Adds the SoftDeleteMixin to Dashboard, ships a new RestoreDashboardCommand and POST /api/v1/dashboard/<uuid>/restore endpoint, adds the dashboard_deleted_state rison filter, updates the v1 importer to bypass the visibility filter on UUID lookup, and ships a one-table migration adding deleted_at to dashboards. Note that DeleteDashboardCommand needs no source change in this PR - the BaseDAO.delete() routing introduced in sc-106188 detects the SoftDeleteMixin on Dashboard automatically and routes to soft_delete. Migration * New migration 9e1f3b8c4d2a (down_revision 33d7e0e21daa) adding nullable deleted_at column + ix_dashboards_deleted_at index. Reversible. Model + DAO * Dashboard inherits SoftDeleteMixin -> deleted_at column on the ORM, the global do_orm_execute listener begins filtering Dashboard queries (and Dashboard relationships - lazy-loaded charts on a soft-deleted dashboard are also hidden). * DeleteDashboardCommand routes through BaseDAO.delete() unchanged. API * RestoreDashboardCommand subclasses BaseRestoreCommand[Dashboard] (4 lines). * POST /api/v1/dashboard/<uuid>/restore endpoint with the standard decorator stack. Permissions mirror delete exactly: endpoint-level can_write, resource-level raise_for_ownership. * DashboardDeletedStateFilter subclasses BaseDeletedStateFilter (2 lines), wired into search filters. * DashboardRestoreFailedError exception type. * Import pipeline uses find_existing_for_import. Embedded-dashboard regression * tests/integration_tests/dashboards/soft_delete_tests.py covers the case where a parent dashboard is soft-deleted: the embedded iframe URL still loads 200 (it doesn't dereference the parent dashboard during render), and the dashboard API returns 404 cleanly. The assertions are ordered so the API check runs before the embedded GET (the embedded handler clears the test-client session in CI; reordering keeps both assertions in scope). Tests * tests/unit_tests/commands/dashboard/restore_test.py - 4 unit tests. * tests/integration_tests/dashboards/soft_delete_tests.py - integration coverage for delete -> soft, restore -> active + reattach to charts, list-with-filter, importer-handles-soft-deleted, embedded-with-soft- deleted-parent. Depends on sc-106188 (apache#39977). Do NOT merge until sc-106188 is in master. Migration shares down_revision (33d7e0e21daa) with sc-106189 and sc-106191; whichever lands second/third needs a rebase or merge migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire datasets to the soft-delete infrastructure (sc-106188). Adds the SoftDeleteMixin to SqlaTable, ships a new RestoreDatasetCommand and POST /api/v1/dataset/<uuid>/restore endpoint, adds the dataset_deleted_state rison filter, updates the v1 importer to bypass the visibility filter on UUID lookup, and ships a one-table migration adding deleted_at to tables. DeleteDatasetCommand needs no source change in this PR - the BaseDAO.delete() routing introduced in sc-106188 detects the SoftDeleteMixin on SqlaTable automatically and routes to soft_delete. Migration * New migration 3a8e6f2c1b95 (down_revision 33d7e0e21daa) adding nullable deleted_at column + ix_tables_deleted_at index. Reversible. Model + DAO * SqlaTable inherits SoftDeleteMixin -> deleted_at column on the ORM, the global do_orm_execute listener begins filtering SqlaTable queries (and SqlaTable relationships). * DeleteDatasetCommand routes through BaseDAO.delete() unchanged. API * RestoreDatasetCommand subclasses BaseRestoreCommand[SqlaTable] (4 lines). * POST /api/v1/dataset/<uuid>/restore endpoint with the standard decorator stack. Permissions mirror delete exactly. * DatasetDeletedStateFilter subclasses BaseDeletedStateFilter (2 lines), wired into search filters. * DatasetRestoreFailedError exception type. * Import pipeline uses find_existing_for_import. Cascade behaviour (V1) Per SIP-208, soft-delete does not cascade in V1. Charts that reference a soft-deleted dataset render an error at chart-load time. That's the documented behaviour - a future ticket may add a "degraded chart" state. The integration tests cover the chart-on-soft-deleted-dataset case. Tests * tests/unit_tests/commands/dataset/restore_test.py - 4 unit tests. * tests/integration_tests/datasets/soft_delete_tests.py - integration coverage for delete -> soft, restore -> active, list-with-filter, importer-handles-soft-deleted, chart-on-soft-deleted-dataset. Depends on sc-106188 (apache#39977). Do NOT merge until sc-106188 is in master. Migration shares down_revision (33d7e0e21daa) with sc-106189 and sc-106190; whichever lands second/third needs a rebase or merge migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire charts to the soft-delete infrastructure (sc-106188). Adds the SoftDeleteMixin to Slice, routes DeleteChartCommand through BaseDAO to soft-delete, ships a new RestoreChartCommand and POST /api/v1/chart/<uuid>/restore endpoint, adds the chart_deleted_state rison filter, updates the v1 importer to bypass the visibility filter on UUID lookup, and ships a one-table migration adding deleted_at to slices. Migration * New migration 7c4a8d09ca37 (down_revision 33d7e0e21daa) adding nullable deleted_at column + ix_slices_deleted_at index. Uses superset.migrations.shared.utils helpers so it's idempotent and re-runnable. Reversible. Model + DAO * Slice inherits SoftDeleteMixin -> deleted_at column appears on the ORM, the global do_orm_execute listener begins filtering Slice queries. * DeleteChartCommand routes through BaseDAO.delete() which now detects SoftDeleteMixin and calls soft_delete(). API * RestoreChartCommand subclasses BaseRestoreCommand[Slice] (4 lines). * POST /api/v1/chart/<uuid>/restore endpoint with @Protect / @safe / @statsd_metrics decorators. Permissions mirror delete exactly: endpoint-level can_write, resource-level raise_for_ownership. * ChartDeletedStateFilter subclasses BaseDeletedStateFilter (2 lines) and is wired into the search filters. * ChartRestoreFailedError exception type. * Import pipeline uses find_existing_for_import to bypass the visibility filter and hard-delete soft-deleted rows before re-import. Tests * tests/unit_tests/commands/chart/restore_test.py - 4 unit tests. * tests/integration_tests/charts/soft_delete_tests.py - integration coverage for delete -> soft, restore -> active, list-with-filter, importer-handles-soft-deleted. Depends on sc-106188 (apache#39977). Do NOT merge until sc-106188 is in master. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire dashboards to the soft-delete infrastructure (sc-106188). Adds the SoftDeleteMixin to Dashboard, ships a new RestoreDashboardCommand and POST /api/v1/dashboard/<uuid>/restore endpoint, adds the dashboard_deleted_state rison filter, updates the v1 importer to bypass the visibility filter on UUID lookup, and ships a one-table migration adding deleted_at to dashboards. Note that DeleteDashboardCommand needs no source change in this PR - the BaseDAO.delete() routing introduced in sc-106188 detects the SoftDeleteMixin on Dashboard automatically and routes to soft_delete. Migration * New migration 9e1f3b8c4d2a (down_revision 33d7e0e21daa) adding nullable deleted_at column + ix_dashboards_deleted_at index. Reversible. Model + DAO * Dashboard inherits SoftDeleteMixin -> deleted_at column on the ORM, the global do_orm_execute listener begins filtering Dashboard queries (and Dashboard relationships - lazy-loaded charts on a soft-deleted dashboard are also hidden). * DeleteDashboardCommand routes through BaseDAO.delete() unchanged. API * RestoreDashboardCommand subclasses BaseRestoreCommand[Dashboard] (4 lines). * POST /api/v1/dashboard/<uuid>/restore endpoint with the standard decorator stack. Permissions mirror delete exactly: endpoint-level can_write, resource-level raise_for_ownership. * DashboardDeletedStateFilter subclasses BaseDeletedStateFilter (2 lines), wired into search filters. * DashboardRestoreFailedError exception type. * Import pipeline uses find_existing_for_import. Embedded-dashboard regression * tests/integration_tests/dashboards/soft_delete_tests.py covers the case where a parent dashboard is soft-deleted: the embedded iframe URL still loads 200 (it doesn't dereference the parent dashboard during render), and the dashboard API returns 404 cleanly. The assertions are ordered so the API check runs before the embedded GET (the embedded handler clears the test-client session in CI; reordering keeps both assertions in scope). Tests * tests/unit_tests/commands/dashboard/restore_test.py - 4 unit tests. * tests/integration_tests/dashboards/soft_delete_tests.py - integration coverage for delete -> soft, restore -> active + reattach to charts, list-with-filter, importer-handles-soft-deleted, embedded-with-soft- deleted-parent. Depends on sc-106188 (apache#39977). Do NOT merge until sc-106188 is in master. Migration shares down_revision (33d7e0e21daa) with sc-106189 and sc-106191; whichever lands second/third needs a rebase or merge migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire datasets to the soft-delete infrastructure (sc-106188). Adds the SoftDeleteMixin to SqlaTable, ships a new RestoreDatasetCommand and POST /api/v1/dataset/<uuid>/restore endpoint, adds the dataset_deleted_state rison filter, updates the v1 importer to bypass the visibility filter on UUID lookup, and ships a one-table migration adding deleted_at to tables. DeleteDatasetCommand needs no source change in this PR - the BaseDAO.delete() routing introduced in sc-106188 detects the SoftDeleteMixin on SqlaTable automatically and routes to soft_delete. Migration * New migration 3a8e6f2c1b95 (down_revision 33d7e0e21daa) adding nullable deleted_at column + ix_tables_deleted_at index. Reversible. Model + DAO * SqlaTable inherits SoftDeleteMixin -> deleted_at column on the ORM, the global do_orm_execute listener begins filtering SqlaTable queries (and SqlaTable relationships). * DeleteDatasetCommand routes through BaseDAO.delete() unchanged. API * RestoreDatasetCommand subclasses BaseRestoreCommand[SqlaTable] (4 lines). * POST /api/v1/dataset/<uuid>/restore endpoint with the standard decorator stack. Permissions mirror delete exactly. * DatasetDeletedStateFilter subclasses BaseDeletedStateFilter (2 lines), wired into search filters. * DatasetRestoreFailedError exception type. * Import pipeline uses find_existing_for_import. Cascade behaviour (V1) Per SIP-208, soft-delete does not cascade in V1. Charts that reference a soft-deleted dataset render an error at chart-load time. That's the documented behaviour - a future ticket may add a "degraded chart" state. The integration tests cover the chart-on-soft-deleted-dataset case. Tests * tests/unit_tests/commands/dataset/restore_test.py - 4 unit tests. * tests/integration_tests/datasets/soft_delete_tests.py - integration coverage for delete -> soft, restore -> active, list-with-filter, importer-handles-soft-deleted, chart-on-soft-deleted-dataset. Depends on sc-106188 (apache#39977). Do NOT merge until sc-106188 is in master. Migration shares down_revision (33d7e0e21daa) with sc-106189 and sc-106190; whichever lands second/third needs a rebase or merge migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire charts to the soft-delete infrastructure (sc-106188). Adds the SoftDeleteMixin to Slice, routes DeleteChartCommand through BaseDAO to soft-delete, ships a new RestoreChartCommand and POST /api/v1/chart/<uuid>/restore endpoint, adds the chart_deleted_state rison filter, updates the v1 importer to bypass the visibility filter on UUID lookup, and ships a one-table migration adding deleted_at to slices. Migration * New migration 7c4a8d09ca37 (down_revision 33d7e0e21daa) adding nullable deleted_at column + ix_slices_deleted_at index. Uses superset.migrations.shared.utils helpers so it's idempotent and re-runnable. Reversible. Model + DAO * Slice inherits SoftDeleteMixin -> deleted_at column appears on the ORM, the global do_orm_execute listener begins filtering Slice queries. * DeleteChartCommand routes through BaseDAO.delete() which now detects SoftDeleteMixin and calls soft_delete(). API * RestoreChartCommand subclasses BaseRestoreCommand[Slice] (4 lines). * POST /api/v1/chart/<uuid>/restore endpoint with @Protect / @safe / @statsd_metrics decorators. Permissions mirror delete exactly: endpoint-level can_write, resource-level raise_for_ownership. * ChartDeletedStateFilter subclasses BaseDeletedStateFilter (2 lines) and is wired into the search filters. * ChartRestoreFailedError exception type. * Import pipeline uses find_existing_for_import to bypass the visibility filter and hard-delete soft-deleted rows before re-import. Tests * tests/unit_tests/commands/chart/restore_test.py - 4 unit tests. * tests/integration_tests/charts/soft_delete_tests.py - integration coverage for delete -> soft, restore -> active, list-with-filter, importer-handles-soft-deleted. Depends on sc-106188 (apache#39977). Do NOT merge until sc-106188 is in master. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire dashboards to the soft-delete infrastructure (sc-106188). Adds the SoftDeleteMixin to Dashboard, ships a new RestoreDashboardCommand and POST /api/v1/dashboard/<uuid>/restore endpoint, adds the dashboard_deleted_state rison filter, updates the v1 importer to bypass the visibility filter on UUID lookup, and ships a one-table migration adding deleted_at to dashboards. Note that DeleteDashboardCommand needs no source change in this PR - the BaseDAO.delete() routing introduced in sc-106188 detects the SoftDeleteMixin on Dashboard automatically and routes to soft_delete. Migration * New migration 9e1f3b8c4d2a (down_revision 33d7e0e21daa) adding nullable deleted_at column + ix_dashboards_deleted_at index. Reversible. Model + DAO * Dashboard inherits SoftDeleteMixin -> deleted_at column on the ORM, the global do_orm_execute listener begins filtering Dashboard queries (and Dashboard relationships - lazy-loaded charts on a soft-deleted dashboard are also hidden). * DeleteDashboardCommand routes through BaseDAO.delete() unchanged. API * RestoreDashboardCommand subclasses BaseRestoreCommand[Dashboard] (4 lines). * POST /api/v1/dashboard/<uuid>/restore endpoint with the standard decorator stack. Permissions mirror delete exactly: endpoint-level can_write, resource-level raise_for_ownership. * DashboardDeletedStateFilter subclasses BaseDeletedStateFilter (2 lines), wired into search filters. * DashboardRestoreFailedError exception type. * Import pipeline uses find_existing_for_import. Embedded-dashboard regression * tests/integration_tests/dashboards/soft_delete_tests.py covers the case where a parent dashboard is soft-deleted: the embedded iframe URL still loads 200 (it doesn't dereference the parent dashboard during render), and the dashboard API returns 404 cleanly. The assertions are ordered so the API check runs before the embedded GET (the embedded handler clears the test-client session in CI; reordering keeps both assertions in scope). Tests * tests/unit_tests/commands/dashboard/restore_test.py - 4 unit tests. * tests/integration_tests/dashboards/soft_delete_tests.py - integration coverage for delete -> soft, restore -> active + reattach to charts, list-with-filter, importer-handles-soft-deleted, embedded-with-soft- deleted-parent. Depends on sc-106188 (apache#39977). Do NOT merge until sc-106188 is in master. Migration shares down_revision (33d7e0e21daa) with sc-106189 and sc-106191; whichever lands second/third needs a rebase or merge migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire datasets to the soft-delete infrastructure (sc-106188). Adds the SoftDeleteMixin to SqlaTable, ships a new RestoreDatasetCommand and POST /api/v1/dataset/<uuid>/restore endpoint, adds the dataset_deleted_state rison filter, updates the v1 importer to bypass the visibility filter on UUID lookup, and ships a one-table migration adding deleted_at to tables. DeleteDatasetCommand needs no source change in this PR - the BaseDAO.delete() routing introduced in sc-106188 detects the SoftDeleteMixin on SqlaTable automatically and routes to soft_delete. Migration * New migration 3a8e6f2c1b95 (down_revision 33d7e0e21daa) adding nullable deleted_at column + ix_tables_deleted_at index. Reversible. Model + DAO * SqlaTable inherits SoftDeleteMixin -> deleted_at column on the ORM, the global do_orm_execute listener begins filtering SqlaTable queries (and SqlaTable relationships). * DeleteDatasetCommand routes through BaseDAO.delete() unchanged. API * RestoreDatasetCommand subclasses BaseRestoreCommand[SqlaTable] (4 lines). * POST /api/v1/dataset/<uuid>/restore endpoint with the standard decorator stack. Permissions mirror delete exactly. * DatasetDeletedStateFilter subclasses BaseDeletedStateFilter (2 lines), wired into search filters. * DatasetRestoreFailedError exception type. * Import pipeline uses find_existing_for_import. Cascade behaviour (V1) Per SIP-208, soft-delete does not cascade in V1. Charts that reference a soft-deleted dataset render an error at chart-load time. That's the documented behaviour - a future ticket may add a "degraded chart" state. The integration tests cover the chart-on-soft-deleted-dataset case. Tests * tests/unit_tests/commands/dataset/restore_test.py - 4 unit tests. * tests/integration_tests/datasets/soft_delete_tests.py - integration coverage for delete -> soft, restore -> active, list-with-filter, importer-handles-soft-deleted, chart-on-soft-deleted-dataset. Depends on sc-106188 (apache#39977). Do NOT merge until sc-106188 is in master. Migration shares down_revision (33d7e0e21daa) with sc-106189 and sc-106190; whichever lands second/third needs a rebase or merge migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The listener's loader_criteria was passing a concrete SQL expression (``cls.where_not_deleted()``) which rendered as the raw table name (``slices.deleted_at``) regardless of how the table was aliased in the statement. When the same soft-deletable model appeared under an alias in a JOIN — e.g., FAB's outer/inner reconstruction or ``report_schedule.chart`` aliasing ``slices AS chart`` — the JOIN's ON clause produced ``Unknown column 'slices.deleted_at' in 'on clause'`` and broke the entire query. Surfaced as CI failures on the entity-rollout PRs (apache#40128 / apache#40129 / apache#40130) — specifically pre-existing report-schedule tests that join to ``slices AS chart`` started failing once ``Slice`` inherited ``SoftDeleteMixin``. The infrastructure PR itself doesn't exercise this because no entity has the mixin yet. Fix: switch the criteria to a lambda (the form Bayer's canonical pattern uses). SQLAlchemy invokes the lambda per occurrence and adapts the column reference to the alias at each site. The lambda's body is trivial (``c.deleted_at.is_(None)``) so the ``DeferredLambdaElement`` parser handles it without issue — we originally moved AWAY from a lambda because of an ``if cls in bypass_classes`` conditional that DeferredLambdaElement mis-parsed as a SQL ``IN`` operator, but the conditional now lives outside the lambda (in the per-class iteration), so the lambda body itself is clean. Test added: test_listener_adapts_criteria_to_aliased_table_in_joins — reproduces the aliased-join shape using the synthetic ``_SoftDeletableChild`` model. Without the fix, this query raises ``OperationalError: no such column: ..._soft_deletable_child.deleted_at`` because the criteria renders as the raw table name. With the fix, the query runs cleanly. 45 unit tests pass (was 44 + 1 regression test). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire charts to the soft-delete infrastructure (sc-106188). Adds the SoftDeleteMixin to Slice, routes DeleteChartCommand through BaseDAO to soft-delete, ships a new RestoreChartCommand and POST /api/v1/chart/<uuid>/restore endpoint, adds the chart_deleted_state rison filter, updates the v1 importer to bypass the visibility filter on UUID lookup, and ships a one-table migration adding deleted_at to slices. Migration * New migration 7c4a8d09ca37 (down_revision 33d7e0e21daa) adding nullable deleted_at column + ix_slices_deleted_at index. Uses superset.migrations.shared.utils helpers so it's idempotent and re-runnable. Reversible. Model + DAO * Slice inherits SoftDeleteMixin -> deleted_at column appears on the ORM, the global do_orm_execute listener begins filtering Slice queries. * DeleteChartCommand routes through BaseDAO.delete() which now detects SoftDeleteMixin and calls soft_delete(). API * RestoreChartCommand subclasses BaseRestoreCommand[Slice] (4 lines). * POST /api/v1/chart/<uuid>/restore endpoint with @Protect / @safe / @statsd_metrics decorators. Permissions mirror delete exactly: endpoint-level can_write, resource-level raise_for_ownership. * ChartDeletedStateFilter subclasses BaseDeletedStateFilter (2 lines) and is wired into the search filters. * ChartRestoreFailedError exception type. * Import pipeline uses find_existing_for_import to bypass the visibility filter and hard-delete soft-deleted rows before re-import. Tests * tests/unit_tests/commands/chart/restore_test.py - 4 unit tests. * tests/integration_tests/charts/soft_delete_tests.py - integration coverage for delete -> soft, restore -> active, list-with-filter, importer-handles-soft-deleted. Depends on sc-106188 (apache#39977). Do NOT merge until sc-106188 is in master. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire dashboards to the soft-delete infrastructure (sc-106188). Adds the SoftDeleteMixin to Dashboard, ships a new RestoreDashboardCommand and POST /api/v1/dashboard/<uuid>/restore endpoint, adds the dashboard_deleted_state rison filter, updates the v1 importer to bypass the visibility filter on UUID lookup, and ships a one-table migration adding deleted_at to dashboards. Note that DeleteDashboardCommand needs no source change in this PR - the BaseDAO.delete() routing introduced in sc-106188 detects the SoftDeleteMixin on Dashboard automatically and routes to soft_delete. Migration * New migration 9e1f3b8c4d2a (down_revision 33d7e0e21daa) adding nullable deleted_at column + ix_dashboards_deleted_at index. Reversible. Model + DAO * Dashboard inherits SoftDeleteMixin -> deleted_at column on the ORM, the global do_orm_execute listener begins filtering Dashboard queries (and Dashboard relationships - lazy-loaded charts on a soft-deleted dashboard are also hidden). * DeleteDashboardCommand routes through BaseDAO.delete() unchanged. API * RestoreDashboardCommand subclasses BaseRestoreCommand[Dashboard] (4 lines). * POST /api/v1/dashboard/<uuid>/restore endpoint with the standard decorator stack. Permissions mirror delete exactly: endpoint-level can_write, resource-level raise_for_ownership. * DashboardDeletedStateFilter subclasses BaseDeletedStateFilter (2 lines), wired into search filters. * DashboardRestoreFailedError exception type. * Import pipeline uses find_existing_for_import. Embedded-dashboard regression * tests/integration_tests/dashboards/soft_delete_tests.py covers the case where a parent dashboard is soft-deleted: the embedded iframe URL still loads 200 (it doesn't dereference the parent dashboard during render), and the dashboard API returns 404 cleanly. The assertions are ordered so the API check runs before the embedded GET (the embedded handler clears the test-client session in CI; reordering keeps both assertions in scope). Tests * tests/unit_tests/commands/dashboard/restore_test.py - 4 unit tests. * tests/integration_tests/dashboards/soft_delete_tests.py - integration coverage for delete -> soft, restore -> active + reattach to charts, list-with-filter, importer-handles-soft-deleted, embedded-with-soft- deleted-parent. Depends on sc-106188 (apache#39977). Do NOT merge until sc-106188 is in master. Migration shares down_revision (33d7e0e21daa) with sc-106189 and sc-106191; whichever lands second/third needs a rebase or merge migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire datasets to the soft-delete infrastructure (sc-106188). Adds the SoftDeleteMixin to SqlaTable, ships a new RestoreDatasetCommand and POST /api/v1/dataset/<uuid>/restore endpoint, adds the dataset_deleted_state rison filter, updates the v1 importer to bypass the visibility filter on UUID lookup, and ships a one-table migration adding deleted_at to tables. DeleteDatasetCommand needs no source change in this PR - the BaseDAO.delete() routing introduced in sc-106188 detects the SoftDeleteMixin on SqlaTable automatically and routes to soft_delete. Migration * New migration 3a8e6f2c1b95 (down_revision 33d7e0e21daa) adding nullable deleted_at column + ix_tables_deleted_at index. Reversible. Model + DAO * SqlaTable inherits SoftDeleteMixin -> deleted_at column on the ORM, the global do_orm_execute listener begins filtering SqlaTable queries (and SqlaTable relationships). * DeleteDatasetCommand routes through BaseDAO.delete() unchanged. API * RestoreDatasetCommand subclasses BaseRestoreCommand[SqlaTable] (4 lines). * POST /api/v1/dataset/<uuid>/restore endpoint with the standard decorator stack. Permissions mirror delete exactly. * DatasetDeletedStateFilter subclasses BaseDeletedStateFilter (2 lines), wired into search filters. * DatasetRestoreFailedError exception type. * Import pipeline uses find_existing_for_import. Cascade behaviour (V1) Per SIP-208, soft-delete does not cascade in V1. Charts that reference a soft-deleted dataset render an error at chart-load time. That's the documented behaviour - a future ticket may add a "degraded chart" state. The integration tests cover the chart-on-soft-deleted-dataset case. Tests * tests/unit_tests/commands/dataset/restore_test.py - 4 unit tests. * tests/integration_tests/datasets/soft_delete_tests.py - integration coverage for delete -> soft, restore -> active, list-with-filter, importer-handles-soft-deleted, chart-on-soft-deleted-dataset. Depends on sc-106188 (apache#39977). Do NOT merge until sc-106188 is in master. Migration shares down_revision (33d7e0e21daa) with sc-106189 and sc-106190; whichever lands second/third needs a rebase or merge migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bayer's canonical pattern excludes ``is_relationship_load`` events on the assumption that ``with_loader_criteria(propagate_to_loaders=True)`` (the default) carries criteria from the parent statement to the relationship loaders. That works for the simple case where the parent statement references the target class. It does NOT reliably work when the parent statement references a *different* class and the criteria targets the relationship's target only. The Superset case that surfaces this: * A ``Dashboard`` SELECT fires. The listener attaches ``with_loader_criteria(Slice, lambda, propagate_to_loaders=True)``. ``Slice`` never appears in the Dashboard statement. * ``dashboard.slices`` lazy-loads via the many-to-many through ``dashboard_slices``. The criteria targeting ``Slice`` does not reach this lazy load. * The relationship load returns soft-deleted charts the listener was supposed to exclude. Surfaced as the failing ``test_restore_chart_reattaches_to_dashboards`` integration test on the charts-rollout PR (apache#40129): the soft-deleted chart was still in ``dashboard.slices``. Fix: drop the ``is_relationship_load`` exclusion. The listener now fires on relationship loads too and re-attaches the criteria directly. The resulting WHERE clause has ``deleted_at IS NULL`` once (when propagation didn't work) or twice (when it did) — both are idempotent and correct. Bayer's concern was about "redundant clauses," not correctness, and a tiny SQL redundancy is the right trade for closing a real visibility leak. ``is_column_load`` exclusion is kept — those events fire for attribute refreshes on already-loaded objects, not entity loads, and attaching loader criteria there would be both pointless and potentially harmful. The predicate is renamed ``_is_primary_user_select`` → ``_should_attach_soft_delete_criteria`` to reflect the broader scope; docstring updated to explain the deliberate inclusion of relationship loads and the redundancy-vs-correctness trade-off. 899 unit tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire dashboards to the soft-delete infrastructure (sc-106188). Adds the SoftDeleteMixin to Dashboard, ships a new RestoreDashboardCommand and POST /api/v1/dashboard/<uuid>/restore endpoint, adds the dashboard_deleted_state rison filter, updates the v1 importer to bypass the visibility filter on UUID lookup, and ships a one-table migration adding deleted_at to dashboards. Note that DeleteDashboardCommand needs no source change in this PR - the BaseDAO.delete() routing introduced in sc-106188 detects the SoftDeleteMixin on Dashboard automatically and routes to soft_delete. Migration * New migration 9e1f3b8c4d2a (down_revision 33d7e0e21daa) adding nullable deleted_at column + ix_dashboards_deleted_at index. Reversible. Model + DAO * Dashboard inherits SoftDeleteMixin -> deleted_at column on the ORM, the global do_orm_execute listener begins filtering Dashboard queries (and Dashboard relationships - lazy-loaded charts on a soft-deleted dashboard are also hidden). * DeleteDashboardCommand routes through BaseDAO.delete() unchanged. API * RestoreDashboardCommand subclasses BaseRestoreCommand[Dashboard] (4 lines). * POST /api/v1/dashboard/<uuid>/restore endpoint with the standard decorator stack. Permissions mirror delete exactly: endpoint-level can_write, resource-level raise_for_ownership. * DashboardDeletedStateFilter subclasses BaseDeletedStateFilter (2 lines), wired into search filters. * DashboardRestoreFailedError exception type. * Import pipeline uses find_existing_for_import. Embedded-dashboard regression * tests/integration_tests/dashboards/soft_delete_tests.py covers the case where a parent dashboard is soft-deleted: the embedded iframe URL still loads 200 (it doesn't dereference the parent dashboard during render), and the dashboard API returns 404 cleanly. The assertions are ordered so the API check runs before the embedded GET (the embedded handler clears the test-client session in CI; reordering keeps both assertions in scope). Tests * tests/unit_tests/commands/dashboard/restore_test.py - 4 unit tests. * tests/integration_tests/dashboards/soft_delete_tests.py - integration coverage for delete -> soft, restore -> active + reattach to charts, list-with-filter, importer-handles-soft-deleted, embedded-with-soft- deleted-parent. Depends on sc-106188 (apache#39977). Do NOT merge until sc-106188 is in master. Migration shares down_revision (33d7e0e21daa) with sc-106189 and sc-106191; whichever lands second/third needs a rebase or merge migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire datasets to the soft-delete infrastructure (sc-106188). Adds the SoftDeleteMixin to SqlaTable, ships a new RestoreDatasetCommand and POST /api/v1/dataset/<uuid>/restore endpoint, adds the dataset_deleted_state rison filter, updates the v1 importer to bypass the visibility filter on UUID lookup, and ships a one-table migration adding deleted_at to tables. DeleteDatasetCommand needs no source change in this PR - the BaseDAO.delete() routing introduced in sc-106188 detects the SoftDeleteMixin on SqlaTable automatically and routes to soft_delete. Migration * New migration 3a8e6f2c1b95 (down_revision 33d7e0e21daa) adding nullable deleted_at column + ix_tables_deleted_at index. Reversible. Model + DAO * SqlaTable inherits SoftDeleteMixin -> deleted_at column on the ORM, the global do_orm_execute listener begins filtering SqlaTable queries (and SqlaTable relationships). * DeleteDatasetCommand routes through BaseDAO.delete() unchanged. API * RestoreDatasetCommand subclasses BaseRestoreCommand[SqlaTable] (4 lines). * POST /api/v1/dataset/<uuid>/restore endpoint with the standard decorator stack. Permissions mirror delete exactly. * DatasetDeletedStateFilter subclasses BaseDeletedStateFilter (2 lines), wired into search filters. * DatasetRestoreFailedError exception type. * Import pipeline uses find_existing_for_import. Cascade behaviour (V1) Per SIP-208, soft-delete does not cascade in V1. Charts that reference a soft-deleted dataset render an error at chart-load time. That's the documented behaviour - a future ticket may add a "degraded chart" state. The integration tests cover the chart-on-soft-deleted-dataset case. Tests * tests/unit_tests/commands/dataset/restore_test.py - 4 unit tests. * tests/integration_tests/datasets/soft_delete_tests.py - integration coverage for delete -> soft, restore -> active, list-with-filter, importer-handles-soft-deleted, chart-on-soft-deleted-dataset. Depends on sc-106188 (apache#39977). Do NOT merge until sc-106188 is in master. Migration shares down_revision (33d7e0e21daa) with sc-106189 and sc-106190; whichever lands second/third needs a rebase or merge migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire charts to the soft-delete infrastructure (sc-106188). Adds the SoftDeleteMixin to Slice, routes DeleteChartCommand through BaseDAO to soft-delete, ships a new RestoreChartCommand and POST /api/v1/chart/<uuid>/restore endpoint, adds the chart_deleted_state rison filter, updates the v1 importer to bypass the visibility filter on UUID lookup, and ships a one-table migration adding deleted_at to slices. Migration * New migration 7c4a8d09ca37 (down_revision 33d7e0e21daa) adding nullable deleted_at column + ix_slices_deleted_at index. Uses superset.migrations.shared.utils helpers so it's idempotent and re-runnable. Reversible. Model + DAO * Slice inherits SoftDeleteMixin -> deleted_at column appears on the ORM, the global do_orm_execute listener begins filtering Slice queries. * DeleteChartCommand routes through BaseDAO.delete() which now detects SoftDeleteMixin and calls soft_delete(). API * RestoreChartCommand subclasses BaseRestoreCommand[Slice] (4 lines). * POST /api/v1/chart/<uuid>/restore endpoint with @Protect / @safe / @statsd_metrics decorators. Permissions mirror delete exactly: endpoint-level can_write, resource-level raise_for_ownership. * ChartDeletedStateFilter subclasses BaseDeletedStateFilter (2 lines) and is wired into the search filters. * ChartRestoreFailedError exception type. * Import pipeline uses find_existing_for_import to bypass the visibility filter and hard-delete soft-deleted rows before re-import. Tests * tests/unit_tests/commands/chart/restore_test.py - 4 unit tests. * tests/integration_tests/charts/soft_delete_tests.py - integration coverage for delete -> soft, restore -> active, list-with-filter, importer-handles-soft-deleted. Depends on sc-106188 (apache#39977). Do NOT merge until sc-106188 is in master. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review Agent Run #def5a4Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
3 participants
SUMMARY
First of four PRs decomposing the soft-delete work for charts, dashboards, and datasets, SIP-208 — 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.
Entity rollouts (forthcoming, depend on this PR)
Each entity-rollout PR adds
SoftDeleteMixinto its model, ships a one-table migration adding thedeleted_atcolumn + 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 nullabledeleted_atcolumn,is_deletedhybrid property,where_not_deleted()filter clause,soft_delete()andrestore()methods._add_soft_delete_filter—do_orm_executelistener using SQLAlchemy's officially-recommended pattern (Mike Bayer / sqlalchemy#7973). On every primary user SELECT, iterates concreteSoftDeleteMixinsubclasses and attaches awith_loader_criteria(cls, cls.where_not_deleted(), include_aliases=True)for each one not in the request's bypass set. Per-class scoping means a bypass forDashboarddoes not unhide soft-deletedSliceorSqlaTablerows. Subclass iteration is sorted by__qualname__so SQLAlchemy's compiled-statement cache key is stable across processes. No-op until something inherits the mixin.SKIP_VISIBILITY_FILTER_CLASSES) — the listener consults the union of:execution_options[SKIP_VISIBILITY_FILTER_CLASSES] = {Model}on a Query — one-statement narrow scope; used byBaseDAO.find_by_id(skip_visibility_filter=True),security_manager.raise_for_ownership, andfind_existing_for_import.session.info[SKIP_VISIBILITY_FILTER_CLASSES] = {Model, ...}on a Session — session-scoped (i.e., request-scoped under Flask-SQLAlchemy) bypass that survives query reconstruction. Used byBaseDeletedStateFilterbecause FAB'sSQLAInterface.get_outer_query_from_inner_querybuilds the outer fetch query from a freshsession.query(self.obj), dropping the inner query'sexecution_options. Session-level state survives that reconstruction; per-query options do not.skip_visibility_filter(session, *classes)context manager — programmatic bypass with guaranteed cleanup on exception. Wraps thesession.infomechanism.BaseDAO surface (
superset/daos/base.py,superset/daos/database.py)BaseDAO.soft_delete(items)— callsitem.soft_delete()on each.BaseDAO.hard_delete(items)— callsdb.session.delete(item)on each (the originalBaseDAO.deletebehaviour, renamed and preserved).BaseDAO.delete(items)— routes to soft forSoftDeleteMixin-inheriting models, hard otherwise. Backwards-compatible: until any model inherits the mixin,delete()continues to callhard_delete()exactly as before.find_by_id,find_by_uuid,find_by_ids,_find_by_columngain askip_visibility_filter: bool = Falsekeyword-only parameter. Internally translates the boolean toexecution_options(SKIP_VISIBILITY_FILTER_CLASSES={cls.model_cls})— caller-facing API stays ergonomic; per-class scoping happens internally.DatabaseDAO.find_by_idis 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]—Tbound toSoftDeleteMixin. Subclasses are purely declarative (four ClassVars, no methods):dao,not_found_exc,forbidden_exc,restore_failed_exc. The baserun()owns the transaction wrapper, building@transaction(on_error=partial(on_error, reraise=self.restore_failed_exc))at call time. Subclasses can't accidentally drop or mis-wire the transaction (earlier iterations required subclasses to overriderun()purely to add the decorator — fragile contract, now eliminated).validate()callsdao.find_by_id(uuid, id_column="uuid", skip_visibility_filter=True)— the entity's RBACbase_filterstays in effect, matching delete's lookup semantics; only the visibility filter is bypassed. Ownership is verified byraise_for_ownershipafter the lookup.BaseDeletedStateFilter— base class for per-entity rison filters (*_deleted_statewith valuesinclude/only). Adds itsself.modeltosession.info[SKIP_VISIBILITY_FILTER_CLASSES]so the listener stops filtering this entity for the duration of the request. A separate request-scoped flagg._augment_response_with_deleted_atsignals that the response should be augmented with adeleted_atfield on each row — two concerns, two channels.SoftDeleteApiMixin(also inviews/filters.py) — REST API mixin that augments list responses withdeleted_atwhen the augmentation flag is set. Mount on concrete REST API classes for soft-deletable entities. Chains viasuper().pre_get_list(data)so other inheritance-chain behaviour still runs. Keeping the augmentation in a mixin rather than onBaseSupersetModelRestApikeeps the base class focused on its own responsibility.find_existing_for_import(model_cls, uuid)— pure lookup helper for v1 importer pipelines. Bypasses the visibility filter and returns the row as-is whether it's live or soft-deleted (orNone).clear_soft_deleted_for_import(existing)— explicit destructive cleanup. Hard-deletes a soft-deleted row viadb.session.delete()(so ORMafter_deletelisteners fire and clean uptagged_objectrows + dataset permission-view rows that the database cannot cascade). Entity importers (charts/dashboards/datasets) call this after the overwrite/permission check passes, so the destructive op is gated by the same ownership check as the live-overwrite path.MODEL_API_RW_METHOD_PERMISSION_MAP["restore"] = "write"— so future per-entity/restoreendpoints inheritcan_write(the same permission asdelete). No new permission, no role migration.Design decisions documented in code
can_write, resource-levelraise_for_ownership. No new permission plumbing.datetime.now()(naive) fordeleted_atto matchAuditMixinNullable.changed_onprecedent (PR #33693 reverted UTC on those columns; if/when audit columns move to UTC-aware, soft-delete should follow).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only infrastructure with no runtime behaviour change.
TESTING INSTRUCTIONS
pytest tests/unit_tests/models/test_soft_delete_mixin.py \ tests/unit_tests/daos/test_base_dao_soft_delete.py \ tests/unit_tests/commands/test_base_restore_command.py -vExpected: 26 passed.
The tests use synthetic in-memory models (
_SoftDeletable,_SoftDeletableTwo,_SoftDeletableParent,_SoftDeletableChild) that inheritSoftDeleteMixindirectly, exercising the listener, mixin methods,BaseDAOrouting,BaseRestoreCommand.validate, the transactional wrapper, and the context manager — all without any real Superset entity. Real entities acquire the mixin in the entity-rollout PRs above.Key tests worth pointing at:
test_session_bypass_survives_query_reconstructionreproduces the exact FAB outer/inner shape (inner_query.with_entities(pk).subquery()joined to a freshsession.query(Model)) and proves the session-level bypass survives that reconstruction. This is the test that would have caught the original bug.test_per_query_bypass_for_one_class_does_not_unhide_otherandtest_session_bypass_does_not_leak_across_classespin per-class scoping using a second synthetic soft-deletable model.test_relationship_load_filters_child_independently_of_parent_bypassverifies thatwith_loader_criteria(..., propagate_to_loaders=True)carries the per-class criteria through to lazy/selectin loads.test_get_without_bypass_filters_out_soft_deleted_rowandtest_per_query_bypass_via_get_finds_soft_deleted_rowpinQuery.get()behaviour (usingsession.expunge_all()to force a SQL round trip through the listener, not just an identity-map hit) — the pathraise_for_ownershipdepends on.test_run_translates_sqlalchemy_errors_via_restore_failed_excpins the base class's transactional wrapper: aSQLAlchemyErrorraised inside the unit of work is translated to the subclass'srestore_failed_exc.test_listener_does_not_affect_non_soft_deletable_queriespins that the per-class iteration doesn't break queries against classes that don't inheritSoftDeleteMixin.Optional sanity check that nothing broke in master behaviour (since
BaseDAO.deleteis now routed):Expected: all green.
ADDITIONAL INFORMATION
restore→ "write" mapping; no user-facing API surface yet)Reviewer notes
A reviewer may reasonably ask: "why ship abstractions with no callers in master?":
_SoftDeletablemodels exercise the mixin, listener, BaseDAO routing, and BaseRestoreCommand. The tests would fail if the abstractions were broken./sqlalchemy-review(cache-stability + hot-path allocation tidyings),/clean-code-review(Extract Method on the listener gate, filter opt-in, andpre_get_listpolicy/mechanism split),/tidy-first-review,/ultrareview(clean), plus multiple iterations of richardfogaca review (find/clear split,base_filterhonored on restore, transaction-wrapper moved into base class, FAB outer/inner bug surfaced and fixed via session-scoped bypass).