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

Accept IO objects as HTTP request bodies #2096

Closed
wants to merge 4 commits into from
Closed

Accept IO objects as HTTP request bodies #2096

wants to merge 4 commits into from

Conversation

Acconut
Copy link

@Acconut Acconut commented Feb 1, 2016

Previously only strings (or nil) would have been accepted by the HTTP::Client.exec methods.

@Acconut
Copy link
Author

Acconut commented Feb 3, 2016

Tests pass now.

@@ -80,7 +80,7 @@ struct OAuth::Signature
body = request.body
content_type = request.headers["Content-type"]?
if body && content_type == "application/x-www-form-urlencoded"
params.add_query body
params.add_query body.to_s
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this rather raise if it's an IO, or at least read it?

Copy link
Author

Choose a reason for hiding this comment

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

Oh my bad, I thought to_s returned the entire content as a String but this seems to be only the case for MemoryIO and not File etc. I believe the most intuitive thing is that we read it without raising an error.

@asterite
Copy link
Member

asterite commented Feb 3, 2016

This is a good change, we forgot about request's body being able to be an IO.

We should also think if we want to have an IO in the server, so I guess this will need some more thought from our side.

@asterite
Copy link
Member

This was implemented in #3406

@asterite asterite closed this Oct 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants