feat(soft-delete): deletion-retention purge of soft-deleted entities - #41549
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 #41549 +/- ##
==========================================
+ Coverage 65.27% 65.34% +0.07%
==========================================
Files 2795 2807 +12
Lines 157835 158959 +1124
Branches 36080 36234 +154
==========================================
+ Hits 103024 103869 +845
- Misses 52818 53048 +230
- Partials 1993 2042 +49
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:
|
078d330 to
5c6ffc4
Compare
5c6ffc4 to
06769f6
Compare
06769f6 to
dde6b98
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
05ca6de to
2725c6e
Compare
![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 #faba71
Actionable Suggestions - 3
-
tests/unit_tests/tasks/test_deletion_retention.py - 1
- datetime.now without timezone · Line 187-191
-
superset/tasks/deletion_retention.py - 1
- Move try-except outside loop · Line 180-180
-
tests/integration_tests/deletion_retention/purge_tests.py - 1
- Missing timezone in datetime.now() call · Line 197-197
Additional Suggestions - 11
-
superset/commands/deletion_retention/purge_cascade.py - 6
-
Missing unit tests for _dataset_permission_name · Line 375-384The new `_dataset_permission_name` function (lines 375–384) has no unit tests. Per BITO.md rule [11730], this helper that constructs a permission string from entity attributes must be tested to ensure it produces the correct identifier used by _cleanup_dataset_permission.
-
Missing unit tests for _cleanup_dataset_permission · Line 387-403The new `_cleanup_dataset_permission` function (lines 387–403) has no unit tests. Per BITO.md rule [11730], this permission-cleanup mutation that calls `security_manager._delete_pvm_on_sqla_event` with raw SQLAlchemy connection must be tested to ensure the PVM is correctly removed and no exceptions are raised.
-
Missing unit tests for _entity_version_targets · Line 405-434The new `_entity_version_targets` function (lines 405–434) has no unit tests. Per BITO.md rule [11730], this helper that determines which version shadow tables are scoped to an entity must be tested for each model type (Dashboard, Slice, SqlaTable) to ensure correct table/predicate pairs are returned.
-
Missing unit tests for _delete_version_history · Line 437-501The new `_delete_version_history` function (lines 437–501) has no unit tests. Per BITO.md rule [11730], new mutation logic covering version-history deletions and transaction orphan-sweeping must be tested across success paths, error scenarios, and edge cases — particularly the transaction-sharing scenario that exposes the unflushed-DELETE bug.
-
Missing unit tests for _sweep_orphan_transactions · Line 504-534The new `_sweep_orphan_transactions` function (lines 504–534) has no unit tests. Per BITO.md rule [11730], this orphan-sweeping logic — which directly deletes `version_transaction` rows — must be tested for both the orphaned and non-orphaned cases.
-
Missing unit tests for _delete_owned_children · Line 353-372The new `_delete_owned_children` function (lines 353–372) has no unit tests covering its two code paths (SqlaTable vs non-SqlaTable), the hard-deletion of TableColumn and SqlMetric rows, or the interaction with DB-level CASCADE. Per BITO.md rule [11730], new tools and mutation logic must have dedicated unit tests covering success paths, error handling, and edge cases.
-
-
superset/cli/deletion_retention.py - 2
-
Missing unit tests · Line 1-98BITO.md adaptive rule [11730] calls for comprehensive unit tests for new tools. The underlying `ForcePurgeCommand` and `resolve_retention_window` are tested in `tests/integration_tests/deletion_retention/`, but no CLI-level unit test covers the three click commands in this new module — unlike `thumbnails_test.py` and `guest_token_test.py` in the same directory.
-
Unused variable · Line 31-31Remove the unused `logger` declaration at line 31 — it is never referenced in the file. All commands output via `click.echo()`, so no logging call site exists to warrant a module-level logger.
-
-
tests/integration_tests/deletion_retention/window_tests.py - 1
-
Replace blind exception catch · Line 51-51Catching bare `Exception` is overly broad; catch specific exceptions instead.
-
-
tests/integration_tests/deletion_retention/purge_tests.py - 2
-
Boolean default positional argument issue · Line 52-52The `dry_run` parameter is a boolean with a default value used as a positional argument. Consider making it keyword-only to avoid FBT001/FBT002 violations. Multiple similar issues exist across this file (test methods also have boolean positional arguments with defaults).
-
Dynamically typed Any expressions · Line 187-187The function `lose_parent_delete` uses `typing.Any` for dynamically typed expressions. Multiple similar issues exist (lines 187, 197). Consider using more specific types where possible.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
tests/integration_tests/deletion_retention/window_tests.py - 1
- uuid3 enum type mismatch · Line 48-48
-
superset/commands/deletion_retention/purge_cascade.py - 1
- CWE-20: Missing datasource_type filter on dangling chart query · Line 183-183
Review Details
-
Files reviewed - 18 · Commit Range:
05ca6de..05ca6de- docker/pythonpath_dev/superset_config.py
- superset/cli/deletion_retention.py
- superset/commands/deletion_retention/__init__.py
- superset/commands/deletion_retention/audit.py
- superset/commands/deletion_retention/force_purge.py
- superset/commands/deletion_retention/purge_cascade.py
- superset/commands/deletion_retention/window.py
- superset/config.py
- superset/key_value/types.py
- superset/migrations/versions/2026-06-23_13-00_e7d93a524ff6_add_purge_audit_log.py
- superset/tasks/deletion_retention.py
- tests/integration_tests/deletion_retention/__init__.py
- tests/integration_tests/deletion_retention/_base.py
- tests/integration_tests/deletion_retention/audit_tests.py
- tests/integration_tests/deletion_retention/force_purge_tests.py
- tests/integration_tests/deletion_retention/purge_tests.py
- tests/integration_tests/deletion_retention/window_tests.py
- tests/unit_tests/tasks/test_deletion_retention.py
-
Files skipped - 1
- UPDATING.md - Reason: Filter setting
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ 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
|
Ran a 7-lens local review panel (python / sqlalchemy / committer / preset / clean-code / DDD / tidy-first) before merge, given this PR hard-deletes data. Fixes in ac6890f:
Deferred with tickets to follow: IntegrityError-vs-policy-block anti-corruption + persisted block reason, blocked-entity audit-row growth/dedup, docs/ page. |
Code Review Agent Run #c69970Actionable Suggestions - 0Additional Suggestions - 5
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 |
|
Just noting the bito comments tucked away under the accordion menu. Not sure if you want to nab them now, or in a follow-up:
|
…t name Two nits from the bito suggestions @rusackas surfaced on apache#41549: - The dev Celery config schedules deletion_retention.purge_soft_deleted but SOFT_DELETE is off by default, so the task runs and purges nothing. Note the gate where a developer reading the schedule will see it. - test_force_purge_reports_relationship_count_before_db_cascade asserts on removed_dashboard_slices; 'reports' reads as ReportSchedule in this codebase, so name the test after what it actually counts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks for digging those out of the accordions — two of them were worth taking, and they're in
Two I'd push back on:
CLI-level tests — agreed, and taking it as a follow-up. The gap is real: One thing worth flagging since you're here: your approval is from 07-13, and the head has moved a fair bit since — a 7-lens review round hardened the purge (cutoff-clock semantics reverted to match |
Code Review Agent Run #3f2fedActionable 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 |
|
Indeed, the review is/was old, but still LGTM in general. Still open things that are not blockers, but I'm curious how/when you want to track or handle them (is there an overarching tracking issue, perhaps?):
|
|
Thanks for re-confirming — and yes, both are now tracked. There wasn't an overarching issue for them before, which was a fair thing to call out; there is now:
That second one is the interesting one, and I've noted on the ticket that it should land before the cascade grows to more entity types — the registry is much cheaper to introduce while the branch count is still small. The explicit cascade itself stays: it's what keeps purge consistent when SQLite FK enforcement is off, preserves ordinary deletion blockers, avoids traversing large relationship graphs, and handles Continuum transactions being shared across entities. The registry just moves the knowledge out of reviewers' heads and into something CI can fail on. Both are on the internal versioning epic. Happy to mirror the tripwire one as a GitHub issue if you'd rather it were visible to the community — it's arguably a general Superset maintainability concern rather than a Preset one, so say the word and I'll open it. |
…ansactions A shadow row references version_transaction through either end of its lifespan: transaction_id (created at) and, once a later edit closes it, end_transaction_id (closed at). Both are foreign keys. The orphan sweep only counted the created-at side, so a transaction still referenced through a survivor's closing endpoint was judged orphaned and deleted, failing the foreign key. Transactions spanning entities are the normal case, not an edge case: one flush saving a dashboard, chart and dataset gives all three a shadow row at the same transaction. Editing any one of them closes its row there and anchors the others' end pointers, so the hazard appears as soon as two entities are ever saved together and one is edited again. The failure was also misreported. cascade_hard_delete catches IntegrityError alongside PurgeBlockedError, so the operator was told the entity was blocked by existing deletion rules -- a deliberate policy decision -- when the cascade was simply incomplete. That masquerade is tracked separately as sc-115342; this commit removes the instance of it that made versioned entities unpurgeable. version_history_retention hit the identical trap and documents it on _delete_for_transactions; the reasoning there applies verbatim here. Found by the recently-archived archive-listing test on apache#41550, which began exercising this path for the first time once its fixtures were repaired. apache#41549's own suite had no test that purged an entity carrying version history through the API, which is why CI was green. forge_version_row now takes an optional end_tx_id so the regression test can build the shape; the test fails without the production change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t the chart "View version history" was gated on can_overwrite alone, which hydrateExplore computes purely as "the current user is among the slice's editors". A chart with no explicit editors yields false for everyone, so on a fresh install -- where every seeded and example chart has an empty editors list -- the menu item was absent for all users, administrators included. The feature was effectively invisible until someone hand-edited a chart's editors. The server disagrees with that gate. security_manager.is_editor returns True for admins outright, and unions extra_editors (granted by a deployment's EXTRA_EDITORS_RESOLVER) into the editor set before comparing. So the UI was hiding an action the API would have allowed, for two distinct groups of users. SaveModal already worked around the same gap with a broader predicate, which is why an admin could save over a chart but not see its history. That predicate is now a shared helper both call sites use, so the two cannot drift apart again: canOverwrite || admin || in editors || in extra_editors, and never for an externally managed slice Two things fixed while extracting it. SaveModal's isCurrentUserOwner fallback checked slice.owners, which the owners -> editors rename left behind: Slice.data ships editors, extra_editors and viewers, and no owners at all, so that branch could never fire. It is dropped rather than carried into the shared helper. And extra_editors was not consulted anywhere in the frontend despite being in the payload for exactly this purpose. Unit tests mocked redux with can_overwrite: true, so the gate was never exercised against what hydrateExplore actually computes. The new helper tests cover the real shapes, starting with an admin on a chart whose editors list is empty. Reported from local testing against an integration branch of apache#42469 + apache#41549 + apache#41551 + apache#41550. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Heads-up that The orphan-transaction sweep counted a The way it failed is the worse half. This is the maintenance risk you named on 07-13, arriving through a column rather than a missing table. Worth noting how it surfaced: this PR's own suite never purged an entity carrying version history through the API, so CI was honestly green while the feature's core operation was broken for exactly the case it exists to serve. It only appeared because a test on the recovery-UI PR started exercising the path today. There's now a regression test here that fails against the old code. CI is green on the new head. No obligation to re-read — the change is ~10 lines plus a test — but you're welcome to, and I'd rather you heard it from the PR than found it later. |
…t the chart "View version history" was gated on can_overwrite alone, which hydrateExplore computes purely as "the current user is among the slice's editors". A chart with no explicit editors yields false for everyone, so on a fresh install -- where every seeded and example chart has an empty editors list -- the menu item was absent for all users, administrators included. The feature was effectively invisible until someone hand-edited a chart's editors. The server disagrees with that gate. security_manager.is_editor returns True for admins outright, and unions extra_editors (granted by a deployment's EXTRA_EDITORS_RESOLVER) into the editor set before comparing. So the UI was hiding an action the API would have allowed, for two distinct groups of users. SaveModal already worked around the same gap with a broader predicate, which is why an admin could save over a chart but not see its history. That predicate is now a shared helper both call sites use, so the two cannot drift apart again: canOverwrite || admin || in editors || in extra_editors, and never for an externally managed slice Two things fixed while extracting it. SaveModal's isCurrentUserOwner fallback checked slice.owners, which the owners -> editors rename left behind: Slice.data ships editors, extra_editors and viewers, and no owners at all, so that branch could never fire. It is dropped rather than carried into the shared helper. And extra_editors was not consulted anywhere in the frontend despite being in the payload for exactly this purpose. Unit tests mocked redux with can_overwrite: true, so the gate was never exercised against what hydrateExplore actually computes. The new helper tests cover the real shapes, starting with an admin on a chart whose editors list is empty. Reported from local testing against an integration branch of apache#42469 + apache#41549 + apache#41551 + apache#41550. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code Review Agent Run #91f117Actionable 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 |
|
@mikebridge there are some fresh bot findings with "critical" reports... can you assess and resolve those (whether applying the fix or refuting them) before merging? |
…t the chart "View version history" was gated on can_overwrite alone, which hydrateExplore computes purely as "the current user is among the slice's editors". A chart with no explicit editors yields false for everyone, so on a fresh install -- where every seeded and example chart has an empty editors list -- the menu item was absent for all users, administrators included. The feature was effectively invisible until someone hand-edited a chart's editors. The server disagrees with that gate. security_manager.is_editor returns True for admins outright, and unions extra_editors (granted by a deployment's EXTRA_EDITORS_RESOLVER) into the editor set before comparing. So the UI was hiding an action the API would have allowed, for two distinct groups of users. SaveModal already worked around the same gap with a broader predicate, which is why an admin could save over a chart but not see its history. That predicate is now a shared helper both call sites use, so the two cannot drift apart again: canOverwrite || admin || in editors || in extra_editors, and never for an externally managed slice Two things fixed while extracting it. SaveModal's isCurrentUserOwner fallback checked slice.owners, which the owners -> editors rename left behind: Slice.data ships editors, extra_editors and viewers, and no owners at all, so that branch could never fire. It is dropped rather than carried into the shared helper. And extra_editors was not consulted anywhere in the frontend despite being in the payload for exactly this purpose. Unit tests mocked redux with can_overwrite: true, so the gate was never exercised against what hydrateExplore actually computes. The new helper tests cover the real shapes, starting with an admin on a chart whose editors list is empty. Reported from local testing against an integration branch of apache#42469 + apache#41549 + apache#41551 + apache#41550. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…in identity Three findings from the codeant pass. Force-purge resolved a bare UUID by scanning every soft-delete model and taking the first match. UUID uniqueness is only enforced per table, and the import APIs accept caller-supplied UUIDs, so that search can be genuinely ambiguous -- and an operator running a compliance deletion would have no way to say which entity they meant. It now collects every match and refuses when there is more than one, naming the types so the caller can disambiguate. A new model_cls argument lets a caller that already knows the type pin resolution to it, which is what any caller acting for an end user must do: their authorization was checked against one specific entity. require_archived is added alongside it, because the cascade runs with enforce_window=False here and so skips its own deleted_at check. Dry runs called audit.reconcile_pending(), which finalizes stale pending rows -- a durable write. An operator sizing up a rollout was altering the audit record they were inspecting. Reconciliation is now skipped when dry_run is set. The retention task snapshots an entity's uuid, writes the audit row, then re-resolves by integer id. It did not check that the row it got back was the row it described. Worth being precise about the severity: the cascade's conditional claim re-checks deleted_at under lock, so a live entity could never have been purged this way. What could drift is attribution -- if an id were reused between the two resolutions (SQLite recycles rowids; sequences do not) and the new occupant were itself eligible, the purge would proceed while the audit named the previous object. An audit row that identifies the wrong entity is worse than a skipped purge, so identity is now verified before the cascade. Each test fails against the previous code: the dry-run assertion sees reconcile called once, and the ambiguity tests cannot import a symbol that did not exist. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@rusackas assessed all three — two are real and fixed, and the third is real but not at the severity it was given. Ambiguous UUID resolution in force-purge (Critical — valid, fixed). A Dry runs finalizing audit rows (Major — valid, fixed). Identity re-check in the retention task (Critical — valid, but the severity overstates it). The finding is correct that But the destructive half of the claim doesn't hold. What could actually drift is attribution: if an id were reused between the two resolutions and the new occupant were itself soft-deleted and past the cutoff, the purge would proceed while the audit row named the previous object. That needs id reuse, which SQLite does with rowids but Postgres and MySQL sequences do not. So: a real hole in the audit trail, not "concurrent purge can delete the wrong entity". Fixed regardless — an audit row that identifies the wrong entity is worse than a skipped purge, and the check is one comparison. Each new test fails against the previous code: the dry-run assertion sees |
|
Correction to an earlier comment of mine on this PR. I wrote that
The fix — carrying the predicate into both statements — is on the follow-up branch for #41550, along with a control run showing the regression test fails without it. Two of the tests I pointed at as covering this were also passing for the wrong reason (one exited at a Nothing here changes what this PR does — flagging it because my earlier description of it was inaccurate. |
…e CLI Two follow-ups from the review pass, both on the operator path. The ambiguity refusal had no way out. ForcePurgeCommand raises AmbiguousPurgeTargetError telling the operator to "pass the entity type to disambiguate", but the CLI -- its only reachable caller, since the command's other users pin model_cls -- exposed no such option and did not catch the exception. The operator got a raw traceback, and got it *after* answering the irreversible confirmation prompt: precisely the failure mode the type=click.UUID validation a few lines above was added to prevent. force-purge now takes --type/-t and reports the refusal as a ClickException naming the flag. The identity guard in _purge_one had no test. Deleting the entity_uuid(entity) != entity_uuid_value check left every test green, so nothing pinned the invariant that an audit row must name the entity actually purged. Covered by driving a uuid change between the audit write and the re-check, and asserting the cascade does not run and the audit row is failed. Controls: each new test fails against the pre-fix source (CLI reverted to the bare ForcePurgeCommand(str(uuid)).run() with no handler; guard replaced by a constant-false branch) and passes with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed one more commit ( The ambiguity refusal had no escape hatch. The identity guard in Each new test has a verified control: it fails against the pre-fix source and passes with the fix. Reverting the CLI to the bare @eschutho @rusackas — re-request below since this touches the CLI you'd already approved. Happy to split it into a follow-up PR instead if you'd rather not re-review; the tradeoff is that master would briefly ship a |
…its id Closes the id-reuse race rather than narrowing it. The retention task snapshots an entity's uuid, writes a write-ahead audit row, then re-resolves by id and cascades. An id freed and reissued in that gap would be purged while the audit row named the original -- an audit trail that identifies the wrong object. _purge_one already re-checked the uuid before cascading, but that check sits above the row lock, so it narrowed the window instead of closing it. The locked claim and the conditional delete now carry the uuid too, via _identity_predicates, so a reused id matches no row at all. Both callers get this: force-purge resolved by uuid to begin with, and the predicate simply restates what it already established. Extracted as a helper rather than inlined because cascade_hard_delete sits exactly at the C901 limit; the helper adds no branch to it and gives the claim and the delete one definition of identity instead of two. Falls back to the id alone for a model with no uuid column. Control: the new test fails with the uuid predicate removed (the stranger is destroyed) and passes with it. Two earlier drafts of that test passed in both directions -- one committed inside the cascade's savepoint so the row survived on a broken transaction, the other detached the entity and died on DetachedInstanceError -- so the assertion now desynchronizes the in-memory snapshot from the stored row with set_committed_value and calls the cascade directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code Review Agent Run #c21dfdActionable Suggestions - 0Additional Suggestions - 3
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 |
…-positive tests The review pass found that three tests added for the restore/purge race and the bootstrap window did not exercise the code they named. Each is fixed and each now has a verified control run: it fails against the pre-fix source and passes against the fix. 1. require_archived reaches the cascade, not just resolution. ForcePurgeCommand ran the cascade with enforce_window=False, and both the locked claim and the conditional delete added deleted_at predicates only under enforce_window. require_archived therefore narrowed the window between authorization and purge without closing it -- a restore committing after the resolve could still have its live row hard-deleted. The flag is now carried into both statements. An earlier commit message on this branch and a PR comment on apache#41549 both claimed this race was already closed. It was not; that claim was wrong. 2. test_force_purge_refuses_a_row_restored_after_resolution was green either way. run() resolves twice and only the second result reaches the cascade, so restoring during the first call made the second resolve return None under require_archived and the command exited at its not-found guard without running the cascade at all. The restore now lands after the final resolve. Control: fails with the cascade predicates disabled. 3. test_purge_of_restored_chart_reports_not_found never constructed ForcePurgeCommand. It patched PurgeArchivedCommand.validate, whose return value is discarded -- validate reports through self._model -- so run() exited at the "cannot happen" guard marked pragma: no cover and returned 404 from a defensive branch. It now hooks _resolve and lets the real validate run. Control: fails with require_archived=False. 4. The four bootstrap window tests shared one cache entry. cached_common_bootstrap_data is memoized for 60s on (user_id, locale) alone and the branches under test live inside the memoized body, so the first test's payload was served to the rest. They now call the .uncached body directly rather than clearing the cache, which would tie the assertions to a reachable Redis. Control: all four fail against the pre-fix bootstrap, including the phantom-key test. Extracting _eligibility_predicates keeps cascade_hard_delete under the C901 limit and gives the claim and the delete one shared definition of eligibility instead of two parallel branch chains. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t the chart "View version history" was gated on can_overwrite alone, which hydrateExplore computes purely as "the current user is among the slice's editors". A chart with no explicit editors yields false for everyone, so on a fresh install -- where every seeded and example chart has an empty editors list -- the menu item was absent for all users, administrators included. The feature was effectively invisible until someone hand-edited a chart's editors. The server disagrees with that gate. security_manager.is_editor returns True for admins outright, and unions extra_editors (granted by a deployment's EXTRA_EDITORS_RESOLVER) into the editor set before comparing. So the UI was hiding an action the API would have allowed, for two distinct groups of users. SaveModal already worked around the same gap with a broader predicate, which is why an admin could save over a chart but not see its history. That predicate is now a shared helper both call sites use, so the two cannot drift apart again: canOverwrite || admin || in editors || in extra_editors, and never for an externally managed slice Two things fixed while extracting it. SaveModal's isCurrentUserOwner fallback checked slice.owners, which the owners -> editors rename left behind: Slice.data ships editors, extra_editors and viewers, and no owners at all, so that branch could never fire. It is dropped rather than carried into the shared helper. And extra_editors was not consulted anywhere in the frontend despite being in the payload for exactly this purpose. Unit tests mocked redux with can_overwrite: true, so the gate was never exercised against what hydrateExplore actually computes. The new helper tests cover the real shapes, starting with an admin on a chart whose editors list is empty. Reported from local testing against an integration branch of apache#42469 + apache#41549 + apache#41551 + apache#41550. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t the chart "View version history" was gated on can_overwrite alone, which hydrateExplore computes purely as "the current user is among the slice's editors". A chart with no explicit editors yields false for everyone, so on a fresh install -- where every seeded and example chart has an empty editors list -- the menu item was absent for all users, administrators included. The feature was effectively invisible until someone hand-edited a chart's editors. The server disagrees with that gate. security_manager.is_editor returns True for admins outright, and unions extra_editors (granted by a deployment's EXTRA_EDITORS_RESOLVER) into the editor set before comparing. So the UI was hiding an action the API would have allowed, for two distinct groups of users. SaveModal already worked around the same gap with a broader predicate, which is why an admin could save over a chart but not see its history. That predicate is now a shared helper both call sites use, so the two cannot drift apart again: canOverwrite || admin || in editors || in extra_editors, and never for an externally managed slice Two things fixed while extracting it. SaveModal's isCurrentUserOwner fallback checked slice.owners, which the owners -> editors rename left behind: Slice.data ships editors, extra_editors and viewers, and no owners at all, so that branch could never fire. It is dropped rather than carried into the shared helper. And extra_editors was not consulted anywhere in the frontend despite being in the payload for exactly this purpose. Unit tests mocked redux with can_overwrite: true, so the gate was never exercised against what hydrateExplore actually computes. The new helper tests cover the real shapes, starting with an admin on a chart whose editors list is empty. Reported from local testing against an integration branch of apache#42469 + apache#41549 + apache#41551 + apache#41550. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4 participants
SUMMARY
Adds permanent deletion retention for soft-deleted dashboards, charts, and datasets.
deletion_retention.purge_soft_deleted, scheduled at midnight, including the shipped Docker development Celery configuration.The prerequisite soft-delete support for charts, dashboards, and datasets is present on
master. This PR adds the scheduled retention pipeline and operator CLI; it does not add a REST API endpoint.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable; this is a backend task, CLI, and database change.
TESTING INSTRUCTIONS
Run the focused unit and integration suite:
Expected result: 44 tests pass.
Inspect the effective operator configuration:
The migration was tested from a clean SQLite integration database and through an explicit downgrade/upgrade round trip:
The upgraded schema has a UUID primary key plus
entity_uuidand(status, created_on)indexes, with one migration head ate7d93a524ff6.A Docker smoke test was also run against the isolated PostgreSQL stack:
8f3a1b2c4d5etoe7d93a524ff6.confirmedaudit result.deletion_retention.purge_soft_deletedthrough Redis to the Docker Celery worker and received the expected safe{"skipped": 1}result whileSOFT_DELETEwas disabled.Changed-file pre-commit hooks pass.
pre-commit run --all-fileswas also executed; its remaining failures are pre-existing errors in semantic-layer test typing, frontend Select/DatePicker typing, and SQL Lab MCP complexity, outside this PR's files.ADDITIONAL INFORMATION
SOFT_DELETE