8000 🐛Fix `RuntimeError` raised when `HTTPException` has a status code with no content by iudeen · Pull Request #5365 · fastapi/fastapi · GitHub
[go: up one dir, main page]
Skip to content

🐛Fix RuntimeError raised when HTTPException has a status code with no content #5365

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.

Sign up for GitHub

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

Jump to bottom
Merged
tiangolo merged 8 commits into from
Sep 11, 2022
Merged

🐛Fix RuntimeError raised when HTTPException has a status code with no content #5365

tiangolo merged 8 commits into from
Sep 11, 2022

Conversation

@iudeen
Copy link
Contributor
@iudeen iudeen commented Sep 8, 2022 •
edited by tiangolo
Loading

Fixes/related to #4949 (comment)

Please advice if this approach is correct one.

Suggestions welcome!

@codecov
Copy link
codecov bot commented Sep 8, 2022 •
edited
Loading

Codecov Report

Merging #5365 (1fe25cf) into master (6620273) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #5365   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          540       540           
  Lines        13936     13951   +15     
=========================================
+ Hits         13936     13951   +15
Impacted Files Coverage Δ
fastapi/exception_handlers.py 100.00% <100.00%> (ø)
tests/test_starlette_exception.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 8, 2022

📝 Docs preview for commit afecf9b at: https://631a48f65533fb0233157a85--fastapi.netlify.app

@github-actions
Copy link
Contributor
github-actions bot commented Sep 8, 2022

📝 Docs preview for commit f96a078 at: https://631a4cce339597069b76e096--fastapi.netlify.app

@github-actions
Copy link
Contributor
github-actions bot commented Sep 8, 2022

📝 Docs preview for commit c357017 at: https://631a4e7d882b77006ea53e5b--fastapi.netlify.app

@iudeen iudeen changed the title fix: handle no body status codes in HTTPExceptions 🐛Fix Handle HTTPExceptions with status codes that should not support body Sep 8, 2022
@iudeen iudeen changed the title 🐛Fix Handle HTTPExceptions with status codes that should not support body 🐛Fix RuntimeError that got raised when HTTPExceptions with status codes that should not support body was raised Sep 8, 2022
Kludex
Kludex approved these changes Sep 8, 2022
View reviewed changes
Copy link
Member
@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

Yep, same idea as what we have in Starlette: https://github.com/encode/starlette/blob/bc61505faef15b673bf4bf65db4927daa354d6b8/starlette/middleware/exceptions.py#L99-L104

@iudeen @Kludex
Update tests/test_starlette_exception.py
f26af44 
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
@iudeen
Copy link
Contributor Author
iudeen commented Sep 8, 2022 •
edited
Loading

Yep, same idea as what we have in Starlette: https://github.com/encode/starlette/blob/bc61505faef15b673bf4bf65db4927daa354d6b8/starlette/middleware/exceptions.py#L99-L104

I was able to find the bug by adding debug points to ExceptionMiddleware while running the app using FastAPI and Starlette in parallel.

And thus, the code was referred from Starlette! (Sorry, should have mentioned)

@github-actions
Copy link
Contributor
github-actions bot commented Sep 8, 2022

📝 Docs preview for commit f26af44 at: https://631a5fedd66a3a17d38dcc68--fastapi.netlify.app

@github-actions
Copy link
Contributor
github-actions bot commented Sep 9, 2022 8000

📝 Docs preview for commit fda6d88 at: https://631a9d31d66a3a47438dc97b--fastapi.netlify.app

JarroVGIT
JarroVGIT approved these changes Sep 9, 2022
View reviewed changes
Copy link
Contributor
@JarroVGIT JarroVGIT left a comment

Choose a reason for hiding this comment

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

LGTM

@tiangolo tiangolo changed the title 🐛Fix RuntimeError that got raised when HTTPExceptions with status codes that should not support body was raised 🐛Fix RuntimeError raised when HTTPException has a status code with no content Sep 11, 2022
@tiangolo
Copy link
Member
tiangolo commented Sep 11, 2022

Awesome, thanks for the fix @iudeen! 🍰 🚀

And thanks for the reviews and comments @Kludex, @JarroVGIT, @BilalAlpaslan 🙇 ☕

@tiangolo tiangolo enabled auto-merge (squash) September 11, 2022 16:12
@tiangolo tiangolo merged commit 4d27046 into fastapi:master Sep 11, 2022
@tiangolo tiangolo mentioned this pull request Sep 11, 2022
Closed
9 tasks
This was referenced Sep 11, 2022
Closed
Merged
@gazpachoking gazpachoking mentioned this pull request Sep 18, 2022
Closed
9 tasks
@ramnes ramnes mentioned this pull request Dec 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kludex Kludex Kludex approved these changes

+2 more reviewers

@JarroVGIT JarroVGIT JarroVGIT approved these changes

@BilalAlpaslan BilalAlpaslan BilalAlpaslan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@iudeen @tiangolo @Kludex @JarroVGIT @BilalAlpaslan
0