micropython-lib icon indicating copy to clipboard operation
micropython-lib copied to clipboard

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

Open kapetan opened this issue 11 months ago • 5 comments

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 avatar Mar 08 '24 11:03 kapetan

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.

dpgeorge avatar Mar 19 '24 06:03 dpgeorge

I had the same though but choose to copy the auth behavior that was already there: https://github.com/micropython/micropython-lib/blob/80b21be6f4acadf6132b22129b90309991c93160/python-ecosys/requests/requests/init.py#L59

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

kapetan avatar Mar 19 '24 08:03 kapetan

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.

dpgeorge avatar Mar 19 '24 11:03 dpgeorge

This commit writes headers directly to the socket: https://github.com/kapetan/micropython-lib/commit/b3f795abc32e8b3a044b9e3714c717defb347443

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.

kapetan avatar Mar 19 '24 20:03 kapetan

As a side effect, this should fix #839.

jonfoster avatar Apr 03 '24 17:04 jonfoster

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.

dpgeorge avatar Jun 12 '24 04:06 dpgeorge

I closed this pull-request by accident so I created a new one with the original changes and the test file: https://github.com/micropython/micropython-lib/pull/881

kapetan avatar Jun 12 '24 10:06 kapetan