aminghadersohi commented on code in PR #39920:
URL: https://github.com/apache/superset/pull/39920#discussion_r3198835042
##########
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:
Addressed. — agor claude on Amin's behalf
##########
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:
Addressed. — agor claude on Amin's behalf
##########
superset/mcp_service/dataset/tool/list_datasets.py:
##########
@@ -96,8 +96,23 @@ async def list_datasets(
Returns dataset metadata including table name, schema, and last modified
time.
- Sortable columns for order_column: id, table_name, schema, changed_on,
- created_on
+ **IMPORTANT**: All parameters must be wrapped in a ``request`` object.
+ Do NOT pass ``search``, ``page``, ``page_size``, etc. as top-level
+ keyword arguments — they will be rejected. Use the ``request`` wrapper::
+
+ # Correct usage
+ list_datasets(request={"search": "sales", "page": 1, "page_size": 10})
+ list_datasets(request={"filters": [{"col": "table_name", "opr": "ct",
"value": "orders"}]})
Review Comment:
Addressed. — agor claude on Amin's behalf
##########
superset/mcp_service/chart/tool/list_charts.py:
##########
@@ -91,8 +91,24 @@ async def list_charts(
Returns chart metadata including id, name, viz_type, URL, and last
modified time.
- Sortable columns for order_column: id, slice_name, viz_type, description,
- changed_on, created_on
+ **IMPORTANT**: All parameters must be wrapped in a ``request`` object.
+ Do NOT pass ``search``, ``page``, ``page_size``, etc. as top-level
+ keyword arguments — they will be rejected. Use the ``request`` wrapper::
+
+ # Correct usage
+ list_charts(request={"search": "revenue", "page": 1, "page_size": 10})
+ list_charts(request={"filters": [{"col": "slice_name", "opr": "ct",
"value": "sales"}]})
Review Comment:
Addressed. — agor claude on Amin's behalf
##########
superset/mcp_service/dashboard/tool/list_dashboards.py:
##########
@@ -85,8 +85,24 @@ async def list_dashboards(
including title, slug, URL, and last modified time. Use select_columns to
request additional fields.
- Sortable columns for order_column: id, dashboard_title, slug, published,
- changed_on, created_on
+ **IMPORTANT**: All parameters must be wrapped in a ``request`` object.
+ Do NOT pass ``search``, ``page``, ``page_size``, etc. as top-level
+ keyword arguments — they will be rejected. Use the ``request`` wrapper::
+
+ # Correct usage
+ list_dashboards(request={"search": "sales", "page": 1, "page_size":
10})
+ list_dashboards(request={"filters": [{"col": "dashboard_title", "opr":
"ct", "value": "exec"}]})
Review Comment:
Addressed. — agor claude on Amin's behalf
--
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]
