aminghadersohi commented on code in PR #41166:
URL: https://github.com/apache/superset/pull/41166#discussion_r3500300319
##########
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:
NIT: This description is ~360 characters — roughly 5× longer than
neighboring entries (e.g., `"Enables Table V2 (AG Grid) viz plugin"`). The
detail is valuable (resurrection risk, two gate points to remove), but
documentation UIs may clip it. The verbose explanation already lives in
`config.py`'s inline comment; a shorter flag description here pointing
operators there might serve better.
##########
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:
NIT: The test matrix covers 3 of the 4 `(gate, model_type)` combinations:
- gate=ON + SoftDeleteMixin → `soft_delete()` ✓
- gate=OFF + SoftDeleteMixin → `hard_delete()` ✓
- gate=ON + plain → `hard_delete()` ✓
- gate=OFF + plain → `hard_delete()` ✗
The fourth case is trivially implied — `issubclass(cls.model_cls,
SoftDeleteMixin)` short-circuits to `False` before the gate is evaluated — but
adding it explicitly means a future refactor of the `delete()` conditional is
caught immediately. Consider adding a
`test_delete_hard_deletes_plain_model_when_gate_off` alongside this test.
--
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]
