bito-code-review[bot] commented on code in PR #44905:
URL: https://github.com/apache/superset/pull/44905#discussion_r4171161241
##########
superset/security/manager.py:
##########
@@ -4334,6 +4335,74 @@ def semantic_layer_before_update(
.values(perm=new_view_perm)
)
+ def semantic_layer_before_delete(
+ self,
+ mapper: Mapper,
+ connection: Connection,
+ target: "SemanticLayer",
+ ) -> None:
+ """
+ Remove child view permissions before the layer row is deleted.
+
+ Views the session has not loaded are deleted by the database
+ ``ON DELETE CASCADE`` (``passive_deletes=True``), so their ORM
+ ``after_delete`` hook never runs. Read their perms through the
+ connection while the rows still exist; views the ORM deletes itself
+ are already gone by now and clean up in ``semantic_view_after_delete``.
+ """
+ from superset.semantic_layers.models import ( # pylint:
disable=import-outside-toplevel
+ SemanticView,
+ )
+
+ sv_table = SemanticView.__table__ # pylint: disable=no-member
+ views: Sequence[Row[Any]] = connection.execute(
+ sv_table.select().where(sv_table.c.semantic_layer_uuid ==
target.uuid)
+ ).fetchall()
+ deleted_view_ids: set[int] = {view_row.id for view_row in views}
+ view_row: Row[Any]
+ for view_row in views:
+ if view_row.perm and not self._semantic_view_perm_owned_elsewhere(
+ connection, view_row.perm, deleted_view_ids
+ ):
+ self._delete_pvm_on_sqla_event(
+ mapper, connection, "datasource_access", view_row.perm
+ )
+
+ def _semantic_view_perm_owned_elsewhere(
+ self,
+ connection: Connection,
+ perm: str,
+ deleted_view_ids: set[int],
+ ) -> bool:
+ """
+ Whether a live resource other than the deleted views owns *perm*.
+
+ A deleted view's permission is removed only when no dataset and no
+ other semantic view still uses the same permission name; removing it
+ would otherwise revoke that resource's grants.
+ """
+ from superset.connectors.sqla.models import ( # pylint:
disable=import-outside-toplevel
+ SqlaTable,
+ )
+ from superset.semantic_layers.models import ( # pylint:
disable=import-outside-toplevel
+ SemanticView,
+ )
+
+ table = SqlaTable.__table__ # pylint: disable=no-member
+ if connection.execute(
+ table.select().where(table.c.perm == perm).limit(1)
+ ).first():
+ return True
+ sv_table = SemanticView.__table__ # pylint: disable=no-member
+ return (
+ connection.execute(
+ sv_table.select()
+ .where(sv_table.c.perm == perm,
sv_table.c.id.not_in(deleted_view_ids))
+ .limit(1)
+ ).first()
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unindexed perm equality scans</b></div>
<div id="fix">
`SqlaTable.perm` and `SemanticView.perm` are unindexed `String(1000)`
columns; the equality lookups here full-scan `datasets` and `semantic_views`,
once per deleted view (see the loop in `semantic_layer_before_delete`). An
index on `perm` would bound this cleanup cost on large deployments.
</div>
</div>
<small><i>Code Review Run #089298</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/security/manager.py:
##########
@@ -4334,6 +4335,74 @@ def semantic_layer_before_update(
.values(perm=new_view_perm)
)
+ def semantic_layer_before_delete(
+ self,
+ mapper: Mapper,
+ connection: Connection,
+ target: "SemanticLayer",
+ ) -> None:
+ """
+ Remove child view permissions before the layer row is deleted.
+
+ Views the session has not loaded are deleted by the database
+ ``ON DELETE CASCADE`` (``passive_deletes=True``), so their ORM
+ ``after_delete`` hook never runs. Read their perms through the
+ connection while the rows still exist; views the ORM deletes itself
+ are already gone by now and clean up in ``semantic_view_after_delete``.
+ """
+ from superset.semantic_layers.models import ( # pylint:
disable=import-outside-toplevel
+ SemanticView,
+ )
+
+ sv_table = SemanticView.__table__ # pylint: disable=no-member
+ views: Sequence[Row[Any]] = connection.execute(
+ sv_table.select().where(sv_table.c.semantic_layer_uuid ==
target.uuid)
+ ).fetchall()
+ deleted_view_ids: set[int] = {view_row.id for view_row in views}
+ view_row: Row[Any]
+ for view_row in views:
+ if view_row.perm and not self._semantic_view_perm_owned_elsewhere(
+ connection, view_row.perm, deleted_view_ids
+ ):
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>N+1 queries in delete hook</b></div>
<div id="fix">
Each iteration of the `views` loop calls
`_semantic_view_perm_owned_elsewhere`, which issues two queries
(`datasets.perm` and `semantic_views.perm` lookups). Deleting a layer with N
child views runs 2N+1 queries inside the flush. Consider batching: collect
distinct perms, resolve ownership with one query per table, then delete the
PVMs.
</div>
</div>
<small><i>Code Review Run #089298</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
