Skip to content

Comments

fix: make MultiPartParser read default max_part_size from the class attr#2915

Closed
reith wants to merge 2 commits intoKludex:masterfrom
reith:master
Closed

fix: make MultiPartParser read default max_part_size from the class attr#2915
reith wants to merge 2 commits intoKludex:masterfrom
reith:master

Conversation

@reith
Copy link

@reith reith commented Apr 1, 2025

Summary

In FastAPI, the only way to change the maximum part size in a multipart request is to override MultiPartParser.max_part_size globally. That has stopped working since the changes introduced by this commit. I think the change in this PR is benign to keep. Even without considering compatibility with FastAPI, I think falling back to the class attribute to read the default value is a good idea but I don't mind if you think that problem should be addressed separately by FastAPI.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@Kludex
Copy link
Owner

Kludex commented Apr 2, 2025

The class attributes should be removed, they are still there by mistake.

FastAPI should be smart about this.

@Kludex
Copy link
Owner

Kludex commented Apr 2, 2025

That said... I understand it can be a bit annoying to have to wait for FastAPI...

I think we need a constant DEFAULT_MAX_SIZE_... or something like that, which will allow users to change the behavior while waiting for FastAPI.

@reith
Copy link
Author

reith commented Apr 3, 2025

I ended up getting the raw request in my handler and using the Starlette's Request API there to change the part size limit. That’s better than changing the default since I perform authorization in a middleware, and it makes more sense not to raise the limit for unauthenticated requests.

Thanks for being considerate, though!

@reith reith closed this Apr 3, 2025
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.

2 participants