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

add content-length to post requests #47

Closed
wants to merge 1 commit into from

Conversation

srozb
Copy link
Contributor

@srozb srozb commented Nov 22, 2021

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
Copy link
Collaborator

guzba commented Nov 24, 2021

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: #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
Copy link
Contributor Author

srozb commented Nov 24, 2021

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
Copy link
Collaborator

guzba commented Nov 24, 2021

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: #50

@srozb srozb closed this Nov 24, 2021
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.

2 participants