-
-
Notifications
You must be signed in to change notification settings - Fork 531
fix(openapi): fix specifications for responses without content #4602
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,11 +51,10 @@ | |
| Send, | ||
| TypeEncodersMap, | ||
| ) | ||
| from litestar.types.builtin_types import NoneType | ||
| from litestar.utils import deprecated as litestar_deprecated | ||
| from litestar.utils import ensure_async_callable | ||
| from litestar.utils.empty import value_or_default | ||
| from litestar.utils.predicates import is_async_callable, is_class_and_subclass | ||
| from litestar.utils.predicates import is_async_callable | ||
| from litestar.utils.scope.state import ScopeState | ||
| from litestar.utils.warnings import warn_implicit_sync_to_thread, warn_sync_to_thread_with_async_callable | ||
|
|
||
|
|
@@ -531,6 +530,15 @@ def resolve_request_max_body_size(self) -> int | None: | |
| def request_max_body_size(self) -> int | None: | ||
| return value_or_default(self._request_max_body_size, None) # pyright: ignore | ||
|
|
||
| @property | ||
| def returns_content(self) -> bool: | ||
| """Whether the route handler returns any content in the response body.""" | ||
| return not ( | ||
| self.status_code < 200 | ||
| or self.status_code in {HTTP_204_NO_CONTENT, HTTP_304_NOT_MODIFIED} | ||
| or self.http_methods == {HttpMethod.HEAD} | ||
| ) | ||
|
Comment on lines
+533
to
+540
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know what the best place for this logic is. For now, I assumed that the handler is the owner of this information, because below it also validates the annotation based on the same logic. |
||
|
|
||
| def on_registration(self, route: BaseRoute, app: Litestar) -> None: | ||
| super().on_registration(route=route, app=app) | ||
|
|
||
|
|
@@ -579,6 +587,15 @@ def _validate_handler_function(self) -> None: | |
| f"If {self} should return a value, change the route handler status code to an appropriate value.", | ||
| ) | ||
|
|
||
| if self.http_methods == {HttpMethod.HEAD} and not ( | ||
| is_empty_response_annotation(return_type) | ||
| or return_type.is_subclass_of(File) | ||
| or return_type.is_subclass_of(ASGIFileResponse) | ||
| ): | ||
| raise ImproperlyConfiguredException( | ||
| f"{self}: Handlers for 'HEAD' requests must not return a value. Either return 'None' or a response type without a body." | ||
| ) | ||
|
|
||
| if not self.media_type: | ||
| if return_type.is_subclass_of((str, bytes)) or return_type.annotation is AnyStr: | ||
| self.media_type = MediaType.TEXT | ||
|
|
@@ -591,23 +608,6 @@ def _validate_handler_function(self) -> None: | |
| if "data" in self.parsed_fn_signature.parameters and "GET" in self.http_methods: | ||
| raise ImproperlyConfiguredException("'data' kwarg is unsupported for 'GET' request handlers") | ||
|
|
||
| if self.http_methods == {HttpMethod.HEAD} and not self.parsed_fn_signature.return_type.is_subclass_of( | ||
| ( | ||
| NoneType, | ||
| File, | ||
| ASGIFileResponse, | ||
| ) | ||
| ): | ||
| field_definition = self.parsed_fn_signature.return_type | ||
| if not ( | ||
| is_empty_response_annotation(field_definition) | ||
| or is_class_and_subclass(field_definition.annotation, File) | ||
| or is_class_and_subclass(field_definition.annotation, ASGIFileResponse) | ||
| ): | ||
| raise ImproperlyConfiguredException( | ||
| f"{self}: Handlers for 'HEAD' requests must not return a value. Either return 'None' or a response type without a body." | ||
| ) | ||
|
|
||
|
Comment on lines
590
to
-610
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know why this check was so complicated before. I simplified it, and I think it does the same as before? Correct me if I'm missing something. I also moved it be closer to the other check with status codes, as both of them are about empty annotations. |
||
| if (body_param := self.parsed_fn_signature.parameters.get("body")) and not body_param.is_subclass_of(bytes): | ||
| raise ImproperlyConfiguredException( | ||
| f"Invalid type annotation for 'body' parameter in route handler {self}. 'body' will always receive the " | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,24 +55,15 @@ def handler_2() -> None: | |
| openapi_config=OpenAPIConfig(title="my title", version="1.0.0", operation_id_creator=operation_id_creator), | ||
| ) | ||
|
|
||
| assert app.openapi_schema.to_schema()["paths"] == { | ||
| "/1": { | ||
| "get": { | ||
| "deprecated": False, | ||
| "operationId": "id_x", | ||
| "responses": {"200": {"description": "Request fulfilled, document follows", "headers": {}}}, | ||
| "summary": "Handler1", | ||
| } | ||
| }, | ||
| "/2": { | ||
| "get": { | ||
| "deprecated": False, | ||
| "operationId": "id_y", | ||
| "responses": {"200": {"description": "Request fulfilled, document follows", "headers": {}}}, | ||
| "summary": "Handler2", | ||
| } | ||
| }, | ||
| } | ||
|
Comment on lines
-58
to
-75
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These handlers are annotated with But I think it's pointless to compare the whole thing anyway. This test only cares about customising operation IDs, so I restricted it to check only that. And the test whether handlers like this include |
||
| assert app.openapi_schema.paths is not None | ||
|
|
||
| assert "/1" in app.openapi_schema.paths | ||
| assert app.openapi_schema.paths["/1"].get is not None | ||
| assert app.openapi_schema.paths["/1"].get.operation_id == "id_x" | ||
|
|
||
| assert "/2" in app.openapi_schema.paths | ||
| assert app.openapi_schema.paths["/2"].get is not None | ||
| assert app.openapi_schema.paths["/2"].get.operation_id == "id_y" | ||
|
|
||
|
|
||
| def test_raises_exception_when_no_config_in_place() -> None: | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redirect,Fileare checked first, because they also might fall intoreturns_contentbeingFalse, but they need their own special handling.Checking
ASGIResponseis still there, because I guess the assumption is that you can't infer any meaningful content schema for it, so you fall back to specifying there is no content. And it works like that regardless of whether there will be actual content in the response or not.Checking
NoneTypeis removed, becauseNonedoesn't necessarily mean there is no content. It also depends on the status code and method.GETwith200returningNonewill includenullin the actual response, and now it will also include content in OpenAPI specification.