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

requests: Make possible to override all request headers and allow raw data upload #823

Closed
wants to merge 0 commits into from

Conversation

kapetan
Copy link
Contributor

@kapetan kapetan commented Mar 8, 2024

This removes all the hard-coded request headers from the requests module so they can be overridden by user provided headers dict.
Furthermore allow streaming request data without chunk encoding in those cases where content length is known but it's not desirable to load the whole content into memory. Also some servers (e.g. nginx) reject HTTP/1.0 requests with the Transfer-Encoding header set.
The change should be backwards compatible as long as the user hasn't provided any of the previously hard-coded headers.

@kapetan kapetan force-pushed the requests-headers branch from 040c4d1 to 80b21be Compare March 8, 2024 11:51
@dpgeorge
Copy link
Member

Thanks for the contribution, this change looks very reasonable.

The only concern is that it may use a little more memory, and make more allocations when it populates the headers dict. The way it works prior to this patch is that the write of the hard-coded header is basically allocation free.

@kapetan
Copy link
Contributor Author

kapetan commented Mar 19, 2024

I had the same though but choose to copy the auth behavior that was already there:

headers["Authorization"] = "Basic {}".format(formated)

But I can change it so it writes to the socket directly and doesn't populate the headers dict. Would that make sense?

@dpgeorge
Copy link
Member

I can change it so it writes to the socket directly and doesn't populate the headers dict.

How exactly would you do that, do you mean something like the current if "Host" not in headers: s.write(...)?

If you could do that without making it too complex, then that might be preferable. Maybe keep both versions so they can be compared.

@kapetan
Copy link
Contributor Author

kapetan commented Mar 19, 2024

This commit writes headers directly to the socket: kapetan@b3f795a

I think that could work.

I also added a test file where I mock the usocket module. The test file is not strictly necessary but I think it's nice to have. It's not possible to run the requests code with CPython so I couldn't use unittest. CPython doesn't allow mixing bytes and strings when formatting.

@jonfoster
Copy link
Contributor

As a side effect, this should fix #839.

@dpgeorge
Copy link
Member

Thanks @kapetan for following up with that alternative version.

But I think the version you have in this PR (creating a dict of headers to write out) is cleaner and easier to maintain (especially since we'll want to update to use HTTP/1.1).

Can you please add the test that you wrote for that alternative version to this PR? Then it should be good to merge.

@kapetan kapetan closed this Jun 12, 2024
@kapetan kapetan force-pushed the requests-headers branch from 8ec735c to 50ed36f Compare June 12, 2024 09:39
@kapetan
Copy link
Contributor Author

kapetan commented Jun 12, 2024

I closed this pull-request by accident so I created a new one with the original changes and the test file: #881

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.

3 participants