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.

Sign up for GitHub

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

Jump to bottom

Handle compressed body in successful response #148

Merged
merged 2 commits into from
Dec 2, 2016
Merged

Handle compressed body in successful response #148

merged 2 commits into from
Dec 2, 2016

Conversation

tlvenn
Copy link
Contributor

@tlvenn tlvenn commented Nov 28, 2016

Fix #147

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 13.924% when pulling d47e622 on tlvenn:unzip into b531058 on code-corps:2.0.

@tlvenn
Copy link
Contributor Author

tlvenn commented Nov 28, 2016

The assumption being that in case of errors, the body is actually not compressed even if requested which is usually the default behaviour.

@joshsmith
Copy link
Contributor

Where does this happen? Can you add some docs here to this?

Also this is now outdated due to the merge of the headers.

@tlvenn
Copy link
Contributor Author

tlvenn commented Dec 2, 2016

The add_default_headers/1 is adding the following header to the http request: "Accept-Encoding" => "gzip" so the server should normally gzip its response. However, right now, nothing in the handling of the response is unzipping the body if it's needed. The PR addresses this.

@DavidAntaramian
Copy link
Contributor

@tlvenn Thank you! Thank you thank you thank you. Totally my fault.

Can you rebase against your previous change and then I will merge?

@tlvenn
Copy link
Contributor Author

tlvenn commented Dec 2, 2016

Done @DavidAntaramian

DavidAntaramian
DavidAntaramian reviewed Dec 2, 2016
View reviewed changes
lib/stripe.ex
decoded_body =
body
|> Poison.decode!
|> decompress_body(headers)
Copy link
Contributor

Choose a reason for hiding this comment

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

@tlvenn Should this be before Poison.decode!/2? Otherwise won't it try to decode to JSON the compressed body?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oups... good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@DavidAntaramian DavidAntaramian merged commit 9232d71 into beam-community:2.0 Dec 2, 2016
@DavidAntaramian
Copy link
Contributor

🐛 🔨

@DavidAntaramian DavidAntaramian mentioned this pull request Dec 2, 2016
Closed
@joshsmith
Copy link
Contributor

🖐 💛

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavidAntaramian DavidAntaramian DavidAntaramian left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tlvenn @coveralls @joshsmith @DavidAntaramian