From 538739e95cf75e04aef6a424058606f995afeb22 Mon Sep 17 00:00:00 2001 From: Peter Morton <83794272+peter-greenatlas@users.noreply.github.com> Date: Mon, 8 Apr 2024 09:50:18 +1000 Subject: [PATCH 1/4] validate value of 'const' properties For 'const' properties, the parsed value will now be checked against the value in the schema at runtime and a ValueError will be raised if they do not match. This helps with parsing `oneOf` when matching a const property is the only way to resolve which of the types should be returned. --- .../api/const/post_const_path.py | 8 ++++++- .../models/post_const_path_body.py | 22 +++++++++++++++++-- .../parser/properties/const.py | 3 ++- .../const_property.py.jinja | 19 ++++++++++++++++ 4 files changed, 48 insertions(+), 4 deletions(-) create mode 100644 openapi_python_client/templates/property_templates/const_property.py.jinja diff --git a/end_to_end_tests/test-3-1-golden-record/test_3_1_features_client/api/const/post_const_path.py b/end_to_end_tests/test-3-1-golden-record/test_3_1_features_client/api/const/post_const_path.py index 3f864b3dc..852d44325 100644 --- a/end_to_end_tests/test-3-1-golden-record/test_3_1_features_client/api/const/post_const_path.py +++ b/end_to_end_tests/test-3-1-golden-record/test_3_1_features_client/api/const/post_const_path.py @@ -45,7 +45,13 @@ def _parse_response( *, client: Union[AuthenticatedClient, Client], response: httpx.Response ) -> Optional[Literal["Why have a fixed response? I dunno"]]: if response.status_code == HTTPStatus.OK: - response_200 = cast(Literal["Why have a fixed response? I dunno"], response.json()) + _response_200 = response.json() + if _response_200 != "Why have a fixed response? I dunno": + raise ValueError( + f"response_200 must match const 'Why have a fixed response? I dunno', got '{ _response_200 }'" + ) + response_200 = cast(Literal["Why have a fixed response? I dunno"], _response_200) + return response_200 if client.raise_on_unexpected_status: raise errors.UnexpectedStatus(response.status_code, response.content) diff --git a/end_to_end_tests/test-3-1-golden-record/test_3_1_features_client/models/post_const_path_body.py b/end_to_end_tests/test-3-1-golden-record/test_3_1_features_client/models/post_const_path_body.py index 387e693e0..4f31ed36b 100644 --- a/end_to_end_tests/test-3-1-golden-record/test_3_1_features_client/models/post_const_path_body.py +++ b/end_to_end_tests/test-3-1-golden-record/test_3_1_features_client/models/post_const_path_body.py @@ -46,16 +46,34 @@ def to_dict(self) -> Dict[str, Any]: @classmethod def from_dict(cls: Type[T], src_dict: Dict[str, Any]) -> T: d = src_dict.copy() - required = d.pop("required") + _required = d.pop("required") + if _required != "this always goes in the body": + raise ValueError(f"required must match const 'this always goes in the body', got '{ _required }'") + required = cast(Literal["this always goes in the body"], _required) def _parse_nullable(data: object) -> Union[Literal["this or null goes in the body"], None]: if data is None: return data + _nullable_type_1 = data + if _nullable_type_1 != "this or null goes in the body": + raise ValueError( + f"nullable_type_1 must match const 'this or null goes in the body', got '{ _nullable_type_1 }'" + ) + nullable_type_1 = cast(Literal["this or null goes in the body"], _nullable_type_1) + + return nullable_type_1 return cast(Union[Literal["this or null goes in the body"], None], data) nullable = _parse_nullable(d.pop("nullable")) - optional = d.pop("optional", UNSET) + _optional = d.pop("optional", UNSET) + optional: Union[Literal["this sometimes goes in the body"], Unset] + if isinstance(_optional, Unset): + optional = UNSET + else: + if _optional != "this sometimes goes in the body": + raise ValueError(f"optional must match const 'this sometimes goes in the body', got '{ _optional }'") + optional = cast(Union[Literal["this sometimes goes in the body"], Unset], _optional) post_const_path_body = cls( required=required, diff --git a/openapi_python_client/parser/properties/const.py b/openapi_python_client/parser/properties/const.py index 88a398893..07817cfec 100644 --- a/openapi_python_client/parser/properties/const.py +++ b/openapi_python_client/parser/properties/const.py @@ -12,7 +12,7 @@ @define class ConstProperty(PropertyProtocol): - """A property representing a Union (anyOf) of other properties""" + """A property representing a const value""" name: str required: bool @@ -21,6 +21,7 @@ class ConstProperty(PropertyProtocol): python_name: PythonIdentifier description: str | None example: None + template: ClassVar[str] = "const_property.py.jinja" @classmethod def build( diff --git a/openapi_python_client/templates/property_templates/const_property.py.jinja b/openapi_python_client/templates/property_templates/const_property.py.jinja new file mode 100644 index 000000000..cb37e92e2 --- /dev/null +++ b/openapi_python_client/templates/property_templates/const_property.py.jinja @@ -0,0 +1,19 @@ +{% macro construct_impl(property, source) %} +if {{ source }} != {{ property.value }}: + raise ValueError(f"{{ property.name }} must match const {{ property.value }}, got '{ {{ source }} }'") +{{ property.python_name }} = cast({{ property.get_type_string() }} , {{ source }}) +{%- endmacro %} + + +{% macro construct(property, source) %} +_{{ property.python_name }} = {{ source }} +{% if property.required %} +{{ construct_impl(property, "_" + property.python_name) }} +{% else %} +{{ property.python_name }} : {{ property.get_type_string() }} +if isinstance(_{{ property.python_name }}, Unset): + {{ property.python_name }} = UNSET +else: + {{ construct_impl(property, "_" + property.python_name) | indent }} +{% endif %} +{%- endmacro %} From 0558361a6297f6a15064305f9c1f8c114c80e47b Mon Sep 17 00:00:00 2001 From: Dylan Anthony Date: Fri, 19 Apr 2024 14:53:53 -0600 Subject: [PATCH 2/4] Fix mypy complaint --- openapi_python_client/parser/properties/const.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openapi_python_client/parser/properties/const.py b/openapi_python_client/parser/properties/const.py index 07817cfec..aec624afd 100644 --- a/openapi_python_client/parser/properties/const.py +++ b/openapi_python_client/parser/properties/const.py @@ -1,6 +1,6 @@ from __future__ import annotations -from typing import Any, overload +from typing import Any, ClassVar, overload from attr import define From 337b1b97fada69fca42aa6b89512ea6cf99517f7 Mon Sep 17 00:00:00 2001 From: Dylan Anthony Date: Fri, 19 Apr 2024 15:10:17 -0600 Subject: [PATCH 3/4] Simplify generated code --- .../api/const/post_const_path.py | 8 +++--- .../models/post_const_path_body.py | 26 +++++++------------ .../const_property.py.jinja | 20 +++----------- 3 files changed, 15 insertions(+), 39 deletions(-) diff --git a/end_to_end_tests/test-3-1-golden-record/test_3_1_features_client/api/const/post_const_path.py b/end_to_end_tests/test-3-1-golden-record/test_3_1_features_client/api/const/post_const_path.py index 852d44325..929f417f4 100644 --- a/end_to_end_tests/test-3-1-golden-record/test_3_1_features_client/api/const/post_const_path.py +++ b/end_to_end_tests/test-3-1-golden-record/test_3_1_features_client/api/const/post_const_path.py @@ -45,13 +45,11 @@ def _parse_response( *, client: Union[AuthenticatedClient, Client], response: httpx.Response ) -> Optional[Literal["Why have a fixed response? I dunno"]]: if response.status_code == HTTPStatus.OK: - _response_200 = response.json() - if _response_200 != "Why have a fixed response? I dunno": + response_200 = cast(Literal["Why have a fixed response? I dunno"], response.json()) + if response_200 != "Why have a fixed response? I dunno": raise ValueError( - f"response_200 must match const 'Why have a fixed response? I dunno', got '{ _response_200 }'" + f"response_200 must match const 'Why have a fixed response? I dunno', got '{response_200}'" ) - response_200 = cast(Literal["Why have a fixed response? I dunno"], _response_200) - return response_200 if client.raise_on_unexpected_status: raise errors.UnexpectedStatus(response.status_code, response.content) diff --git a/end_to_end_tests/test-3-1-golden-record/test_3_1_features_client/models/post_const_path_body.py b/end_to_end_tests/test-3-1-golden-record/test_3_1_features_client/models/post_const_path_body.py index 4f31ed36b..9ac2f9102 100644 --- a/end_to_end_tests/test-3-1-golden-record/test_3_1_features_client/models/post_const_path_body.py +++ b/end_to_end_tests/test-3-1-golden-record/test_3_1_features_client/models/post_const_path_body.py @@ -46,34 +46,26 @@ def to_dict(self) -> Dict[str, Any]: @classmethod def from_dict(cls: Type[T], src_dict: Dict[str, Any]) -> T: d = src_dict.copy() - _required = d.pop("required") - if _required != "this always goes in the body": - raise ValueError(f"required must match const 'this always goes in the body', got '{ _required }'") - required = cast(Literal["this always goes in the body"], _required) + required = cast(Literal["this always goes in the body"], d.pop("required")) + if required != "this always goes in the body": + raise ValueError(f"required must match const 'this always goes in the body', got '{required}'") def _parse_nullable(data: object) -> Union[Literal["this or null goes in the body"], None]: if data is None: return data - _nullable_type_1 = data - if _nullable_type_1 != "this or null goes in the body": + nullable_type_1 = cast(Literal["this or null goes in the body"], data) + if nullable_type_1 != "this or null goes in the body": raise ValueError( - f"nullable_type_1 must match const 'this or null goes in the body', got '{ _nullable_type_1 }'" + f"nullable_type_1 must match const 'this or null goes in the body', got '{nullable_type_1}'" ) - nullable_type_1 = cast(Literal["this or null goes in the body"], _nullable_type_1) - return nullable_type_1 return cast(Union[Literal["this or null goes in the body"], None], data) nullable = _parse_nullable(d.pop("nullable")) - _optional = d.pop("optional", UNSET) - optional: Union[Literal["this sometimes goes in the body"], Unset] - if isinstance(_optional, Unset): - optional = UNSET - else: - if _optional != "this sometimes goes in the body": - raise ValueError(f"optional must match const 'this sometimes goes in the body', got '{ _optional }'") - optional = cast(Union[Literal["this sometimes goes in the body"], Unset], _optional) + optional = cast(Union[Literal["this sometimes goes in the body"], Unset], d.pop("optional", UNSET)) + if optional != "this sometimes goes in the body" and not isinstance(optional, Unset): + raise ValueError(f"optional must match const 'this sometimes goes in the body', got '{optional}'") post_const_path_body = cls( required=required, diff --git a/openapi_python_client/templates/property_templates/const_property.py.jinja b/openapi_python_client/templates/property_templates/const_property.py.jinja index cb37e92e2..ea48ab73e 100644 --- a/openapi_python_client/templates/property_templates/const_property.py.jinja +++ b/openapi_python_client/templates/property_templates/const_property.py.jinja @@ -1,19 +1,5 @@ -{% macro construct_impl(property, source) %} -if {{ source }} != {{ property.value }}: - raise ValueError(f"{{ property.name }} must match const {{ property.value }}, got '{ {{ source }} }'") -{{ property.python_name }} = cast({{ property.get_type_string() }} , {{ source }}) -{%- endmacro %} - - {% macro construct(property, source) %} -_{{ property.python_name }} = {{ source }} -{% if property.required %} -{{ construct_impl(property, "_" + property.python_name) }} -{% else %} -{{ property.python_name }} : {{ property.get_type_string() }} -if isinstance(_{{ property.python_name }}, Unset): - {{ property.python_name }} = UNSET -else: - {{ construct_impl(property, "_" + property.python_name) | indent }} -{% endif %} +{{ property.python_name }} = cast({{ property.get_type_string() }} , {{ source }}) +if {{ property.python_name }} != {{ property.value }}{% if not property.required %}and not isinstance({{ property.python_name }}, Unset){% endif %}: + raise ValueError(f"{{ property.name }} must match const {{ property.value }}, got '{{'{' + property.python_name + '}' }}'") {%- endmacro %} From 91a72d304957d18bec29ac026acd737f80bd2dbb Mon Sep 17 00:00:00 2001 From: Dylan Anthony Date: Fri, 19 Apr 2024 16:30:20 -0600 Subject: [PATCH 4/4] Changeset --- .../const_values_are_now_validated_at_runtime.md | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 .changeset/const_values_are_now_validated_at_runtime.md diff --git a/.changeset/const_values_are_now_validated_at_runtime.md b/.changeset/const_values_are_now_validated_at_runtime.md new file mode 100644 index 000000000..0987d18ce --- /dev/null +++ b/.changeset/const_values_are_now_validated_at_runtime.md @@ -0,0 +1,10 @@ +--- +default: major +--- + +# `const` values in responses are now validated at runtime + +Prior to this version, `const` values returned from servers were assumed to always be correct. Now, if a server returns +an unexpected value, the client will raise a `ValueError`. This should enable better usage with `oneOf`. + +PR #1024. Thanks @peter-greenatlas!