Skip to content

chore(tags): TAGGING_SYSTEM to True by default #39888

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
sfirke wants to merge 2 commits into
base: master
Choose a base branch
from
+31 −13
Draft

chore(tags): TAGGING_SYSTEM to True by default #39888

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f98a31b
flip TAGGING_SYSTEM to True by default
sfirke May 5, 2026
3f2c5d9
combine feature flags
sfirke May 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion UPDATING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,12 @@ assists people when migrating to a new version.

## Next

### Granular Export Controls
### Feature Flag Changes

#### `TAGGING_SYSTEM`
The default value of **TAGGING_SYSTEM** was flipped from `False` to `True`.

#### `GRANULAR_EXPORT_CONTROLS`

A new feature flag `GRANULAR_EXPORT_CONTROLS` introduces three fine-grained permissions that replace the legacy `can_csv` permission:

Expand Down
12 changes: 6 additions & 6 deletions docs/static/feature-flags.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,6 @@
"default": false,
"lifecycle": "development",
"description": "Enable Table V2 time comparison feature"
},
{
"name": "TAGGING_SYSTEM",
"default": false,
"lifecycle": "development",
"description": "Enables the tagging system for organizing assets"
}
],
"testing": [
Expand Down Expand Up @@ -208,6 +202,12 @@
"description": "Allow users to enable SSH tunneling when creating a DB connection. DB engine must support SSH Tunnels.",
"docs": "https://superset.apache.org/docs/configuration/setup-ssh-tunneling"
},
{
"name": "TAGGING_SYSTEM",
"default": true,
"lifecycle": "testing",
"description": "Enables the tagging system for organizing assets"
},
{
"name": "USE_ANALOGOUS_COLORS",
"default": false,
Expand Down
6 changes: 3 additions & 3 deletions superset/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,9 +592,6 @@ class D3TimeFormat(TypedDict, total=False):
# Enable Table V2 time comparison feature
# @lifecycle: development
"TABLE_V2_TIME_COMPARISON_ENABLED": False,
# Enables the tagging system for organizing assets
# @lifecycle: development
"TAGGING_SYSTEM": False,
# =================================================================
# IN TESTING
# =================================================================
Expand Down Expand Up @@ -667,6 +664,9 @@ class D3TimeFormat(TypedDict, total=False):
# @lifecycle: testing
# @docs: https://superset.apache.org/docs/configuration/setup-ssh-tunneling
"SSH_TUNNELING": False,
# Enables the tagging system for organizing assets
# @lifecycle: testing
"TAGGING_SYSTEM": True,
Copy link
Copy Markdown
Contributor

@codeant-ai-for-open-source codeant-ai-for-open-source Bot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Setting TAGGING_SYSTEM to enabled by default causes SQLAlchemy tagging listeners to be registered during app initialization, and those listeners continue firing even when tests or runtime code later mock the flag to disabled. This breaks the expected feature-flag contract (disabled mode should not create tags), because listener lifecycle is startup-bound while flag checks are runtime-bound. Keep the default off until listener registration/unregistration is made fully dynamic with flag changes. [logic error]

Severity Level: Major ⚠️ 
- ❌ Tagging disabled-mode test fails; tags still created.
- ⚠️ Runtime flag toggles cannot disable SQLA tagging listeners.
- ⚠️ TAGGING_SYSTEM feature flag semantics become inconsistent, surprising.
Steps of Reproduction ✅ 
1. Initialize the integration test application using `create_app` in
`tests/integration_tests/test_app.py:20-32`, which builds the global `app`. During app
initialization, `SupersetApp.sync_config_to_db` in `superset/app.py:161-186` is called by
the initializer, and because `DEFAULT_FEATURE_FLAGS["TAGGING_SYSTEM"]` is set to `True` in
`superset/config.py:669`, the check
`feature_flag_manager.is_feature_enabled("TAGGING_SYSTEM")` at `superset/app.py:183`
returns True and `register_sqla_event_listeners()` is invoked at `superset/app.py:186`.

2. The call to `register_sqla_event_listeners` at `superset/tags/core.py:20-53` registers
SQLAlchemy `after_insert`, `after_update`, and `after_delete` event listeners on
`SqlaTable`, `Slice`, `Dashboard`, `FavStar`, and `SavedQuery` (e.g.
`sqla.event.listen(SqlaTable, "after_insert", DatasetUpdater.after_insert)` at lines
36-52). These listeners do not consult the feature flag; once registered, they fire on
every corresponding SQLA event for the lifetime of the process.

3. Run the integration test `TestTagging.test_tagging_system` defined in
`tests/integration_tests/tagging_tests.py:231-259`. This test is decorated with
`@with_feature_flags(TAGGING_SYSTEM=False)` at
`tests/integration_tests/tagging_tests.py:231`, which uses the helper `with_feature_flags`
in `tests/integration_tests/conftest.py:224-243` to patch
`feature_flag_manager.get_feature_flags` so that it returns `TAGGING_SYSTEM=False` during
the test, but it does not unregister or modify any already-registered SQLAlchemy
listeners.

4. Inside `test_tagging_system`, the test clears the `TaggedObject` table, then creates
and commits a `SqlaTable`, `Slice`, `Dashboard`, `SavedQuery`, and `FavStar` (object
creation and `db.session.commit()` around
`tests/integration_tests/tagging_tests.py:244-259`). These commits trigger the previously
registered listeners from `register_sqla_event_listeners`, which create
`Tag`/`TaggedObject` rows via the updater `after_insert` methods in
`superset/tags/models.py:221-236`. The test then asserts that no tags exist (checking
`self.query_tagged_object_table()` and asserting length 0 at
`tests/integration_tests/tagging_tests.py:238-244`), but the assertion fails because tags
were still created even though `TAGGING_SYSTEM` was mocked to False, demonstrating that
once the feature was enabled at startup, disabling it via the flag no longer prevents
tagging.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** superset/config.py
**Line:** 669:669
**Comment:**
	*Logic Error: Setting `TAGGING_SYSTEM` to enabled by default causes SQLAlchemy tagging listeners to be registered during app initialization, and those listeners continue firing even when tests or runtime code later mock the flag to disabled. This breaks the expected feature-flag contract (disabled mode should not create tags), because listener lifecycle is startup-bound while flag checks are runtime-bound. Keep the default off until listener registration/unregistration is made fully dynamic with flag changes.

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
👍 | 👎

# Enable AWS IAM authentication for database connections (Aurora, Redshift).
# Allows cross-account role assumption via STS AssumeRole.
# Security note: When enabled, ensure Superset's IAM role has restricted
Expand Down
19 changes: 16 additions & 3 deletions tests/integration_tests/fixtures/tags.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,36 @@
# under the License.

import pytest
import sqlalchemy as sqla

from superset import db
from superset.models.slice import Slice
from superset.tags.core import clear_sqla_event_listeners, register_sqla_event_listeners
from superset.tags.models import Tag
from superset.tags.models import ChartUpdater, Tag
from tests.integration_tests.test_app import app


@pytest.fixture
def with_tagging_system_feature():
is_enabled = app.config["DEFAULT_FEATURE_FLAGS"]["TAGGING_SYSTEM"]
listeners_registered = sqla.event.contains(
Slice, "after_insert", ChartUpdater.after_insert
)

if not is_enabled:
app.config["DEFAULT_FEATURE_FLAGS"]["TAGGING_SYSTEM"] = True

if not listeners_registered:
register_sqla_event_listeners()
yield
app.config["DEFAULT_FEATURE_FLAGS"]["TAGGING_SYSTEM"] = False

yield

if not listeners_registered:
clear_sqla_event_listeners()

if not is_enabled:
app.config["DEFAULT_FEATURE_FLAGS"]["TAGGING_SYSTEM"] = False


@pytest.fixture
def create_custom_tags():
Expand Down
Loading