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.

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

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

Merged
merged 8 commits into from Sep 11, 2022
Merged

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

merged 8 commits into from Sep 11, 2022

Conversation

iudeen
Copy link
Contributor

@iudeen iudeen commented Sep 8, 2022 •
edited by tiangolo

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

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
Sponsor Collaborator

@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

tests/test_starlette_exception.py Outdated Show resolved Hide resolved
@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

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

📝 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
Owner

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 tiangolo: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

@JarroVGIT JarroVGIT JarroVGIT approved these changes

@BilalAlpaslan BilalAlpaslan BilalAlpaslan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@iudeen @tiangolo @Kludex @JarroVGIT @BilalAlpaslan