automatically handle 204 status code when not overriding twith custom… #2833
Conversation
|
📝 Docs preview for commit d764fba at: https://602e657f13921611efaf3e78--fastapi.netlify.app |
|
📝 Docs preview for commit 8b30b9f at: https://602e66cf025bf7197c121ffd--fastapi.netlify.app |
|
📝 Docs preview for commit b3aa37d at: https://602e6b07ec1288f69e2e539d--fastapi.netlify.app |
Codecov Report
@@ Coverage Diff @@
## master #2833 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 243 243
Lines 7419 7427 +8
=========================================
+ Hits 7419 7427 +8
Continue to review full report at Codecov.
|
|
📝 Docs preview for commit 06ff6a2 at: https://602e7299b2a3060696389d6c--fastapi.netlify.app |
| if status_code == HTTP_204_NO_CONTENT: | ||
| actual_response_class: Type[Response] = Response | ||
| else: | ||
| actual_response_class = response_class.value |
There was a problem hiding this comment.
the type annotation was dropped here
There was a problem hiding this comment.
Pylance (Pyright) called this a redefinition, I'll check if mypy throws the same errors and if not I'll add type annotation to the else clasue
There was a problem hiding this comment.
I found some solution, not best either but maybe better?
fastapi/routing.py
Outdated
| @@ -168,7 +168,10 @@ def get_request_handler( | |||
| is_coroutine = asyncio.iscoroutinefunction(dependant.call) | |||
| is_body_form = body_field and isinstance(body_field.field_info, params.Form) | |||
| if isinstance(response_class, DefaultPlaceholder): | |||
| actual_response_class: Type[Response] = response_class.value | |||
| if status_code == HTTP_204_NO_CONTENT: | |||
| actual_response_class: Type[Response] = Response | |||
There was a problem hiding this comment.
This seems to work as expected, even when you override the response status code, as documented here: https://fastapi.tiangolo.com/advanced/response-change-status-code/#response-change-status-code
I'm not opposed to this, but I wish we didn't have to add another conditional here, and it seems like we might get some unintended behavior, as we're really just taking advantage of the fact that Response converts None to an empty byte string, not that we're actually doing any validation of the result (I'm not proposing that we should be... either).
There was a problem hiding this comment.
I understand your concern, yet I didn't find any more gentle solution.. feel free to propose if you have any :)
|
📝 Docs preview for commit f704126 at: https://602fcc91101e6627f5e32cc1--fastapi.netlify.app |
|
Hi, |
|
Hello. Some news about this MR ? 😅 (we lost a lot of time because of this bug) |
|
Hi, any updates on this PR? |
|
With only a brief glance, it seems this won't work if you set the status code using the temporary So for example, this idiom would still slip "null" out as the response body in violation of the RFC. @app.get(
"/d",
status_code=200,
responses={
200: {}
204: {}
}
)
async def d(response: Response):
if do_not_return_content():
response.status_code = 204
return
return {"field": "value" } |
|
Is this PR superseded by #5145? |
|
It seems this was fixed in #5145, right? That fix is available since FastAPI I just tried the test you have here and it seems to pass with the current code, so I would think this is already handled. 🎉 But let me know if you think there's something else missing. |
|
Assuming the original need was handled, this will be automatically closed now. But feel free to add more comments or create new issues or PRs. |
… response class
related to #1458
related to #2832
related to: encode/httpx#1474