codeant-ai-for-open-source[bot] commented on code in PR #39977:
URL: https://github.com/apache/superset/pull/39977#discussion_r3220689675
##########
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:
**Suggestion:** The global `do_orm_execute` hook applies
`with_loader_criteria` to every SELECT, including relationship and column
loader queries; SQLAlchemy's recommended pattern excludes those loader paths to
avoid repeatedly stacking the same predicate. Without those guards, lazy/eager
relationship loads can accumulate redundant `deleted_at IS NULL` clauses and
increase query complexity/latency. Gate this block with `not
execute_state.is_column_load` and `not execute_state.is_relationship_load`.
[performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Soft-deletable model queries gain redundant soft-delete predicates.
- ⚠️ Relationship loads perform extra filtering work per lazy load.
- ⚠️ High-traffic chart and dashboard APIs may slow slightly.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. At app initialization,
`SupersetAppInitializer.setup_soft_delete_listener()` (added in
this PR at `superset/initialization/__init__.py:770-771`) registers
`_add_soft_delete_filter` from `superset/models/helpers.py:708-746` as a
`do_orm_execute`
listener on SQLAlchemy `Session`, so it runs on every ORM statement.
2. `_add_soft_delete_filter()` computes `skip_visibility_filter` from
`execute_state.execution_options` and `flask.g` at
`superset/models/helpers.py:729-735`,
then unconditionally applies `with_loader_criteria(SoftDeleteMixin, ...)`
for any
`execute_state.is_select` where `skip_visibility_filter` is false at lines
739-746.
3. SQLAlchemy's documented best-practice pattern for this hook (referenced
in the
docstring at `superset/models/helpers.py:712-713`) includes additional
guards `not
execute_state.is_column_load` and `not execute_state.is_relationship_load`
to avoid
reapplying identical `with_loader_criteria` options on per-column and
per-relationship
loader SELECTs; those guards are currently missing in this implementation.
4. Once real Superset entities (e.g. charts, dashboards, datasets) adopt
`SoftDeleteMixin`
in their respective rollout PRs as described in the SIP-208 summary, any ORM
operation
that performs lazy or eager relationship loading for those models (for
example, API
handlers like `ChartRestApi` and `DashboardRestApi` registered in
`init_views()` at
`superset/initialization/__init__.py:154-269`) will trigger `do_orm_execute`
both for the
primary SELECT and for each relationship/column load;
`_add_soft_delete_filter()` will
reapply `with_loader_criteria` on each of those loader queries, stacking
redundant
`deleted_at IS NULL` criteria and increasing query complexity and latency
compared to the
recommended guarded pattern.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmodels%2Fhelpers.py%0A%2A%2ALine%3A%2A%2A%20739%3A746%0A%2A%2AComment%3A%2A%2A%0A%09%2APerformance%3A%20The%20global%20%60do_orm_execute%60%20hook%20applies%20%60with_loader_criteria%60%20to%20every%20SELECT%2C%20including%20relationship%20and%20column%20loader%20queries%3B%20SQLAlchemy%27s%20recommended%20pattern%20excludes%20those%20loader%20paths%20to%20avoid%20repeatedly%20stacking%20the%20same%20predicate.%20Without%20those%20guards%2C%20lazy%2Feager%20relationship%20loads%20can%20accumulate%20redundant%20%60deleted_at%20IS%20NULL%60%20clauses%20and%20increase%20query%20complexity%2Flatency.%20Gate%20this%20block%20with%20%60not%20execute_state.is_column_load%60%20and%20%60not%20execute_state.is_relationship_load%60.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%2
0I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmodels%2Fhelpers.py%0A%2A%2ALine%3A%2A%2A%20739%3A746%0A%2A%2AComment%3A%2A%2A%0A%09%2APerformance%3A%20The%20global%20%60do_orm_execute%60%20hook%20applies%20%60with_loader_criteria%60%20to%20every%20SELECT%2C%20including%20relationship%20and%20column%20loader%20queries%3B%20SQLAlchemy%27s%20recommended%20pattern%20excludes%20those%20loader%20paths%20to%20avoid%20rep
eatedly%20stacking%20the%20same%20predicate.%20Without%20those%20guards%2C%20lazy%2Feager%20relationship%20loads%20can%20accumulate%20redundant%20%60deleted_at%20IS%20NULL%60%20clauses%20and%20increase%20query%20complexity%2Flatency.%20Gate%20this%20block%20with%20%60not%20execute_state.is_column_load%60%20and%20%60not%20execute_state.is_relationship_load%60.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 739:746
**Comment:**
*Performance: The global `do_orm_execute` hook applies
`with_loader_criteria` to every SELECT, including relationship and column
loader queries; SQLAlchemy's recommended pattern excludes those loader paths to
avoid repeatedly stacking the same predicate. Without those guards, lazy/eager
relationship loads can accumulate redundant `deleted_at IS NULL` clauses and
increase query complexity/latency. Gate this block with `not
execute_state.is_column_load` and `not execute_state.is_relationship_load`.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39977&comment_hash=d180fe9bf0502362a0a16b59f634fb4edb8d854a07eacdb05f51bbae773a68ca&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39977&comment_hash=d180fe9bf0502362a0a16b59f634fb4edb8d854a07eacdb05f51bbae773a68ca&reaction=dislike'>👎</a>
--
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]
