-
Notifications
You must be signed in to change notification settings - Fork 2k
Remove OpenAPI timeout parameter, make client optional, surface timeout errors #3067
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,7 +3,8 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from __future__ import annotations | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from collections import Counter | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from collections.abc import Sequence | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from collections.abc import AsyncIterator, Sequence | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from contextlib import asynccontextmanager | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import Any, Literal | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import httpx | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -44,6 +45,8 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger = get_logger(__name__) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| DEFAULT_TIMEOUT: float = 30.0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class OpenAPIProvider(Provider): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Provider that creates MCP components from an OpenAPI specification. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -68,31 +71,34 @@ class OpenAPIProvider(Provider): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def __init__( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| openapi_spec: dict[str, Any], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| client: httpx.AsyncClient, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| client: httpx.AsyncClient | None = None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| *, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| route_maps: list[RouteMap] | None = None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| route_map_fn: RouteMapFn | None = None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mcp_component_fn: ComponentFn | None = None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mcp_names: dict[str, str] | None = None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tags: set[str] | None = None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| timeout: float | None = None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Initialize provider by parsing OpenAPI spec and creating components. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| openapi_spec: OpenAPI schema as a dictionary | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| client: httpx AsyncClient for making HTTP requests | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| client: Optional httpx AsyncClient for making HTTP requests. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| If not provided, a default client is created using the first | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| server URL from the OpenAPI spec with a 30-second timeout. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| To customize timeout or other settings, pass your own client. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| route_maps: Optional list of RouteMap objects defining route mappings | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| route_map_fn: Optional callable for advanced route type mapping | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mcp_component_fn: Optional callable for component customization | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mcp_names: Optional dictionary mapping operationId to component names | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tags: Optional set of tags to add to all components | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| timeout: Optional timeout (in seconds) for all requests | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| super().__init__() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self._owns_client = client is None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if client is None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| client = self._create_default_client(openapi_spec) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self._client = client | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self._timeout = timeout | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self._mcp_component_fn = mcp_component_fn | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Keep track of names to detect collisions | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -153,6 +159,27 @@ def __init__( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.debug(f"Created OpenAPIProvider with {len(http_routes)} routes") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @classmethod | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _create_default_client(cls, openapi_spec: dict[str, Any]) -> httpx.AsyncClient: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Create a default httpx client from the OpenAPI spec's server URL.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| servers = openapi_spec.get("servers", []) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not servers or not servers[0].get("url"): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise ValueError( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "No server URL found in OpenAPI spec. Either add a 'servers' " | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "entry to the spec or provide an httpx.AsyncClient explicitly." | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| base_url = servers[0]["url"] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return httpx.AsyncClient(base_url=base_url, timeout=DEFAULT_TIMEOUT) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+162
to
+173
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result:
Sources: HTTPX API reference for 🌐 Web query:
💡 Result:
What So, instead of: httpx.Client(base_url="https://api.example.com/{version}")
client.get("/users/{id}")you should fill variables yourself first, e.g.: version = "v1"
user_id = 123
with httpx.Client(base_url=f"https://api.example.com/{version}") as client:
r = client.get(f"/users/{user_id}")Sources: HTTPX [1] https://www.python-httpx.org/advanced/clients/ Handle templated and relative server URLs before constructing the default client. OpenAPI server URLs can be templated (e.g., Detect and resolve server URL variables against the OpenAPI 🔧 Suggested hardening for default client creation+from string import Formatter
@@
`@classmethod`
def _create_default_client(cls, openapi_spec: dict[str, Any]) -> httpx.AsyncClient:
"""Create a default httpx client from the OpenAPI spec's server URL."""
servers = openapi_spec.get("servers", [])
- if not servers or not servers[0].get("url"):
- raise ValueError(
- "No server URL found in OpenAPI spec. Either add a 'servers' "
- "entry to the spec or provide an httpx.AsyncClient explicitly."
- )
- base_url = servers[0]["url"]
+ if not servers or not servers[0].get("url"):
+ raise ValueError("Missing OpenAPI server URL; provide an AsyncClient.")
+ server = servers[0]
+ base_url = server["url"]
+ if "{" in base_url:
+ variables = server.get("variables") or {}
+ formatter = Formatter()
+ parts: list[str] = []
+ for literal, field_name, *_ in formatter.parse(base_url):
+ parts.append(literal)
+ if field_name:
+ default = variables.get(field_name, {}).get("default")
+ if default is None:
+ raise ValueError(
+ f"Server URL variable '{field_name}' has no default; "
+ "provide an AsyncClient explicitly."
+ )
+ parts.append(str(default))
+ base_url = "".join(parts)
+ if not base_url.startswith(("http://", "https://")):
+ raise ValueError(
+ "Server URL must be absolute (http/https); provide an AsyncClient."
+ )
return httpx.AsyncClient(base_url=base_url, timeout=DEFAULT_TIMEOUT)📝 Committable suggestion
Suggested change
🧰 Tools🪛 Ruff (0.14.14)[warning] 167-170: Avoid specifying long messages outside the exception class (TRY003) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @asynccontextmanager | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def lifespan(self) -> AsyncIterator[None]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Manage the lifecycle of the auto-created httpx client.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if self._owns_client: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async with self._client: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| yield | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| yield | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _generate_default_name( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self, route: HTTPRoute, mcp_names_map: dict[str, str] | None = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> str: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -225,7 +252,6 @@ def _create_openapi_tool( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| parameters=combined_schema, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| output_schema=output_schema, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tags=set(route.tags or []) | tags, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| timeout=self._timeout, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if self._mcp_component_fn is not None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -263,7 +289,6 @@ def _create_openapi_resource( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name=resource_name, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| description=enhanced_description, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tags=set(route.tags or []) | tags, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| timeout=self._timeout, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if self._mcp_component_fn is not None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -331,7 +356,6 @@ def _create_openapi_template( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| description=enhanced_description, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| parameters=template_params_schema, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tags=set(route.tags or []) | tags, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| timeout=self._timeout, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if self._mcp_component_fn is not None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
clientis omitted, the provider constructs its ownhttpx.AsyncClient, butOpenAPIProviderdoes not overridelifespan()to close it, so servers created with the default client will leave connections open on shutdown (often emittingResourceWarningand leaking sockets in long-lived processes). Consider tracking ownership of the created client and callingawait client.aclose()in a provider lifespan teardown.Useful? React with 👍 / 👎.