codeant-ai-for-open-source[bot] commented on code in PR #38035:
URL: https://github.com/apache/superset/pull/38035#discussion_r2955419580
##########
superset/mcp_service/utils/schema_utils.py:
##########
@@ -444,85 +448,230 @@ async def list_charts(
"""
def decorator(func: Callable[..., Any]) -> Callable[..., Any]:
- import types
-
- def _maybe_parse(request: Any) -> Any:
- if _is_parse_request_enabled():
- try:
- return parse_json_or_model(request, request_class,
"request")
- except ValidationError as e:
- from fastmcp.exceptions import ToolError
-
- details = []
- for err in e.errors():
- field = " -> ".join(str(loc) for loc in err["loc"])
- details.append(f"{field}: {err['msg']}")
- required_fields = [
- f.alias or name
- for name, f in request_class.model_fields.items()
- if f.is_required()
- ]
- raise ToolError(
- f"Invalid request parameters: {'; '.join(details)}. "
- f"Required fields for {request_class.__name__}: "
- f"{', '.join(required_fields)}"
- ) from None
- return request
-
- if asyncio.iscoroutinefunction(func):
-
- @wraps(func)
- async def async_wrapper(request: Any, *args: Any, **kwargs: Any)
-> Any:
- from fastmcp.server.dependencies import get_context
-
- ctx = get_context()
- return await func(_maybe_parse(request), ctx, *args, **kwargs)
-
- wrapper = async_wrapper
- else:
+ if _is_parse_request_enabled():
+ return _create_string_parsing_wrapper(func, request_class)
+ return _create_flattened_wrapper(func, request_class)
+
+ return decorator
- @wraps(func)
- def sync_wrapper(request: Any, *args: Any, **kwargs: Any) -> Any:
- from fastmcp.server.dependencies import get_context
-
- ctx = get_context()
- return func(_maybe_parse(request), ctx, *args, **kwargs)
-
- wrapper = sync_wrapper
-
- # Merge original function's __globals__ into wrapper's __globals__
- # This allows get_type_hints() to resolve type annotations from the
- # original module (e.g., Context from fastmcp)
- # FastMCP 2.13.2+ uses get_type_hints() which needs access to these
types
- merged_globals = {**wrapper.__globals__, **func.__globals__} # type:
ignore[attr-defined]
- new_wrapper = types.FunctionType(
- wrapper.__code__, # type: ignore[attr-defined]
- merged_globals,
- wrapper.__name__,
- wrapper.__defaults__, # type: ignore[attr-defined]
- wrapper.__closure__, # type: ignore[attr-defined]
- )
- # Copy __dict__ but exclude __wrapped__
- # NOTE: We intentionally do NOT preserve __wrapped__ here.
- # Setting __wrapped__ causes inspect.signature() to follow the chain
- # and find 'ctx' in the original function's signature, even after
- # FastMCP's create_function_without_params removes it from annotations.
- # This breaks Pydantic's TypeAdapter which expects signature params
- # to match type_hints.
- new_wrapper.__dict__.update(
- {k: v for k, v in wrapper.__dict__.items() if k != "__wrapped__"}
- )
- new_wrapper.__module__ = wrapper.__module__
- new_wrapper.__qualname__ = wrapper.__qualname__
- # Copy docstring from original function (not wrapper, which has no
docstring)
- new_wrapper.__doc__ = func.__doc__
- request_annotation = str | request_class
- _apply_signature_for_fastmcp(new_wrapper, func, request_annotation)
+def _create_string_parsing_wrapper(
+ func: Callable[..., Any],
+ request_class: Type[BaseModel],
+) -> Callable[..., Any]:
+ """Create a wrapper that accepts a single `request: str | Model` parameter.
- return new_wrapper
+ This is the original parse_request behavior: at runtime, if the request
+ is a JSON string it gets parsed into the Pydantic model. Used when
+ MCP_PARSE_REQUEST_ENABLED is True.
+ """
+ import types
+
+ def _maybe_parse(request: Any) -> Any:
+ if _is_parse_request_enabled():
+ try:
+ return parse_json_or_model(request, request_class, "request")
+ except ValidationError as e:
+ from fastmcp.exceptions import ToolError
+
Review Comment:
**Suggestion:** Malformed JSON requests currently raise `JSONParseError`
from `parse_json_or_model`, but this wrapper only catches `ValidationError`.
That uncaught exception bypasses the MCP-friendly `ToolError` path and can
surface as an internal server error instead of a user-facing parameter error.
Catch `JSONParseError` explicitly and convert it to `ToolError`. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Stdio MCP parse failures return less actionable errors.
- ⚠️ All `@parse_request` tools share this malformed-JSON path.
```
</details>
```suggestion
from fastmcp.exceptions import ToolError
try:
return parse_json_or_model(request, request_class,
"request")
except JSONParseError as e:
raise ToolError(str(e)) from None
except ValidationError as e:
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start MCP via stdio path (`python -m superset.mcp_service`):
`superset/mcp_service/__main__.py:52-55` defaults transport to `stdio`, and
`__main__.py:99` calls `init_fastmcp_server()` without passing middleware.
2. In that path, `superset/mcp_service/app.py:51-54` only adds middleware if
provided, so
the global `ValueError -> ToolError` conversion from
`GlobalErrorHandlerMiddleware` is not
guaranteed in stdio startup.
3. Call any tool decorated with `@parse_request`, e.g. `list_charts` at
`superset/mcp_service/chart/tool/list_charts.py:16-26`, with malformed JSON
string
arguments.
4. Execution reaches `_maybe_parse`
(`superset/mcp_service/utils/schema_utils.py:470-474`), which calls
`parse_json_or_model`;
that calls `parse_json_or_passthrough(..., strict=True)`
(`schema_utils.py:186-220`), and
malformed JSON raises `JSONParseError` (`schema_utils.py:108`).
5. `_maybe_parse` catches only `ValidationError` (`schema_utils.py:474`), so
`JSONParseError` bypasses this decorator's MCP-friendly `ToolError`
formatting and
surfaces as a less user-friendly exception path.
```
</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:** 472:476
**Comment:**
*Possible Bug: Malformed JSON requests currently raise `JSONParseError`
from `parse_json_or_model`, but this wrapper only catches `ValidationError`.
That uncaught exception bypasses the MCP-friendly `ToolError` path and can
surface as an internal server error instead of a user-facing parameter error.
Catch `JSONParseError` explicitly and convert it to `ToolError`.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=681eb2b787a2a179da9be06c4fad1b02439b163cf9213884eff44b8a027811c1&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=681eb2b787a2a179da9be06c4fad1b02439b163cf9213884eff44b8a027811c1&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]
