Merge pull request #1107 from jlowin/fix-openapi-parameter-collisions

Fix OpenAPI parameter name collisions with location suffixing

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:

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
 

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