chore(tags): stop auto-generating type:/editor:/favorited_by: tags - #43471
chore(tags): stop auto-generating type:/editor:/favorited_by: tags#43471rusackas wants to merge 2 commits into
Conversation
Code Review Agent Run #cf379aActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
github-actions
Bot
added
api
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue regarding the non-idempotent registration of the To resolve this, you should register and remove the Regarding other comments on this PR, there are no additional review comments in the provided |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43471 +/- ##
==========================================
+ Coverage 78.96% 78.99% +0.03%
==========================================
Files 2878 2876 -2
Lines 165183 164937 -246
Branches 38170 38156 -14
==========================================
- Hits 130430 130298 -132
+ Misses 32301 32190 -111
+ Partials 2452 2449 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
villebro noted on #43390 that these system-generated tags appear to be unused. Confirmed: every tags list and filter in the frontend explicitly excludes non-custom tags (ChartList, DashboardList, SavedQueryList, the chart PropertiesModal, the dashboard Header), so nothing ever surfaced them to a user. What remained was pure write-side overhead: 13 SQLAlchemy event listeners across 5 models firing on every chart/ dashboard/query/dataset save and every favorite/unfavorite, plus a whole performance-optimization mixin (CustomTagsOptimizationMixin, DASHBOARD_LIST_CUSTOM_TAGS_ONLY) that existed purely to strip the resulting noise back out of dashboard-list responses. This removes the generation: - superset/tags/models.py: drop ObjectUpdater's editor:/type: generation (after_insert/after_update) and FavStarUpdater's favorited_by: generation entirely. Keeps after_delete (tagged_object cleanup applies to every tag, custom included, and nothing else removes those rows since tagged_object.object_id has no FK - see its column comment). - superset/tags/core.py: only registers the delete-cleanup listeners now. - superset/common/tags.py + the `sync_tags` CLI command: removed (the backfill path for the generation this removes). - superset/views/custom_tags_api_mixin.py, DASHBOARD_LIST_CUSTOM_TAGS_ONLY, Dashboard.custom_tags, and the schema/API plumbing built around them: removed - nothing left to optimize away once implicit tags stop accumulating. Kept for backward compatibility, since MCP's list_tags/get_tag_info tools document these tag types and upgraded deployments may already have rows of these types: the TagType enum values, the custom_tag API filter, and bulk-delete protection for non-custom tags. Docstrings updated to say these are legacy/no longer generated rather than actively implicit. Also fixes a real, currently-broken import in superset/daos/tag.py (current_user_can_modify_object doesn't live in superset.commands.tag.utils, only in superset.commands.utils) that otherwise blocks every test in this area from running at all. Filed and fixed separately as #43467; this commit will collapse away on rebase once that merges. Follow-up to #43390. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Guard sqla.event.listen/remove for the SavedQuery after_delete listener with sa.event.contains(), matching the pattern register_delete_listener already uses, so repeated register_sqla_event_listeners() calls don't stack duplicate cleanup callbacks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
009847e to
f638a55
Compare
Code Review Agent Run #c4c797Actionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| 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 |
There was a problem hiding this comment.
Upgrades retain existing editor: and favorited_by: associations as queryable rows, but this removes the update and favorite listeners that kept them current. Changing an asset's editors or unfavoriting it after upgrading therefore leaves tag lookups reporting relationships that no longer exist. Should we clear these legacy associations during upgrade or continue maintaining them while they remain queryable?
3 participants
SUMMARY
On #43390, villebro noted that these system-generated tags appear to be unused, and suggested considering removing them entirely.
Confirmed: every tags list and filter in the frontend explicitly excludes non-custom tags —
ChartList,DashboardList,SavedQueryList, the chartPropertiesModal, the dashboardHeader, andfeatures/tags/tags.ts'sfetchAllTags/fetchTagsall filter toTagType.customonly. Nothing in the UI ever surfaced atype:/editor:/favorited_by:tag or let a user search/filter by one — the docstring's own promise ("user alice can find all their objects by querying for the tageditor:alice") had no UI path to actually do that.What remained was pure write-side overhead: with
TAGGING_SYSTEMenabled, 13 SQLAlchemy event listeners across 5 models (chart/dashboard/query/dataset/favorite) fired on every insert/update/delete, writingTag/TaggedObjectrows nobody ever read. There was even a whole performance-optimization mixin (CustomTagsOptimizationMixin,DASHBOARD_LIST_CUSTOM_TAGS_ONLY) that existed purely to strip this noise back out of dashboard-list API responses — evidence this was a known cost nobody had gotten around to addressing at the source.What this removes:
superset/tags/models.py:ObjectUpdater'seditor:/type:generation (after_insert/after_update) andFavStarUpdater'sfavorited_by:generation, entirely.superset/tags/core.py: only registers the delete-cleanup listeners now (see below).superset/common/tags.pyand thesync_tagsCLI command: the backfill path for the generation this removes.superset/views/custom_tags_api_mixin.py,DASHBOARD_LIST_CUSTOM_TAGS_ONLY,Dashboard.custom_tags, and the schema/API plumbing built around them: nothing left to optimize away once implicit tags stop accumulating.What's kept, on purpose:
ObjectUpdater.after_delete— cleans uptagged_objectrows for every tag on a deleted object, custom tags included.tagged_object.object_idcarries no foreign key (see its column comment), so this is the only thing that prevents orphaned rows when a chart/dashboard/query/dataset is hard-deleted — not part of the removed generation, added a regression test for it (test_tagged_object_cleanup_on_dashboard_delete).TagTypeenum values, thecustom_tagAPI filter, and bulk-delete protection for non-custom tags — for backward compatibility with upgraded deployments that already havetype:/editor:/favorited_by:rows, and because MCP'slist_tags/get_tag_infotools document and support filtering by these tag types. Docstrings updated to say these are legacy/no longer generated rather than actively implicit.Unrelated but blocking: this branch also carries #43467 as its first commit — a genuinely broken import in
superset/daos/tag.pyfrom the just-merged #43390 (current_user_can_modify_objectdoesn't live insuperset.commands.tag.utils) that otherwise breaks Flask app boot entirely and blocks every test in this area. Filed separately since it's unrelated; this branch will drop that commit on its own once #43467 merges and this rebases.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — internal cleanup, no UI changes (the UI never showed these tags to begin with).
TESTING INSTRUCTIONS
pytest tests/unit_tests/tags/ tests/unit_tests/commands/test_utils.py tests/unit_tests/initialization_test.py tests/unit_tests/dashboards/api_test.py— updated/removed tests for the removed generation and mixin.pytest tests/integration_tests/tags/dao_tests.py::TestTagsDAO::test_tagged_object_cleanup_on_dashboard_delete— new regression test for the preserved delete-cleanup behavior.pytest tests/unit_tests/run: 13172 passed (the only failures are pre-existing, unrelated to this change — a missing optionalpymysqldependency and afreezegunversion quirk in files this PR doesn't touch).TAGGING_SYSTEMenabled, create/edit/delete a chart, dashboard, saved query, and dataset, and favorite/unfavorite one — confirm notype:/editor:/favorited_by:tags appear in thetagtable anymore, while manually adding a custom tag and then deleting the tagged object still cleans up itstagged_objectrow.ADDITIONAL INFORMATION