bito-code-review[bot] commented on code in PR #43471:
URL: https://github.com/apache/superset/pull/43471#discussion_r4166814415
##########
superset/tags/core.py:
##########
@@ -54,80 +54,40 @@ def _tag_delete_listener_declarations() ->
tuple[DeleteListenerDeclaration, ...]
def register_sqla_event_listeners() -> None:
+ """Register cleanup of ``tagged_object`` rows on object deletion.
+
+ Only deletion is handled here: Superset no longer auto-generates
+ ``type:``/``editor:``/``favorited_by:`` tags (see ``TagType``'s docstring),
+ so there's nothing left to do on insert/update. Deletion cleanup stays,
+ since it applies to every tag on the object, custom tags included, and
+ ``tagged_object.object_id`` has no foreign key to cascade on its own.
+ """
import sqlalchemy as sqla
- from superset.connectors.sqla.models import SqlaTable
- from superset.models.core import FavStar
- from superset.models.dashboard import Dashboard
- from superset.models.slice import Slice
from superset.models.sql_lab import SavedQuery
- from superset.tags.models import (
- ChartUpdater,
- DashboardUpdater,
- DatasetUpdater,
- FavStarUpdater,
- QueryUpdater,
- )
-
- declarations: tuple[DeleteListenerDeclaration, ...] = (
- _tag_delete_listener_declarations()
- )
-
- sqla.event.listen(SqlaTable, "after_insert", DatasetUpdater.after_insert)
- sqla.event.listen(SqlaTable, "after_update", DatasetUpdater.after_update)
- register_delete_listener(declarations[0])
-
- sqla.event.listen(Slice, "after_insert", ChartUpdater.after_insert)
- sqla.event.listen(Slice, "after_update", ChartUpdater.after_update)
- register_delete_listener(declarations[1])
+ from superset.tags.models import QueryUpdater
- sqla.event.listen(Dashboard, "after_insert", DashboardUpdater.after_insert)
- sqla.event.listen(Dashboard, "after_update", DashboardUpdater.after_update)
- register_delete_listener(declarations[2])
+ declarations = _tag_delete_listener_declarations()
- sqla.event.listen(FavStar, "after_insert", FavStarUpdater.after_insert)
- sqla.event.listen(FavStar, "after_delete", FavStarUpdater.after_delete)
+ register_delete_listener(declarations[0]) # dataset
+ register_delete_listener(declarations[1]) # chart
+ register_delete_listener(declarations[2]) # dashboard
- sqla.event.listen(SavedQuery, "after_insert", QueryUpdater.after_insert)
- sqla.event.listen(SavedQuery, "after_update", QueryUpdater.after_update)
- sqla.event.listen(SavedQuery, "after_delete", QueryUpdater.after_delete)
+ if not sqla.event.contains(SavedQuery, "after_delete",
QueryUpdater.after_delete):
+ sqla.event.listen(SavedQuery, "after_delete",
QueryUpdater.after_delete)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated idempotency guard</b></div>
<div id="fix">
The `SavedQuery` delete listener is registered via raw `sqla.event.listen`
with a hand-rolled `event.contains` guard, while the other three object types
use `register_delete_listener`, which already provides lock-protected
idempotency (`superset/utils/sqlalchemy_events.py`). Declaring `SavedQuery` as
a fourth `DeleteListenerDeclaration` in `_tag_delete_listener_declarations`
would remove the duplicated guard here and the mirrored one in
`clear_sqla_event_listeners`.
</div>
</div>
<small><i>Code Review Run #1b1640</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]
