mikebridge commented on code in PR #39977:
URL: https://github.com/apache/superset/pull/39977#discussion_r3227828640
##########
superset/models/helpers.py:
##########
@@ -654,7 +656,94 @@ def created_on_humanized(self) -> str:
@renders("changed_on")
def modified(self) -> Markup:
- return Markup(f'<span
class="no-wrap">{self.changed_on_humanized}</span>')
+ return Markup(f'<span
class="no-wrap">{self.changed_on_humanized}</span>') # noqa: S704
+
+
+SKIP_VISIBILITY_FILTER = "skip_visibility_filter"
+
+
+class SoftDeleteMixin:
+ """Mixin that adds soft-delete support to a SQLAlchemy model.
+
+ Adds a nullable ``deleted_at`` column. When set, the row is treated as
+ deleted and excluded from standard ORM queries via a global
+ ``do_orm_execute`` listener registered at app init.
+
+ Delete commands should call ``soft_delete()`` to mark the object as
+ deleted. Use ``BaseDAO.delete()`` (which calls ``session.delete()``)
+ for permanent hard deletion.
+
+ See also: ``_add_soft_delete_filter`` (the listener function) and
+ ``SKIP_VISIBILITY_FILTER`` (the execution-option key used to opt out
+ of the filter in restore commands and admin tooling).
+ """
+
+ deleted_at = sa.Column(sa.DateTime, nullable=True, index=True)
+
+ @hybrid_property
+ def is_deleted(self) -> bool:
+ return self.deleted_at is not None
+
+ @is_deleted.expression # type: ignore
+ def is_deleted(cls) -> ColumnElement: # noqa: N805
+ return cls.deleted_at.is_not(None)
+
+ @classmethod
+ def not_deleted(cls) -> ColumnElement:
+ """Filter clause for active (non-deleted) rows."""
+ return cls.deleted_at.is_(None)
+
+ def soft_delete(self) -> None:
+ """Mark this object as soft-deleted."""
+ # Naive datetime, mirroring AuditMixinNullable.changed_on. PR #33693
+ # reverted a UTC migration on the audit columns; if/when those move
+ # to UTC-aware, this assignment should follow.
+ self.deleted_at = datetime.now()
+
+ def restore(self) -> None:
+ """Clear the soft-delete marker, making this object active again."""
+ self.deleted_at = None
+
+
+def _add_soft_delete_filter(execute_state: ORMExecuteState) -> None:
+ """Global ``do_orm_execute`` listener that automatically excludes
+ soft-deleted rows from every ORM SELECT.
+
+ Uses SQLAlchemy's recommended soft-delete pattern
+ (``do_orm_execute`` + ``with_loader_criteria``).
+
+ Two opt-out paths:
+
+ * **Per-query** — ``execution_options(skip_visibility_filter=True)``.
+ The narrow tool. Use this from any non-user-facing code path
+ (background jobs, import pipelines, internal admin tools) that
+ needs to query soft-deleted rows.
+ * **Per-request** — ``flask.g.skip_visibility_filter = True``.
+ The broad tool. Reserved for user-facing list endpoints whose
+ rison filter (``*_deleted_state=include|only``) explicitly asks
+ to surface soft-deleted rows for the rest of that request.
+ Anything else needing to bypass the filter should use the
+ per-query option, since the request-scoped flag also affects
+ any incidental query inside the same request.
+ """
+ skip_visibility_filter = execute_state.execution_options.get(
+ SKIP_VISIBILITY_FILTER, False
+ )
+ try:
+ skip_visibility_filter = skip_visibility_filter or getattr(
+ g, SKIP_VISIBILITY_FILTER, False
+ )
+ except RuntimeError:
+ pass
+
+ if execute_state.is_select and not skip_visibility_filter:
+ execute_state.statement = execute_state.statement.options(
+ with_loader_criteria(
+ SoftDeleteMixin,
+ lambda cls: cls.deleted_at.is_(None),
+ include_aliases=True,
+ )
+ )
Review Comment:
Confirmed and fixed in 44e1d1710b. The two loader guards are now in place:
```python
if (
execute_state.is_select
and not execute_state.is_column_load
and not execute_state.is_relationship_load
and not skip_visibility_filter
):
execute_state.statement = execute_state.statement.options(...)
```
This matches Mike Bayer's documented pattern in sqlalchemy/sqlalchemy#7973 —
the same reference the PR description cites, so we should have had it from the
start. Thanks for catching this; without these guards, lazy/eager relationship
loads accumulate redundant `deleted_at IS NULL` clauses on every reload, which
is both a perf regression and a correctness risk if SQLAlchemy stacks the same
criteria across loader paths.
--
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]
