Skip to content

fix: support $ref from endpoint response to components/responses BNCH-109662 #213

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

Merged
merged 7 commits into from
Oct 28, 2024
Merged

fix: support $ref from endpoint response to components/responses BNCH-109662 #213

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
41e954d
fix: support $ref from endpoint response to components/responses
eli-bl Oct 24, 2024
fb0182e
lint
eli-bl Oct 25, 2024
fe6b7dd
lint
eli-bl Oct 25, 2024
8b0aca1
test coverage
eli-bl Oct 25, 2024
2edc6fe
test coverage
eli-bl Oct 25, 2024
9df3bb4
fix test to test the right thing
eli-bl Oct 25, 2024
78bd860
add a helper
eli-bl Oct 28, 2024
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
26 changes: 26 additions & 0 deletions end_to_end_tests/baseline_openapi_3.0.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -991,6 +991,20 @@
}
}
},
Copy link
Author

Choose a reason for hiding this comment

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

All of the files under end_to_end_tests can be ignored - they're just example data that's used by a test that runs the generator against a big spec file and checks for the expected output files.

"/responses/reference": {
"get": {
"tags": [
"responses"
],
"summary": "Endpoint using predefined response",
"operationId": "reference_response",
"responses": {
"200": {
"$ref": "#/components/responses/AResponse"
}
}
}
},
"/auth/token_with_cookie": {
"get": {
"tags": [
Expand Down Expand Up @@ -2930,6 +2944,18 @@
}
}
}
},
"responses": {
"AResponse": {
"description": "OK",
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/AModel"
}
}
}
}
}
}
}
21 changes: 21 additions & 0 deletions end_to_end_tests/baseline_openapi_3.1.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -983,6 +983,20 @@ info:
}
}
},
"/responses/reference": {
"get": {
"tags": [
"responses"
],
"summary": "Endpoint using predefined response",
"operationId": "reference_response",
"responses": {
"200": {
"$ref": "#/components/responses/AResponse"
}
}
}
},
"/auth/token_with_cookie": {
"get": {
"tags": [
Expand Down Expand Up @@ -2921,3 +2935,10 @@ info:
"application/json":
"schema":
"$ref": "#/components/schemas/AModel"
responses:
AResponse:
description: OK
content:
"application/json":
"schema":
"$ref": "#/components/schemas/AModel"
9 changes: 8 additions & 1 deletion end_to_end_tests/custom-templates-golden-record/my_test_api_client/api/responses/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import types

from . import post_responses_unions_simple_before_complex, text_response
from . import post_responses_unions_simple_before_complex, reference_response, text_response


class ResponsesEndpoints:
Expand All @@ -19,3 +19,10 @@ def text_response(cls) -> types.ModuleType:
Text Response
"""
return text_response

@classmethod
def reference_response(cls) -> types.ModuleType:
"""
Endpoint using predefined response
"""
return reference_response
122 changes: 122 additions & 0 deletions end_to_end_tests/golden-record/my_test_api_client/api/responses/reference_response.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
from http import HTTPStatus
from typing import Any, Dict, Optional, Union

import httpx

from ... import errors
from ...client import AuthenticatedClient, Client
from ...models.a_model import AModel
from ...types import Response


def _get_kwargs() -> Dict[str, Any]:
_kwargs: Dict[str, Any] = {
"method": "get",
"url": "/responses/reference",
}

return _kwargs


def _parse_response(*, client: Union[AuthenticatedClient, Client], response: httpx.Response) -> Optional[AModel]:
if response.status_code == 200:
response_200 = AModel.from_dict(response.json())

return response_200
if client.raise_on_unexpected_status:
raise errors.UnexpectedStatus(response.status_code, response.content)
else:
return None


def _build_response(*, client: Union[AuthenticatedClient, Client], response: httpx.Response) -> Response[AModel]:
return Response(
status_code=HTTPStatus(response.status_code),
content=response.content,
headers=response.headers,
parsed=_parse_response(client=client, response=response),
)


def sync_detailed(
*,
client: Union[AuthenticatedClient, Client],
) -> Response[AModel]:
"""Endpoint using predefined response

Raises:
errors.UnexpectedStatus: If the server returns an undocumented status code and Client.raise_on_unexpected_status is True.
httpx.TimeoutException: If the request takes longer than Client.timeout.

Returns:
Response[AModel]
"""

kwargs = _get_kwargs()

response = client.get_httpx_client().request(
**kwargs,
)

return _build_response(client=client, response=response)


def sync(
*,
client: Union[AuthenticatedClient, Client],
) -> Optional[AModel]:
"""Endpoint using predefined response

Raises:
errors.UnexpectedStatus: If the server returns an undocumented status code and Client.raise_on_unexpected_status is True.
httpx.TimeoutException: If the request takes longer than Client.timeout.

Returns:
AModel
"""

return sync_detailed(
client=client,
).parsed


async def asyncio_detailed(
*,
client: Union[AuthenticatedClient, Client],
) -> Response[AModel]:
"""Endpoint using predefined response

Raises:
errors.UnexpectedStatus: If the server returns an undocumented status code and Client.raise_on_unexpected_status is True.
httpx.TimeoutException: If the request takes longer than Client.timeout.

Returns:
Response[AModel]
"""

kwargs = _get_kwargs()

response = await client.get_async_httpx_client().request(**kwargs)

return _build_response(client=client, response=response)


async def asyncio(
*,
client: Union[AuthenticatedClient, Client],
) -> Optional[AModel]:
"""Endpoint using predefined response

Raises:
errors.UnexpectedStatus: If the server returns an undocumented status code and Client.raise_on_unexpected_status is True.
httpx.TimeoutException: If the request takes longer than Client.timeout.

Returns:
AModel
"""

return (
await asyncio_detailed(
client=client,
)
).parsed
27 changes: 24 additions & 3 deletions openapi_python_client/parser/openapi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ def from_data(
schemas: Schemas,
parameters: Parameters,
request_bodies: Dict[str, Union[oai.RequestBody, oai.Reference]],
responses: Dict[str, Union[oai.Response, oai.Reference]],
config: Config,
) -> Tuple[Dict[utils.PythonIdentifier, "EndpointCollection"], Schemas, Parameters]:
"""Parse the openapi paths data to get EndpointCollections by tag"""
Expand All @@ -72,6 +73,7 @@ def from_data(
schemas=schemas,
parameters=parameters,
request_bodies=request_bodies,
responses=responses,
config=config,
)
# Add `PathItem` parameters
Expand Down Expand Up @@ -144,7 +146,12 @@ class Endpoint:

@staticmethod
def _add_responses(
*, endpoint: "Endpoint", data: oai.Responses, schemas: Schemas, config: Config
*,
endpoint: "Endpoint",
data: oai.Responses,
schemas: Schemas,
responses: Dict[str, Union[oai.Response, oai.Reference]],
config: Config,
) -> Tuple["Endpoint", Schemas]:
endpoint = deepcopy(endpoint)
for code, response_data in data.items():
Expand All @@ -167,6 +174,7 @@ def _add_responses(
status_code=status_code,
data=response_data,
schemas=schemas,
responses=responses,
parent_name=endpoint.name,
config=config,
)
Expand Down Expand Up @@ -396,6 +404,7 @@ def from_data(
schemas: Schemas,
parameters: Parameters,
request_bodies: Dict[str, Union[oai.RequestBody, oai.Reference]],
responses: Dict[str, Union[oai.Response, oai.Reference]],
config: Config,
) -> Tuple[Union["Endpoint", ParseError], Schemas, Parameters]:
"""Construct an endpoint from the OpenAPI data"""
Expand Down Expand Up @@ -424,7 +433,13 @@ def from_data(
)
if isinstance(result, ParseError):
return result, schemas, parameters
result, schemas = Endpoint._add_responses(endpoint=result, data=data.responses, schemas=schemas, config=config)
result, schemas = Endpoint._add_responses(
endpoint=result,
data=data.responses,
schemas=schemas,
responses=responses,
config=config,
)
if isinstance(result, ParseError):
return result, schemas, parameters
bodies, schemas = body_from_data(
Expand Down Expand Up @@ -514,8 +529,14 @@ def from_dict(data: Dict[str, Any], *, config: Config) -> Union["GeneratorData",
config=config,
)
request_bodies = (openapi.components and openapi.components.requestBodies) or {}
responses = (openapi.components and openapi.components.responses) or {}
endpoint_collections_by_tag, schemas, parameters = EndpointCollection.from_data(
data=openapi.paths, schemas=schemas, parameters=parameters, request_bodies=request_bodies, config=config
data=openapi.paths,
schemas=schemas,
parameters=parameters,
request_bodies=request_bodies,
responses=responses,
config=config,
)

enums = (
Expand Down
26 changes: 15 additions & 11 deletions openapi_python_client/parser/responses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
__all__ = ["Response", "response_from_data"]

from http import HTTPStatus
from typing import Optional, Tuple, TypedDict, Union
from typing import Dict, Optional, Tuple, TypedDict, Union

from attrs import define

from openapi_python_client import utils
from openapi_python_client.parser.properties.schemas import parse_reference_path

from .. import Config
from .. import schema as oai
Expand Down Expand Up @@ -79,27 +80,30 @@ def empty_response(
)


def response_from_data(
def response_from_data( # noqa: PLR0911
*,
status_code: HTTPStatus,
data: Union[oai.Response, oai.Reference],
schemas: Schemas,
responses: Dict[str, Union[oai.Response, oai.Reference]],
Copy link

Choose a reason for hiding this comment

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

this is a breaking change to this function's signature....What's the general methodology to validate that this is safe (possibly other places that might call this function?)
I think we should have good confidence from passing tests but just calling it out in case there's anything additional we need to consider

Copy link
Author

@eli-bl eli-bl Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's validated by the existing unit tests for the repo, which include 1. a bunch of tests of building an Endpoint (which is literally the only code path response_from_data is called from), and 2. a full end-to-end test of running the generator on an entire API spec that includes endpoints with responses.

Copy link

Choose a reason for hiding this comment

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

building an Endpoint (which is literally the only code path response_from_data is called from

oh okayy cool... this is what I wanted to confirm!

parent_name: str,
config: Config,
) -> Tuple[Union[Response, ParseError], Schemas]:
"""Generate a Response from the OpenAPI dictionary representation of it"""

response_name = f"response_{status_code}"
if isinstance(data, oai.Reference):
return (
empty_response(
status_code=status_code,
response_name=response_name,
config=config,
data=data,
),
schemas,
)
ref_path = parse_reference_path(data.ref)
if isinstance(ref_path, ParseError):
return ref_path, schemas
if not ref_path.startswith("/components/responses/"):
return ParseError(data=data, detail=f"$ref to {data.ref} not allowed in responses"), schemas
resp_data = responses.get(ref_path.split("/")[-1], None)
Copy link

Choose a reason for hiding this comment

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

I'm a little surprised that getting reference data is not a little more standardized or at least with a helper.... is doing a split and -1 the general pattern in the rest of the repo?

Copy link
Author

Choose a reason for hiding this comment

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

There's no use case in the rest of the repo for getting just the type name from a reference.

Copy link
Author

Choose a reason for hiding this comment

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

Note that this isn't really "getting reference data". The helper is just converting the string "#/components/schemas/MyTypeName" to the string "MyTypeName".

Copy link
Author

Choose a reason for hiding this comment

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

Whoops, I was wrong, there are a couple other usages like this. So I've added a helper.

if not resp_data:
return ParseError(data=data, detail=f"Could not find reference: {data.ref}"), schemas
if not isinstance(resp_data, oai.Response):
return ParseError(data=data, detail="Top-level $ref inside components/responses is not supported"), schemas
Copy link

Choose a reason for hiding this comment

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

instead of individual return statements like these,... what if we used a error_detail: str variable and just have one return statement at the end of this function that has one return statement for ParseError -- this may or may not look very pretty but at least it would avoid disabling PLR0911 rule

Copy link
Author

Choose a reason for hiding this comment

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

I honestly find it easier to follow the logic when there's explicit short-circuiting like this. Maybe it's because I did a lot of Go and that's the preferred pattern in Go. But if I see a bunch of this kind of thing—

if bad_condition_1:
    error_detail = message1
elif bad_condition_2:
    error_detail = message2
else:
    error_detail = None
if error_detail:
    return ParseError(data=data, detail=error_detail)

—it bothers me a little because now the code paths for the error conditions are not really independent of each other; if for instance I messed up and put "if" instead of "elif" for the second one, now the bad_condition_1 test would be effectively a no-op. It also means that I have to be constructing the same kind of error, ParseError, each time (and the same way each time: data=data), which happens to be true in this case but isn't logically inevitable; if I had to make a change where now one of those cases raises a different error, I'd have to refactor the whole thing.

PLR0911 is only a helpful rule when it's helpful, and this codebase disables it in quite a few parsing/validation functions like this.

Copy link

Choose a reason for hiding this comment

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

PLR0911 is only a helpful rule when it's helpful, and
agree

this codebase disables it in quite a few parsing/validation functions like this

okay this sounds good to me

data = resp_data

content = data.content
if not content:
Expand Down
Loading