-
-
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
Add support for new Starlette request.form() parameters max_files #9640
base: master
Are you sure you want to change the base?
Add support for new Starlette request.form() parameters max_files #9640
Conversation
5c25967
to
c2680db
Compare
📝 Docs preview for commit c2680db at: https://6481b17137b13e10e928e328--fastapi.netlify.app |
dd5934e
to
2d889ca
Compare
📝 Docs preview for commit 2d889ca at: https://6481b5a0e9cd8a1455412467--fastapi.netlify.app |
files = [("files", ("foo.txt", "foo"))] * 1500 | ||
response = client.post("/route-with-default-limit", files=files) | ||
# Fails with {'detail': 'Too many files. Maximum number of files is 1000.'} | ||
assert response.status_code == 400 |
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.
Why don't you assert against error details ? Like this is too broad.
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.
Sound sensible, will include! It is annoying to couple against the text in starlette
How about using Usage would be |
@adriangb Interesting! Will take a look at that approach From what I can see I believe this would be the first usage of the annotated-types library in FastAPI - I would feel hesitant to put forward such a dependency without it being the recommended approach |
It’s going to be a dependency anyway via Pydantic V2 |
2d889ca
to
29ad27f
Compare
@adriangb Ah great, that's good to know thanks! Do you know at all if FastAPI will start using them as a common pattern? |
Yes it will be, Sebastián is working on but not sure when he’ll be merging the PR with Pydantic V2, but I’m guessing before this PR |
📝 Docs preview for commit 6212e1f at: https://6486235a6038ce493d4f630f--fastapi.netlify.app |
Having the ability to do this would be useful for many usecases. Looking forward for updates. |
For my specific use case this is pretty important to me. Anyone aware of any workarounds? Any updates on this? |
any updates on this |
Hi, Basically, you need to set the max_fields & max_files in request.form on Starlette's Request object as below : Set max files / max fields
then on your route, use above function as Depend on body
Hope this helps |
Your solution looks to work, but it’s not clear how to combine it with this:
without losing the default behavior (pydantic type validations, auto documentation, etc..). So far the only option that comes to mind is: parse the form, create a pydantic model, perform validation, pass the fields directly to the method
But in my mind it looks cumbersome and ugly. Maybe I'm complicating things and there is a more correct and simpler way? |
Context:
max_files
torequest.form()
parameters and a default limit to limiting max files. See the changes here due to this advisoryChanges:
max_files
to the route decorators to set thisTests:
Questions and TODOs: