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 AutoInflate with encoding other than Encoding::BINARY. #721

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

benubois
Copy link
Contributor

@benubois benubois commented Aug 4, 2022

Fix for #535

@tarcieri tarcieri requested a review from ixti August 4, 2022 01:47
Comment on lines +460 to +464
it "returns decoded body with the correct charset" do
encoding = Encoding::Shift_JIS
client = HTTP.use(:auto_inflate).headers("Accept-Encoding" => "gzip").encoding(encoding)
body = "もしもし!".encode(encoding, Encoding::UTF_8)
response = client.post("#{dummy.endpoint}/encoded-body", :body => body)
Copy link
Member

Choose a reason for hiding this comment

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

I'm struggling to understand this test.

client   = HTTP.use(:auto_inflate).headers("Accept-Encoding" => "gzip").encoding(encoding)

^^^ encoding(...) enforces encoding of the response.

Comment on lines +27 to +28
def stream_for(connection, encoding: Encoding::BINARY)
Response::Body.new(Response::Inflater.new(connection), :encoding => encoding)
Copy link
Member

Choose a reason for hiding this comment

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

stream_for can and should be private, and encoding: seems like don't need default value.

@@ -17,15 +17,15 @@ def wrap_response(response)
:headers => response.headers,
:proxy_headers => response.proxy_headers,
:connection => response.connection,
:body => stream_for(response.connection),
:body => stream_for(response.connection, :encoding => response.body.encoding),
Copy link
Member

Choose a reason for hiding this comment

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

Let's move logic to stream_for:

Suggested change
:body => stream_for(response.connection, :encoding => response.body.encoding),
:body => wrap_body(response),

Then move and rename stream_for to be private wrap_body:

private

def wrap_body(response)
  stream = Response::Inflater.new(response.connection)

  Response::Body.new(stream, :encoding => response.body.encoding)
end

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.

3 participants