Skip to content
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

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

Merged
merged 7 commits into from
Oct 28, 2024
Merged
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
26 changes: 26 additions & 0 deletions end_to_end_tests/baseline_openapi_3.0.json
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
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"
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
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
3 changes: 2 additions & 1 deletion openapi_python_client/parser/bodies.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
Schemas,
property_from_data,
)
from openapi_python_client.parser.properties.schemas import get_reference_simple_name

from .. import schema as oai
from ..config import Config
Expand Down Expand Up @@ -138,7 +139,7 @@ def _resolve_reference(
references_seen = []
while isinstance(body, oai.Reference) and body.ref not in references_seen:
references_seen.append(body.ref)
body = request_bodies.get(body.ref.split("/")[-1])
body = request_bodies.get(get_reference_simple_name(body.ref))
if isinstance(body, oai.Reference):
return ParseError(detail="Circular $ref in request body", data=body)
if body is None and references_seen:
Expand Down
27 changes: 24 additions & 3 deletions openapi_python_client/parser/openapi.py
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
9 changes: 8 additions & 1 deletion openapi_python_client/parser/properties/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,13 @@ def parse_reference_path(ref_path_raw: str) -> Union[ReferencePath, ParseError]:
return cast(ReferencePath, parsed.fragment)


def get_reference_simple_name(ref_path: str) -> str:
"""
Takes a path like `/components/schemas/NameOfThing` and returns a string like `NameOfThing`.
"""
return ref_path.split("/", 3)[-1]


Comment on lines +49 to +55

Choose a reason for hiding this comment

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

nice 👍

@define
class Class:
"""Represents Python class which will be generated from an OpenAPI schema"""
Expand All @@ -56,7 +63,7 @@ class Class:
@staticmethod
def from_string(*, string: str, config: Config) -> "Class":
"""Get a Class from an arbitrary string"""
class_name = string.split("/")[-1] # Get rid of ref path stuff
class_name = get_reference_simple_name(string) # Get rid of ref path stuff
class_name = ClassName(class_name, config.field_prefix)
override = config.class_overrides.get(class_name)

Expand Down
26 changes: 15 additions & 11 deletions openapi_python_client/parser/responses.py
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 get_reference_simple_name, 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]],

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

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.

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(get_reference_simple_name(ref_path), None)
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
data = resp_data

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