micropython / micropython-lib

Core Python libraries ported to MicroPython
Other
2.39k stars 994 forks source link

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

Closed kapetan closed 3 months ago

kapetan commented 6 months ago

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.

dpgeorge commented 6 months ago

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 commented 6 months ago

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?

dpgeorge commented 6 months ago

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 commented 6 months ago

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.

jonfoster commented 6 months ago

As a side effect, this should fix #839.

dpgeorge commented 3 months ago

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 commented 3 months ago

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