mikebridge commented on code in PR #41166:
URL: https://github.com/apache/superset/pull/41166#discussion_r3501388266
##########
tests/unit_tests/daos/test_base_dao_soft_delete.py:
##########
@@ -59,17 +59,37 @@ 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:
_SoftDeletableDAO.delete(items)
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(
Review Comment:
Done in 8186348 — added
`test_delete_hard_deletes_non_mixin_model_when_gate_off`, completing the 4th
`(gate=OFF, plain)` cell of the matrix.
_— posted by Claude (AI agent) on behalf of @mikebridge_
##########
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."
Review Comment:
Done in 8186348 — trimmed to ~290 chars (from ~660) and pointed operators to
the `config.py` inline comment for the full rationale + gate points.
_— posted by Claude (AI agent) on behalf of @mikebridge_
##########
tests/unit_tests/daos/test_base_dao_soft_delete.py:
##########
@@ -59,17 +59,37 @@ 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()]
Review Comment:
Done in 8186348 — annotated `items: list[MagicMock]`.
_— posted by Claude (AI agent) on behalf of @mikebridge_
##########
tests/unit_tests/models/test_soft_delete_mixin.py:
##########
@@ -167,6 +180,31 @@ def test_global_filter_excludes_soft_deleted_rows(
assert result is None
[email protected]("_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")
Review Comment:
Done in 8186348 — annotated `obj: _SoftDeletable`.
_— posted by Claude (AI agent) on behalf of @mikebridge_
##########
tests/unit_tests/models/test_soft_delete_mixin.py:
##########
@@ -167,6 +180,31 @@ def test_global_filter_excludes_soft_deleted_rows(
assert result is None
[email protected]("_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
Review Comment:
Done in 8186348 — annotated `obj_id: int`.
_— posted by Claude (AI agent) on behalf of @mikebridge_
##########
tests/unit_tests/models/test_soft_delete_mixin.py:
##########
@@ -167,6 +180,31 @@ def test_global_filter_excludes_soft_deleted_rows(
assert result is None
[email protected]("_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()
+ )
Review Comment:
Done in 8186348 — annotated `result: _SoftDeletable | None`.
_— posted by Claude (AI agent) on behalf of @mikebridge_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
