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 support for strings in OpenAPI status codes: default, 1XX, 2XX, 3XX, 4XX, 5XX #5187

Merged

Conversation

JarroVGIT
Copy link
Contributor

@JarroVGIT JarroVGIT commented Jul 23, 2022

Related to #5166

@codecov
Copy link

codecov bot commented Jul 23, 2022

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (d66a851) compared to base (5c576e4).
Patch coverage: 100.00% of modified lines in pull request are covered.

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     
Impacted Files Coverage Δ
fastapi/utils.py 100.00% <100.00%> (ø)
tests/test_additional_responses_router.py 100.00% <100.00%> (ø)
tests/main.py 100.00% <0.00%> (ø)
tests/utils.py 100.00% <0.00%> (ø)
fastapi/routing.py 100.00% <0.00%> (ø)
fastapi/encoders.py 100.00% <0.00%> (ø)
tests/test_query.py 100.00% <0.00%> (ø)
fastapi/websockets.py 100.00% <0.00%> (ø)
fastapi/concurrency.py 100.00% <0.00%> (ø)
... and 24 more

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

fastapi/utils.py Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

📝 Docs preview for commit 85013ce at: https://62defab160c28910ef24b405--fastapi.netlify.app

@github-actions
Copy link
Contributor

📝 Docs preview for commit 97e9dcb at: https://62defcc48287b21398bc80e5--fastapi.netlify.app

@JarroVGIT
Copy link
Contributor Author

@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?

@github-actions
Copy link
Contributor

📝 Docs preview for commit 2973bfd at: https://62e6cf08c322be1653055c67--fastapi.netlify.app

@JarroVGIT JarroVGIT force-pushed the allow-for-default-response-openapi branch from 2973bfd to 97e9dcb Compare July 31, 2022 18:51
@github-actions
Copy link
Contributor

📝 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 [
Copy link
Contributor

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?

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.

@Kyle-sandeman-mrdfood
Copy link

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

@github-actions
Copy link
Contributor

📝 Docs preview for commit a88f50c at: https://6349c8f8dea29b00a780a286--fastapi.netlify.app

@tiangolo tiangolo changed the title allow for 'default' in response in OpenAPI json 🐛 Fix support for strings in OpenAPI status codes: default, 1XX, 2XX, 3XX, 4XX, 5XX Oct 14, 2022
@tiangolo tiangolo merged commit 0ae8db4 into tiangolo:master Oct 14, 2022
@tiangolo
Copy link
Owner

Thanks for the contribution @JarroVGIT! 🍰

And thanks for the input here everyone! ☕

I updated it a bit to include 1XX and remove lower case versions to keep in line with the spec. This will be available in the next release, in the next hours. 🚀

@@ -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
Copy link
Sponsor Collaborator

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.

Copy link
Owner

Choose a reason for hiding this comment

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

Right!

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

9 participants