refactor(deletion-retention): add declarative purge policies - #42888
refactor(deletion-retention): add declarative purge policies#42888mikebridge wants to merge 5 commits into
Conversation
Code Review Agent Run #f96b7dActionable Suggestions - 0Additional Suggestions - 1
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 |
dosubot
Bot
added
change:backend
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
…licy module Simplification pass over the declarative purge-policy registry: - drop the write-only DependencyKey.relationship_aliases field, the discovery-time alias merge that populated it, and its identity test; nothing ever read the aliases - drop ExecutionPhase.SNAPSHOT (referenced nowhere) and the unreachable LISTENER_EFFECT entry in the classification-to-phase map (synthetic listener dependencies always carry an explicit phase) - replace five single-method Protocol classes with one-line Callable aliases; the signatures are all positional, so the protocols bought no extra type safety - cache _validated_purge_policy with lru_cache instead of a hand-rolled module-level dict (parameters annotated as bare type: mypy's functools stubs reject type[Any] against lru_cache's Hashable bound) - resolve each dependency table once and check the inbound-FK guard in one place, passing the table into _dependency_predicates - extract _ownership_edge() for the ownership-path lookup previously duplicated between _owner_value_select and _dependency_owner_depth - reduce the callback-typing test to the phase assertion that can actually fail; assert callable() on typed dataclass fields is enforced by mypy already Verified: 30/30 unit tests, mypy, ruff, changed-file pre-commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The flagged issue is correct. Capturing the permission name before the row is locked creates a race condition where the permission identifier could become stale if the dataset's To implement this, move the permission name capture inside the superset/commands/deletion_retention/purge_cascade.py |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #42888 +/- ##
==========================================
+ Coverage 66.36% 66.38% +0.01%
==========================================
Files 2857 2859 +2
Lines 161048 161367 +319
Branches 37046 37083 +37
==========================================
+ Hits 106886 107116 +230
- Misses 52147 52215 +68
- Partials 2015 2036 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…he permission-capture race Two fixes for review findings on the purge-policy registry PR: 1. CI: test_real_mapper_graph_has_complete_policy failed in full-suite runs with 'Could not locate any simple equality expressions ... on relationship Tag.created_by'. Root cause is the 2018 add_implicit_tags migration script: its throwaway declarative models inherited FAB's AuditMixin, whose created_by/changed_by relationships leave a permanently unconfigurable mapper in the global registry once alembic imports the script (the unit-test app fixture's pending-migration check does exactly that). The first test to trigger configure_mappers() afterwards fails — the new coverage test was simply the first caller. The script now declares the audit columns directly (identical DDL, no relationships), and a regression test imports the script and configures mappers. 2. codeant finding (seconded by bito): the dataset permission name was captured from the in-memory entity before the purge claimed and locked the row, so a rename or database move committed in that window made the cleanup delete a stale permission while orphaning the real one. The capture now runs under the row lock and reads table_name/database_name from the database, and the callback takes (session, policy, entity_id) like every other policy action. Verified: 146 unit (incl. the previously failing suite-order combination), 26 integration purge tests, mypy, changed-file pre-commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the automated review feedback:
Fresh local verification after the latest fix: |
Code Review Agent Run #faf318Actionable Suggestions - 0Additional Suggestions - 1
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 |
1 participant
SUMMARY
Replace the hard-coded soft-delete purge cascade with a declarative policy registry for charts, dashboards, and datasets.
The registry classifies discovered dependencies as owned, association, preserved, blocked, version-owned, or listener-driven, then validates policy completeness before executing Core SQL deletes. It also makes persistent
after_deleteeffects explicit through typed listener declarations and derives version-shadow cleanup from policy metadata.Key safeguards include:
SoftDeleteMixinmodel to have a purge policyBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable; this is a backend retention refactor with no UI changes.
TESTING INSTRUCTIONS
Expected result: 60 passed, 1 opt-in timing benchmark skipped, and all applicable pre-commit hooks pass.
To run the manual elapsed-time protocol, set
SUPERSET_PURGE_BENCHMARK=1and supply the matching merge-base medians through:SUPERSET_PURGE_BASELINE_CHART_SECONDSSUPERSET_PURGE_BASELINE_DASHBOARD_SECONDSSUPERSET_PURGE_BASELINE_DATASET_SECONDSADDITIONAL INFORMATION