From fd5629f5ac7b8af3499bde8d39a647a54d3ff99f Mon Sep 17 00:00:00 2001 From: Taras Sotnikov Date: Fri, 14 May 2021 17:03:16 +0200 Subject: [PATCH 1/5] Path parameters as function positional arguments OpenAPI Specification indicates that if parameter location is `path` the `required` property is REQUIRED and its value MUST be `true` Ref: https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#parameter-object With this in mind, this PR: - Tighten the parsing by throwing an error for path parameters with required=false - Update the templates to pass the path parameters to the function as positional arguments instead of kwargs to improve usability --- .../get_same_name_multiple_locations_param.py | 10 +-- .../parameters/multiple_path_parameters.py | 71 +++++++++++++++++++ end_to_end_tests/openapi.json | 33 +++++++++ openapi_python_client/parser/openapi.py | 2 + .../templates/endpoint_macros.py.jinja | 10 +-- tests/test_parser/test_openapi.py | 17 +++++ 6 files changed, 133 insertions(+), 10 deletions(-) create mode 100644 end_to_end_tests/golden-record/my_test_api_client/api/parameters/multiple_path_parameters.py diff --git a/end_to_end_tests/golden-record/my_test_api_client/api/parameters/get_same_name_multiple_locations_param.py b/end_to_end_tests/golden-record/my_test_api_client/api/parameters/get_same_name_multiple_locations_param.py index eeb4daff7..cec075ba0 100644 --- a/end_to_end_tests/golden-record/my_test_api_client/api/parameters/get_same_name_multiple_locations_param.py +++ b/end_to_end_tests/golden-record/my_test_api_client/api/parameters/get_same_name_multiple_locations_param.py @@ -7,9 +7,9 @@ def _get_kwargs( + param_path: str, *, client: Client, - param_path: Union[Unset, str] = UNSET, param_query: Union[Unset, str] = UNSET, ) -> Dict[str, Any]: url = "{}/same-name-multiple-locations/{param}".format(client.base_url, param=param_path) @@ -41,14 +41,14 @@ def _build_response(*, response: httpx.Response) -> Response[None]: def sync_detailed( + param_path: str, *, client: Client, - param_path: Union[Unset, str] = UNSET, param_query: Union[Unset, str] = UNSET, ) -> Response[None]: kwargs = _get_kwargs( - client=client, param_path=param_path, + client=client, param_query=param_query, ) @@ -60,14 +60,14 @@ def sync_detailed( async def asyncio_detailed( + param_path: str, *, client: Client, - param_path: Union[Unset, str] = UNSET, param_query: Union[Unset, str] = UNSET, ) -> Response[None]: kwargs = _get_kwargs( - client=client, param_path=param_path, + client=client, param_query=param_query, ) diff --git a/end_to_end_tests/golden-record/my_test_api_client/api/parameters/multiple_path_parameters.py b/end_to_end_tests/golden-record/my_test_api_client/api/parameters/multiple_path_parameters.py new file mode 100644 index 000000000..207025f6a --- /dev/null +++ b/end_to_end_tests/golden-record/my_test_api_client/api/parameters/multiple_path_parameters.py @@ -0,0 +1,71 @@ +from typing import Any, Dict + +import httpx + +from ...client import Client +from ...types import Response + + +def _get_kwargs( + param_1: str, + param_2: int, + *, + client: Client, +) -> Dict[str, Any]: + url = "{}/multiple-path-parameters/{param1}/{param2}".format(client.base_url, param1=param_1, param2=param_2) + + headers: Dict[str, Any] = client.get_headers() + cookies: Dict[str, Any] = client.get_cookies() + + return { + "url": url, + "headers": headers, + "cookies": cookies, + "timeout": client.get_timeout(), + } + + +def _build_response(*, response: httpx.Response) -> Response[None]: + return Response( + status_code=response.status_code, + content=response.content, + headers=response.headers, + parsed=None, + ) + + +def sync_detailed( + param_1: str, + param_2: int, + *, + client: Client, +) -> Response[None]: + kwargs = _get_kwargs( + param_1=param_1, + param_2=param_2, + client=client, + ) + + response = httpx.get( + **kwargs, + ) + + return _build_response(response=response) + + +async def asyncio_detailed( + param_1: str, + param_2: int, + *, + client: Client, +) -> Response[None]: + kwargs = _get_kwargs( + param_1=param_1, + param_2=param_2, + client=client, + ) + + async with httpx.AsyncClient() as _client: + response = await _client.get(**kwargs) + + return _build_response(response=response) diff --git a/end_to_end_tests/openapi.json b/end_to_end_tests/openapi.json index 64ccfdbc9..0295b84d8 100644 --- a/end_to_end_tests/openapi.json +++ b/end_to_end_tests/openapi.json @@ -782,6 +782,7 @@ { "name": "param", "in": "path", + "required": true, "schema": { "type": "string" } @@ -793,6 +794,38 @@ } } } + }, + "/multiple-path-parameters/{param1}/{param2}": { + "description": "Test with multiple path parameters", + "get": { + "tags": [ + "parameters" + ], + "operationId": "multiple_path_parameters", + "parameters": [ + { + "name": "param1", + "in": "path", + "required": true, + "schema": { + "type": "string" + } + }, + { + "name": "param2", + "in": "path", + "required": true, + "schema": { + "type": "integer" + } + } + ], + "responses": { + "200": { + "description": "Success" + } + } + } } }, "components": { diff --git a/openapi_python_client/parser/openapi.py b/openapi_python_client/parser/openapi.py index f0b9774ce..26f33ea04 100644 --- a/openapi_python_client/parser/openapi.py +++ b/openapi_python_client/parser/openapi.py @@ -245,6 +245,8 @@ def _add_parameters( if param.param_in == oai.ParameterLocation.QUERY: endpoint.query_parameters.append(prop) elif param.param_in == oai.ParameterLocation.PATH: + if not param.required: + return ParseError(data=param, detail="Path parameter must be required"), schemas endpoint.path_parameters.append(prop) elif param.param_in == oai.ParameterLocation.HEADER: endpoint.header_parameters.append(prop) diff --git a/openapi_python_client/templates/endpoint_macros.py.jinja b/openapi_python_client/templates/endpoint_macros.py.jinja index 66d6209b3..23149b9e3 100644 --- a/openapi_python_client/templates/endpoint_macros.py.jinja +++ b/openapi_python_client/templates/endpoint_macros.py.jinja @@ -73,6 +73,10 @@ params = {k: v for k, v in params.items() if v is not UNSET and v is not None} {# The all the kwargs passed into an endpoint (and variants thereof)) #} {% macro arguments(endpoint) %} +{# path parameters #} +{% for parameter in endpoint.path_parameters %} +{{ parameter.to_string() }}, +{% endfor %} *, {# Proper client based on whether or not the endpoint requires authentication #} {% if endpoint.requires_security %} @@ -80,10 +84,6 @@ client: AuthenticatedClient, {% else %} client: Client, {% endif %} -{# path parameters #} -{% for parameter in endpoint.path_parameters %} -{{ parameter.to_string() }}, -{% endfor %} {# Form data if any #} {% if endpoint.form_body_class %} form_data: {{ endpoint.form_body_class.name }}, @@ -111,10 +111,10 @@ json_body: {{ endpoint.json_body.get_type_string() }}, {# Just lists all kwargs to endpoints as name=name for passing to other functions #} {% macro kwargs(endpoint) %} -client=client, {% for parameter in endpoint.path_parameters %} {{ parameter.python_name }}={{ parameter.python_name }}, {% endfor %} +client=client, {% if endpoint.form_body_class %} form_data=form_data, {% endif %} diff --git a/tests/test_parser/test_openapi.py b/tests/test_parser/test_openapi.py index a220d243c..4d3f78853 100644 --- a/tests/test_parser/test_openapi.py +++ b/tests/test_parser/test_openapi.py @@ -447,6 +447,23 @@ def test__add_parameters_parse_error(self, mocker): property_schemas, ) + def test__add_parameters_parse_error_on_non_required_path_param(self, mocker): + from openapi_python_client.parser.openapi import Endpoint, Schemas + + endpoint = self.make_endpoint() + parsed_schemas = mocker.MagicMock() + mocker.patch(f"{MODULE_NAME}.property_from_data", return_value=(mocker.MagicMock(), parsed_schemas)) + param = oai.Parameter.construct( + name="test", required=False, param_schema=mocker.MagicMock(), param_in=oai.ParameterLocation.PATH + ) + schemas = Schemas() + config = MagicMock() + + result = Endpoint._add_parameters( + endpoint=endpoint, data=oai.Operation.construct(parameters=[param]), schemas=schemas, config=config + ) + assert result == (ParseError(data=param, detail="Path parameter must be required"), parsed_schemas) + def test__add_parameters_fail_loudly_when_location_not_supported(self, mocker): from openapi_python_client.parser.openapi import Endpoint, Schemas From b9e8057c3dc96057ee609f7a61311a0f832b7b65 Mon Sep 17 00:00:00 2001 From: Taras Sotnikov Date: Tue, 18 May 2021 19:17:35 +0200 Subject: [PATCH 2/5] Ensure path parameters are ordered by appearance in path --- .../parameters/multiple_path_parameters.py | 24 +++++++++---- end_to_end_tests/openapi.json | 24 +++++++++++-- openapi_python_client/parser/openapi.py | 19 +++++++++++ tests/test_parser/test_openapi.py | 34 +++++++++++++++++++ 4 files changed, 92 insertions(+), 9 deletions(-) diff --git a/end_to_end_tests/golden-record/my_test_api_client/api/parameters/multiple_path_parameters.py b/end_to_end_tests/golden-record/my_test_api_client/api/parameters/multiple_path_parameters.py index 207025f6a..b5df06c1f 100644 --- a/end_to_end_tests/golden-record/my_test_api_client/api/parameters/multiple_path_parameters.py +++ b/end_to_end_tests/golden-record/my_test_api_client/api/parameters/multiple_path_parameters.py @@ -7,12 +7,16 @@ def _get_kwargs( - param_1: str, + param_4: str, param_2: int, + param_1: str, + param_3: int, *, client: Client, ) -> Dict[str, Any]: - url = "{}/multiple-path-parameters/{param1}/{param2}".format(client.base_url, param1=param_1, param2=param_2) + url = "{}/multiple-path-parameters/{param4}/{param2}/{param1}/{param3}".format( + client.base_url, param4=param_4, param2=param_2, param1=param_1, param3=param_3 + ) headers: Dict[str, Any] = client.get_headers() cookies: Dict[str, Any] = client.get_cookies() @@ -35,14 +39,18 @@ def _build_response(*, response: httpx.Response) -> Response[None]: def sync_detailed( - param_1: str, + param_4: str, param_2: int, + param_1: str, + param_3: int, *, client: Client, ) -> Response[None]: kwargs = _get_kwargs( - param_1=param_1, + param_4=param_4, param_2=param_2, + param_1=param_1, + param_3=param_3, client=client, ) @@ -54,14 +62,18 @@ def sync_detailed( async def asyncio_detailed( - param_1: str, + param_4: str, param_2: int, + param_1: str, + param_3: int, *, client: Client, ) -> Response[None]: kwargs = _get_kwargs( - param_1=param_1, + param_4=param_4, param_2=param_2, + param_1=param_1, + param_3=param_3, client=client, ) diff --git a/end_to_end_tests/openapi.json b/end_to_end_tests/openapi.json index 0295b84d8..fffe74ba3 100644 --- a/end_to_end_tests/openapi.json +++ b/end_to_end_tests/openapi.json @@ -795,8 +795,8 @@ } } }, - "/multiple-path-parameters/{param1}/{param2}": { - "description": "Test with multiple path parameters", + "/multiple-path-parameters/{param4}/{param2}/{param1}/{param3}": { + "description": "Test that multiple path parameters are ordered by appearance in path", "get": { "tags": [ "parameters" @@ -825,7 +825,25 @@ "description": "Success" } } - } + }, + "parameters": [ + { + "name": "param4", + "in": "path", + "required": true, + "schema": { + "type": "string" + } + }, + { + "name": "param3", + "in": "path", + "required": true, + "schema": { + "type": "integer" + } + } + ] } }, "components": { diff --git a/openapi_python_client/parser/openapi.py b/openapi_python_client/parser/openapi.py index 26f33ea04..4c8eff115 100644 --- a/openapi_python_client/parser/openapi.py +++ b/openapi_python_client/parser/openapi.py @@ -1,4 +1,5 @@ import itertools +import re from copy import deepcopy from dataclasses import dataclass, field from typing import Any, Dict, Iterator, List, Optional, Set, Tuple, Union @@ -12,6 +13,8 @@ from .properties import Class, EnumProperty, ModelProperty, Property, Schemas, build_schemas, property_from_data from .responses import Response, response_from_data +_PATH_PARAM_REGEX = re.compile("{([a-zA-Z_][a-zA-Z0-9_]*)}") + def import_string_from_class(class_: Class, prefix: str = "") -> str: """Create a string which is used to import a reference""" @@ -49,6 +52,8 @@ def from_data( endpoint, schemas = Endpoint._add_parameters( endpoint=endpoint, data=path_data, schemas=schemas, config=config ) + if not isinstance(endpoint, ParseError): + endpoint = Endpoint._sort_parameters(endpoint=endpoint, path=path) if isinstance(endpoint, ParseError): endpoint.header = ( f"ERROR parsing {method.upper()} {path} within {tag}. Endpoint will not be generated." @@ -268,6 +273,20 @@ def _add_parameters( return endpoint, schemas + @staticmethod + def _sort_parameters(*, endpoint: "Endpoint", path: str) -> Union["Endpoint", ParseError]: + endpoint = deepcopy(endpoint) + parameters_form_path = re.findall(_PATH_PARAM_REGEX, path) + path_parameter_names = [p.name for p in endpoint.path_parameters] + if sorted(parameters_form_path) != sorted(path_parameter_names): + return ParseError( + data=endpoint.path_parameters, + detail="Incorrect path templating (Path parameters do not match with path)", + ) + + endpoint.path_parameters.sort(key=lambda p: parameters_form_path.index(p.name)) + return endpoint + @staticmethod def from_data( *, data: oai.Operation, path: str, method: str, tag: str, schemas: Schemas, config: Config diff --git a/tests/test_parser/test_openapi.py b/tests/test_parser/test_openapi.py index 4d3f78853..7fcc786b5 100644 --- a/tests/test_parser/test_openapi.py +++ b/tests/test_parser/test_openapi.py @@ -606,6 +606,40 @@ def test__add_parameters_duplicate_properties_different_location(self): assert result.query_parameters[0].python_name == "test_query" assert result.query_parameters[0].name == "test" + def test__sort_parameters(self, mocker): + from openapi_python_client.parser.openapi import Endpoint + + endpoint = self.make_endpoint() + path = "/multiple-path-parameters/{param4}/{param2}/{param1}/{param3}" + + for i in range(1, 5): + param = oai.Parameter.construct( + name=f"param{i}", required=True, param_schema=mocker.MagicMock(), param_in=oai.ParameterLocation.PATH + ) + endpoint.path_parameters.append(param) + + result = Endpoint._sort_parameters(endpoint=endpoint, path=path) + result_names = [p.name for p in result.path_parameters] + expected_names = [f"param{i}" for i in (4, 2, 1, 3)] + + assert result_names == expected_names + + def test__sort_parameters_invalid_path_templating(self, mocker): + from openapi_python_client.parser.openapi import Endpoint + + endpoint = self.make_endpoint() + path = "/multiple-path-parameters/{param1}/{param2}" + param = oai.Parameter.construct( + name=f"param1", required=True, param_schema=mocker.MagicMock(), param_in=oai.ParameterLocation.PATH + ) + endpoint.path_parameters.append(param) + + result = Endpoint._sort_parameters(endpoint=endpoint, path=path) + + assert isinstance(result, ParseError) + assert result.data == [param] + assert "Incorrect path templating" in result.detail + def test_from_data_bad_params(self, mocker): from openapi_python_client.parser.openapi import Endpoint From 432188ac8b6da19c4d65d7ff93188cef358cabfe Mon Sep 17 00:00:00 2001 From: Taras Sotnikov Date: Tue, 18 May 2021 20:45:55 +0200 Subject: [PATCH 3/5] Fix unused fstring --- tests/test_parser/test_openapi.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_parser/test_openapi.py b/tests/test_parser/test_openapi.py index 7fcc786b5..3f850d5c8 100644 --- a/tests/test_parser/test_openapi.py +++ b/tests/test_parser/test_openapi.py @@ -630,7 +630,7 @@ def test__sort_parameters_invalid_path_templating(self, mocker): endpoint = self.make_endpoint() path = "/multiple-path-parameters/{param1}/{param2}" param = oai.Parameter.construct( - name=f"param1", required=True, param_schema=mocker.MagicMock(), param_in=oai.ParameterLocation.PATH + name="param1", required=True, param_schema=mocker.MagicMock(), param_in=oai.ParameterLocation.PATH ) endpoint.path_parameters.append(param) From a5b6e5fe3b303738a75e0c6c8ff964ec33a3ae0b Mon Sep 17 00:00:00 2001 From: Taras Sotnikov Date: Tue, 25 May 2021 10:13:54 +0200 Subject: [PATCH 4/5] Improve sort of path parameters Also fix typo Co-authored-by: Dylan Anthony <43723790+dbanty@users.noreply.github.com> --- openapi_python_client/parser/openapi.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/openapi_python_client/parser/openapi.py b/openapi_python_client/parser/openapi.py index 4c8eff115..50e31ffa6 100644 --- a/openapi_python_client/parser/openapi.py +++ b/openapi_python_client/parser/openapi.py @@ -276,15 +276,17 @@ def _add_parameters( @staticmethod def _sort_parameters(*, endpoint: "Endpoint", path: str) -> Union["Endpoint", ParseError]: endpoint = deepcopy(endpoint) - parameters_form_path = re.findall(_PATH_PARAM_REGEX, path) + parameters_from_path = re.findall(_PATH_PARAM_REGEX, path) + try: + endpoint.path_parameters.sort(key=lambda p: parameters_from_path.index(p.name)) + except ValueError: + pass # We're going to catch the difference down below path_parameter_names = [p.name for p in endpoint.path_parameters] - if sorted(parameters_form_path) != sorted(path_parameter_names): + if parameters_from_path != path_parameter_names: return ParseError( data=endpoint.path_parameters, detail="Incorrect path templating (Path parameters do not match with path)", ) - - endpoint.path_parameters.sort(key=lambda p: parameters_form_path.index(p.name)) return endpoint @staticmethod From dbd3f1b31e8f414ec3afef20ec0c6da3a5d4edfe Mon Sep 17 00:00:00 2001 From: Taras Sotnikov Date: Tue, 25 May 2021 10:27:37 +0200 Subject: [PATCH 5/5] Pass endpoint instead of endpoint.path_parameters to ParseError --- openapi_python_client/parser/openapi.py | 2 +- tests/test_parser/test_openapi.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/openapi_python_client/parser/openapi.py b/openapi_python_client/parser/openapi.py index 50e31ffa6..48fb7f2dc 100644 --- a/openapi_python_client/parser/openapi.py +++ b/openapi_python_client/parser/openapi.py @@ -284,7 +284,7 @@ def _sort_parameters(*, endpoint: "Endpoint", path: str) -> Union["Endpoint", Pa path_parameter_names = [p.name for p in endpoint.path_parameters] if parameters_from_path != path_parameter_names: return ParseError( - data=endpoint.path_parameters, + data=endpoint, detail="Incorrect path templating (Path parameters do not match with path)", ) return endpoint diff --git a/tests/test_parser/test_openapi.py b/tests/test_parser/test_openapi.py index 3f850d5c8..59fa520ad 100644 --- a/tests/test_parser/test_openapi.py +++ b/tests/test_parser/test_openapi.py @@ -637,7 +637,7 @@ def test__sort_parameters_invalid_path_templating(self, mocker): result = Endpoint._sort_parameters(endpoint=endpoint, path=path) assert isinstance(result, ParseError) - assert result.data == [param] + assert result.data == endpoint assert "Incorrect path templating" in result.detail def test_from_data_bad_params(self, mocker):