Document and type annotate UploadFile as a bytes-only interface. Not bytes or text. #1312
Conversation
| ) -> None: | ||
| self.filename = filename | ||
| self.content_type = content_type | ||
| if file is None: | ||
| file = tempfile.SpooledTemporaryFile(max_size=self.spool_max_size) | ||
| self.file = file | ||
| self.file = tempfile.SpooledTemporaryFile(max_size=self.spool_max_size) # type: ignore # noqa: E501 |
There was a problem hiding this comment.
SpooledTemporaryFile isn't fully typed, see python/typeshed#1780
|
It looks like tests are failing due to missing coverage, which is because I split up the branch on line 428, which I only did because type checkers didn't like assigning SpooledTemporaryFile to BytesIO (as per #1312 (comment)) So I guess the thing to do here is add a test for that branch? |
|
Why is this a bug? Documentation needs to be fixed in case this is a bug: https://www.starlette.io/requests/#request-files, jfyk |
|
Hmm, good point. I was looking at this from the perspective of actual usage within Starlette. starlette/starlette/formparsers.py Line 225 in 6af5c51 The default file is also opened in bytes mode: starlette/starlette/datastructures.py Line 425 in 6af5c51 Finally, I don't see anywhere in the tests where UploadFile.write or UploadFile.read are called with str.
So I assumed these were just bogus type annotations. |
adriangb
changed the title
|
I pushed commits to:
I also renamed the issue from a bug to a request for deprecation (technically a breaking change, although I doubt anyone was using UploadFile directly w/ str). Also, this is just type hints and docs, so even if it is a breaking change it isn't going to break anyone's code aside from static type checking. |
|
@tomchristie I'm curious if you remember why UploadFile accepts strings in the first place? Like I mentioned above I don't see anywhere in Starlette where it is used that way, and I can't think of a reason why it would. |
| @pytest.mark.anyio | ||
| async def test_upload_file_file_input(): | ||
| """Test passing file/stream into the UploadFile constructor""" | ||
| stream = io.BytesIO(b"data") | ||
| file = UploadFile(filename="file", file=stream) | ||
| assert await file.read() == b"data" | ||
| await file.write(b" and more data!") | ||
| assert await file.read() == b"" | ||
| await file.seek(0) | ||
| assert await file.read() == b"data and more data!" |
There was a problem hiding this comment.
Also this, I don't see anywhere else where the file parameter to UploadFile is being used. It makes sense as an attribute, but we're not using it in the constructor for testing or anything. But that would really be a breaking change (instead of being just type annotations, like this PR) so it probably doesn't make sense to change.
|
It's not really clear to me what's motiving this change. We have str and bytes here because files might be in text or binary mode. It's possible that we only end up using binary mode within starlette, but it'd be good to have a better idea from an end-user / API perspective why the change is valuable. (Since all change also risks unintended breakages) |
|
I view it as a breaking change because:
But like I said, in practice I don't think this would break anything for anyone.
Are you referring to general file-like-object?
I think it's valuable for 2 reasons:
another option would be to make upload file a generic class and then annotate it as UploadFile[bytes] in FormData. But my understanding is thst you don't want to introduce generics to the codebase, so I did not consider that. |
There was a problem hiding this comment.
I'm fine with this yup.
I think it could do with a slight change of title perhaps, and I don't consider it a breaking change. (the typing is tightened up, but the behaviour isn't actually changed.)
Eg. "Document and type annotate UploadFile as a bytes-only interface. Not bytes or text."
adriangb
changed the title
|
Done @tomchristie , and synced with the main branch |
No description provided.