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.
As far as I can tell, UploadFile is only ever used in formparsers.py, where only bytes are written to it:
https://github.com/encode/starlette/blob/6af5c515e0a896cbf3f86ee043b88f6c24200bcf/starlette/formparsers.py#L225
The default file is also opened in bytes mode:
https://github.com/encode/starlette/blob/6af5c515e0a896cbf3f86ee043b88f6c24200bcf/starlette/datastructures.py#L425
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.
But I guess if it is in the docs, then this is not a bug report and instead is a request to deprecate the functionality of using str with UploadFile since it's is not used by Starlette and is not tested anywhere.
This would make it easier to use UploadFile in a typed environment since currently every time you call UploadFile.read you have to do assert isinstance(data, bytes) or a cast to make type checkers happy.

I pushed commits to:

• Add the missing test for coverage (again, this is just testing something that was previously untested but was not being counted towards coverage because branch coverage is not being used)
• Updates the docs to reflect the change

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.

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:

1. It changes the docs
2. It changes type annotations which might break user's tests if they are using static type checkers.

But like I said, in practice I don't think this would break anything for anyone.

We have str and bytes here because files might be in text or binary mode.

Are you referring to general file-like-object?
To me if UploadFile is only ever used in binary mode, it should be typed as such. I do not see the value in treating it as if it could be a file in text mode if in practice it is always going to be binary mode.

it'd be good to have a better idea from an end-user / API perspective why the change is valuable

I think it's valuable for 2 reasons:

1. Saves boilerplate for users that run MyPy on their codebase. Currently, we have to do assert isinstance(await file.read(), UploadFile) or cast(...) or # type: ignore
2. Simpler documentation and cognitive load. I for one was pretty confused from the documentation because I was trying to figure out in what situation I'd be getting back strings.

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.

Done @tomchristie , and synced with the main branch