diff --git a/tools/serper/manifest.yaml b/tools/serper/manifest.yaml index 33cd9c114..74133adbe 100644 --- a/tools/serper/manifest.yaml +++ b/tools/serper/manifest.yaml @@ -37,4 +37,4 @@ resource: tags: - search type: plugin -version: 0.0.7 +version: 0.0.8 diff --git a/tools/serper/tests/__init__.py b/tools/serper/tests/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tools/serper/tests/test_serper.py b/tools/serper/tests/test_serper.py new file mode 100644 index 000000000..d0d034399 --- /dev/null +++ b/tools/serper/tests/test_serper.py @@ -0,0 +1,213 @@ +"""In-process tests for tools/serper (pytest-shaped). + +These tests cover two reliability fixes shipped in this PR: + +- Bug 1: missing `timeout=` on `requests.get` calls. +- Bug 2: `KeyError` when `serperapi_api_key` is missing from credentials. + +`serper_search.py` does not take `hl` / `gl` user input (the values are hardcoded +to "us" / "en"), so the third bug from PR #3456 (invoke fall-through on invalid +hl / gl) does not apply. + +Run with: python3 -m pytest tests/test_serper.py -v + +They use `unittest.mock` and tiny in-test stubs for `requests` and `dify_plugin` +so they can run in environments where the plugin dependencies are not installed. +""" + +from __future__ import annotations + +import sys +import types +from pathlib import Path +from types import SimpleNamespace +from unittest import mock + +_HERE = Path(__file__).resolve().parent # tests/ +_PLUGIN_ROOT = _HERE.parent # tools/serper/ +sys.path.insert(0, str(_PLUGIN_ROOT)) # so `tools.serper_search` works + + +# ---- Stub modules ---------------------------------------------------------- + +def _ensure_stub_modules() -> None: + """Insert minimal stubs into sys.modules if a dep is not installed.""" + if "requests" not in sys.modules: + requests_stub = types.ModuleType("requests") + requests_stub.exceptions = types.SimpleNamespace( + RequestException=type("RequestException", (Exception,), {}) + ) + + def _default_get(*args, **kwargs): + raise AssertionError( + "requests.get was called in a test that did not patch it" + ) + + requests_stub.get = _default_get + sys.modules["requests"] = requests_stub + + if "dify_plugin" not in sys.modules: + dify_plugin_stub = types.ModuleType("dify_plugin") + + class _BaseTool: + """Bare-minimum stand-in for dify_plugin.Tool.""" + + def __init__(self): + self.runtime = SimpleNamespace(credentials={}) + + def create_text_message(self, text): + return ("text", text) + + def create_json_message(self, json): + return ("json", json) + + dify_plugin_stub.Tool = _BaseTool + + dify_plugin_entities_stub = types.ModuleType("dify_plugin.entities") + tool_module_stub = types.ModuleType("dify_plugin.entities.tool") + + class _ToolInvokeMessage: + def __init__(self, *args, **kwargs): + self.args = args + self.kwargs = kwargs + + tool_module_stub.ToolInvokeMessage = _ToolInvokeMessage + + dify_plugin_entities_stub.tool = tool_module_stub + sys.modules["dify_plugin"] = dify_plugin_stub + sys.modules["dify_plugin.entities"] = dify_plugin_entities_stub + sys.modules["dify_plugin.entities.tool"] = tool_module_stub + + +_ensure_stub_modules() + +# Force-fresh imports so we get the patched stubs above. +for _name in list(sys.modules): + if _name.startswith("tools.serper"): + sys.modules.pop(_name, None) + +from importlib import util as _importlib_util # noqa: E402 + + +def _load_module(file_name: str): + path = _PLUGIN_ROOT / "tools" / file_name + spec = _importlib_util.spec_from_file_location(f"serper.{file_name[:-3]}", path) + mod = _importlib_util.module_from_spec(spec) + sys.modules[spec.name] = mod + spec.loader.exec_module(mod) + return mod + + +serper = _load_module("serper_search.py") + + +# ---- Helpers --------------------------------------------------------------- + +class _FakeResponse: + def __init__(self, payload=None, *, status_code=200): + self._payload = payload if payload is not None else { + "knowledgeGraph": {"title": "Example", "description": "An example."}, + "organic": [{"title": "T", "link": "https://x", "snippet": "S"}], + } + self.status_code = status_code + + def raise_for_status(self): + if self.status_code >= 400: + raise RuntimeError(f"HTTP {self.status_code}") + + def json(self): + return self._payload + + +def _make_tool(credentials): + tool = object.__new__(serper.SerperSearchTool) + + def _text(text): + return ("text", text) + + def _json(json=None, **_kw): + return ("json", json) + + tool.create_text_message = _text + tool.create_json_message = _json + tool.runtime = SimpleNamespace(credentials=credentials) + return tool + + +def _flatten(messages): + return list(messages) + + +# ============================================================================= +# Bug 2: missing-credential guard +# ============================================================================= + +def test_serper_missing_api_key_returns_message_and_no_http_call(): + tool = _make_tool(credentials={}) + fake_get = mock.Mock(side_effect=AssertionError("requests.get must not be called")) + with mock.patch.object(serper.requests, "get", fake_get): + messages = _flatten(tool._invoke({"query": "hello"})) + assert len(messages) == 1 + assert messages[0][0] == "text" + assert messages[0][1] == "Serper.dev API key is required." + assert fake_get.call_count == 0 + + +def test_serper_empty_api_key_returns_message_and_no_http_call(): + tool = _make_tool(credentials={"serperapi_api_key": ""}) + fake_get = mock.Mock(side_effect=AssertionError("requests.get must not be called")) + with mock.patch.object(serper.requests, "get", fake_get): + messages = _flatten(tool._invoke({"query": "hello"})) + assert messages[0][1] == "Serper.dev API key is required." + assert fake_get.call_count == 0 + + +def test_serper_missing_api_key_does_not_raise_keyerror(): + """Lock the post-fix contract: missing key never raises KeyError.""" + tool = _make_tool(credentials={}) + fake_get = mock.Mock(side_effect=AssertionError("requests.get must not be called")) + with mock.patch.object(serper.requests, "get", fake_get): + messages = _flatten(tool._invoke({"query": "hello"})) + assert messages[0] == ("text", "Serper.dev API key is required.") + + +# ============================================================================= +# Bug 1: requests.get forwards a 10s timeout. +# ============================================================================= + +def test_serper_requests_get_uses_10s_timeout(): + tool = _make_tool(credentials={"serperapi_api_key": "secret"}) + fake_response = _FakeResponse() + fake_get = mock.Mock(return_value=fake_response) + with mock.patch.object(serper.requests, "get", fake_get): + _flatten(tool._invoke({"query": "hello"})) + assert fake_get.call_count == 1 + _, kwargs = fake_get.call_args + assert kwargs.get("timeout") == 10, f"expected timeout=10 kwarg, got {kwargs!r}" + + +# ============================================================================= +# Happy-path smoke +# ============================================================================= + +def test_serper_happy_path_yields_json_message(): + tool = _make_tool(credentials={"serperapi_api_key": "secret"}) + fake_response = _FakeResponse() + fake_get = mock.Mock(return_value=fake_response) + with mock.patch.object(serper.requests, "get", fake_get): + messages = _flatten(tool._invoke({"query": "hello"})) + assert any(m[0] == "json" for m in messages) + + +# ============================================================================= +# Network failure surfaces as a friendly message, not a stack trace. +# ============================================================================= + +def test_serper_request_exception_yields_friendly_message(): + tool = _make_tool(credentials={"serperapi_api_key": "secret"}) + fake_get = mock.Mock(side_effect=serper.requests.exceptions.RequestException("boom")) + with mock.patch.object(serper.requests, "get", fake_get): + messages = _flatten(tool._invoke({"query": "hello"})) + assert any( + m[0] == "text" and "boom" in m[1] for m in messages + ), f"expected friendly error message containing 'boom', got {messages!r}" diff --git a/tools/serper/tools/serper_search.py b/tools/serper/tools/serper_search.py index 6dc23dfd3..589321982 100644 --- a/tools/serper/tools/serper_search.py +++ b/tools/serper/tools/serper_search.py @@ -22,9 +22,22 @@ def _parse_response(self, response: dict) -> dict: def _invoke( self, tool_parameters: dict[str, Any] ) -> Generator[ToolInvokeMessage, None, None]: + serperapi_api_key = self.runtime.credentials.get("serperapi_api_key") + if not serperapi_api_key: + yield self.create_text_message("Serper.dev API key is required.") + return + params = {"q": tool_parameters["query"], "gl": "us", "hl": "en"} - headers = {"X-API-KEY": self.runtime.credentials["serperapi_api_key"], "Content-Type": "application/json"} - response = requests.get(url=SERPER_API_URL, params=params, headers=headers) - response.raise_for_status() - valuable_res = self._parse_response(response.json()) + headers = {"X-API-KEY": serperapi_api_key, "Content-Type": "application/json"} + try: + response = requests.get( + url=SERPER_API_URL, params=params, headers=headers, timeout=10 + ) + response.raise_for_status() + valuable_res = self._parse_response(response.json()) + except requests.exceptions.RequestException as exc: + yield self.create_text_message( + f"An error occurred while invoking the tool: {exc}." + ) + return yield self.create_json_message(valuable_res)