aminghadersohi commented on code in PR #40906:
URL: https://github.com/apache/superset/pull/40906#discussion_r3404396814
##########
superset/mcp_service/server.py:
##########
@@ -643,6 +690,9 @@ def _make_call_tool(self) -> Any:
"""Build the normalized ``call_tool`` proxy for regex
search."""
return make_normalizing_call_tool(self)
+ def _make_search_tool(self) -> Any:
+ return _make_optional_query_search_tool(self)
Review Comment:
Done in efb0dac53dc888cf39b618a0216ccccc8b8ee290 — added a one-line
docstring to the `_make_search_tool` override, matching the sibling
`_make_call_tool` overrides.
##########
superset/mcp_service/server.py:
##########
@@ -659,6 +709,9 @@ def _make_call_tool(self) -> Any:
"""Build the normalized ``call_tool`` proxy for BM25 search."""
return make_normalizing_call_tool(self)
+ def _make_search_tool(self) -> Any:
+ return _make_optional_query_search_tool(self)
Review Comment:
Done in efb0dac53dc888cf39b618a0216ccccc8b8ee290 — added a one-line
docstring to the `_make_search_tool` override, matching the sibling
`_make_call_tool` overrides.
##########
tests/unit_tests/mcp_service/test_tool_search_transform.py:
##########
@@ -1226,3 +1226,92 @@ def
test_create_serializer_include_schemas_true_with_compact():
assert result[0]["inputSchema"]["properties"]["filters"]["items"] == {
"type": "object"
}
+
+
+# -- search_tools optional query tests --
+
+
+def test_search_tool_query_is_optional_in_schema():
Review Comment:
Done in efb0dac53dc888cf39b618a0216ccccc8b8ee290 — added `-> None` return
annotations to the new test functions.
##########
tests/unit_tests/mcp_service/test_tool_search_transform.py:
##########
@@ -1226,3 +1226,92 @@ def
test_create_serializer_include_schemas_true_with_compact():
assert result[0]["inputSchema"]["properties"]["filters"]["items"] == {
"type": "object"
}
+
+
+# -- search_tools optional query tests --
+
+
+def test_search_tool_query_is_optional_in_schema():
+ """search_tools schema marks query optional with a flat concrete type.
+
+ The query schema must not use ``anyOf`` — MCP bridges (mcp-remote,
+ Claude Desktop) strip ``anyOf`` and leave the field typeless, the same
+ failure mode ``_fix_call_tool_arguments`` guards against.
+ """
+ mock_mcp = MagicMock()
+ config = {
+ "strategy": "bm25",
+ "max_results": 5,
+ "always_visible": [],
+ "search_tool_name": "search_tools",
+ "call_tool_name": "call_tool",
+ }
+ _apply_tool_search_transform(mock_mcp, config)
+ transform = mock_mcp.add_transform.call_args[0][0]
+ search_tool = transform._make_search_tool()
+
+ params = search_tool.parameters
+ assert "query" not in params.get("required", [])
+ query_schema = params["properties"]["query"]
+ assert query_schema["type"] == "string"
+ assert "anyOf" not in query_schema
+
+
+def test_search_tool_with_no_query_returns_all_visible_tools():
Review Comment:
Done in efb0dac53dc888cf39b618a0216ccccc8b8ee290 — added `-> None` return
annotations to the new test functions.
##########
tests/unit_tests/mcp_service/test_tool_search_transform.py:
##########
@@ -1226,3 +1226,92 @@ def
test_create_serializer_include_schemas_true_with_compact():
assert result[0]["inputSchema"]["properties"]["filters"]["items"] == {
"type": "object"
}
+
+
+# -- search_tools optional query tests --
+
+
+def test_search_tool_query_is_optional_in_schema():
+ """search_tools schema marks query optional with a flat concrete type.
+
+ The query schema must not use ``anyOf`` — MCP bridges (mcp-remote,
+ Claude Desktop) strip ``anyOf`` and leave the field typeless, the same
+ failure mode ``_fix_call_tool_arguments`` guards against.
+ """
+ mock_mcp = MagicMock()
+ config = {
+ "strategy": "bm25",
+ "max_results": 5,
+ "always_visible": [],
+ "search_tool_name": "search_tools",
+ "call_tool_name": "call_tool",
+ }
+ _apply_tool_search_transform(mock_mcp, config)
+ transform = mock_mcp.add_transform.call_args[0][0]
+ search_tool = transform._make_search_tool()
+
+ params = search_tool.parameters
+ assert "query" not in params.get("required", [])
+ query_schema = params["properties"]["query"]
+ assert query_schema["type"] == "string"
+ assert "anyOf" not in query_schema
+
+
+def test_search_tool_with_no_query_returns_all_visible_tools():
+ """search_tools returns all visible tools when called with no arguments."""
+ import asyncio
+ from unittest.mock import AsyncMock
+
+ mock_mcp = MagicMock()
+ config = {
+ "strategy": "bm25",
+ "max_results": 5,
+ "always_visible": [],
+ "search_tool_name": "search_tools",
+ "call_tool_name": "call_tool",
+ }
+ _apply_tool_search_transform(mock_mcp, config)
+ transform = mock_mcp.add_transform.call_args[0][0]
+
+ tool_a = MagicMock()
+ tool_b = MagicMock()
+ all_tools = [tool_a, tool_b]
+
+ async def run():
+ transform._get_visible_tools = AsyncMock(return_value=all_tools)
+ transform._render_results = AsyncMock(return_value=[{"name":
"tool_a"}])
+ search_tool = transform._make_search_tool()
+ await search_tool.run({}) # must not raise ValidationError
+ return transform._render_results.call_args[0][0]
+
+ rendered_with = asyncio.run(run())
+ assert rendered_with == all_tools
+
+
+def test_search_tool_regex_with_no_query_returns_all_visible_tools():
Review Comment:
Done in efb0dac53dc888cf39b618a0216ccccc8b8ee290 — added `-> None` return
annotations to the new test functions.
##########
tests/unit_tests/mcp_service/test_tool_search_transform.py:
##########
@@ -1226,3 +1226,92 @@ def
test_create_serializer_include_schemas_true_with_compact():
assert result[0]["inputSchema"]["properties"]["filters"]["items"] == {
"type": "object"
}
+
+
+# -- search_tools optional query tests --
+
+
+def test_search_tool_query_is_optional_in_schema():
+ """search_tools schema marks query optional with a flat concrete type.
+
+ The query schema must not use ``anyOf`` — MCP bridges (mcp-remote,
+ Claude Desktop) strip ``anyOf`` and leave the field typeless, the same
+ failure mode ``_fix_call_tool_arguments`` guards against.
+ """
+ mock_mcp = MagicMock()
+ config = {
+ "strategy": "bm25",
+ "max_results": 5,
+ "always_visible": [],
+ "search_tool_name": "search_tools",
+ "call_tool_name": "call_tool",
+ }
+ _apply_tool_search_transform(mock_mcp, config)
+ transform = mock_mcp.add_transform.call_args[0][0]
+ search_tool = transform._make_search_tool()
+
+ params = search_tool.parameters
+ assert "query" not in params.get("required", [])
+ query_schema = params["properties"]["query"]
+ assert query_schema["type"] == "string"
+ assert "anyOf" not in query_schema
+
+
+def test_search_tool_with_no_query_returns_all_visible_tools():
+ """search_tools returns all visible tools when called with no arguments."""
+ import asyncio
+ from unittest.mock import AsyncMock
+
+ mock_mcp = MagicMock()
+ config = {
+ "strategy": "bm25",
+ "max_results": 5,
+ "always_visible": [],
+ "search_tool_name": "search_tools",
+ "call_tool_name": "call_tool",
+ }
+ _apply_tool_search_transform(mock_mcp, config)
+ transform = mock_mcp.add_transform.call_args[0][0]
+
+ tool_a = MagicMock()
+ tool_b = MagicMock()
+ all_tools = [tool_a, tool_b]
+
+ async def run():
Review Comment:
Done in efb0dac53dc888cf39b618a0216ccccc8b8ee290 — annotated the nested
async helper as `-> list[MagicMock]`.
##########
tests/unit_tests/mcp_service/test_tool_search_transform.py:
##########
@@ -1226,3 +1226,92 @@ def
test_create_serializer_include_schemas_true_with_compact():
assert result[0]["inputSchema"]["properties"]["filters"]["items"] == {
"type": "object"
}
+
+
+# -- search_tools optional query tests --
+
+
+def test_search_tool_query_is_optional_in_schema():
+ """search_tools schema marks query optional with a flat concrete type.
+
+ The query schema must not use ``anyOf`` — MCP bridges (mcp-remote,
+ Claude Desktop) strip ``anyOf`` and leave the field typeless, the same
+ failure mode ``_fix_call_tool_arguments`` guards against.
+ """
+ mock_mcp = MagicMock()
+ config = {
+ "strategy": "bm25",
+ "max_results": 5,
+ "always_visible": [],
+ "search_tool_name": "search_tools",
+ "call_tool_name": "call_tool",
+ }
+ _apply_tool_search_transform(mock_mcp, config)
+ transform = mock_mcp.add_transform.call_args[0][0]
+ search_tool = transform._make_search_tool()
+
+ params = search_tool.parameters
+ assert "query" not in params.get("required", [])
+ query_schema = params["properties"]["query"]
+ assert query_schema["type"] == "string"
+ assert "anyOf" not in query_schema
+
+
+def test_search_tool_with_no_query_returns_all_visible_tools():
+ """search_tools returns all visible tools when called with no arguments."""
+ import asyncio
+ from unittest.mock import AsyncMock
+
+ mock_mcp = MagicMock()
+ config = {
+ "strategy": "bm25",
+ "max_results": 5,
+ "always_visible": [],
+ "search_tool_name": "search_tools",
+ "call_tool_name": "call_tool",
+ }
+ _apply_tool_search_transform(mock_mcp, config)
+ transform = mock_mcp.add_transform.call_args[0][0]
+
+ tool_a = MagicMock()
+ tool_b = MagicMock()
+ all_tools = [tool_a, tool_b]
+
+ async def run():
+ transform._get_visible_tools = AsyncMock(return_value=all_tools)
+ transform._render_results = AsyncMock(return_value=[{"name":
"tool_a"}])
+ search_tool = transform._make_search_tool()
+ await search_tool.run({}) # must not raise ValidationError
+ return transform._render_results.call_args[0][0]
+
+ rendered_with = asyncio.run(run())
+ assert rendered_with == all_tools
+
+
+def test_search_tool_regex_with_no_query_returns_all_visible_tools():
+ """Regex strategy returns all visible tools when called with no
arguments."""
+ import asyncio
+ from unittest.mock import AsyncMock
+
+ mock_mcp = MagicMock()
+ config = {
+ "strategy": "regex",
+ "max_results": 5,
+ "always_visible": [],
+ "search_tool_name": "search_tools",
+ "call_tool_name": "call_tool",
+ }
+ _apply_tool_search_transform(mock_mcp, config)
+ transform = mock_mcp.add_transform.call_args[0][0]
+
+ all_tools = [MagicMock(), MagicMock()]
+
+ async def run():
Review Comment:
Done in efb0dac53dc888cf39b618a0216ccccc8b8ee290 — annotated the nested
async helper as `-> list[MagicMock]`.
--
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]
