codeant-ai-for-open-source[bot] commented on code in PR #39888:
URL: https://github.com/apache/superset/pull/39888#discussion_r3188789624
##########
superset/config.py:
##########
@@ -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,
Review Comment:
**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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Tagging disabled-mode test fails; tags still created.
- ⚠️ Runtime flag toggles cannot disable SQLA tagging listeners.
- ⚠️ TAGGING_SYSTEM feature flag semantics become inconsistent, surprising.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fconfig.py%0A%2A%2ALine%3A%2A%2A%20669%3A669%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20Setting%20%60TAGGING_SYSTEM%60%20to%20enabled%20by%20default%20causes%20SQLAlchemy%20tagging%20listeners%20to%20be%20registered%20during%20app%20initialization%2C%20and%20those%20listeners%20continue%20firing%20even%20when%20tests%20or%20runtime%20code%20later%20mock%20the%20flag%20to%20disabled.%20This%20breaks%20the%20expected%20feature-flag%20contract%20%28disabled%20mode%20should%20not%20create%20tags%29%2C%20because%20listener%20lifecycle%20is%20startup-bound%20while%20flag%20checks%20are%20runtime-bound.%20Keep%20the%20default%20off%20until%20listener%20registration%2Funregistration%20is%20made%20fully%20dynamic%20with%20flag%20changes.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2
C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fconfig.py%0A%2A%2ALine%3A%2A%2A%20669%3A669%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20Setting%20%60TAGGING_SYSTEM%60%20to%20enabled%20by%20default%20causes%20SQLAlchemy%20tagging%20listeners%20to%20be%20registered%20during%20app%20initialization%2C%20and%20those%20listeners%20continue%20firing%20even%20when%20tests%20or%20runtime%20code%20late
r%20mock%20the%20flag%20to%20disabled.%20This%20breaks%20the%20expected%20feature-flag%20contract%20%28disabled%20mode%20should%20not%20create%20tags%29%2C%20because%20listener%20lifecycle%20is%20startup-bound%20while%20flag%20checks%20are%20runtime-bound.%20Keep%20the%20default%20off%20until%20listener%20registration%2Funregistration%20is%20made%20fully%20dynamic%20with%20flag%20changes.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39888&comment_hash=b2c0dc75aef7bb041b9daf06172adca9e09f5683960612c2e97da5da30416a01&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39888&comment_hash=b2c0dc75aef7bb041b9daf06172adca9e09f5683960612c2e97da5da30416a01&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]
