codeant-ai-for-open-source[bot] commented on code in PR #38731:
URL: https://github.com/apache/superset/pull/38731#discussion_r2956320167
##########
tests/unit_tests/mcp_service/utils/test_schema_utils.py:
##########
@@ -758,19 +758,45 @@ async def my_tool(request, ctx=None):
for param in sig.parameters.values():
assert param.kind == inspect.Parameter.KEYWORD_ONLY
- def test_flattened_validation_error(self):
- """Pydantic validation errors should propagate when constructing
model."""
+ def test_flattened_validation_error_raises_tool_error(self):
+ """Pydantic validation errors should be converted to ToolError.
+
+ Raw ValidationError propagation causes LangGraph serialization failures
+ (TypeError: encoding without a string argument).
+ """
from unittest.mock import MagicMock, patch
+ from fastmcp.exceptions import ToolError
+
@parse_request(self.SimpleRequest)
def my_tool(request, ctx=None):
return "ok"
mock_ctx = MagicMock()
with patch("fastmcp.server.dependencies.get_context",
return_value=mock_ctx):
- with pytest.raises(ValidationError):
+ with pytest.raises(ToolError):
my_tool(name="test", count="not_a_number")
Review Comment:
**Suggestion:** This test only exercises the sync flattened wrapper, so a
regression in the async flattened path could still leak raw `ValidationError`
and break downstream error serialization. Cover both sync and async invocations
in this test. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Async flattened wrapper lacks direct regression test coverage.
- ⚠️ Many async MCP tools use `@parse_request`.
- ❌ Async validation regression could escape CI checks.
```
</details>
```suggestion
@pytest.mark.parametrize("is_async", [False, True])
def test_flattened_validation_error_raises_tool_error(self, is_async):
"""Pydantic validation errors should be converted to ToolError.
Raw ValidationError propagation causes LangGraph serialization
failures
(TypeError: encoding without a string argument).
"""
from unittest.mock import MagicMock, patch
from fastmcp.exceptions import ToolError
if is_async:
@parse_request(self.SimpleRequest)
async def my_tool(request, ctx=None):
return "ok"
else:
@parse_request(self.SimpleRequest)
def my_tool(request, ctx=None):
return "ok"
mock_ctx = MagicMock()
with patch("fastmcp.server.dependencies.get_context",
return_value=mock_ctx):
with pytest.raises(ToolError, match="Invalid request
parameters"):
if is_async:
asyncio.run(my_tool(name="test", count="not_a_number"))
else:
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In flattened-mode tests,
`TestParseRequestFlattenedDecorator._disable_parse_request`
forces `MCP_PARSE_REQUEST_ENABLED=False`
(`tests/unit_tests/mcp_service/utils/test_schema_utils.py`, class fixture in
same class),
so `parse_request()` routes to `_create_flattened_wrapper()`
(`superset/mcp_service/utils/schema_utils.py:547`).
2. `_create_flattened_wrapper()` has separate async and sync branches
(`schema_utils.py:560-68` async, `schema_utils.py:596-614` sync), each with
its own
`ValidationError` -> `ToolError` handling logic.
3. The current test at `test_schema_utils.py:761-779` defines only a sync
`def
my_tool(...)` and invokes it directly, so only `sync_wrapper` is executed;
`async_wrapper`
is never exercised.
4. Run this single test (`pytest
tests/unit_tests/mcp_service/utils/test_schema_utils.py
-k flattened_validation_error_raises_tool_error`) and observe it passes
while providing no
assertion on async behavior; this leaves an async-specific regression
undetected.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/utils/test_schema_utils.py
**Line:** 761:778
**Comment:**
*Logic Error: This test only exercises the sync flattened wrapper, so a
regression in the async flattened path could still leak raw `ValidationError`
and break downstream error serialization. Cover both sync and async invocations
in this test.
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%2F38731&comment_hash=25abbc437b19e1e3fba93a204f76536c94217fc4aed628c2bcc9ede9f4092208&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38731&comment_hash=25abbc437b19e1e3fba93a204f76536c94217fc4aed628c2bcc9ede9f4092208&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/utils/test_schema_utils.py:
##########
@@ -758,19 +758,45 @@ async def my_tool(request, ctx=None):
for param in sig.parameters.values():
assert param.kind == inspect.Parameter.KEYWORD_ONLY
- def test_flattened_validation_error(self):
- """Pydantic validation errors should propagate when constructing
model."""
+ def test_flattened_validation_error_raises_tool_error(self):
+ """Pydantic validation errors should be converted to ToolError.
+
+ Raw ValidationError propagation causes LangGraph serialization failures
+ (TypeError: encoding without a string argument).
+ """
from unittest.mock import MagicMock, patch
+ from fastmcp.exceptions import ToolError
+
@parse_request(self.SimpleRequest)
def my_tool(request, ctx=None):
return "ok"
mock_ctx = MagicMock()
with patch("fastmcp.server.dependencies.get_context",
return_value=mock_ctx):
- with pytest.raises(ValidationError):
+ with pytest.raises(ToolError):
my_tool(name="test", count="not_a_number")
+ def test_flattened_missing_required_field_raises_tool_error(self):
+ """Missing required fields should raise ToolError with helpful
message."""
+ from unittest.mock import MagicMock, patch
+
+ from fastmcp.exceptions import ToolError
+
+ class RequiredFieldRequest(BaseModel):
+ model_config = ConfigDict(extra="forbid")
+ name: str = Field(..., description="Required name")
+ count: int = Field(..., description="Required count")
+
+ @parse_request(RequiredFieldRequest)
+ def my_tool(request, ctx=None):
+ return "ok"
+
+ mock_ctx = MagicMock()
+ with patch("fastmcp.server.dependencies.get_context",
return_value=mock_ctx):
+ with pytest.raises(ToolError, match="Invalid request parameters"):
+ my_tool(name="test") # missing count
Review Comment:
**Suggestion:** This test validates missing-required-field handling only for
the sync flattened wrapper, so async behavior can regress without detection and
reintroduce raw validation failures in async tools. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Missing-field async branch remains untested in flattened mode.
- ⚠️ CI cannot catch async-only validation message regressions.
- ❌ Async MCP calls may leak wrong error type.
```
</details>
```suggestion
@pytest.mark.parametrize("is_async", [False, True])
def test_flattened_missing_required_field_raises_tool_error(self,
is_async):
"""Missing required fields should raise ToolError with helpful
message."""
from unittest.mock import MagicMock, patch
from fastmcp.exceptions import ToolError
class RequiredFieldRequest(BaseModel):
model_config = ConfigDict(extra="forbid")
name: str = Field(..., description="Required name")
count: int = Field(..., description="Required count")
if is_async:
@parse_request(RequiredFieldRequest)
async def my_tool(request, ctx=None):
return "ok"
else:
@parse_request(RequiredFieldRequest)
def my_tool(request, ctx=None):
return "ok"
mock_ctx = MagicMock()
with patch("fastmcp.server.dependencies.get_context",
return_value=mock_ctx):
with pytest.raises(ToolError, match="Invalid request
parameters"):
if is_async:
asyncio.run(my_tool(name="test")) # missing count
else:
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Flattened mode is active in this test class via fixture patching
(`tests/unit_tests/mcp_service/utils/test_schema_utils.py`,
`TestParseRequestFlattenedDecorator`), so decorated tools use
`_create_flattened_wrapper()`
(`superset/mcp_service/utils/schema_utils.py:547`).
2. Missing required fields are converted in branch-local handlers after
`request_class.model_validate(kwargs)` fails (`schema_utils.py:569-665`
async and sync
equivalents).
3. Current test `test_flattened_missing_required_field_raises_tool_error`
(`test_schema_utils.py:780-799`) defines only sync `my_tool` and calls
`my_tool(name="test")`; async missing-field path is not executed.
4. Execute `pytest tests/unit_tests/mcp_service/utils/test_schema_utils.py -k
flattened_missing_required_field_raises_tool_error`; it validates sync
behavior only,
leaving async missing-field conversion without explicit guard.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/utils/test_schema_utils.py
**Line:** 780:798
**Comment:**
*Logic Error: This test validates missing-required-field handling only
for the sync flattened wrapper, so async behavior can regress without detection
and reintroduce raw validation failures in async tools.
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%2F38731&comment_hash=182424bb106ba6f1fdb2b99e268384032ae333fcf5896e3e78ad309b5c5b3a96&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38731&comment_hash=182424bb106ba6f1fdb2b99e268384032ae333fcf5896e3e78ad309b5c5b3a96&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]
