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.

Sign up for GitHub

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
wants to merge 5 commits into from
Closed

Path parameters as function positional arguments #429

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fd5629f
Path parameters as function positional arguments
tsotnikov May 14, 2021
b9e8057
Ensure path parameters are ordered by appearance in path
tsotnikov May 18, 2021
432188a
Fix unused fstring
tsotnikov May 18, 2021
a5b6e5f
Improve sort of path parameters
tsotnikov May 25, 2021
dbd3f1b
Pass endpoint instead of endpoint.path_parameters to ParseError
tsotnikov May 25, 2021
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
10 changes: 5 additions & 5 deletions ...golden-record/my_test_api_client/api/parameters/get_same_name_multiple_locations_param.py
View file
Open in desktop
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
71 changes: 71 additions & 0 deletions end_to_end_tests/golden-record/my_test_api_client/api/parameters/multiple_path_parameters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
33 changes: 33 additions & 0 deletions end_to_end_tests/openapi.json
View file
Open in desktop
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,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": {
Expand Down
2 changes: 2 additions & 0 deletions openapi_python_client/parser/openapi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
10 changes: 5 additions & 5 deletions openapi_python_client/templates/endpoint_macros.py.jinja
View file
Open in desktop
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
17 changes: 17 additions & 0 deletions tests/test_parser/test_openapi.py
View file
Open in desktop
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