Skip to content
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

Allow str content for multipart upload files #2400

Merged
merged 1 commit into from Oct 6, 2022

Conversation

tomchristie
Copy link
Member

@tomchristie tomchristie commented Oct 6, 2022

Closes #2069.

Allow str for simple multipart uploads...

httpx.post(url, files={"upload": "some content"})

However we (continue to) explicitly disallow io.StringIO and files opened in text mode, since we can't upfront determine their byte-length.

I think this is probably a pretty okay middle-ground, supporting the simple text-use-case, while enforcing bytes for the file-like cases. Not a breaking change, since we're expanding our supported cases here.


Edit: This change enforces the constraint that upload files must be opened in binary mode. That's the right thing to do but I'm unclear why I hadn't called it out explicitly in this description.

@tomchristie tomchristie requested a review from a team October 6, 2022 16:04
Copy link
Member

@adriangb adriangb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me! Do we just encode it as utf-8?

@tomchristie
Copy link
Member Author

Makes sense to me! Do we just encode it as utf-8?

Yup.

@tomchristie tomchristie merged commit 0ebe925 into master Oct 6, 2022
@tomchristie tomchristie deleted the allow-plain-str-for-multipart-uploads branch October 6, 2022 16:53
@tomchristie tomchristie mentioned this pull request Nov 17, 2022
@euri10
Copy link
Member

euri10 commented Nov 18, 2022

I think this is probably a pretty okay middle-ground, supporting the simple text-use-case, while enforcing bytes for the file-like cases. Not a breaking change, since we're expanding our supported cases here.

I think this is a breaking change, at least it breaks my test suite with

        if "b" not in getattr(fileobj, "mode", "b"):
>           raise TypeError(
                "Multipart file uploads must be opened in binary mode, not text mode."
            )
E           TypeError: Multipart file uploads must be opened in binary mode, not text mode.

this is the snippet that was previously working:

    files: List[Tuple[str, IO]] = [
        ("files", (open(Path(__file__).parent / "data_upload" / "ls" / ls_file)))
        for ls_file in to_upload
    ]
    response = await client.post(
        "/uploads/ls",
        files=files,
        headers={"Authorization": f"Bearer {token}"},
    )

@tomchristie
Copy link
Member Author

Okay. Error on my part - we shouldn't have issued that as a minor point release.
Still, we're here now.

Make sure to open upload files in binary mode.

We have that as a constraint since it ensures that you're uploading the actual binary file content, rather than decoding it into text and then re-encoding it into bytes when it goes out onto the wire. This also means we can correct determine the content length in advance.

    files: List[Tuple[str, IO]] = [
        ("files", (open(Path(__file__).parent / "data_upload" / "ls" / ls_file, "rb")))  # Added "rb" mode.
        for ls_file in to_upload
    ]
    response = await client.post(
        "/uploads/ls",
        files=files,
        headers={"Authorization": f"Bearer {token}"},
    )

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.

The code in the document does not work
3 participants