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

fix: curl async http client empty-valued header #2953

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

curme
Copy link

@curme curme commented Nov 11, 2020

Behavior

I found that some of the requests I sent by tornado would lose part of headers, exactly the empty-valued ones.

For example, when posting a request with headers {'Content-Type': "application/json", 'Authorization': ""}, only header Content-Type will be sent out actually, with the header Authorization dropped because it's empty.

For some requests, header with blank content is indispensable. In my case, I met an odd server must make sure there is an Authorization in headers even it is blank.

Reason

The behavior caused by the way curl-http-client constructing headers, which converting all of headers in "%s: %s" format. The headers having no value, such as 'xxxx: ', would be removed by cURL.

To keep those headers in request, the colon should be replaced with semicolon, such as 'xxxx;'. libcurl examples

Copy link
Member

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This functionality needs a test to demonstrate that it's working correctly (and curl is translating the semicolon into a colon for the server's consumption). A test in httpclient_test would also verify that simple_httpclient is handling this edge case correctly.

It would be nice to link to the docs for CURLOPT_HTTPHEADER because the syntax used by this feature is quite surprising.

@@ -351,6 +351,7 @@ def _curl_setup_request(
pycurl.HTTPHEADER,
[
"%s: %s" % (native_str(k), native_str(v))
if v is not '' else "%s;" % native_str(k)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be != (or just if v), not is not. The use of is here is relying on interning of small strings which I believe is an undocumented implementation detail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants