aminghadersohi commented on code in PR #37216: URL: https://github.com/apache/superset/pull/37216#discussion_r2706486104
##########
superset/mcp_service/auth.py:
##########
@@ -127,14 +127,22 @@ def has_dataset_access(dataset: "SqlaTable") -> bool:
return False # Deny access on error
-def _setup_user_context() -> User:
+def _setup_user_context() -> User | None:
"""
Set up user context for MCP tool execution.
Returns:
- User object with roles and groups loaded
+ User object with roles and groups loaded, or None if no Flask context
"""
- user = get_user_from_request()
+ try:
+ user = get_user_from_request()
+ except RuntimeError as e:
+ # No Flask application context (e.g., prompts before middleware runs)
+ # This is expected for some FastMCP operations - return None gracefully
+ if "application context" in str(e):
Review Comment:
Options:
Option A: Check context proactively (avoid exception entirely)
Instead of catching the exception and parsing the message, check
has_app_context() BEFORE
calling code that needs it:
from flask import has_app_context
def _setup_user_context() -> User | None:
if not has_app_context():
logger.debug("No Flask app context available for user setup")
return None
user = get_user_from_request()
# ... rest of function
Pros: Cleaner, no string matching, explicit check
Cons: Might miss other RuntimeErrors that should also return None
---
Option B: Keep RuntimeError but document why
Flask doesn't expose a specific exception class for context errors - it's
always RuntimeError.
We could add a comment explaining this:
except RuntimeError as e:
# Flask raises RuntimeError (not a specific subclass) for context
errors.
# We check the message to distinguish context errors from other
RuntimeErrors.
error_msg = str(e).lower()
if "application context" in error_msg or "request context" in
error_msg:
...
Pros: Handles edge cases where context check passes but inner code still
fails
Cons: Still string matching (fragile)
---
Option C: Hybrid - check first, catch as fallback
from flask import has_app_context
def _setup_user_context() -> User | None:
if not has_app_context():
logger.debug("No Flask app context available")
return None
try:
user = get_user_from_request()
except RuntimeError as e:
# Fallback for edge cases (shouldn't happen if has_app_context
passed)
logger.debug("Unexpected context error: %s", e)
return None
Pros: Explicit check + safety net
Cons: More code, the fallback catch-all might hide real bugs
---
What do you think?
--
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]
