codeant-ai-for-open-source[bot] commented on code in PR #37617:
URL: https://github.com/apache/superset/pull/37617#discussion_r2755629447
##########
superset/mcp_service/utils/schema_utils.py:
##########
@@ -383,6 +383,24 @@ def validator(cls: Type[BaseModel], v: Any, info: Any =
None) -> List[BaseModel]
return validator
+def _is_parse_request_enabled() -> bool:
+ """Check if parse_request decorator is enabled via config."""
+ try:
+ from flask import current_app, has_app_context
+
+ if has_app_context():
+ return current_app.config.get("MCP_PARSE_REQUEST_ENABLED", True)
+ except (ImportError, RuntimeError):
+ pass
+ try:
+ from superset.mcp_service.flask_singleton import app as flask_app
+
+ return flask_app.config.get("MCP_PARSE_REQUEST_ENABLED", True)
Review Comment:
**Suggestion:** The helper that checks whether the decorator is enabled
returns the raw config value, so if the setting is provided as a non-boolean
(for example, the string "False" from an environment-based config) it will be
treated as truthy and fail to disable the decorator, and it also violates its
own `-> bool` return type contract. Coercing the config value to a real boolean
avoids this misbehavior and makes the function's behavior predictable. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ parse_request decorator remains enabled when disabled.
- ❌ Cannot opt-out workaround via superset_config.py.
- ⚠️ Affects MCP tool request parsing behavior.
- ⚠️ Admins using env-string configs hit this silently.
```
</details>
```suggestion
def _to_bool(value: Any) -> bool:
# Normalize common string/other representations into a real bool
if isinstance(value, str):
return value.strip().lower() not in {"false", "0", "off", ""}
return bool(value)
try:
from flask import current_app, has_app_context
if has_app_context():
return _to_bool(
current_app.config.get("MCP_PARSE_REQUEST_ENABLED", True)
)
except (ImportError, RuntimeError):
pass
try:
from superset.mcp_service.flask_singleton import app as flask_app
return _to_bool(flask_app.config.get("MCP_PARSE_REQUEST_ENABLED",
True))
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Edit superset_config.py to set the flag as a string:
MCP_PARSE_REQUEST_ENABLED =
"False".
2. Start a Flask app context (or run a test app) so current_app is available:
- Example test snippet: create Flask app, set config, then push context.
- File/lines involved: push context and import decorator at
superset/mcp_service/utils/schema_utils.py:404 (parse_request).
3. Apply the decorator (decoration time calls helper):
- In the app context, import parse_request from
superset/mcp_service/utils/schema_utils.py (parse_request defined at line
404).
- Decorating any function triggers decorator(func) which executes the
check at
superset/mcp_service/utils/schema_utils.py:445-447:
"if not _is_parse_request_enabled(): return func".
4. Observe behavior:
- _is_parse_request_enabled() (defined at
superset/mcp_service/utils/schema_utils.py:386-401) returns the raw
string "False" from
current_app.config.
- Because the raw string is truthy, the condition is false and the
decorator remains
enabled contrary to the intent.
- Result: parse_request still wraps functions despite
MCP_PARSE_REQUEST_ENABLED
appearing disabled.
Explanation:
- This reproducer uses actual code paths: config read via current_app (lines
386-392) and
decorator early-return check (lines 445-447). It demonstrates the real-world
misbehavior
when config is provided as environment-derived strings.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/utils/schema_utils.py
**Line:** 388:398
**Comment:**
*Logic Error: The helper that checks whether the decorator is enabled
returns the raw config value, so if the setting is provided as a non-boolean
(for example, the string "False" from an environment-based config) it will be
treated as truthy and fail to disable the decorator, and it also violates its
own `-> bool` return type contract. Coercing the config value to a real boolean
avoids this misbehavior and makes the function's behavior predictable.
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]
