From 11df5968a326e54704bf4789ed948930b32c7c13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Janek=20Nouvertn=C3=A9?= Date: Sun, 22 Dec 2024 19:46:05 +0100 Subject: [PATCH] fix(OpenAPI): Query-only properties included in path, cookie and header parameter schema and response headers (#3908) (#3909) --- litestar/datastructures/response_header.py | 27 ++++++++++++-- litestar/openapi/spec/base.py | 7 ++++ litestar/openapi/spec/header.py | 40 ++++++++++++++++----- litestar/openapi/spec/parameter.py | 9 +++++ tests/e2e/test_openapi/test_spec_headers.py | 2 -- tests/unit/test_openapi/test_config.py | 4 --- tests/unit/test_openapi/test_parameters.py | 30 ++++++++++++++++ tests/unit/test_openapi/test_responses.py | 32 ++++++++++++++--- 8 files changed, 130 insertions(+), 21 deletions(-) diff --git a/litestar/datastructures/response_header.py b/litestar/datastructures/response_header.py index f781d0c37e..807eec6bd5 100644 --- a/litestar/datastructures/response_header.py +++ b/litestar/datastructures/response_header.py @@ -4,6 +4,7 @@ from typing import TYPE_CHECKING, Any from litestar.exceptions import ImproperlyConfiguredException +from litestar.utils import warn_deprecation if TYPE_CHECKING: from litestar.openapi.spec import Example @@ -46,7 +47,7 @@ class ResponseHeader: Default value is `false`. """ - allow_empty_value: bool = False + allow_empty_value: bool = None # type: ignore[assignment] """Sets the ability to pass empty-valued parameters. This is valid only for `query` parameters and allows sending a parameter with an empty value. Default value is `false`. If. @@ -80,7 +81,7 @@ class ResponseHeader: For all other styles, the default value is `false`. """ - allow_reserved: bool = False + allow_reserved: bool = None # type: ignore[assignment] """Determines whether the parameter value SHOULD allow reserved characters, as defined by. @@ -121,5 +122,27 @@ def __post_init__(self) -> None: if not self.documentation_only and self.value is None: raise ImproperlyConfiguredException("value must be set if documentation_only is false") + if self.allow_reserved is None: + self.allow_reserved = False # type: ignore[unreachable] + else: + warn_deprecation( + "2.13.1", + "allow_reserved", + kind="parameter", + removal_in="4", + info="This property is invalid for headers and will be ignored", + ) + + if self.allow_empty_value is None: + self.allow_empty_value = False # type: ignore[unreachable] + else: + warn_deprecation( + "2.13.1", + "allow_empty_value", + kind="parameter", + removal_in="4", + info="This property is invalid for headers and will be ignored", + ) + def __hash__(self) -> int: return hash(self.name) diff --git a/litestar/openapi/spec/base.py b/litestar/openapi/spec/base.py index 873a26adbc..f5ae453036 100644 --- a/litestar/openapi/spec/base.py +++ b/litestar/openapi/spec/base.py @@ -42,6 +42,10 @@ def _normalize_value(value: Any) -> Any: class BaseSchemaObject: """Base class for schema spec objects""" + @property + def _exclude_fields(self) -> set[str]: + return set() + def _iter_fields(self) -> Iterator[Field[Any]]: yield from fields(self) @@ -50,8 +54,11 @@ def to_schema(self) -> dict[str, Any]: recursively. """ result: dict[str, Any] = {} + exclude = self._exclude_fields for field in self._iter_fields(): + if field.name in exclude: + continue value = _normalize_value(getattr(self, field.name, None)) if value is not None: diff --git a/litestar/openapi/spec/header.py b/litestar/openapi/spec/header.py index 32f4ad95bc..4425f52bf7 100644 --- a/litestar/openapi/spec/header.py +++ b/litestar/openapi/spec/header.py @@ -1,11 +1,10 @@ from __future__ import annotations -from dataclasses import Field, dataclass -from typing import TYPE_CHECKING, Any, Iterator, Literal - -from typing_extensions import override +from dataclasses import dataclass +from typing import TYPE_CHECKING, Any, Literal from litestar.openapi.spec.base import BaseSchemaObject +from litestar.utils import warn_deprecation if TYPE_CHECKING: from litestar.openapi.spec.example import Example @@ -55,7 +54,7 @@ class OpenAPIHeader(BaseSchemaObject): deprecated: bool = False """Specifies that a parameter is deprecated and SHOULD be transitioned out of usage. Default value is ``False``.""" - allow_empty_value: bool = False + allow_empty_value: bool = None # type: ignore[assignment] """Sets the ability to pass empty-valued parameters. This is valid only for ``query`` parameters and allows sending a parameter with an empty value. Default value is ``False``. If `style `__ is used, and if behavior is ``n/a`` (cannot be @@ -87,7 +86,7 @@ class OpenAPIHeader(BaseSchemaObject): other styles, the default value is ``False``. """ - allow_reserved: bool = False + allow_reserved: bool = None # type: ignore[assignment] """Determines whether the parameter value SHOULD allow reserved characters, as defined by. :rfc:`3986` (``:/?#[]@!$&'()*+,;=``) to be included without percent-encoding. @@ -122,6 +121,29 @@ class OpenAPIHeader(BaseSchemaObject): The key is the media type and the value describes it. The map MUST only contain one entry. """ - @override - def _iter_fields(self) -> Iterator[Field[Any]]: - yield from (f for f in super()._iter_fields() if f.name not in {"name", "param_in"}) + @property + def _exclude_fields(self) -> set[str]: + return {"name", "param_in", "allow_reserved", "allow_empty_value"} + + def __post_init__(self) -> None: + if self.allow_reserved is None: + self.allow_reserved = False # type: ignore[unreachable] + else: + warn_deprecation( + "2.13.1", + "allow_reserved", + kind="parameter", + removal_in="4", + info="This property is invalid for headers and will be ignored", + ) + + if self.allow_empty_value is None: + self.allow_empty_value = False # type: ignore[unreachable] + else: + warn_deprecation( + "2.13.1", + "allow_empty_value", + kind="parameter", + removal_in="4", + info="This property is invalid for headers and will be ignored", + ) diff --git a/litestar/openapi/spec/parameter.py b/litestar/openapi/spec/parameter.py index 74a100fe4c..81305d134a 100644 --- a/litestar/openapi/spec/parameter.py +++ b/litestar/openapi/spec/parameter.py @@ -134,3 +134,12 @@ class Parameter(BaseSchemaObject): The key is the media type and the value describes it. The map MUST only contain one entry. """ + + @property + def _exclude_fields(self) -> set[str]: + exclude = set() + if self.param_in != "query": + # these are only allowed in query params + exclude.update({"allow_empty_value", "allow_reserved"}) + + return exclude diff --git a/tests/e2e/test_openapi/test_spec_headers.py b/tests/e2e/test_openapi/test_spec_headers.py index cb0b52216f..7a9b4fd7b5 100644 --- a/tests/e2e/test_openapi/test_spec_headers.py +++ b/tests/e2e/test_openapi/test_spec_headers.py @@ -19,8 +19,6 @@ def test_included_header_fields() -> None: assert app1.openapi_schema.to_schema()["paths"]["/"]["get"]["responses"]["200"]["headers"] == { "X-Version": { - "allowEmptyValue": False, - "allowReserved": False, "deprecated": False, "description": "Test", "required": False, diff --git a/tests/unit/test_openapi/test_config.py b/tests/unit/test_openapi/test_config.py index c16eeb9a41..705c2e00d8 100644 --- a/tests/unit/test_openapi/test_config.py +++ b/tests/unit/test_openapi/test_config.py @@ -30,14 +30,10 @@ def test_merged_components_correct() -> None: "one": { "required": False, "deprecated": False, - "allowEmptyValue": False, - "allowReserved": False, }, "two": { "required": False, "deprecated": False, - "allowEmptyValue": False, - "allowReserved": False, }, }, } diff --git a/tests/unit/test_openapi/test_parameters.py b/tests/unit/test_openapi/test_parameters.py index 8ef236e12b..663f2b2efe 100644 --- a/tests/unit/test_openapi/test_parameters.py +++ b/tests/unit/test_openapi/test_parameters.py @@ -445,3 +445,33 @@ async def handler( testmodel_schema_name = app.openapi_schema.paths["/"].get.parameters[0].schema.value # type: ignore[index, union-attr] assert app.openapi_schema.components.schemas[testmodel_schema_name].properties["param"].type == OpenAPIType.STRING # type: ignore[index, union-attr] + + +def test_query_param_only_properties() -> None: + # https://github.com/litestar-org/litestar/issues/3908 + @get("/{path_param:str}") + def handler( + path_param: str, + query_param: str, + header_param: Annotated[str, Parameter(header="header_param")], + cookie_param: Annotated[str, Parameter(cookie="cookie_param")], + ) -> None: + pass + + app = Litestar([handler]) + params = {p.name: p for p in app.openapi_schema.paths["/{path_param}"].get.parameters} # type: ignore[union-attr, index] + + for key in ["path_param", "header_param", "cookie_param"]: + schema = params[key].to_schema() + assert "allowEmptyValue" not in schema + assert "allowReserved" not in schema + + assert params["query_param"].to_schema() == { + "name": "query_param", + "in": "query", + "schema": {"type": "string"}, + "required": True, + "deprecated": False, + "allowEmptyValue": False, + "allowReserved": False, + } diff --git a/tests/unit/test_openapi/test_responses.py b/tests/unit/test_openapi/test_responses.py index bb5c213397..1681740e3a 100644 --- a/tests/unit/test_openapi/test_responses.py +++ b/tests/unit/test_openapi/test_responses.py @@ -188,10 +188,12 @@ def handler() -> list: assert isinstance(response.headers, dict) assert isinstance(response.headers["special-header"], OpenAPIHeader) - assert response.headers["special-header"].description == "super-duper special" - headers_schema = response.headers["special-header"].schema - assert isinstance(headers_schema, Schema) - assert headers_schema.type == OpenAPIType.STRING + assert response.headers["special-header"].to_schema() == { + "schema": {"type": "string"}, + "description": "super-duper special", + "required": False, + "deprecated": False, + } def test_create_success_response_with_cookies(create_factory: CreateFactoryFixture) -> None: @@ -538,3 +540,25 @@ def handler() -> File: response = create_factory(handler).create_success_response() assert next(iter(response.content.values())).schema.content_media_type == expected # type: ignore[union-attr] + + +def test_response_header_deprecated_properties() -> None: + assert ResponseHeader(name="foo", value="bar").allow_empty_value is False + assert ResponseHeader(name="foo", value="bar").allow_reserved is False + + with pytest.warns(DeprecationWarning, match="property is invalid for headers"): + ResponseHeader(name="foo", value="bar", allow_empty_value=True) + + with pytest.warns(DeprecationWarning, match="property is invalid for headers"): + ResponseHeader(name="foo", value="bar", allow_reserved=True) + + +def test_header_deprecated_properties() -> None: + assert OpenAPIHeader().allow_empty_value is False + assert OpenAPIHeader().allow_reserved is False + + with pytest.warns(DeprecationWarning, match="property is invalid for headers"): + OpenAPIHeader(allow_empty_value=True) + + with pytest.warns(DeprecationWarning, match="property is invalid for headers"): + OpenAPIHeader(allow_reserved=True)