Skip to content

Path parameters as function positional arguments #429

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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,
)

Expand All @@ -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,
)

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
from typing import Any, Dict

import httpx

from ...client import Client
from ...types import Response


def _get_kwargs(
param_4: str,
param_2: int,
param_1: str,
param_3: int,
*,
client: Client,
) -> Dict[str, Any]:
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()

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_4: str,
param_2: int,
param_1: str,
param_3: int,
*,
client: Client,
) -> Response[None]:
kwargs = _get_kwargs(
param_4=param_4,
param_2=param_2,
param_1=param_1,
param_3=param_3,
client=client,
)

response = httpx.get(
**kwargs,
)

return _build_response(response=response)


async def asyncio_detailed(
param_4: str,
param_2: int,
param_1: str,
param_3: int,
*,
client: Client,
) -> Response[None]:
kwargs = _get_kwargs(
param_4=param_4,
param_2=param_2,
param_1=param_1,
param_3=param_3,
client=client,
)

async with httpx.AsyncClient() as _client:
response = await _client.get(**kwargs)

return _build_response(response=response)
51 changes: 51 additions & 0 deletions end_to_end_tests/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,7 @@
{
"name": "param",
"in": "path",
"required": true,
"schema": {
"type": "string"
}
Expand All @@ -793,6 +794,56 @@
}
}
}
},
"/multiple-path-parameters/{param4}/{param2}/{param1}/{param3}": {
"description": "Test that multiple path parameters are ordered by appearance in path",
"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"
}
}
},
"parameters": [
{
"name": "param4",
"in": "path",
"required": true,
"schema": {
"type": "string"
}
},
{
"name": "param3",
"in": "path",
"required": true,
"schema": {
"type": "integer"
}
}
]
}
},
"components": {
Expand Down
23 changes: 23 additions & 0 deletions openapi_python_client/parser/openapi.py
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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"""
Expand Down Expand Up @@ -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."
Expand Down Expand Up @@ -245,6 +250,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)
Expand All @@ -266,6 +273,22 @@ def _add_parameters(

return endpoint, schemas

@staticmethod
def _sort_parameters(*, endpoint: "Endpoint", path: str) -> Union["Endpoint", ParseError]:
endpoint = deepcopy(endpoint)
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 parameters_from_path != path_parameter_names:
return ParseError(
data=endpoint,
detail="Incorrect path templating (Path parameters do not match with path)",
)
return endpoint

@staticmethod
def from_data(
*, data: oai.Operation, path: str, method: str, tag: str, schemas: Schemas, config: Config
Expand Down
10 changes: 5 additions & 5 deletions openapi_python_client/templates/endpoint_macros.py.jinja
Original file line number Diff line number Diff line change
Expand Up @@ -73,17 +73,17 @@ 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 %}
Comment on lines +76 to +79
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only problem with this is that the order of endpoint.path_parameters is not guaranteed. If the declaration order swaps around or there's a slight change in implementation (e.g. at some point we decide to alphabetize kwargs) then generated clients will break and potentially do so in a way that consumers don't catch.

If we want to allow positional args, we'll have to guarantee the order for them. For Path params we could use the order in which they appear in the path, which seems the most logical solution to me. It's going to require a bit more up front parsing work though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! I will work on sort them in the order they appear in the path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in b9e8057

*,
{# Proper client based on whether or not the endpoint requires authentication #}
{% if endpoint.requires_security %}
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 }},
Expand Down Expand Up @@ -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 %}
Expand Down
51 changes: 51 additions & 0 deletions tests/test_parser/test_openapi.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -589,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="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 == endpoint
assert "Incorrect path templating" in result.detail

def test_from_data_bad_params(self, mocker):
from openapi_python_client.parser.openapi import Endpoint

Expand Down