async-http-faraday icon indicating copy to clipboard operation
async-http-faraday copied to clipboard

Ignore Content-Length from env.request_headers

Open pilaf opened this issue 9 months ago • 3 comments

Closes #51

This makes the adapter ignore Content-Length if given in Faraday's env.request_headers, but only if capitalized, since Async::HTTP will set content-length anyway.

I'd like to add tests too, but I wasn't sure how to test what headers end up in the final request without setting up a dummy server to receive them. Any suggestions?

pilaf avatar Feb 24 '25 07:02 pilaf

@ioquatix Sorry for the delay, please check the latest commit, I think it mostly addresses your comments.

Again I couldn't write tests for it, but I did test that this code works separately.

pilaf avatar Mar 09 '25 06:03 pilaf

Thanks, this looks like what I was thinking of, I'll try to add some tests, and if it looks good we should be able to move forward.

ioquatix avatar Mar 09 '25 07:03 ioquatix

This is perfect, thanks.

ioquatix avatar Mar 10 '25 20:03 ioquatix

I ran into this issue myself today, so just came here to say thanks in advance 😅 and let me know if I can help with anything to move things forward! 🙏

balvig avatar Jun 30 '25 03:06 balvig

@balvig @ioquatix AFAIK this is only missing tests, but I'm still unsure how to best approach that. If there were any good ideas I could give it a shot at implementing them.

pilaf avatar Jun 30 '25 05:06 pilaf

Apologies, this one slipped through the cracks. I'll revisit it.

ioquatix avatar Jun 30 '25 09:06 ioquatix

Thanks everyone, I re-reviewed it and it looks good. Will do some more extensive testing locally.

ioquatix avatar Jun 30 '25 23:06 ioquatix

For reference, the original test case in the bug report no longer works because https://github.com/lostisland/faraday/issues/1614 is fixed.

ioquatix avatar Jun 30 '25 23:06 ioquatix

I made it an error to specify singleton headers more than once, and now it fails:

> bundle exec sus
  0.0s     warn: Async::Task: Reading HTTP/1.1 requests for Async::HTTP::Protocol::HTTP1::Server. [oid=0x718] [ec=0x720] [pid=981323] [2025-07-01 21:35:34 +1200]
               | Task may have ended with unhandled exception.
               |   Protocol::HTTP::DuplicateHeaderError: Duplicate singleton header key: "content-length"
               |   → /home/samuel/.gem/ruby/3.4.1/gems/protocol-http-0.51.0/lib/protocol/http/headers.rb:336 in 'Protocol::HTTP::Headers#merge_into'
               |     /home/samuel/.gem/ruby/3.4.1/gems/protocol-http-0.51.0/lib/protocol/http/headers.rb:356 in 'block in Protocol::HTTP::Headers#to_h'
               |     /home/samuel/.gem/ruby/3.4.1/gems/protocol-http-0.51.0/lib/protocol/http/headers.rb:355 in 'Array#each'
               |     /home/samuel/.gem/ruby/3.4.1/gems/protocol-http-0.51.0/lib/protocol/http/headers.rb:355 in 'Enumerable#inject'
               |     /home/samuel/.gem/ruby/3.4.1/gems/protocol-http-0.51.0/lib/protocol/http/headers.rb:355 in 'Protocol::HTTP::Headers#to_h'
               |     /home/samuel/.gem/ruby/3.4.1/gems/protocol-http-0.51.0/lib/protocol/http/headers.rb:348 in 'Protocol::HTTP::Headers#[]'
               |     /home/samuel/.gem/ruby/3.4.1/gems/protocol-http1-0.34.0/lib/protocol/http1/connection.rb:169 in 'Protocol::HTTP1::Connection#persistent?'
               |     /home/samuel/.gem/ruby/3.4.1/gems/protocol-http1-0.34.0/lib/protocol/http1/connection.rb:401 in 'Protocol::HTTP1::Connection#read_request'
               |     /home/samuel/.gem/ruby/3.4.1/gems/async-http-0.89.0/lib/async/http/protocol/http1/request.rb:26 in 'Async::HTTP::Protocol::HTTP1::Request.read'
               |     /home/samuel/.gem/ruby/3.4.1/gems/async-http-0.89.0/lib/async/http/protocol/http1/server.rb:49 in 'Async::HTTP::Protocol::HTTP1::Server#next_request'
               |     /home/samuel/.gem/ruby/3.4.1/gems/async-http-0.89.0/lib/async/http/protocol/http1/server.rb:66 in 'Async::HTTP::Protocol::HTTP1::Server#each'
               |     /home/samuel/.gem/ruby/3.4.1/gems/async-http-0.89.0/lib/async/http/server.rb:50 in 'Async::HTTP::Server#accept'
               |     /home/samuel/.gem/ruby/3.4.1/gems/io-endpoint-0.15.2/lib/io/endpoint/wrapper.rb:216 in 'block (2 levels) in IO::Endpoint::Wrapper#accept'
               |     /home/samuel/.gem/ruby/3.4.1/gems/async-2.25.0/lib/async/task.rb:201 in 'block in Async::Task#run'
               |     /home/samuel/.gem/ruby/3.4.1/gems/async-2.25.0/lib/async/task.rb:439 in 'block in Async::Task#schedule'
24 passed 1 errored out of 25 total (132 assertions)
🏁 Finished in 2.7s; 49.787 assertions per second.
🐢 Slow tests:
	992.7ms: describe Async::HTTP::Faraday::Adapter with a remote http server it can use a multi-part post body test/async/http/faraday/adapter.rb:270
	559.9ms: describe Async::HTTP::Faraday::Adapter with a remote http server it works without top level reactor test/async/http/faraday/adapter.rb:256
	472.2ms: describe Async::HTTP::Faraday::Adapter with a remote http server it works without initial url and trailing slash (compatiblisity to the original behaviour) test/async/http/faraday/adapter.rb:262
	409.7ms: describe Async::HTTP::Faraday::Adapter with a remote http server it can get remote resource test/async/http/faraday/adapter.rb:248
	103.8ms: describe Async::HTTP::Faraday::Adapter with a local http server with a slow response it client can get resource test/async/http/faraday/adapter.rb:145

🔥 Errored assertions:
describe Async::HTTP::Faraday::Adapter with a local http server it client can post resource test/async/http/faraday/adapter/post.rb:23
	⚠ Faraday::ConnectionFailed: EOFError

ioquatix avatar Jul 01 '25 09:07 ioquatix

I added a test for this: https://github.com/socketry/async-http-faraday/commit/9ecca0ae9a576dd8f8f083b94d983ef5d3055ed6

ioquatix avatar Jul 01 '25 09:07 ioquatix

@ioquatix Awesome!

pilaf avatar Jul 01 '25 09:07 pilaf