fix(openapi): fix specifications for responses without content#4602
fix(openapi): fix specifications for responses without content#4602spietras wants to merge 1 commit intolitestar-org:mainfrom
Conversation
github-actions
bot
added
area/openapi
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4602 +/- ##
=======================================
Coverage 97.85% 97.85%
=======================================
Files 297 297
Lines 15344 15345 +1
Branches 1722 1721 -1
=======================================
+ Hits 15015 15016 +1
Misses 188 188
Partials 141 141 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
spietras
force-pushed
the
fix/4598-response-none
branch
from
7b48c69 to
b9990f7
Compare
spietras
force-pushed
the
fix/4598-response-none
branch
from
b9990f7 to
edee808
Compare
|
Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/4602 |
| if self.field_definition.is_subclass_of(Redirect): | ||
| response = self.create_redirect_response() | ||
| elif self.field_definition.is_subclass_of(File): | ||
| response = self.create_file_response() | ||
| elif self.field_definition.is_subclass_of(ASGIResponse) or not self.route_handler.returns_content: | ||
| response = self.create_empty_response() |
There was a problem hiding this comment.
Redirect, File are checked first, because they also might fall into returns_content being False, but they need their own special handling.
Checking ASGIResponse is 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 NoneType is removed, because None doesn't necessarily mean there is no content. It also depends on the status code and method. GET with 200 returning None will include null in the actual response, and now it will also include content in OpenAPI specification.
| @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} | ||
| ) |
There was a problem hiding this comment.
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.
| @@ -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." | |||
| ) | |||
|
|
|||
There was a problem hiding this comment.
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.
| 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", | ||
| } | ||
| }, | ||
| } |
There was a problem hiding this comment.
These handlers are annotated with None, but they actually return null. So now the OpenAPI specification correctly includes a schema for the response with null. So I would need to add this here.
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 null content schema in response specifications is added in another place.
1 participant
Description
OpenAPI response specifications for responses without content were different from the actual behaviour in some cases.
This change aligns OpenAPI response specifications with the actual behaviour to not include any content when:
200204304HEADCloses
Closes #4598