diff --git a/src/google/adk/tools/openapi_tool/openapi_spec_parser/openapi_spec_parser.py b/src/google/adk/tools/openapi_tool/openapi_spec_parser/openapi_spec_parser.py index 2e0e543fa37..b4d321677ea 100644 --- a/src/google/adk/tools/openapi_tool/openapi_spec_parser/openapi_spec_parser.py +++ b/src/google/adk/tools/openapi_tool/openapi_spec_parser/openapi_spec_parser.py @@ -20,6 +20,7 @@ from typing import List from typing import Optional from typing import Set +from typing import Tuple from fastapi.openapi.models import Operation from pydantic import BaseModel @@ -171,6 +172,47 @@ def sanitize_recursive(obj: Any, *, in_schema: bool) -> Any: return sanitize_recursive(openapi_spec, in_schema=False) + def _merge_parameters( + self, + operation_parameters: List[Dict[str, Any]], + path_parameters: List[Dict[str, Any]], + ) -> List[Dict[str, Any]]: + """Merges an operation's parameters over the path-level ones. + + Per the OpenAPI 3 Path Item Object, an operation-level parameter + overrides a path-level parameter that has the same `name` and `in`. + Declaring a shared parameter once at the path level and refining it on + an operation (a more specific description, pattern or enum) is common, + so both must not be collected: the duplicate would otherwise be renamed + to `_0` and asked of the model as a second required argument. + + Args: + operation_parameters: The parameters declared on the operation. + path_parameters: The parameters declared on the path item. + + Returns: + The operation's parameters followed by the path-level parameters that + the operation does not override. + """ + merged: List[Dict[str, Any]] = list(operation_parameters) + + overridden: Set[Tuple[Any, Any]] = set() + for parameter in merged: + key = (parameter.get("name"), parameter.get("in")) + # Skip malformed parameters so they are never deduplicated away. + if key[0] is not None and key[1] is not None: + overridden.add(key) + + for parameter in path_parameters: + key = (parameter.get("name"), parameter.get("in")) + if key in overridden: + continue + if key[0] is not None and key[1] is not None: + overridden.add(key) + merged.append(parameter) + + return merged + def _collect_operations( self, openapi_spec: Dict[str, Any] ) -> List[ParsedOperation]: @@ -205,10 +247,12 @@ def _collect_operations( if operation_dict is None: continue - # Append path-level parameters - operation_dict["parameters"] = operation_dict.get( - "parameters", [] - ) + path_item.get("parameters", []) + # Merge path-level parameters in, letting the operation's own + # parameters override the ones it re-declares. + operation_dict["parameters"] = self._merge_parameters( + operation_dict.get("parameters", []), + path_item.get("parameters", []), + ) # If operation ID is missing, assign an operation id based on path # and method diff --git a/tests/unittests/tools/openapi_tool/openapi_spec_parser/test_openapi_spec_parser.py b/tests/unittests/tools/openapi_tool/openapi_spec_parser/test_openapi_spec_parser.py index e5bff337cec..10e4fc2abca 100644 --- a/tests/unittests/tools/openapi_tool/openapi_spec_parser/test_openapi_spec_parser.py +++ b/tests/unittests/tools/openapi_tool/openapi_spec_parser/test_openapi_spec_parser.py @@ -866,3 +866,91 @@ def test_sanitize_schema_types_removes_all_invalid_list(openapi_spec_generator): # Type field should be removed entirely assert "type" not in sanitized["schema"] + + +def create_spec_with_path_level_parameters() -> Dict[str, Any]: + """Creates a spec whose path item and operation both declare accountId.""" + return { + "openapi": "3.0.0", + "info": {"title": "CRM API", "version": "1.0.0"}, + "servers": [{"url": "https://crm.example.com"}], + "paths": { + "/accounts/{accountId}": { + "parameters": [ + { + "name": "accountId", + "in": "path", + "required": True, + "description": "Account id (path level).", + "schema": {"type": "string"}, + }, + { + "name": "trace", + "in": "query", + "description": "Trace flag (path level).", + "schema": {"type": "boolean"}, + }, + ], + "get": { + "operationId": "getAccount", + "parameters": [ + { + "name": "accountId", + "in": "path", + "required": True, + "description": "Account id (operation level).", + "schema": {"type": "string", "pattern": "^ACC-"}, + }, + ], + "responses": {"200": {"description": "Successful response"}}, + }, + } + }, + } + + +def test_operation_parameter_overrides_path_level_parameter( + openapi_spec_generator, +): + """Test that an operation parameter overrides the path-level one.""" + spec = create_spec_with_path_level_parameters() + + op = openapi_spec_generator.parse(spec)[0] + + # The path-level declaration must not survive as a second argument. + assert [param.py_name for param in op.parameters] == ["account_id", "trace"] + account_id = op.parameters[0] + assert account_id.original_name == "accountId" + assert account_id.description == "Account id (operation level)." + + +def test_path_level_parameters_are_still_collected(openapi_spec_generator): + """Test that path-level parameters the operation does not redeclare remain.""" + spec = create_spec_with_path_level_parameters() + + op = openapi_spec_generator.parse(spec)[0] + + trace = op.parameters[1] + assert trace.original_name == "trace" + assert trace.param_location == "query" + assert trace.description == "Trace flag (path level)." + + +def test_parameters_with_same_name_different_location_are_both_kept( + openapi_spec_generator, +): + """Test that `in` participates in the override key.""" + spec = create_spec_with_path_level_parameters() + spec["paths"]["/accounts/{accountId}"]["get"]["parameters"].append({ + "name": "accountId", + "in": "query", + "description": "Account id as a query parameter.", + "schema": {"type": "string"}, + }) + + op = openapi_spec_generator.parse(spec)[0] + + # The path-level `accountId` is overridden, the query one is not. + names = [param.py_name for param in op.parameters] + assert names == ["account_id", "account_id_0", "trace"] + assert op.parameters[1].param_location == "query"