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 removing body from status codes that do not support it #5145

Merged
merged 2 commits into from Jul 14, 2022

Conversation

tiangolo
Copy link
Owner

@tiangolo tiangolo commented Jul 14, 2022

🐛 Fix removing body from status codes that do not support it

This is related to this issue here: #4050

Related to this issue in Starlette: encode/starlette#1764

And related to this PR in Starlette: encode/starlette#1765

This removes fastapi.openapi.constants.STATUS_CODES_WITH_NO_BODY, it is replaced by a function in utils.

@codecov
Copy link

codecov bot commented Jul 14, 2022

Codecov Report

Merging #5145 (ca239a6) into master (a0fd613) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #5145   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          532       532           
  Lines        13672     13684   +12     
=========================================
+ Hits         13672     13684   +12     
Impacted Files Coverage Δ
fastapi/openapi/constants.py 100.00% <ø> (ø)
fastapi/openapi/utils.py 100.00% <100.00%> (ø)
fastapi/routing.py 100.00% <100.00%> (ø)
fastapi/utils.py 100.00% <100.00%> (ø)
tests/test_response_code_no_body.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0fd613...ca239a6. Read the comment docs.

@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

📝 Docs preview for commit ca239a6 at: https://62cffb47f3a67d4fbe8539c7--fastapi.netlify.app

@Kyle-sandeman-mrdfood
Copy link

Kyle-sandeman-mrdfood commented Aug 18, 2022

Good day, I may file an issue if you'd like but this commit introduces a regression

Previously you could do this

responses = {"4XX": {"Model": SomeModel}}
app = FastAPI(..., responses=responses)

As per https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.2.md#patterned-fields-1

As of this commit/PR it throws an error upon adding a new router:

Traceback (most recent call last):
  File "service.py", line 82, in <module>
    app.include_router(auth_router, prefix=f'{BASE}/auth')
  File "/usr/local/lib/python3.8/site-packages/fastapi/applications.py", line 420, in include_router
    self.router.include_router(
  File "/usr/local/lib/python3.8/site-packages/fastapi/routing.py", line 734, in include_router
    self.add_api_route(
  File "/usr/local/lib/python3.8/site-packages/fastapi/routing.py", line 561, in add_api_route
    route = route_class(
  File "/usr/local/lib/python3.8/site-packages/fastapi/routing.py", line 418, in __init__
    assert is_body_allowed_for_status_code(
  File "/usr/local/lib/python3.8/site-packages/fastapi/utils.py", line 24, in is_body_allowed_for_status_code
    current_status_code = int(status_code)
ValueError: invalid literal for int() with base 10: '4XX'

(similar to #5166 (comment))
I can confirm this works in fastapi[all]~=0.75.0 (which I have pinned in another repo)

@tiangolo
Copy link
Owner Author

Hey @Kyle-sandeman-mrdfood, I just tried a lot of things to replicate your problem, but I can't seem to make it "break".

Maybe it was fixed in a recent release. If you are still having problems, please create a new issue.

Here's the example app I used, based on the docs to extend OpenAPI responses and adding a 5XX group of responses.

from typing import Union

from fastapi import FastAPI, APIRouter
from fastapi.responses import FileResponse
from pydantic import BaseModel


class Item(BaseModel):
    id: str
    value: str


responses = {
    404: {"description": "Item not found"},
    302: {"description": "The item was moved"},
    403: {"description": "Not enough privileges"},
    "5XX": {"description": "Server errors"},
}


app = FastAPI()
router = APIRouter()

@router.get(
    "/items/{item_id}",
    response_model=Item,
    responses={**responses, 200: {"content": {"image/png": {}}}},
)
async def read_item(item_id: str, img: Union[bool, None] = None):
    if img:
        return FileResponse("image.png", media_type="image/png")
    else:
        return {"id": "foo", "value": "there goes my hero"}


app.include_router(router)

@Kyle-sandeman-mrdfood
Copy link

Hi again, it may be related to the model parameter of the responses.
I have tested the below on 0.79.1 and 0.81.0

from typing import Union

from fastapi import FastAPI, APIRouter
from fastapi.responses import FileResponse
from pydantic import BaseModel


class Item(BaseModel):
    id: str
    value: str


responses = {
    404: {"description": "Item not found"},
    302: {"description": "The item was moved"},
    403: {"description": "Not enough privileges"},
    "5XX": {"description": "Server errors", "model": Item},
}


app = FastAPI(responses=responses)
router = APIRouter()

@router.get(
    "/items/{item_id}",
    response_model=Item,
)
async def read_item(item_id: str, img: Union[bool, None] = None):
    if img:
        return FileResponse("image.png", media_type="image/png")
    else:
        return {"id": "foo", "value": "there goes my hero"}

app.include_router(router)

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

2 participants