fix(mcp): Instance not bound to session error #37548
Conversation
Code Review Agent Run #7c8c32Actionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
|
||
| from flask import g | ||
| from flask_appbuilder.security.sqla.models import User | ||
| from flask_appbuilder.security.sqla.models import Group, User |
There was a problem hiding this comment.
Suggestion: Top-level import of ORM model classes (Group, User) at module import time can trigger import-time side effects (like requiring a Flask app context or creating circular imports). Move runtime imports into functions or keep them under TYPE_CHECKING to avoid "working outside of application context" errors and circular import problems. [possible bug]
Severity Level: Major ⚠️
- ❌ MCP module import fails during standalone startup.
- ⚠️ FastMCP tool discovery may error on import.
- ⚠️ Some unit tests importing auth will fail.| from flask_appbuilder.security.sqla.models import Group, User | |
| # Import model classes at runtime inside functions to avoid import-time side effects |
Steps of Reproduction ✅
1. Open a Python REPL or a process that does not create a Flask app context.
2. Execute: import superset.mcp_service.auth (file: superset/mcp_service/auth.py line 35).
- This triggers the module top-level import at line 35: "from
flask_appbuilder.security.sqla.models import Group, User".
3. If flask_appbuilder.security.sqla.models requires an application context or touches
SQLAlchemy
engine setup during import, the import will raise a RuntimeError (e.g., "Working
outside
of application context") or other import-time failure.
4. Observed symptom: module import fails before any MCP decorator or runtime logic runs.
- Reproduces concretely by running `python -c "import superset.mcp_service.auth"` in
an environment where no Flask app is configured.
Note: The PR's file actually contains this top-level import at line 35, so the suggested
mitigation (defer import) is relevant if this module is imported in contexts without a
Flask app.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/auth.py
**Line:** 35:35
**Comment:**
*Possible Bug: Top-level import of ORM model classes (`Group`, `User`) at module import time can trigger import-time side effects (like requiring a Flask app context or creating circular imports). Move runtime imports into functions or keep them under TYPE_CHECKING to avoid "working outside of application context" errors and circular import problems.
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #37548 +/- ##
===========================================
+ Coverage 0 66.58% +66.58%
===========================================
Files 0 643 +643
Lines 0 49050 +49050
Branches 0 5501 +5501
===========================================
+ Hits 0 32662 +32662
- Misses 0 15093 +15093
- Partials 0 1295 +1295
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
SUMMARY
Don't rollback the session on finally, fixes
Error calling tool 'list_datasets': Parent instance <Group at 0x7fffaf7346a0> is not bound to a Session; lazy load operation of attribute 'roles' cannot proceed (Background on this error at: https://sqlalche.me/e/14/bhk3)and similar errorsCC @aminghadersohi
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION