-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
🐛 Ensure handlers receive fastapi.UploadFile instances, not Starlette's
#13605
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
base: master
Are you sure you want to change the base?
Conversation
45cc706 to
0b51a5e
Compare
fastapi.UploadFile instances, not Starlette's
This comment was marked as resolved.
This comment was marked as resolved.
f181f15 to
a27771d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
a27771d to
5b820bd
Compare
5b820bd to
4e49249
Compare
4e49249 to
06ec400
Compare
56e5dc7 to
cbc18c6
Compare
cbc18c6 to
f9ad724
Compare
This comment was marked as resolved.
This comment was marked as resolved.
f9ad724 to
687442f
Compare
This comment was marked as resolved.
This comment was marked as resolved.
687442f to
6a7d0fb
Compare
This comment was marked as resolved.
This comment was marked as resolved.
6a7d0fb to
1293cac
Compare
This comment was marked as resolved.
This comment was marked as resolved.
1293cac to
7e29c99
Compare
7e29c99 to
480b267
Compare
480b267 to
bc9ad12
Compare
|
@tiangolo: Do you think this is an easy enough bugfix you could merge? |
bc9ad12 to
ecfbcc2
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
YuriiMotov
left a comment
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.
LGTM!
This comment was marked as resolved.
This comment was marked as resolved.
|
This pull request has a merge conflict that needs to be resolved. |
79f225f to
2234be9
Compare
Merging this PR will not alter performanceSummary
Comparing Footnotes
|
To start, thank you all for FastAPI! It's been a a joy to use FastAPI at $DAYJOB.
The problem
While writing a unit test for an endpoint that handles a file, I stumbled across a discrepancy in what FastAPI documentation describes and what actually gets passed to the handler (a
starlette.datastructures.UploadFileviastarlette.requests.Request.form).The coding standard at $DAYJOB includes typeguard (via the
pytest-typeguardplugin). In the unit test, typeguard enforcement fails because the upload file was not a FastAPI instance, but a Starlette instance. (Of course, bypassing typeguard allows the tests to pass, as expected.)A quick look at the implementation reveals the
UploadFilecreation is handled bystarlette.requestsand then passed to FastAPI'srun_function_endpoint, without any transforms to afastapi.datastructures.UploadFile. Thankfully, in practice, this doesn't appear to be an issue at runtime, since the FastAPI UploadFile merely extends the Starlette UploadFile to add Pydantic support.A solution
UploadFile.from_starletteclassmethod is added to transform from aStarletteUploadFileto Starlette'sUploadFile. FastAPI is already aware of the Starlette UploadFile implementation because it's extending it, so adding the transform to the FastAPI class felt like adding a missing puzzle piecefastapi.routing.run_endpoint_functionto check thevaluesdict for non-FastAPIUploadFiles and convert them prior to handling off to any endpoint functionThanks again!
I look forward to feedback!