-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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 support for strings in OpenAPI status codes: default
, 1XX
, 2XX
, 3XX
, 4XX
, 5XX
#5187
🐛 Fix support for strings in OpenAPI status codes: default
, 1XX
, 2XX
, 3XX
, 4XX
, 5XX
#5187
Conversation
Codecov ReportBase: 100.00% // Head: 100.00% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## master #5187 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 532 540 +8
Lines 13684 13946 +262
==========================================
+ Hits 13684 13946 +262
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
📝 Docs preview for commit 85013ce at: https://62defab160c28910ef24b405--fastapi.netlify.app |
📝 Docs preview for commit 97e9dcb at: https://62defcc48287b21398bc80e5--fastapi.netlify.app |
@tiangolo I know you are a busy man, but 0.79 unfortunately introduced a regression. This is fixed with this commit, and I've written the test case to prove it. Perhaps if you have the time, you could glance over it and produce a minor update? |
📝 Docs preview for commit 2973bfd at: https://62e6cf08c322be1653055c67--fastapi.netlify.app |
2973bfd
to
97e9dcb
Compare
📝 Docs preview for commit 97e9dcb at: https://62e6d001c9d3741248ac8c59--fastapi.netlify.app |
fastapi/utils.py
Outdated
@@ -21,6 +21,18 @@ | |||
def is_body_allowed_for_status_code(status_code: Union[int, str, None]) -> bool: | |||
if status_code is None: | |||
return True | |||
if status_code in [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it'd be more efficient to have a set here instead of list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be also more consistent below where its also doing set-based check. So here using instead:
# https://swagger.io/docs/specification/describing-responses/#default
# https://swagger.io/docs/specification/describing-responses/#status-codes
if status_code in {
"default",
"2XX",
"2xx",
"3XX",
"3xx",
"4XX",
"4xx",
"5XX",
"5xx",
}:
return True
Also adding a comment where these come from would be a good idea.
Any status update on this PR? For our use case we have to keep the library pinned on an older version until this is merged. Or reduce the quality of our docs |
📝 Docs preview for commit a88f50c at: https://6349c8f8dea29b00a780a286--fastapi.netlify.app |
default
, 1XX
, 2XX
, 3XX
, 4XX
, 5XX
Thanks for the contribution @JarroVGIT! 🍰 And thanks for the input here everyone! ☕ I updated it a bit to include |
@@ -21,6 +21,16 @@ | |||
def is_body_allowed_for_status_code(status_code: Union[int, str, None]) -> bool: | |||
if status_code is None: | |||
return True | |||
# Ref: https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#patterned-fields-1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FastAPI supports only 3.0.2, not 3.1.0.
Both versions are compliant with this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right!
Related to #5166