Ignore Content-Length from env.request_headers#52
Conversation
lib/async/http/faraday/adapter.rb
Outdated
| end | ||
|
|
||
| if headers = env.request_headers | ||
| # Ignore Content-Length if given, it will be set for us later anyway (lowercased) |
There was a problem hiding this comment.
This is looking good.
I'd suggest extracting this into length and then assigning to BodyReadWrapper, which should have this:
class BodyReadWrapper
def initialize(body, length = nil, block_size: 4096)
@body = body
@length = length
@block_size = block_size
end
# ...
def length
@length # if known
end
You'll need to move the if headers = env.request_headers first to extract a local variable if possible.
Is headers.key?(...) case sensitive?
Testing is a bit tricky, but maybe we can mock the client. If you can get the code updated, I can help write the tests.
There was a problem hiding this comment.
Is headers.key?(...) case sensitive?
Yes. I'd change this to check for both Content-Length and content-length if the intent is to use that value for BodyReadWrapper#length, if you agree.
There was a problem hiding this comment.
It might be easier, for a first pass, to construct headers = ::Protocol::HTTP::Headers[headers] first, and then length = headers.delete("content-length").
lib/async/http/faraday/adapter.rb
Outdated
| body = ::Protocol::HTTP::Body::Buffered.wrap(body).then do |buffered| | ||
| ::Protocol::HTTP::Body::Buffered.new(buffered.chunks, content_length) |
There was a problem hiding this comment.
This double initialization is ugly but I had no choice since Protocol::HTTP::Body::Buffered.wrap doesn't take a length argument.
There was a problem hiding this comment.
I understand how you are thinking about the problem.
A buffered body can compute its size accurately and efficiently from the chunks. That's different from a readable body where we can only call #read to stream the data.
Therefore, I wouldn't worry about this code path - just leave it as the original implementation.
| if body.is_a?(::Protocol::HTTP::Body::Readable) | ||
| # Good to go |
There was a problem hiding this comment.
Should this case try to rebuild the body with the given length too?
There was a problem hiding this comment.
No I wouldn’t worry about this case either, it’s expected the body would know its size already if appropriate.
The main issue we are addressing is Faradays opaque “body#read” which may have length but the only way to know that is via the supplied header.
|
@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. |
|
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. |
|
This is perfect, thanks. |
|
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! 🙏 |
|
Apologies, this one slipped through the cracks. I'll revisit it. |
|
Thanks everyone, I re-reviewed it and it looks good. Will do some more extensive testing locally. |
|
For reference, the original test case in the bug report no longer works because lostisland/faraday#1614 is fixed. |
|
I made it an error to specify singleton headers more than once, and now it fails: |
|
I added a test for this: 9ecca0a |
|
@ioquatix Awesome! |
3 participants
Closes #51
This makes the adapter ignore
Content-Lengthif given in Faraday'senv.request_headers, but only if capitalized, sinceAsync::HTTPwill setcontent-lengthanyway.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?