feat(soft-delete): gate soft delete behind a temporary SOFT_DELETE release toggle#41166
Conversation
Code Review Agent Run #0cc334Actionable 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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
3318c24 to
71bc1f2
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41166 +/- ##
==========================================
- Coverage 64.60% 64.49% -0.12%
==========================================
Files 2664 2666 +2
Lines 146433 146880 +447
Branches 33859 33913 +54
==========================================
+ Hits 94607 94729 +122
- Misses 50110 50434 +324
- Partials 1716 1717 +1
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:
|
71bc1f2 to
0b47f63
Compare
Code Review Agent Run #dc874cActionable 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
changed the title
0b47f63 to
3d8d479
Compare
Code Review Agent Run #a0c034Actionable 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 |
3d8d479 to
49defd4
Compare
|
The PR itself seems fine (happy to approve on that aspect), but this might warrant some discussion with other orgs, like @michael-s-molina / @villebro The SIP already approved the feature, so we COULD just go full-launch during the 7.0 breaking window, I would think. If we add this flag during the 7.0 cycle, we probably can't change/remove it until 8.0, so I'm not sure if we want that overhead or not. |
|
...unless, of course, your plan is to also remove it during the 7.0 window and it's more temporary than I'm assuming here. I get the difference between the release toggle and feature flag, but it seems like a fine line. If we're going to be releasing 7.0 with it off, and people are flipping it on in prod, with the intent that it becomes True by default in the next major and/or gets removed in 8.0, then it gets a bit muddy, no? I maybe misunderstanding, which is why it might just be worth a 5 minute chat :D |
Soft-delete rollout — related PRs & merge orderThis is one of four coordinated PRs implementing soft-delete + restore per SIP-208, on top of the merged infrastructure (#39977). Merge order matters — the gate must land first so the entity PRs ship behind the
After the gate merges, each entity PR is rebased onto the gated The gate ← this PR |
Adds a temporary rollout / kill-switch feature flag (`SOFT_DELETE`, default off) so the soft-delete substrate can ship dark and be activated per-deploy once validated — separating deploy from release for a behavior change with cross-entity blast radius. This is deployment scaffolding, not a permanent toggle: it is removed once soft delete is stable, leaving the unconditional "always active" end state. Until an operator opts in, deployments retain legacy DELETE (hard-delete) behavior. Two gate points (both no-op to legacy behavior when off): - `BaseDAO.delete()` routes to soft_delete only when the flag is on; otherwise hard_delete. - The `do_orm_execute` visibility listener attaches no criteria when off. Both write- and read-path gate on the same flag, so they cannot diverge. Restore needs no explicit gate: with the flag off no row carries `deleted_at`, so a restore call has nothing to act on. The flag docstring documents the kill-switch semantics (flipping off after rows are soft-deleted resurrects them — emergency stop, not a clean rollback) and that the import pipeline's existing-row detection is gated transitively via the listener. The `deleted_at` migration stays un-gated (additive). Existing soft-delete unit tests are updated to enable the flag; new tests pin the gate-off path: a mixin model hard-deletes and the listener does not filter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
49defd4 to
3029043
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Scans 1–20 run; no violations. Mode: code (6 files, ~70 changed lines). Second opinion not needed — small focused diff, no migrations/multi-tenant/submodule changes.
Gate correctness ✓
Both documented gate points are correctly placed:
-
BaseDAO.delete()routing (daos/base.py:540–548):is_feature_enabled("SOFT_DELETE")is the last condition in theand-chain, so non-SoftDeleteMixinmodels always hard-delete regardless of gate state. Short-circuit evaluation is correct. -
_should_attach_soft_delete_criteria()(models/helpers.py:806–810): Gating the entire return here makes the_add_soft_delete_filterbody unreachable when the flag is off — zerowith_loader_criteriaoptions attached, no per-class allocation. Because the flag defaults toFalse, this is also a perf win for the off-state (the listener exits before the options loop).
Gate completeness ✓
No restore API endpoint exists yet — SoftDeleteMixin.restore() is defined but not exposed via any route — so no third gate point is needed in this PR. When a restore endpoint is added it will need its own gate (or the flag will already be removed by then).
Toggling OFF — no leakage ✓
When OFF: delete path → hard_delete() (no deleted_at rows created); listener → no-op (no filtering). The resurrection edge case (rows soft-deleted while ON become visible when gate turns OFF) is documented in both config.py and feature-flags.json, and is explicitly pinned by test_listener_noop_when_gate_off. That test correctly creates a genuinely soft-deleted row via the autouse fixture (gate ON), then re-queries inside a nested patch(..., return_value=False) to assert re-appearance.
Test coverage ✓ (minor gap — see inline)
The DAO matrix covers 3 of 4 (gate, model_type) combinations; the 4th (gate=OFF + plain) is trivially implied but not explicitly tested.
Toggle naming and placement ✓
"SOFT_DELETE"follows ALL_CAPS convention;lifecycle: developmentmatches comparable in-flight toggles.is_feature_enabled("SOFT_DELETE")call style matches existing usage inhelpers.py:225andcharts/api.py:118.- Config placement in
config.pyis alphabetically correct betweenSEMANTIC_LAYERSandTABLE_V2_TIME_COMPARISON_ENABLED.
The comment change from # pylint: disable=import-outside-toplevel to # avoid circular import: models.helpers <-> daos (base.py:538) is a net improvement — .pylintrc has disable=all so the suppression was always redundant; the descriptive comment is more useful.
No blockers or high-severity issues. Approving. CI passing + all findings NIT level.
@rusackas Yes, the plan is to remove it entirely before the release, because we don't want two paths through the code, doubling the cyclomatic complexity (as per my initial discussion with Max---we just want the feature to be awesome). In my conception, it is totally orthogonal from a feature flag, because it's focused purely on the release, and not intended to give end users choices about functionality. My hope is: nobody ever flips it except me or QA. We could leave it in if we want a MTTR fallback but the best case scenario is that everything is stable enough that we don't need it. |
- Add the missing 4th (gate, model_type) matrix cell: gate=OFF + plain model still hard-deletes (per @aminghadersohi). - Trim the SOFT_DELETE feature-flag description (~660→~290 chars), pointing to config.py for the full rationale (per @aminghadersohi). - Add explicit type annotations on the new test locals (per @codeant). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
If the feature flag gets deleted before the release, it's all good with me.
I'd just note the risk of the project not finishing before the cut, in which case the feature flag would already be stuck in place.
The only way around that might be to merge ALL the features on a feature branch (which would require a lot of rebasing pain). Then, if all the individual PRs going into it are thoroughly reviewed, it could be quick to re-check and merge the whole thing in one shot.
That said, approving, and hoping the flag/switch/toggle (whatever we call it) can disappear soon :D
feature-flags.json is generated from the config.py flag comment, so the earlier trim of the JSON alone failed the feature-flags-sync hook. Trim the source comment instead and regenerate the JSON so the two are in sync (addresses @aminghadersohi's over-long-description nit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review Agent Run #4daa3dActionable 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 |
…tests The soft-delete behaviour is gated behind the temporary SOFT_DELETE rollout flag (superset/daos/base.py, superset/models/helpers.py), which apache#41166 landed on master as off-by-default. These tests were written before the gate and did not enable it, so once rebased onto the gated master every soft-delete assertion failed (rows hard-deleted, deleted-state filters empty, embedded view 404). Decorate each test with @with_feature_flags(SOFT_DELETE=True) so they exercise the gated behaviour.
Soft-delete is gated behind the temporary SOFT_DELETE rollout flag (off by default since apache#41166). Decorate each soft-delete test with @with_feature_flags(SOFT_DELETE=True) so they exercise the gated behaviour once rebased onto the gated master.
Soft-delete is gated behind the temporary SOFT_DELETE rollout flag (off by default since apache#41166). Decorate each soft-delete test with @with_feature_flags(SOFT_DELETE=True) so they exercise the gated behaviour once rebased onto the gated master.
test_delete_dataset_item and test_bulk_delete_dataset_items were updated to assert soft-delete behaviour (FAB view-menu/permissions preserved after a DELETE), but the SOFT_DELETE gate ships off by default (apache#41166). With the flag off the delete is a hard delete that removes the view-menu, so the "is not None" assertions failed. Decorate both with @with_feature_flags(SOFT_DELETE=True), matching soft_delete_tests.py.
Soft-delete is gated behind the temporary SOFT_DELETE rollout flag (off by default since apache#41166). Decorate each soft-delete test with @with_feature_flags(SOFT_DELETE=True) so they exercise the gated behaviour once rebased onto the gated master.
Soft-delete is gated behind the temporary SOFT_DELETE rollout flag (off by default since apache#41166). Decorate each soft-delete test with @with_feature_flags(SOFT_DELETE=True) so they exercise the gated behaviour once rebased onto the gated master.
Soft-delete is gated behind the temporary SOFT_DELETE rollout flag (off by default since apache#41166). Decorate each soft-delete test with @with_feature_flags(SOFT_DELETE=True) so they exercise the gated behaviour once rebased onto the gated master.
Soft-delete is gated behind the temporary SOFT_DELETE rollout flag (off by default since apache#41166). Decorate each soft-delete test with @with_feature_flags(SOFT_DELETE=True) so they exercise the gated behaviour once rebased onto the gated master.
4 participants
SUMMARY
Adds a temporary rollout / kill-switch feature flag —
SOFT_DELETE(default off) — so the soft-delete substrate can ship dark and be activated per-deploy once validated, separating deploy from release for a behavior change with cross-entity blast radius (DELETE semantics + read-path visibility filtering).This is deployment scaffolding, not a permanent toggle: it is removed once soft delete is stable, leaving the unconditional "always active once deployed" end state. Until an operator opts in, existing deployments retain byte-for-byte legacy
DELETE(hard-delete) behavior.Two gate points, both no-op to legacy behavior when off:
BaseDAO.delete()routes tosoft_delete()only when the flag is on; otherwisehard_delete().do_orm_executevisibility listener attaches nodeleted_at IS NULLcriteria when off.Both the write path and the read path gate on the same flag, so they cannot diverge. Restore needs no explicit gate — with the flag off no row carries
deleted_at, so a restore call has nothing to act on. The flag docstring documents the kill-switch semantics (flipping off after rows are soft-deleted resurrects them — emergency stop, not a clean rollback) and that the import pipeline's existing-row detection is gated transitively via the listener.The
deleted_atmigration (from the substrate PRs) stays un-gated (additive). Removal of this flag + its two gate points is a small follow-up once soaked.THIS IS A RELEASE TOGGLE, NOT A FEATURE FLAG
A common and reasonable objection is "feature flags that change functionality are a maintenance hazard." That objection is about long-lived flags — permanent configuration surface that parameterizes product behavior, branches the test matrix indefinitely, and accumulates as config sprawl. This flag is a different category.
In the Continuous Delivery taxonomy this is a Release Toggle: a transitory toggle whose only job is to decouple deploy from release so a not-yet-validated change can land on
masterand ship dark, then be switched on once trusted — and then deleted. It is not a Permission/Experiment/Ops toggle that lives forever.Why the distinction matters here:
So the change is deliberately scoped as scaffolding with a removal plan, default off, both gate points keyed to the same flag so the write and read paths can't diverge. It exists to make a behavior change with cross-entity reach safe to deploy continuously — not to add a permanent toggle to the product.
TESTING INSTRUCTIONS
Automated — unit tests cover both flag states (default-off and on):
FEATURE_FLAGS = {"SOFT_DELETE": False}(default): aDELETEon aSoftDeleteMixinmodel hard-deletes, and the read-path listener does not hide soft-deleted rows.SOFT_DELETEon:DELETEsoft-deletes and the listener filters soft-deleted rows out of reads (existing substrate behavior).QA / manual — this PR is the gate only; to exercise it end-to-end, build it together with the entity PRs (#40129 charts / #40128 dashboards / #40130 datasets), since on master alone no model carries
SoftDeleteMixinyet.SOFT_DELETEunset/false,DELETEa chart/dashboard/dataset (UI or API). Confirm the row is physically gone (nodeleted_atghost; absent from both the API and a direct DB query), exactly as today — even though the model now carries the mixin and thedeleted_atcolumn exists. The gate is inert: the substrate ships dark.FEATURE_FLAGS = {"SOFT_DELETE": True}and restart.DELETEthe same entity types. Confirm the row survives withdeleted_atstamped, is hidden from list/detail/relationship loads, and is invisible to normal ORM reads (only a raw SQL query or the deleted-state filter sees it).POST /api/v1/{chart,dashboard,dataset}/<uuid>/restore→200; confirmdeleted_atis cleared and the row reappears in the default API.SOFT_DELETEoff after rows are soft-deleted makes those rows visible again to the legacy code path (resurrection) — this is an emergency stop, not a clean rollback. Pair any rollback with a data decision.Both gate points (write path
BaseDAO.delete()and read pathdo_orm_executelistener) key off the same flag, so they cannot diverge between on/off.ADDITIONAL INFORMATION
SOFT_DELETE(default off; temporary rollout/kill-switch — to be removed after stabilization)