diff --git a/src/fastmcp/server/openapi.py b/src/fastmcp/server/openapi.py index d3b30cad9..35e5d996e 100644 --- a/src/fastmcp/server/openapi.py +++ b/src/fastmcp/server/openapi.py @@ -261,19 +261,45 @@ class OpenAPITool(Tool): async def run(self, arguments: dict[str, Any]) -> ToolResult: """Execute the HTTP request based on the route configuration.""" + # Create mapping from suffixed parameter names back to original names and locations + # This handles parameter collisions where suffixes were added during schema generation + param_mapping = {} # suffixed_name -> (original_name, location) + + # First, check if we have request body properties to detect collisions + body_props = set() + if self._route.request_body and self._route.request_body.content_schema: + content_type = next(iter(self._route.request_body.content_schema)) + body_schema = self._route.request_body.content_schema[content_type] + body_props = set(body_schema.get("properties", {}).keys()) + + # Build parameter mapping for potentially suffixed parameters + for param in self._route.parameters: + original_name = param.name + suffixed_name = f"{param.name}__{param.location}" + + # If parameter name collides with body property, it would have been suffixed + if param.name in body_props: + param_mapping[suffixed_name] = (original_name, param.location) + # Also map original name for backward compatibility when no collision + param_mapping[original_name] = (original_name, param.location) + # Prepare URL path = self._route.path - # Replace path parameters with values from kwargs - # Path parameters should never be None as they're typically required - # but we'll handle that case anyway - path_params = { - p.name: arguments.get(p.name) - for p in self._route.parameters - if p.location == "path" - and p.name in arguments - and arguments.get(p.name) is not None - } + # Replace path parameters with values from arguments + # Look for both original and suffixed parameter names + path_params = {} + for p in self._route.parameters: + if p.location == "path": + # Try suffixed name first, then original name + suffixed_name = f"{p.name}__{p.location}" + if ( + suffixed_name in arguments + and arguments.get(suffixed_name) is not None + ): + path_params[p.name] = arguments[suffixed_name] + elif p.name in arguments and arguments.get(p.name) is not None: + path_params[p.name] = arguments[p.name] # Ensure all path parameters are provided required_path_params = { @@ -312,35 +338,49 @@ class OpenAPITool(Tool): # Prepare query parameters - filter out None and empty strings query_params = {} for p in self._route.parameters: - if ( - p.location == "query" - and p.name in arguments - and arguments.get(p.name) is not None - and arguments.get(p.name) != "" - ): - param_value = arguments.get(p.name) + if p.location == "query": + # Try suffixed name first, then original name + suffixed_name = f"{p.name}__{p.location}" + param_value = None - # Format array query parameters as comma-separated strings - # following OpenAPI form style (default for query parameters) - if isinstance(param_value, list) and p.schema_.get("type") == "array": - # Get explode parameter from the parameter info, default is True for query parameters - # If explode is True, the array is serialized as separate parameters - # If explode is False, the array is serialized as a comma-separated string - explode = p.explode if p.explode is not None else True + if ( + suffixed_name in arguments + and arguments.get(suffixed_name) is not None + and arguments.get(suffixed_name) != "" + ): + param_value = arguments[suffixed_name] + elif ( + p.name in arguments + and arguments.get(p.name) is not None + and arguments.get(p.name) != "" + ): + param_value = arguments[p.name] - if explode: - # When explode=True, we pass the array directly, which HTTPX will serialize - # as multiple parameters with the same name - query_params[p.name] = param_value + if param_value is not None: + # Format array query parameters as comma-separated strings + # following OpenAPI form style (default for query parameters) + if ( + isinstance(param_value, list) + and p.schema_.get("type") == "array" + ): + # Get explode parameter from the parameter info, default is True for query parameters + # If explode is True, the array is serialized as separate parameters + # If explode is False, the array is serialized as a comma-separated string + explode = p.explode if p.explode is not None else True + + if explode: + # When explode=True, we pass the array directly, which HTTPX will serialize + # as multiple parameters with the same name + query_params[p.name] = param_value + else: + # Format array as comma-separated string when explode=False + formatted_value = format_array_parameter( + param_value, p.name, is_query_parameter=True + ) + query_params[p.name] = formatted_value else: - # Format array as comma-separated string when explode=False - formatted_value = format_array_parameter( - param_value, p.name, is_query_parameter=True - ) - query_params[p.name] = formatted_value - else: - # Non-array parameters are passed as is - query_params[p.name] = param_value + # Non-array parameters are passed as is + query_params[p.name] = param_value # Prepare headers - fix typing by ensuring all values are strings headers = {} @@ -348,12 +388,21 @@ class OpenAPITool(Tool): # Start with OpenAPI-defined header parameters openapi_headers = {} for p in self._route.parameters: - if ( - p.location == "header" - and p.name in arguments - and arguments[p.name] is not None - ): - openapi_headers[p.name.lower()] = str(arguments[p.name]) + if p.location == "header": + # Try suffixed name first, then original name + suffixed_name = f"{p.name}__{p.location}" + param_value = None + + if ( + suffixed_name in arguments + and arguments.get(suffixed_name) is not None + ): + param_value = arguments[suffixed_name] + elif p.name in arguments and arguments.get(p.name) is not None: + param_value = arguments[p.name] + + if param_value is not None: + openapi_headers[p.name.lower()] = str(param_value) headers.update(openapi_headers) # Add headers from the current MCP client HTTP request (these take precedence) @@ -363,16 +412,22 @@ class OpenAPITool(Tool): # Prepare request body json_data = None if self._route.request_body and self._route.request_body.content_schema: - # Extract body parameters, excluding path/query/header params that were already used - path_query_header_params = { - p.name - for p in self._route.parameters - if p.location in ("path", "query", "header") - } + # Extract body parameters with collision-aware logic + # Exclude all parameter names that belong to path/query/header locations + params_to_exclude = set() + + for p in self._route.parameters: + if ( + p.name in body_props + ): # This parameter had a collision, so it was suffixed + params_to_exclude.add(f"{p.name}__{p.location}") + else: # No collision, parameter keeps original name but should still be excluded from body + params_to_exclude.add(p.name) + body_params = { k: v for k, v in arguments.items() - if k not in path_query_header_params and k != "context" + if k not in params_to_exclude and k != "context" } if body_params: diff --git a/src/fastmcp/utilities/openapi.py b/src/fastmcp/utilities/openapi.py index be08d4dc3..97f0e89fb 100644 --- a/src/fastmcp/utilities/openapi.py +++ b/src/fastmcp/utilities/openapi.py @@ -1060,6 +1060,7 @@ def _replace_ref_with_defs( def _combine_schemas(route: HTTPRoute) -> dict[str, Any]: """ Combines parameter and request body schemas into a single schema. + Handles parameter name collisions by adding location suffixes. Args: route: HTTPRoute object @@ -1070,17 +1071,19 @@ def _combine_schemas(route: HTTPRoute) -> dict[str, Any]: properties = {} required = [] - # Add path parameters - for param in route.parameters: - if param.required: - required.append(param.name) - properties[param.name] = _replace_ref_with_defs( - param.schema_.copy(), param.description - ) + # First pass: collect parameter names by location and body properties + param_names_by_location = { + "path": set(), + "query": set(), + "header": set(), + "cookie": set(), + } + body_props = {} + + for param in route.parameters: + param_names_by_location[param.location].add(param.name) - # Add request body if it exists if route.request_body and route.request_body.content_schema: - # For now, just use the first content type's schema content_type = next(iter(route.request_body.content_schema)) body_schema = _replace_ref_with_defs( route.request_body.content_schema[content_type].copy(), @@ -1088,7 +1091,44 @@ def _combine_schemas(route: HTTPRoute) -> dict[str, Any]: ) body_props = body_schema.get("properties", {}) - # Add request body properties + # Detect collisions: parameters that exist in both body and path/query/header + all_non_body_params = set() + for location_params in param_names_by_location.values(): + all_non_body_params.update(location_params) + + body_param_names = set(body_props.keys()) + colliding_params = all_non_body_params & body_param_names + + # Add parameters with suffixes for collisions + for param in route.parameters: + if param.name in colliding_params: + # Add suffix for non-body parameters when collision detected + suffixed_name = f"{param.name}__{param.location}" + if param.required: + required.append(suffixed_name) + + # Add location info to description + param_schema = _replace_ref_with_defs( + param.schema_.copy(), param.description + ) + original_desc = param_schema.get("description", "") + location_desc = f"({param.location.capitalize()} parameter)" + if original_desc: + param_schema["description"] = f"{original_desc} {location_desc}" + else: + param_schema["description"] = location_desc + + properties[suffixed_name] = param_schema + else: + # No collision, use original name + if param.required: + required.append(param.name) + properties[param.name] = _replace_ref_with_defs( + param.schema_.copy(), param.description + ) + + # Add request body properties (no suffixes for body parameters) + if route.request_body and route.request_body.content_schema: for prop_name, prop_schema in body_props.items(): properties[prop_name] = prop_schema diff --git a/tests/server/openapi/test_parameter_collisions.py b/tests/server/openapi/test_parameter_collisions.py new file mode 100644 index 000000000..78dd363cb --- /dev/null +++ b/tests/server/openapi/test_parameter_collisions.py @@ -0,0 +1,258 @@ +"""Tests for handling parameter name collisions between different OpenAPI parameter locations.""" + +from unittest.mock import AsyncMock, MagicMock + +import httpx +import pytest + +from fastmcp.server.openapi import OpenAPITool +from fastmcp.utilities.openapi import HTTPRoute, ParameterInfo, RequestBodyInfo + + +@pytest.fixture +def mock_client(): + """Create a mock httpx.AsyncClient.""" + client = AsyncMock(spec=httpx.AsyncClient) + mock_response = MagicMock() + mock_response.json.return_value = {"result": "success"} + mock_response.raise_for_status.return_value = None + client.request.return_value = mock_response + return client + + +class TestParameterCollisions: + """Test parameter name collisions between path/query/header and body parameters.""" + + async def test_path_body_collision_current_broken_behavior(self, mock_client): + """ + Demonstrates the current broken behavior when a parameter exists in both path and body. + This test should FAIL with the current implementation. + """ + # Create route with collision: id in both path and body + route = HTTPRoute( + path="/users/{id}", + method="PUT", + operation_id="update_user", + parameters=[ + ParameterInfo( + name="id", + location="path", + required=True, + schema={"type": "integer"}, + ) + ], + request_body=RequestBodyInfo( + content_schema={ + "application/json": { + "type": "object", + "properties": { + "id": {"type": "integer", "description": "User ID"}, + "name": {"type": "string", "description": "User name"}, + "email": {"type": "string", "description": "User email"}, + }, + "required": ["id", "name"], + } + } + ), + ) + + # Create tool with current implementation + tool = OpenAPITool( + client=mock_client, + route=route, + name="update_user", + description="Update user", + parameters={}, # Schema would be generated by _combine_schemas + ) + + # This call should work but currently fails because body 'id' is excluded + arguments = {"id": 123, "name": "John Doe", "email": "john@example.com"} + + await tool.run(arguments) + + # Check what was actually sent + call_args = mock_client.request.call_args + assert call_args is not None + + # Current broken behavior: id goes to path but is excluded from body + # This means the body is missing the required 'id' field + assert call_args[1]["url"] == "/users/123" # Path parameter works + + # This assertion will FAIL with current implementation because 'id' is excluded from body + expected_body = {"id": 123, "name": "John Doe", "email": "john@example.com"} + assert call_args[1]["json"] == expected_body, ( + "Body should include 'id' parameter" + ) + + async def test_path_body_collision_with_suffixing(self, mock_client): + """ + Test the desired behavior with parameter suffixing. + This test should PASS after implementing the fix. + """ + # Create route with collision: id in both path and body + route = HTTPRoute( + path="/users/{id}", + method="PUT", + operation_id="update_user", + parameters=[ + ParameterInfo( + name="id", + location="path", + required=True, + schema={"type": "integer"}, + ) + ], + request_body=RequestBodyInfo( + content_schema={ + "application/json": { + "type": "object", + "properties": { + "id": {"type": "integer", "description": "User ID"}, + "name": {"type": "string", "description": "User name"}, + "email": {"type": "string", "description": "User email"}, + }, + "required": ["id", "name"], + } + } + ), + ) + + # Tool should be created with suffixed schema + tool = OpenAPITool( + client=mock_client, + route=route, + name="update_user", + description="Update user", + parameters={}, # Schema would include id__path and id + ) + + # LLM would call with suffixed parameters + arguments = { + "id__path": 123, # Goes to path parameter + "id": 123, # Goes to body parameter + "name": "John Doe", + "email": "john@example.com", + } + + await tool.run(arguments) + + # Verify correct request was made + call_args = mock_client.request.call_args + assert call_args is not None + + # Path parameter should be populated from id__path + assert call_args[1]["url"] == "/users/123" + + # Body should include id (from unsuffixed parameter) + expected_body = {"id": 123, "name": "John Doe", "email": "john@example.com"} + assert call_args[1]["json"] == expected_body + + async def test_query_body_collision_with_suffixing(self, mock_client): + """Test parameter collision between query and body parameters.""" + route = HTTPRoute( + path="/search", + method="POST", + operation_id="search_users", + parameters=[ + ParameterInfo( + name="limit", + location="query", + required=False, + schema={"type": "integer", "default": 10}, + ) + ], + request_body=RequestBodyInfo( + content_schema={ + "application/json": { + "type": "object", + "properties": { + "limit": { + "type": "integer", + "description": "Max results in response", + }, + "query": {"type": "string", "description": "Search query"}, + }, + "required": ["query"], + } + } + ), + ) + + tool = OpenAPITool( + client=mock_client, + route=route, + name="search_users", + description="Search users", + parameters={}, + ) + + # LLM call with suffixed parameters + arguments = { + "limit__query": 5, # Goes to query parameter + "limit": 100, # Goes to body parameter + "query": "john", + } + + await tool.run(arguments) + + call_args = mock_client.request.call_args + assert call_args is not None + + # Query parameter from limit__query + assert call_args[1]["params"] == {"limit": 5} + + # Body includes limit from unsuffixed parameter + expected_body = {"limit": 100, "query": "john"} + assert call_args[1]["json"] == expected_body + + async def test_no_collisions_unchanged_behavior(self, mock_client): + """Test that parameters with no collisions keep original names.""" + route = HTTPRoute( + path="/users/{user_id}", + method="POST", + operation_id="create_user", + parameters=[ + ParameterInfo( + name="user_id", + location="path", + required=True, + schema={"type": "integer"}, + ) + ], + request_body=RequestBodyInfo( + content_schema={ + "application/json": { + "type": "object", + "properties": { + "name": {"type": "string"}, + "email": {"type": "string"}, + }, + "required": ["name"], + } + } + ), + ) + + tool = OpenAPITool( + client=mock_client, + route=route, + name="create_user", + description="Create user", + parameters={}, + ) + + # No collisions, so original parameter names should work + arguments = { + "user_id": 123, # Path parameter (no suffix needed) + "name": "John", # Body parameter + "email": "john@example.com", + } + + await tool.run(arguments) + + call_args = mock_client.request.call_args + assert call_args is not None + + assert call_args[1]["url"] == "/users/123" + expected_body = {"name": "John", "email": "john@example.com"} + assert call_args[1]["json"] == expected_body