codeant-ai-for-open-source[bot] commented on code in PR #37252:
URL: https://github.com/apache/superset/pull/37252#discussion_r2704957755
##########
superset/mcp_service/flask_singleton.py:
##########
@@ -33,34 +33,47 @@
logger.info("Creating Flask app instance for MCP service")
try:
- from superset.app import create_app
- from superset.mcp_service.mcp_config import get_mcp_config
-
- # Create a temporary context to avoid
- # "Working outside of application context" errors.
- _temp_app = create_app()
-
- # Push an application context and initialize core dependencies and
extensions
- with _temp_app.app_context():
- # Apply MCP configuration - reads from app.config first, falls back to
defaults
- mcp_config = get_mcp_config(_temp_app.config)
- _temp_app.config.update(mcp_config)
- try:
- from superset.initialization import SupersetAppInitializer
-
- # Create initializer and run only dependency injection
- # NOT the full init_app_in_ctx which includes web views
- initializer = SupersetAppInitializer(_temp_app)
- initializer.init_all_dependencies_and_extensions()
-
- logger.info("Core dependencies and extensions initialized for MCP
service")
- except Exception as e:
- logger.warning("Failed to initialize dependencies for MCP service:
%s", e)
-
- # Store the app instance for later use
- app = _temp_app
-
- logger.info("Minimal Flask app instance created successfully for MCP
service")
+ from superset.extensions import appbuilder
+
+ # Check if appbuilder is already initialized (main Superset app is
running).
+ # If so, reuse that app to avoid corrupting the shared appbuilder
singleton.
+ # Calling create_app() again would re-initialize appbuilder and break view
endpoints.
+ if appbuilder.app is not None:
+ logger.info("Reusing existing Flask app from appbuilder for MCP
service")
+ app = appbuilder.app
+ else:
+ from superset.app import create_app
+ from superset.mcp_service.mcp_config import get_mcp_config
+
+ # Create a temporary context to avoid
+ # "Working outside of application context" errors.
+ _temp_app = create_app()
Review Comment:
**Suggestion:** Logic error: calling `create_app()` here will run the full
application bootstrap (including `app_initializer.init_app()`), which
re-initializes Flask-AppBuilder and can recreate the shared `appbuilder`
singleton — exactly what this PR was trying to avoid. Replace creation of a
fully-initialized app with creating a minimal app instance (e.g.,
`SupersetApp(__name__)`) and explicitly disable debug to avoid side-effects
like starting a file-watcher. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Login page URL building fails after double initialization.
- ❌ View endpoints become None causing BuildError.
- ⚠️ MCP tool import can corrupt shared appbuilder singleton.
- ⚠️ _temp app runs full initializer during module import.
```
</details>
```suggestion
from superset.app import SupersetApp
from superset.mcp_service.mcp_config import get_mcp_config
# Create a minimal Superset app instance (don't run the full
initializer).
# This avoids calling `app_initializer.init_app()` which would
register views
# and re-initialize the shared appbuilder singleton.
_temp_app = SupersetApp(__name__)
# Ensure debug is disabled to avoid starting debug-only background
threads.
_temp_app.debug = False
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset with MCP enabled (follow PR testing steps). The main
process imports MCP
helpers during module import.
2. During import, `superset/mcp_service/flask_singleton.py` is executed (see
file lines
where import occurs: line 36 imports `appbuilder`, lines 45-50 call
`create_app()`).
3. `create_app()` in `superset/app.py` (shown in provided context starting
at line 47)
executes `app_initializer.init_app()` as part of its normal flow (see
`app_initializer.init_app()` in `superset/app.py:~79`), registering views
and extensions.
4. That registration can reinitialize Flask-AppBuilder, causing duplicate
registration and
leaving view `.endpoint` attributes unset — reproducing the BuildError
described in the PR
(login URL build failures observed when `create_app()` is invoked here).
Note: existing PR already guards re-initialization when `appbuilder.app` is
present, but
when `appbuilder.app` is None this path runs the full app bootstrap (via
`create_app()`),
which is the exact root cause this suggestion highlights.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/flask_singleton.py
**Line:** 45:50
**Comment:**
*Logic Error: Logic error: calling `create_app()` here will run the
full application bootstrap (including `app_initializer.init_app()`), which
re-initializes Flask-AppBuilder and can recreate the shared `appbuilder`
singleton — exactly what this PR was trying to avoid. Replace creation of a
fully-initialized app with creating a minimal app instance (e.g.,
`SupersetApp(__name__)`) and explicitly disable debug to avoid side-effects
like starting a file-watcher.
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.
```
</details>
--
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]
