Copilot commented on code in PR #39920:
URL: https://github.com/apache/superset/pull/39920#discussion_r3198679303
##########
tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py:
##########
@@ -2080,3 +2080,63 @@ def test_owned_by_me_and_created_by_me_allowed(self):
request = ListDatasetsRequest(owned_by_me=True, created_by_me=True)
assert request.owned_by_me is True
assert request.created_by_me is True
+
+
+class TestListDatasetsRequestWrapper:
+ """
+ Tests verifying that list_datasets requires a ``request`` wrapper object.
+
+ LLMs sometimes pass parameters like ``search``, ``page``, or ``page_size``
+ as flat top-level kwargs instead of nesting them inside a ``request``
+ object. These tests confirm the correct call shape and that the schema
+ rejects invalid filter column names (e.g. ``created_by_fk``).
+ """
+
+ def test_request_wrapper_with_search(self):
+ """Parameters passed inside request= are accepted."""
+ request = ListDatasetsRequest(search="sales", page=1, page_size=10)
+ assert request.search == "sales"
+ assert request.page == 1
+ assert request.page_size == 10
+
+ def test_request_wrapper_defaults(self):
+ """No-arg constructor produces valid defaults."""
+ request = ListDatasetsRequest()
+ assert request.search is None
+ assert request.page == 1
+ assert request.filters == []
+
+ def test_dataset_filter_valid_col(self):
+ """Valid col values are accepted by DatasetFilter."""
+ for col in ("table_name", "schema", "database_name"):
+ f = DatasetFilter(col=col, opr="ct", value="test")
+ assert f.col == col
+
+ def test_dataset_filter_invalid_col_raises(self):
+ """Column names not in the Literal are rejected with a validation
error.
+
+ This guards against LLMs passing ``created_by_fk`` or similar
+ internal column names that are not exposed as filter fields.
+ """
+ from pydantic import ValidationError
+
+ for bad_col in ("created_by_fk", "id", "database_id", "owner"):
+ with pytest.raises(ValidationError):
+ DatasetFilter(col=bad_col, opr="eq", value="1")
+
+ @pytest.mark.asyncio
+ async def test_flat_kwargs_rejected(self, mcp_server):
+ """Passing search/page/page_size as top-level kwargs raises a
ToolError.
+
+ This is the exact failure pattern reported in story #105712: LLMs call
+ ``list_datasets(search=..., page=..., page_size=...)`` instead of
+ ``list_datasets(request={...})``.
+ """
+ from fastmcp.exceptions import ToolError
+
+ with pytest.raises(ToolError):
+ async with Client(mcp_server) as client:
+ await client.call_tool(
+ "list_datasets",
+ {"search": "sales", "page": 1, "page_size": 10},
+ )
Review Comment:
`test_flat_kwargs_rejected` only asserts that a `ToolError` is raised, but
doesn’t assert anything about the error content. This can allow false positives
(e.g. failures unrelated to argument-shape validation) and won’t protect the
specific regression this test is meant to catch. Consider matching on a stable
substring like the missing `request` argument / unexpected kwargs message so
the test proves the rejection is due to flat kwargs.
##########
tests/unit_tests/mcp_service/dataset/tool/test_dataset_tools.py:
##########
@@ -2080,3 +2080,63 @@ def test_owned_by_me_and_created_by_me_allowed(self):
request = ListDatasetsRequest(owned_by_me=True, created_by_me=True)
assert request.owned_by_me is True
assert request.created_by_me is True
+
+
+class TestListDatasetsRequestWrapper:
+ """
+ Tests verifying that list_datasets requires a ``request`` wrapper object.
+
+ LLMs sometimes pass parameters like ``search``, ``page``, or ``page_size``
+ as flat top-level kwargs instead of nesting them inside a ``request``
+ object. These tests confirm the correct call shape and that the schema
+ rejects invalid filter column names (e.g. ``created_by_fk``).
+ """
+
+ def test_request_wrapper_with_search(self):
+ """Parameters passed inside request= are accepted."""
+ request = ListDatasetsRequest(search="sales", page=1, page_size=10)
+ assert request.search == "sales"
+ assert request.page == 1
+ assert request.page_size == 10
+
+ def test_request_wrapper_defaults(self):
+ """No-arg constructor produces valid defaults."""
+ request = ListDatasetsRequest()
+ assert request.search is None
+ assert request.page == 1
+ assert request.filters == []
+
Review Comment:
`TestListDatasetsRequestWrapper` claims to verify the required `request`
wrapper for the `list_datasets` tool, but the first two tests only instantiate
`ListDatasetsRequest` directly and never exercise the actual tool invocation
shape (e.g. calling `client.call_tool("list_datasets", {"request": {...}})` and
asserting it succeeds). Consider either adding a positive tool-call test for
the wrapped shape (with DAO mocked as needed) or renaming/rewording these tests
so they don’t imply they cover the MCP wrapper behavior.
--
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]
