-
-
Notifications
You must be signed in to change notification settings - Fork 890
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
Feature: Add size attribute to UploadFile #1405
Conversation
That's what I was thinking before reading the description (I've jumped into the code before reading the description). 🤔 |
Would there be any cons to making it a constructor parameter? Alternatively, do we maybe want to remove the unused Edit: I don't think that would work. UploadFile is used to store streaming data in multipart requests. We don't know the size of the field until after we create the UploadFile. What we could do is have: class UploadFile:
def __init__(
self,
file: Optional[BinaryIO] = None,
file_size: Optional[int] = None,
) -> None:
if file:
self.size = file_size or 0
else:
self.size = 0
async def write(self, data: bytes) -> None:
self.size += len(data)
... So for the edge case where the user provides a file like object, if they provide the size we use that as a starting point, otherwise we assume 0. |
Seeing that It shouldn't be a problem seeing how |
Agreed, I'm +1 for dropping the |
Only scenario for end users doing this that comes to my mind is people creating custom I'm actually doing that but I've resorted to |
Yep that's exactly the only scenario I can think of. So it wouldn't even break their production, it would break their tests, if anything at all? |
I opened #1406 to evaluate that as an option |
What's an example use-case? |
Other frameworks: Django: Use-case for having This logic is relatively painless to implement in Django forms, but requires some research in Starlette. Naturally this isn't here to protect app from huge uploads. There's NGINX in front of app with 32M payload limit in front of app, but if user uploads 2.1M and portrait photo limit is 2M, I'll rather handle this gracefully displaying them an error in app instead of NGINX's error page. |
I suppose we want to defer this to after #1406 - deps. on #1406 (comment) for instance. |
What's the conclusion? |
@NeurlAP its on the agenda for next meeting, which should happen next week. I'll post an update when there is one. |
@NeurlAP we've discussed the course of action for this issue and we've decided the following:
So if you want calculated file size you will do the |
#1413 should be merged soon. Step 3 above needs to happen afterward, jfyk. |
@rafalp I think you can work on this now :) |
I've updated the PR. Not sure if in tests we want to set initial size to literal value, or do |
Can we make size optional? It gives us a bit more flexibility as a toolkit. |
I've updated |
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.
I left one comment but I'm approving either way because we can always change Optional[int]
to int
without breaking anything.
if self.size is not None: | ||
self.size += len(data) |
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.
Hmm this is interesting. I know I just suggested making it a nullable parameter but I don't like how that creates complexity here. It makes me think that ultimately FormParser
should not be relying on UploadFile
to write the data; we should have a standalone async def write_in_thread(file: IO[bytes], data: bytes) -> None
, there's just no reason to expose that in the public API and then have to deal with these sorts of weird knock-on effects.
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.
TBH if we make it default to 0, we should have other way to communicate to the user that size attribute is potentially not trustworthy.
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.
Sorry, but I don’t think I really understood what you mean
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.
With current implementation size is None
means "there's no calculated information about the size of uploaded file". If we replace default size
from None
to 0
, then calls to write()
will increase it, even if initial value 0 was incorrect, potentially giving people wrong (too small) impression about real size of uploaded file.
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.
Yeah I agree with that. So do you think what we have here is ok?
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.
Yes. Extra complexity is minimal, but it carries value for people dealing with file uploads.
I'm going to wait for this PR to release 0.24.0, jfyk. |
@Kludex ok. I guess only thing missing before we can merge this is rebase on master. Unless we want to wait for more reviews? |
I've set the auto-merge - once the pipeline passes, it will be merged. I'm going to release Starlette 0.24.0 tonight. |
This PR adds
size
attribute toUploadFile
, enabling for easy retrieval of uploaded file's size in bytes without need for seeking file's end and telling its size. This is achieved by setting initialsize = 0
attribute on file and increasing it withinwrite
viaself.size += len(data)
.UploadFile
also supportsfile
argument for bytes stream. This seems like its used in tests only so I've resolved to writing naive size resolver that seeks stream's end and telling its size. This implementation is unlikely to work in production though, so maybe we should havesize
argument on constructor or makeasync def size(self) -> int
as getter instead?