Skip to content
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

🐛 Fix empty reponse body when default status_code is empty but the a Response parameter with response.status_code is set #5360

Merged
merged 5 commits into from Sep 8, 2022

Conversation

tmeckel
Copy link
Contributor

@tmeckel tmeckel commented Sep 7, 2022

This PR contains a fix to a bug which cleared the reponse content if the status code is changed via a Resource parameter described in the documentation.

Sample code which creates an empty response body:

@app.delete(
    "/{id}",
    responses={
        204: {"model": None, "description": "No Content"},
    },
    status_code=204,
)
async def delete_deployment(
    id: str = Path(None),
    response: Response = None,
) -> None:
    response.status_code = 400
    return {"msg": "Status overwritten"}

@codecov
Copy link

codecov bot commented Sep 7, 2022

Codecov Report

Merging #5360 (9bf2b96) into master (c4007cb) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #5360   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          539       540    +1     
  Lines        13918     13936   +18     
=========================================
+ Hits         13918     13936   +18     
Impacted Files Coverage Δ
fastapi/routing.py 100.00% <100.00%> (ø)
tests/test_reponse_set_reponse_code_empty.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2022

📝 Docs preview for commit 834bb6d at: https://6318c7d257bf16323c910a72--fastapi.netlify.app

Copy link
Contributor

@iudeen iudeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check if proper response code is reflecting in the api doc?

In Doc, it still shows 204. This PR would be complete if that is handled as well, IMHO.
image

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2022

📝 Docs preview for commit a942d46 at: https://6319ffa9c38fb201c2c7a290--fastapi.netlify.app

@tmeckel
Copy link
Contributor Author

tmeckel commented Sep 8, 2022

@iudeen can you elaborate what you mean by "...proper response code is reflecting in the api doc?" As you can see from the referenced link to the documentation it's supported to override the response code by setting the code on a Reponse parameter object to the API method.

@iudeen
Copy link
Contributor

iudeen commented Sep 8, 2022

can you elaborate what you mean by "...proper response code is reflecting in the api doc?"

I was meaning to say if we change response codes, it should reflect in Swagger (or Redoc) docs as well. But I now realize this behavior was not there before as well and seems its out of scope for this PR.

LGTM! as it avoids the error below

RuntimeError: Response content shorter than Content-Length

👍

@tiangolo
Copy link
Owner

tiangolo commented Sep 8, 2022

Awesome, thank you @tmeckel! 🚀 🍰

I updated the tests a bit to simplify them and cover OpenAPI (what can be supported).

@iudeen thanks for the interest, but the custom response code here is done only at runtime, there's no way to put that in OpenAPI, so that wouldn't apply here.

Thanks! ☕

@tiangolo tiangolo changed the title Fix: empty reponse body when reponse code set via Response parameter 🐛 Fix empty reponse body when default status_code is empty but the a Response parameter with response.status_code is set Sep 8, 2022
@tiangolo tiangolo enabled auto-merge (squash) September 8, 2022 15:02
@tiangolo tiangolo merged commit 0b4fe10 into tiangolo:master Sep 8, 2022
@tmeckel tmeckel deleted the fix/empty-reponse-body branch September 8, 2022 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants