feat(core): SoftDeleteMixin and restore infrastructure#39977
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.07% 64.05% -0.03%
==========================================
Files 2592 2593 +1
Lines 139548 139733 +185
Branches 32421 32442 +21
==========================================
+ Hits 89422 89499 +77
- Misses 48589 48690 +101
- Partials 1537 1544 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
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>
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 |
Code Review Agent Run #575e5bActionable Suggestions - 0Additional Suggestions - 2
Filtered 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 |
| """Look up an existing row by UUID for an import operation, | ||
| bypassing the soft-delete visibility filter so soft-deleted matches | ||
| are returned too. |
There was a problem hiding this comment.
Nit, by convention docstrings should have a 1-line short description. Because it's a convention a lot of tooling will rely on it, and in this case IDEs and other tools would show only "Look up an existing row by UUID for an import operation," as the documentation for the function.
Pure-code infrastructure for soft-delete (sc-103157 / SIP-208). Ships the runtime mechanism and the entity-rollout abstractions in one focused commit. No model uses the mixin yet, no migration runs, no API surface changes — everything is dormant until the entity-rollout PRs (sc-106189 / sc-106190 / sc-106191) opt their concrete entities in. Runtime mechanism * ``SoftDeleteMixin`` in ``superset/models/helpers.py`` — defines the ``deleted_at`` column, ``is_deleted`` hybrid property, ``not_deleted()`` filter clause, and ``soft_delete()`` / ``restore()`` methods. * ``_add_soft_delete_filter`` ``do_orm_execute`` listener — uses SQLAlchemy's officially-recommended pattern (``do_orm_execute`` + ``with_loader_criteria``, per Mike Bayer / sqlalchemy#7973). No-op until something inherits the mixin. * Listener registration in ``setup_soft_delete_listener`` at app init. * Two opt-out paths: per-query ``execution_options(skip_visibility_filter=True)`` for narrow bypasses (admin tooling, import re-lookups), and per-request ``flask.g.skip_visibility_filter = True`` for user-facing list filters that explicitly ask to surface soft-deleted rows. BaseDAO surface * ``BaseDAO.soft_delete()``, ``BaseDAO.hard_delete()``, and a ``BaseDAO.delete()`` that routes between them based on whether ``cls.model_cls`` includes ``SoftDeleteMixin``. * ``find_by_id``, ``find_by_uuid``, ``find_by_ids``, and ``_find_by_column`` gain a ``skip_visibility_filter: bool = False`` parameter, mirroring the existing ``skip_base_filter`` parameter. * ``DatabaseDAO.find_by_id`` is updated for signature compatibility. Restore abstractions * ``BaseRestoreCommand[T]`` in ``superset/commands/restore.py``, with ``T`` bound to ``SoftDeleteMixin``. Subclasses provide ``dao``, ``not_found_exc``, ``forbidden_exc``. Validates ownership via ``security_manager.raise_for_ownership`` (matches existing delete commands). * ``BaseDeletedStateFilter`` in ``superset/views/filters.py`` — base class for per-entity rison filters with ``include`` / ``only`` / default values. * ``find_existing_for_import`` in ``superset/commands/importers/v1/utils.py`` — helper for v1 importer pipelines that takes ``skip_visibility_filter`` and hard-deletes any soft-deleted match before returning so the re-import doesn't hit a unique-constraint violation on uuid. * ``restore`` -> ``"write"`` entry in ``MODEL_API_RW_METHOD_PERMISSION_MAP`` so future per-entity restore endpoints inherit ``can_write`` and no permission migration is needed. * ``BaseSupersetModelRestApi.pre_get_list`` augments list responses with ``deleted_at`` when the request has opted into seeing soft-deleted rows. Tests * ``tests/unit_tests/models/test_soft_delete_mixin.py`` — exercises the mixin and listener via a synthetic ``_SoftDeletable`` model. * ``tests/unit_tests/daos/test_base_dao_soft_delete.py`` — exercises routing (soft vs hard) via synthetic DAOs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the type: ignore on _add_soft_delete_filter with the proper ORMExecuteState annotation from sqlalchemy.orm.session. SQLAlchemy 1.4 exposes this type for do_orm_execute listeners; the type: ignore was masking the missing import, not a real typing gap. mypy passes with the explicit annotation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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>
Previously, BaseRestoreCommand.validate wrapped its raise_for_ownership call with g.skip_visibility_filter=True so the function's internal re-query of the resource could see soft-deleted rows. That worked but left the soft-delete coupling visible at every restore-command site, required dedicated tests for the bypass contract, and added a follow-up ticket (sc-106314) to consolidate the boilerplate. Move the bypass inside raise_for_ownership itself instead. The function becomes soft-delete-aware: its internal re-query always sees the row the caller passed, including when that row is soft-deleted. The bypass is scoped narrowly to the re-query (try/finally restores the previous value) so it doesn't leak past the function. Net effect for callers: - BaseRestoreCommand.validate goes back to a plain raise_for_ownership(model) call. - Every other caller is unaffected — their resources weren't soft-deleted, the re-query wasn't being filtered, the bypass is a no-op for them. The change in security/manager.py is 12 lines (try/finally wrapper + import) and surgical. It preserves the defense-in-depth value of the re-query (defense against form-data tampering of owners, see PR apache#20499 and the original 2016 commit) while making the function correct for the soft-delete case. Removed - The 15-line try/finally bypass wrapper in BaseRestoreCommand.validate - The 'from flask import g' and SKIP_VISIBILITY_FILTER imports in commands/restore.py - tests/unit_tests/commands/test_restore.py (the test file specifically exercised the bypass contract that no longer lives in BaseRestoreCommand) Updated - Listener docstring in models/helpers.py to point at the new canonical caller of the per-request flag (raise_for_ownership itself). - Bypass primer to reflect that the per-request bypass is now mostly an internal mechanism; new external callers should be rare and justified. Follow-up tickets to be updated separately - sc-106314 (context-manager refactor) becomes superseded; only one caller of the bypass remains, and that one is internal. - sc-106319 (remove the defensive re-query entirely) drops from 'fixes the soft-delete coupling' to 'pure perf optimization, optional.' Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ry.get() The internal re-query in security_manager.raise_for_ownership uses Query.get(), which has identity-map short-circuit behavior. There was a real question whether the per-query execution_options(skip_visibility_filter=True) bypass propagates correctly through Query.get() in SQLAlchemy 1.4, vs. only working with the per-request g.skip_visibility_filter flag. Both tests use session.expire_all() to force the identity-map miss path so .get() actually issues SQL. The listener then fires and the bypass is honored in both modes: * test_per_request_bypass_via_get_finds_soft_deleted_row: verifies the current design (per-request g flag) works with .get(). This is what raise_for_ownership currently does internally. * test_per_query_bypass_via_get_finds_soft_deleted_row: verifies the per-query execution_options approach also works with .get(). Removes the empirical uncertainty about whether execution_options propagates through .get(); confirms a future simplification of raise_for_ownership (swap g flag for execution_options on the get() call) is behaviorally safe. Both pass. Both bypass surfaces are validated and equivalent for the raise_for_ownership use case. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…uest to per-query Replaces the try/finally g.skip_visibility_filter dance inside raise_for_ownership with a per-query execution_options attached directly to the internal re-query's .get() call. The listener also drops its g-flag read since no caller uses it anymore. The two approaches are behaviorally equivalent for raise_for_ownership (empirically verified by the previous commit's test_per_query_bypass_ via_get_finds_soft_deleted_row test), but per-query has strict advantages: * Scope: exactly the one statement, never broader. The previous per-request approach was correctly scoped via try/finally but required reasoning about Flask request lifecycle to be confident in the scope. * Listener simplification: drops the getattr(g, ...) read and the try/except RuntimeError that handled non-Flask contexts. The listener is now a pure function of execute_state. * No more g coupling in the listener. The listener no longer reaches into Flask request state for any reason. * Public bypass API is one path (per-query) instead of two. Smaller surface area for external consumers; less documentation needed. * test_get_without_bypass_filters_out_soft_deleted_row pins the baseline that .get() does fire the listener when forced through SQL (via session.expunge_all()), proving the per-query bypass is exercised through the real path rather than being short-circuited by the identity-map cache. The previous test_per_request_bypass_via_get_finds_soft_deleted_row was actually a tautology — it was passing via the identity-map cache, not via the listener bypass. Replaced with the baseline test plus the already-empirically-validated per-query test. Bypass primer updated to remove the per-request bypass section. Decision tree collapses to a single path. Listener docstring updated to describe the single opt-out path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two small follow-ups from the SQLAlchemy-lens review: 1. pre_get_list now resets g._augment_response_with_deleted_at after reading it. Today the flag is request-scoped and dies at request end, but a batch endpoint dispatching multiple list views in one request could leak the flag between them. Read-and-clear keeps the semantics explicit. 2. raise_for_ownership docstring expanded to flag the bypass propagation through subsequent lazy loads. The execution_options on the re-query also affects lazy loads from orig_resource (per with_loader_criteria semantics). Today only owners (User, not SoftDeleteMixin) is read, so the propagation is harmless. Document it so a future contributor doesn't accidentally extend the function to access soft-deletable relationships and silently get the bypass cascading. Co-Authored-By: Mike Bridge <michael.bridge@ext.preset.io>
Three structural follow-ups from the clean-code review:
1. SRP — extract SoftDeleteApiMixin from BaseSupersetModelRestApi
The augmentation methods (_serialize_deleted_at, _get_deleted_at_map,
and the augmenting pre_get_list) lived on BaseSupersetModelRestApi,
coupling the FAB REST API base class to soft-delete semantics.
Moved them into a new SoftDeleteApiMixin in views/filters.py
(alongside BaseDeletedStateFilter, the filter that signals the
augmentation). Concrete REST API classes for soft-deletable entities
opt in by inheritance:
class ChartRestApi(SoftDeleteApiMixin, BaseSupersetModelRestApi):
...
BaseSupersetModelRestApi loses its soft-delete knowledge entirely.
The mixin chains via super().pre_get_list(data) so other behaviour
in the inheritance chain still runs.
2. Naming — rename not_deleted() to where_not_deleted()
The method returns a SQL ColumnElement, not a Python bool. The name
"not_deleted" reads like a predicate, which is misleading. Renamed
to where_not_deleted() to make the SQL-WHERE-clause intent explicit
at call sites: session.query(Slice).filter(Slice.where_not_deleted()).
Updated the only in-tree caller (the test exercising the predicate).
3. Encapsulate Conditional — extract _should_apply_soft_delete_filter
The listener's four-condition guard (is_select + not column_load +
not relationship_load + not skip_visibility_filter) is now a named
helper. The listener body is a one-liner; the helper's docstring
carries the full rationale (citing the SQLAlchemy issue with the
guards).
Also DRYed the criterion lambda: now uses cls.where_not_deleted()
instead of duplicating cls.deleted_at.is_(None). The predicate is
defined once on the mixin and reused by the listener.
Deferred from the review: the skip_visibility_filter selector argument
on BaseDAO methods. The same shape (skip_base_filter) is established
convention in the codebase; splitting both selectors into separate
named methods is a larger refactor outside this PR's scope.
Co-Authored-By: Mike Bridge <michael.bridge@ext.preset.io>
Tests
* New tests/unit_tests/commands/test_base_restore_command.py pins the
BaseRestoreCommand.validate contract directly, without going through
a concrete entity command. Five tests cover the four exits and the
DAO-load shape:
- raises not_found_exc when the DAO returns None
- raises not_found_exc with a distinguishable message when the row
is live (deleted_at is None)
- returns the model when soft-deleted and owned (admin path)
- raises forbidden_exc when raise_for_ownership rejects
- confirms the DAO is called with id_column="uuid",
skip_base_filter=True, skip_visibility_filter=True
These would have caught the bug codeant flagged on the first review
round (when the per-request bypass wasn't yet in place) and gives
refactors fast local feedback rather than waiting for entity-branch
integration CI.
Small tidyings (from the tidy-first review):
* find_existing_for_import: modernized typing (Type[Any] -> type[Any],
Optional[Any] -> Any | None) for consistency with the rest of the
diff.
* find_existing_for_import: moved SKIP_VISIBILITY_FILTER import to
module level. No circular-import concern (verified — models.helpers
doesn't import from commands.importers).
* _should_apply_soft_delete_filter: extracted two explaining
variables (caller_opted_out, is_primary_user_select). The reader
meets the two sub-conditions as named concepts before seeing them
combined.
Co-Authored-By: Mike Bridge <michael.bridge@ext.preset.io>
The per-query ``execution_options(skip_visibility_filter=True)`` bypass is bound to a specific Query instance — it does not survive code paths that build a derived query from a fresh ``session.query(Model)``. Flask-AppBuilder's ``SQLAInterface.get_outer_query_from_inner_query`` does exactly that when ``list_columns`` contains many-to-many fields (charts, dashboards, datasets all hit this): inner query carries the bypass, becomes a ``.subquery()``, outer query is built fresh from ``session.query(self.obj)`` and joined to the subquery, listener fires on the outer query without the bypass option, attaches its ``with_loader_criteria(where_not_deleted)``, and the two predicates collide to zero rows. Live testing on PR apache#39977 reproduced this on ``GET /api/v1/dashboard/?...&dashboard_deleted_state=only`` — ``count`` is 1, ``result`` is empty. Other framework or future code paths could lose the per-query option the same way (Query.from_self(), custom DAO rebuilds, ad-hoc subquery composition). Patching FAB's specific method would fix the known site but leave the rest of the surface unguarded. The fix moves the bypass off the Query and onto the **session**, via ``session.info[SKIP_VISIBILITY_FILTER_CLASSES]``. The listener reads this directly from ``execute_state.session.info`` regardless of how the statement was constructed, so the bypass survives FAB's reconstruction (and any future framework that strips per-query options). The bypass is keyed on a **set of classes** rather than a global boolean. Each ``BaseDeletedStateFilter`` subclass adds only its own ``model`` to the set, so a "deleted dashboards" list response does not unhide soft-deleted Slice or SqlaTable rows even when those entities become soft-deletable (sc-106189 / sc-106191). Listener changes (superset/models/helpers.py): * ``SKIP_VISIBILITY_FILTER`` (boolean key) replaced by ``SKIP_VISIBILITY_FILTER_CLASSES`` (set-of-classes key). * ``_collect_bypass_classes`` unions per-query (``execution_options``) and per-session (``session.info``) sources. * Listener iterates concrete ``SoftDeleteMixin`` subclasses via ``_all_soft_delete_subclasses()`` (transitive) and attaches a ``with_loader_criteria(cls, cls.where_not_deleted(), ...)`` only for those NOT in the bypass set. Concrete SQL expressions rather than callables: passing a callable would trigger SQLAlchemy's ``DeferredLambdaElement`` parsing, which does not support Python control flow like ``if cls in bypass_classes``. * New ``skip_visibility_filter(session, *classes)`` context manager for narrow-scope programmatic opt-in with guaranteed cleanup. Filter (superset/views/filters.py): * ``BaseDeletedStateFilter.apply`` adds ``self.model`` to ``session.info[SKIP_VISIBILITY_FILTER_CLASSES]`` via the new ``_add_session_bypass`` helper. Stale docstring about per-request ``g.skip_visibility_filter`` removed; scope-decision block rewritten to describe the actual current mechanism and the FAB inner/outer reason for choosing session-scoped over per-query. * ``SoftDeleteApiMixin._get_deleted_at_map`` uses the new key with a one-class set for the entity's own model. Programmatic callers (DAOs, security manager, importer): * ``BaseDAO.find_by_id`` / ``find_by_uuid`` / ``find_by_ids`` / ``_find_by_column`` and ``DatabaseDAO.find_by_id`` translate the boolean ``skip_visibility_filter=True`` kwarg into ``execution_options(SKIP_VISIBILITY_FILTER_CLASSES={cls.model_cls})``. Public API stays boolean for ergonomics. * ``security_manager.raise_for_ownership`` uses ``{resource.__class__}`` on the internal re-query. * ``find_existing_for_import`` uses ``{model_cls}`` on the UUID lookup. Tests (tests/unit_tests/models/test_soft_delete_mixin.py): Rewritten around the per-class semantics. Adds a second synthetic soft-deletable model and a parent/child pair so isolation and relationship-load propagation tests are real, not theoretical: * test_per_query_class_bypass_returns_soft_deleted_rows — basic per-query bypass via execution_options. * test_per_query_bypass_for_one_class_does_not_unhide_other — pins per-class scoping at the listener level. * test_session_bypass_survives_query_reconstruction — reproduces the FAB outer/inner shape (inner query as subquery, outer query fresh from session.query() joined to the subquery) and proves session.info bypass survives it. This is the test that would have caught the original bug. * test_session_bypass_does_not_leak_across_classes — same isolation guarantee, session-level vehicle. * test_context_manager_adds_and_removes_bypass and test_context_manager_nested_preserves_outer_scope — context manager lifecycle. * test_relationship_load_filters_child_independently_of_parent_bypass — verifies ``with_loader_criteria``'s ``propagate_to_loaders=True`` carries the per-class criteria to lazy/selectin loads, so a bypass for one entity does not unhide its (un-bypassed) children. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-ups from the SQLAlchemy-lens review of the per-entity bypass refactor (commit 69af6c7): * ``_all_soft_delete_subclasses()`` now returns a list sorted by ``__qualname__`` instead of a set. Set iteration is hash-order, and type hashes derive from ``id()`` which differs per process — so the same logical query could have its ``with_loader_criteria`` options attached in different orders across app processes, fragmenting SQLAlchemy's compiled-statement cache. Negligible at 1 soft-delete class today; gets real once SIP-208 expands to ~10 entities and the listener fires on every primary SELECT. * ``_collect_bypass_classes`` returns a shared empty ``frozenset`` sentinel (``_NO_BYPASS``) on the common path. Previously every ORM SELECT allocated 4 empty sets (two for ``.get(..., set())`` defaults and two for the ``set(per_query) | set(per_session)`` union) even when no bypass was in effect. The new fast path is a single dict lookup with a shared sentinel; allocation only happens when there's an actual bypass. * Listener docstring gains a Performance note acknowledging the linear-in-N cost (one ``with_loader_criteria`` option per non-bypassed subclass on every primary SELECT) so the next person profiling a hot endpoint isn't surprised. * ``_all_soft_delete_subclasses`` docstring notes the eager-import assumption: Superset imports models eagerly via ``superset.models``; a future lazy-import refactor would silently break the listener for un-imported classes. * ``skip_visibility_filter`` docstring notes that calling with zero classes is a deliberate no-op. * New ``test_listener_does_not_affect_non_soft_deletable_queries`` pins the listener's no-op behavior for queries against classes that don't inherit ``SoftDeleteMixin`` — protects against a class of regression where future listener changes could silently break unrelated queries. 24 unit tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four items from the 2026-05-14 review: 1. ``find_existing_for_import`` split into pure lookup + explicit ``clear_soft_deleted_for_import``. Previously the helper hard-deleted any soft-deleted match as a side effect of "find," which meant the destructive action fired before the importer had finished overwrite / permission validation. Today's flow is safe because import is gated by ``can_write`` at the top, but it's a forward-looking footgun: a future change that adds an overwrite-path permission check wouldn't realize the soft-delete path silently bypasses it. Splitting moves the destructive call to the call site where it's explicit and audit-able. 2. ``BaseRestoreCommand.validate`` no longer passes ``skip_base_filter=True`` to the DAO. Existing delete paths load through ``find_by_ids`` *with* the per-entity ``base_filter``, then run ownership; restore now matches. The base filters (DashboardFilter etc.) are RBAC — they don't filter on ``deleted_at``, so leaving them on is correct, and the visibility-filter bypass remains the only escape hatch needed. 3. Transaction wrapping moved into the base ``run()``. The previous contract required every concrete restore command to override ``run()`` ONLY to add ``@transaction(on_error=...)``, which was fragile — every new entity rollout had to remember. Subclasses now declare a ``restore_failed_exc: ClassVar`` and the base ``run()`` builds the transactional wrapper at call time, using that ClassVar as the re-raise type. Entity restore commands become 4 ClassVars with no methods. 4. ``SoftDeleteMixin`` docstring corrected: ``BaseDAO.delete()`` now routes through the mixin to ``soft_delete()``; ``BaseDAO.hard_delete()`` is the permanent path. Tests: * ``test_validate_calls_dao_with_visibility_bypass_only`` replaces ``test_validate_calls_dao_with_skip_visibility_filter`` and asserts the DAO call no longer includes ``skip_base_filter=True``. * ``test_run_calls_model_restore_on_success`` covers the happy path through the new base ``run()``. * ``test_run_translates_sqlalchemy_errors_via_restore_failed_exc`` pins the transaction wrapping: a ``SQLAlchemyError`` raised during the unit of work is translated to the subclass's ``restore_failed_exc``. Prevents a future refactor from silently dropping the wrapper or pointing it at the wrong exception type. 26 unit tests pass across the soft-delete suite. Entity-rollout PRs (sc-106189, sc-106190, sc-106191) need a follow-up to drop their ``run()`` overrides and the ``@transaction`` import, add ``restore_failed_exc = X``, and switch their v1 importer call sites to use the split ``find_existing_for_import`` + ``clear_soft_deleted_for_import``. That's mechanical and lands in each entity branch separately. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three small extractions to lower the abstraction-level mixing in the
listener and the filter without changing behavior. All three motivated
by Bob Martin's stepdown rule / G28 (encapsulate conditional) / G34
(functions descend only one level of abstraction).
* ``_is_primary_user_select(execute_state)`` extracted from the
listener's compound early-return gate. The three-part OR was readable
but required the reader to translate "not select OR column-load OR
relationship-load" into the intent ("primary user SELECT"). Naming
the predicate also gives the docstring a place to explain the
``propagate_to_loaders`` semantics that justify excluding the latter
two.
* ``_opt_into_deleted_state(query)`` extracted from
``BaseDeletedStateFilter.apply`` so the two-step opt-in shared by the
``include`` and ``only`` branches stops being duplicated inline. The
selector argument is forced by FAB's ``BaseFilter.apply(query, value)``
contract — we can't push polymorphism up — but the duplication inside
the branches was fixable.
* ``_consume_augmentation_flag`` + ``_inject_deleted_at`` extracted from
``SoftDeleteApiMixin.pre_get_list`` so the method reads as policy at
one level of abstraction (super → consume flag → inject) instead of
mixing policy with the underlying mechanism (getattr/setattr/zip).
No behavior change. 26 unit tests pass unmodified — the contract is
unchanged; only the internal structure shifted.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two Python-3.10 mypy errors flagged by ``pre-commit (previous)`` on CI (Python 3.11 / current was clean): * ``_all_soft_delete_subclasses() -> list[type]`` was too broad — mypy 3.10 couldn't see that the loop variable in ``_add_soft_delete_filter`` was a ``SoftDeleteMixin`` subclass, so ``cls.where_not_deleted()`` was flagged as ``"type" has no attribute "where_not_deleted"``. Narrowed to ``list[type[SoftDeleteMixin]]`` so the classmethod resolves on the inferred type. * The ``# type: ignore[misc, valid-type]`` on ``_SoftDeletableChild`` was on the ``class`` declaration line, but the inheritance list (where ``_TestBase`` is referenced) had wrapped to the next line, putting the actual errors out of the ignore's scope. Collapsed the declaration so the ignore applies to the same line as the ``_TestBase`` reference, matching the pattern used by the sibling synthetic models in the file. 26 unit tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Eight items from the 2026-05-20 review on the infrastructure PR: * **H1 — subclass walk cached via ``__init_subclass__``.** The previous ``_all_soft_delete_subclasses()`` walked ``SoftDeleteMixin.__subclasses__()`` on every primary ORM SELECT. With one adopter today the cost is negligible, but the listener fires on every query in the app and the walk grows linearly with each adopted entity. ``SoftDeleteMixin`` now maintains a sorted ``_registered_subclasses`` list updated by ``__init_subclass__`` at class-definition time; the listener reads the cached list with no walk. Sort happens once per registration (rare event) rather than per query. * **M3 — ``SoftDeleteMixin`` import in ``BaseDAO.delete`` moved to top.** ``SKIP_VISIBILITY_FILTER_CLASSES`` from the same module was already top-level; ``SoftDeleteMixin`` had no real reason to be inline. Cleaner. * **M4 — pylint-disable in ``raise_for_ownership`` documented.** A top-level import here actually does cause a circular (security ↔ models.core ↔ superset's lazy ``feature_flag_manager``). Kept the inline import but added a comment naming the cycle so future maintainers don't try to "fix" it and re-trip the issue. * **M5 — ``BaseDeletedStateFilter.model`` typed as ``ClassVar[type[SoftDeleteMixin]]``.** Previously ``Any``, which meant a subclass accidentally binding ``model`` to a non-soft-deletable entity would crash at runtime on ``.deleted_at`` rather than failing mypy. * **M6 — listener placement documented.** ``setup_soft_delete_listener`` is called outside the ``with app_context()`` block because the ``do_orm_execute`` hook attaches to the ``Session`` class (process-wide), not to a Session instance, so it doesn't need Flask app context. Added an explaining comment in ``init_app_in_ctx``'s caller; ``setup_db()`` already ran earlier so the Session import is initialised. * **M7 — unit tests for ``BaseDeletedStateFilter`` + ``SoftDeleteApiMixin``.** New ``tests/unit_tests/views/test_soft_delete_filter.py`` (10 tests): filter no-op / include / only / case-insensitivity / unknown value; mixin no-op-without-flag / inject-with-flag / consume-flag-on-call; ``_serialize_deleted_at`` None / datetime handling. * **Nit — explicit ``orig_resource is None`` guard in ``raise_for_ownership``.** Previously fell through to ``hasattr(None, "owners")`` returning ``False`` → ``owners = []`` → generic ownership error. Now raises the proper "resource removed before ownership could be verified" error so the cause is clear in the rare race-with-hard-delete case. * **Nit — unit tests for ``find_existing_for_import`` / ``clear_soft_deleted_for_import``.** New ``tests/unit_tests/commands/importers/v1/test_find_existing_for_import.py`` (5 tests): no-match-returns-None, live-row-as-is, soft-deleted-row-as-is (pure lookup, no side effect), ``clear`` hard-deletes via ORM (listener-firing path), and the composed find-then-clear contract that's the documented caller sequence. Deferred with explanation in the PR-comment reply (not in this commit): * **H2 — non-restorable / GDPR hook**: V2 concern. Designing the interface without a concrete use case risks getting it wrong; follow-up ticket if compliance work is ever scoped. * **M8 — ``deleted_at`` schema mutation**: documented in the entity- rollout PR bodies (apache#40128/apache#40129/apache#40130) rather than blocking infrastructure. 40 unit tests pass (was 26; added 14 covering filter, mixin, importer helpers). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…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>
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>
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>
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>
Addresses betodealmeida's review nit on PR apache#39977: apache#39977 (comment) Per PEP 257, a docstring's first line should be a self-contained one-line summary so IDE tooltips and auto-doc tooling show useful text. Both helpers in superset/commands/importers/v1/utils.py wrapped their first sentence across two lines, causing tooltips to display a trailing comma fragment. Rewrote the leading line of: - find_existing_for_import: "Look up an existing row by UUID for an import, including soft-deleted matches." - clear_soft_deleted_for_import: "Hard-delete a soft-deleted row to free its UUID for re-import." The rest of each docstring is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review Agent Run #98adfdActionable Suggestions - 0Additional Suggestions - 2
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 |
5 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).