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

em-http-request: do not alter non stubbed requests #1005

Closed

Conversation

ylecuyer
Copy link

Following this change: #936

We are now altering the header that are sent when using webmock and em-http-request in WebMock.allow_net_connect! mode

If you check here https://github.com/igrigorik/em-http-request/blob/34919d760b940dfd27159bbebce5c4dac9845eb6/lib/em-http/client.rb#L180 when forwarding the request to em-http-request, the send_request method expects some headers to be downcased (otherwise they are doubled)

This is an issue for example for Content-Length which would be interpreted as an array in the receiving webserver and not as a number.

The idea of the fix is to keep the raw_headers (not normalized by webmock) and use those raw_headers when forwarding the request

@bblimke
Copy link
Owner

bblimke commented Aug 13, 2023

Thank you for raising the issue and offering a solution @ylecuyer . While your solution works, the RequestSignature object should only be responsible for handing required information for matching the request and should not be used to carry additional data required for adapter implementation. Therefore I have introduced an alternative solution that still makes your spec pass: ef566e8 and all the changes stay inside the em_http_request adapter.

@bblimke bblimke closed this Aug 13, 2023
@ylecuyer
Copy link
Author

Thank you !

@ylecuyer ylecuyer deleted the do-not-alter-non-stubbed-requests-yle branch August 16, 2023 08:49
@ylecuyer ylecuyer restored the do-not-alter-non-stubbed-requests-yle branch August 16, 2023 08:49
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

2 participants