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 support for empty bodies having Content-Encoding Gzip #192

Merged
merged 2 commits into from
Aug 3, 2019

Conversation

GibzonDev
Copy link
Contributor

Sometimes empty bodies can be received together with a Content-Encoding of, for example, "gzip". I've come across this a few times working with external API:s which send a 304 (Do not modify), together with a empty body and Content-Encoding "Gzip".

With these changes, Faraday won't raise an unknown gzip format exception, but rather just not try to decompress the body.

Example of similar fix in okhttp: square/okhttp#268

Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

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

Hi @gibzon69 and thanks for raising this!
I'm surprised the decompress libraries don't manage such a simple case like this, so it's great to have Faraday addressing it!
The solution looks good, I've just left a comment about the test as I believe it should be executed for jruby as well

it 'removes the Content-Encoding' do
expect(process(body).headers['Content-Encoding']).to be_nil
end
end unless jruby?
Copy link
Member

Choose a reason for hiding this comment

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

This should be tested and work under jruby as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. Yeah, it seems like a simple case, but I guess it doesn't happen that often.
I have removed the exception for jruby in the test now.

@iMacTia iMacTia merged commit e95c96b into lostisland:master Aug 3, 2019
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.

2 participants