From 3d8d4795795c332cb12df116fd3ceae8c2080eb1 Mon Sep 17 00:00:00 2001 From: Mike Bridge Date: Wed, 17 Jun 2026 11:02:22 -0600 Subject: [PATCH] feat(soft-delete): gate soft delete behind a temporary SOFT_DELETE flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/static/feature-flags.json | 6 +++ superset/config.py | 12 ++++++ superset/daos/base.py | 15 ++++++-- superset/models/helpers.py | 11 +++++- .../daos/test_base_dao_soft_delete.py | 28 ++++++++++++-- .../models/test_soft_delete_mixin.py | 38 +++++++++++++++++++ 6 files changed, 101 insertions(+), 9 deletions(-) diff --git a/docs/static/feature-flags.json b/docs/static/feature-flags.json index 8f7623e471d8..2ed504af6170 100644 --- a/docs/static/feature-flags.json +++ b/docs/static/feature-flags.json @@ -87,6 +87,12 @@ "lifecycle": "development", "description": "Enable semantic layers and show semantic views alongside datasets" }, + { + "name": "SOFT_DELETE", + "default": false, + "lifecycle": "development", + "description": "Temporary rollout / kill-switch gate for soft delete. Default off. When off: DELETE hard-deletes (legacy) and the read-path visibility filter is inactive, so the substrate ships dark; the import pipeline's existing-row detection inherits this via the same listener (no separate check); restore has nothing to act on because no row carries deleted_at. Flipping ON->OFF after rows were soft-deleted RESURRECTS them (they become visible/active again) \u2014 an emergency stop, not a clean rollback. Not a permanent toggle: REMOVE this flag and its two gate points (BaseDAO.delete routing; the do_orm_execute visibility listener) once soft delete is stable." + }, { "name": "TABLE_V2_TIME_COMPARISON_ENABLED", "default": false, diff --git a/superset/config.py b/superset/config.py index 2a44424afc6a..9516ad87aace 100644 --- a/superset/config.py +++ b/superset/config.py @@ -632,6 +632,18 @@ class D3TimeFormat(TypedDict, total=False): # can_copy_clipboard) instead of the single can_csv permission # @lifecycle: development "GRANULAR_EXPORT_CONTROLS": False, + # Temporary rollout / kill-switch gate for soft delete. Default off. When + # off: DELETE hard-deletes (legacy) and + # the read-path visibility filter is inactive, so the substrate ships dark; + # the import pipeline's existing-row detection inherits this via the same + # listener (no separate check); restore has nothing to act on because no row + # carries deleted_at. Flipping ON->OFF after rows were soft-deleted RESURRECTS + # them (they become visible/active again) — an emergency stop, not a clean + # rollback. Not a permanent toggle: REMOVE this flag and its two gate points + # (BaseDAO.delete routing; the do_orm_execute visibility listener) once soft + # delete is stable. + # @lifecycle: development + "SOFT_DELETE": False, # Enable semantic layers and show semantic views alongside datasets # @lifecycle: development "SEMANTIC_LAYERS": False, diff --git a/superset/daos/base.py b/superset/daos/base.py index d10e36b8aca8..66f97ecf3e28 100644 --- a/superset/daos/base.py +++ b/superset/daos/base.py @@ -45,6 +45,7 @@ from superset_core.common.daos import BaseDAO as CoreBaseDAO from superset_core.common.models import CoreModel +from superset import is_feature_enabled from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES from superset.daos.exceptions import ( DAOFindFailedError, @@ -516,16 +517,22 @@ def delete(cls, items: list[T]) -> None: soft delete. For models that include ``SoftDeleteMixin``, this calls - ``soft_delete()``. For all other models, this calls ``hard_delete()`` - (the original behaviour). + ``soft_delete()`` — but only while the temporary ``SOFT_DELETE`` rollout + gate is enabled. When the gate is off, every model + hard-deletes (the original behaviour), so the substrate can ship dark. + For all other models, this always calls ``hard_delete()``. :param items: The items to delete """ from superset.models.helpers import ( - SoftDeleteMixin, # pylint: disable=import-outside-toplevel + SoftDeleteMixin, # avoid circular import: models.helpers <-> daos ) - if cls.model_cls is not None and issubclass(cls.model_cls, SoftDeleteMixin): + if ( + cls.model_cls is not None + and issubclass(cls.model_cls, SoftDeleteMixin) + and is_feature_enabled("SOFT_DELETE") + ): cls.soft_delete(items) else: cls.hard_delete(items) diff --git a/superset/models/helpers.py b/superset/models/helpers.py index b603f0bb3cea..981cd4108985 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -793,8 +793,17 @@ def _should_attach_soft_delete_criteria(execute_state: ORMExecuteState) -> bool: relationship-load event closes that gap. The resulting WHERE clause may have ``deleted_at IS NULL`` twice when propagation DOES work — harmless redundancy, idempotent SQL. + + Gated by the temporary ``SOFT_DELETE`` rollout flag: while it is + off, no criteria are attached, so soft-deleted rows (which the delete path + does not create while the flag is off) are not filtered. The flag and this + gate are removed once soft delete is stable. """ - return execute_state.is_select and not execute_state.is_column_load + return ( + execute_state.is_select + and not execute_state.is_column_load + and is_feature_enabled("SOFT_DELETE") + ) def _all_soft_delete_subclasses() -> list[type[SoftDeleteMixin]]: diff --git a/tests/unit_tests/daos/test_base_dao_soft_delete.py b/tests/unit_tests/daos/test_base_dao_soft_delete.py index f0ef17948574..457fd2b41cf9 100644 --- a/tests/unit_tests/daos/test_base_dao_soft_delete.py +++ b/tests/unit_tests/daos/test_base_dao_soft_delete.py @@ -59,8 +59,11 @@ class _PlainDAO(BaseDAO[_Plain]): model_cls = _Plain -def test_delete_routes_to_soft_delete_for_mixin_models(app_context: None) -> None: - """delete() calls soft_delete() when model_cls includes SoftDeleteMixin.""" +@patch("superset.daos.base.is_feature_enabled", return_value=True) +def test_delete_routes_to_soft_delete_for_mixin_models( + mock_flag: MagicMock, app_context: None +) -> None: + """delete() soft-deletes a mixin model when the SOFT_DELETE gate is ON.""" items = [MagicMock(), MagicMock()] with patch.object(_SoftDeletableDAO, "soft_delete") as mock_soft: @@ -68,8 +71,25 @@ def test_delete_routes_to_soft_delete_for_mixin_models(app_context: None) -> Non mock_soft.assert_called_once_with(items) -def test_delete_routes_to_hard_delete_for_non_mixin_models(app_context: None) -> None: - """delete() calls hard_delete() for non-SoftDeleteMixin models.""" +@patch("superset.daos.base.is_feature_enabled", return_value=False) +def test_delete_hard_deletes_mixin_model_when_gate_off( + mock_flag: MagicMock, app_context: None +) -> None: + """With the SOFT_DELETE gate OFF (default), even a mixin model hard-deletes + — the substrate ships dark.""" + items = [MagicMock(), MagicMock()] + + with patch.object(_SoftDeletableDAO, "hard_delete") as mock_hard: + _SoftDeletableDAO.delete(items) + mock_hard.assert_called_once_with(items) + + +@patch("superset.daos.base.is_feature_enabled", return_value=True) +def test_delete_routes_to_hard_delete_for_non_mixin_models( + mock_flag: MagicMock, app_context: None +) -> None: + """delete() calls hard_delete() for non-SoftDeleteMixin models — regardless + of the gate (here ON, to show the gate doesn't make a plain model soft).""" items = [MagicMock(), MagicMock()] with patch.object(_PlainDAO, "hard_delete") as mock_hard: diff --git a/tests/unit_tests/models/test_soft_delete_mixin.py b/tests/unit_tests/models/test_soft_delete_mixin.py index 364d016d9f9a..e3b03503d717 100644 --- a/tests/unit_tests/models/test_soft_delete_mixin.py +++ b/tests/unit_tests/models/test_soft_delete_mixin.py @@ -27,6 +27,7 @@ from collections.abc import Generator from datetime import datetime +from unittest.mock import patch import pytest from sqlalchemy import Column, ForeignKey, Integer, String @@ -90,6 +91,18 @@ def _synthetic_tables(session: Session) -> Generator[None, None, None]: _TestBase.metadata.drop_all(session.get_bind()) +@pytest.fixture(autouse=True) +def _soft_delete_gate_on() -> Generator[None, None, None]: + """The ``do_orm_execute`` visibility listener is gated by the temporary + ``SOFT_DELETE`` rollout flag, default off. These tests exercise + the listener's filtering, so enable the gate for the whole module. The + gate-off (listener-noop) behaviour is pinned separately by + ``test_listener_noop_when_gate_off``. + """ + with patch("superset.models.helpers.is_feature_enabled", return_value=True): + yield + + @pytest.mark.usefixtures("_synthetic_tables") def test_soft_delete_sets_deleted_at(app_context: None, session: Session) -> None: """soft_delete() sets deleted_at to a non-null datetime.""" @@ -167,6 +180,31 @@ def test_global_filter_excludes_soft_deleted_rows( assert result is None +@pytest.mark.usefixtures("_synthetic_tables") +def test_listener_noop_when_gate_off(app_context: None, session: Session) -> None: + """With the ``SOFT_DELETE`` gate OFF, the listener attaches no criteria, so a + soft-deleted row is NOT hidden — the substrate is dark. (While + the gate is off the delete path also doesn't create such rows; this pins the + listener side.)""" + obj = _SoftDeletable(name="visible_when_gate_off") + session.add(obj) + session.flush() + obj_id = obj.id + + obj.soft_delete() + session.flush() + session.expire_all() + + with patch("superset.models.helpers.is_feature_enabled", return_value=False): + result = ( + session.query(_SoftDeletable) + .filter(_SoftDeletable.id == obj_id) + .one_or_none() + ) + assert result is not None + assert result.id == obj_id + + @pytest.mark.usefixtures("_synthetic_tables") def test_listener_adapts_criteria_to_aliased_table_in_joins( app_context: None, session: Session