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

keeping case for HTTP headers #34

Closed
wants to merge 0 commits into from
Closed

keeping case for HTTP headers #34

wants to merge 0 commits into from

Conversation

Guatom
Copy link
Contributor

@Guatom Guatom commented Oct 25, 2016

Dear reviewer:

I'll start by saying that this is "fix" for a problem I have related to the SOAP web services I need to invoke. My main problem is that http (via request) lowercases the header names, and the web services I invoke have a case-sensitive validation on http headers:

https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L365

I could try to remove that, but I have a strong suspicion that a PR modifying that would be rejected since the HTTP standard states that the HTTP header name validation should be case insensitive:

https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html

I know those WS are not doing what's right, but I'd be surprised if I were the first one having this problem.

Thanks for reading and reviewing.

@slnode
Copy link

slnode commented Oct 25, 2016

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@slnode
Copy link

slnode commented Oct 25, 2016

Can one of the admins verify this patch?

@raymondfeng
Copy link
Contributor

@Guatom Thank you for the patch. A few comments:

  1. Based on https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L365, the header names will be always converted to lower case. How can we keep case-sensitive header names in strong-soap?
  2. Your change allows http headers to be passed from options. I don't understand why it helps casing.
  3. Please separate code style changes (if needed) in separate commits.
  4. Can you add a test case so that we can better understand your proposal and the code have better test coverage?

@Guatom Guatom closed this Oct 26, 2016
@slnode
Copy link

slnode commented Oct 26, 2016

Can one of the admins verify this patch?

@Guatom
Copy link
Contributor Author

Guatom commented Oct 26, 2016

@raymondfeng: I'll be submitting a detailed explanation of everything.

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.

None yet

4 participants