codeant-ai-for-open-source[bot] commented on code in PR #43471:
URL: https://github.com/apache/superset/pull/43471#discussion_r3845583562
##########
superset/tags/core.py:
##########
@@ -54,80 +54,38 @@ 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)
Review Comment:
**Suggestion:** Registering the `SavedQuery` listener directly is not
idempotent, unlike the three listeners registered through
`register_delete_listener`. Every repeated `sync_config_to_db()` call adds
another callback, while `clear_sqla_event_listeners()` removes only one
registration, leaving duplicate cleanup callbacks active and potentially
leaving the listener installed after cleanup. Register and remove the
`SavedQuery` listener through the same declaration-based helper. [possible bug]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Repeated initialization duplicates SavedQuery cleanup callbacks.
- โ ๏ธ SavedQuery deletion performs redundant tagged-object cleanup queries.
- โ ๏ธ Clearing listeners may leave SavedQuery cleanup active.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1b9b8b5db67148a7a3c363d70be0b5ab&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1b9b8b5db67148a7a3c363d70be0b5ab&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/tags/core.py
**Line:** 76:76
**Comment:**
*Possible Bug: Registering the `SavedQuery` listener directly is not
idempotent, unlike the three listeners registered through
`register_delete_listener`. Every repeated `sync_config_to_db()` call adds
another callback, while `clear_sqla_event_listeners()` removes only one
registration, leaving duplicate cleanup callbacks active and potentially
leaving the listener installed after cleanup. Register and remove the
`SavedQuery` listener through the same declaration-based helper.
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%2F43471&comment_hash=3b3d58fb1659eba7fcbf7c192e5c30b0ec73a95307287cc118339f03aa226c66&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43471&comment_hash=3b3d58fb1659eba7fcbf7c192e5c30b0ec73a95307287cc118339f03aa226c66&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]
