search

treeform / puppy

Puppy fetches via HTTP and HTTPS
MIT License
185 stars 27 forks source link

add content-length to post requests #47

Closed srozb closed 2 years ago

srozb commented 2 years ago

Currently puppy is not sending Content-Length header during POST requests. Such request will likely fail as server requires this header to be present if post data is to be received.

My implementation adds it during addDefaultHeaders call if it's not already set by user. Appropriate test was created as well.

guzba commented 2 years ago

Hello, thanks for the PR and issue report. I got curious about this since we were not adding the header manually (as you noticed), but I am seeing this header is being sent to the server. This would be because the system APIs we use on each platform are adding the header.

I added a test to confirm this here: https://github.com/treeform/puppy/pull/49 and you can see the tests passing here: https://github.com/treeform/puppy/runs/4307066585?check_suite_focus=true (this test would fail if the content-length header was missing since the response body would not match: https://github.com/treeform/puppy/blob/master/tests/test.nim#L119).

If you run this without adding the header manually, do you not see the content-length header? If you do not see it, what OS + version are you testing on?

srozb commented 2 years ago

I had some issues sending post requests using puppy and was 100% sure adding Content-Length header was a cure, but after another check it seems @guzba is right and the problem must have been somewhere else. I'm sorry for this useless PR.

I use puppy to communicate with REST API within Windows AD environment with kerberos authentication and got HTTP 400 Invalid Request received by client upon POST requests but it's all good now. Now I suspect it was due to missing/invalid Content-Type header. Anyway solved, sorry once again.

It's worth to notice $ proc is not printing this header even it's there and this might be confusing to puppy users. Perhaps some documentation clarification would help in future so it clearly states that what's printed by $ proc is not exactly what is being sent.

guzba commented 2 years ago

No problem at all, happy to have this is confirmed as working ok.

It's worth to notice $ proc is not printing this header even it's there and this might be confusing to puppy users. Perhaps some documentation clarification would help in future so it clearly states that what's printed by $ proc is not exactly what is being sent.

Good point here, it is misleading. I think we should not attempt to emulate what the HTTP request will look like since we cannot know exactly what the header order, formatting, etc will be when sent by the platform APIs. Instead, just simply log the basics: https://github.com/treeform/puppy/pull/50