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

Add support for making a request and receiving the response as a stream. #983

Merged
merged 3 commits into from
Jun 24, 2021

Conversation

dcr-stripe
Copy link
Contributor

@dcr-stripe dcr-stripe commented Jun 17, 2021

Notify

r? @richardm-stripe

Summary

Adds a new request_stream method to APIResource which allows for requesting the body of the response be streamed instead of treated as JSON and converted into a StripeObject. Clients will then be able to use this stream to pipe content elsewhere. For errors, we still parse the JSON and raise as appropriate.

This should be used with a block, in which case, we return a new StripeHeadersOnlyResponse as the body cannot be read outside the block:

f = ... (some file)
request_stream(:get, "/pdf") do |body_chunk|
  f << body_chunk
end

A key aspect to point out here is that the block can be called multiple times as the HTTP client streams in the body in segments.

As part of this change, I moved StripeResponse::Headers to its own top-level class StripeResponseHeaders. In order to preserve backwards compatibility, I set up an alias for StripeResponse::Headers.

Motivation

The motivation for this is an upcoming /pdf method that will return a pdf instead of JSON.

Test plan

Added unit tests and tested with a local playground that the request streamed as expected.

Copy link
Contributor

@richardm-stripe richardm-stripe left a comment

Choose a reason for hiding this comment

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

Gave this a pass, it looks good!

Do you think there should be some sort of re-use or common interface among StripeResponse, StripeStreamResponse, and StripeHeadersOnly respons in lib/stripe/stripe_response.rb?

Also, a thought, if the only way to stream is via passing in a block, is there much reason to wrap it in IO?
resp.io = StringIO.new http_resp.body vs. resp.body = http_resp.body

@dcr-stripe
Copy link
Contributor Author

Gave this a pass, it looks good!

Do you think there should be some sort of re-use or common interface among StripeResponse, StripeStreamResponse, and StripeHeadersOnly respons in lib/stripe/stripe_response.rb?

Yep, good call. Ended up moving headers, adding an alias, and splitting out a base mixin. The mixin was beacuse we're currently just using these from_net_http static methods instead of passing in the response to the constructor, so inheritance didn't really make sense.

Also, a thought, if the only way to stream is via passing in a block, is there much reason to wrap it in IO?
resp.io = StringIO.new http_resp.body vs. resp.body = http_resp.body

Makese sense - removed the stream. Users can pass this to an IO object directly.

r? @richardm-stripe

@dcr-stripe dcr-stripe changed the title [WIP] Add support for making a request and receiving the response as a stream. Add support for making a request and receiving the response as a stream. Jun 21, 2021
@dcr-stripe
Copy link
Contributor Author

r? @tmaxwell-stripe

for a second set of eyes + Ruby experience! Want to make sure the user-facing interface makes sense. Thanks!

@dcr-stripe dcr-stripe force-pushed the dcr-binary-streaming branch 2 times, most recently from 24c64e6 to 63c7aeb Compare June 22, 2021 17:44
@dcr-stripe dcr-stripe force-pushed the dcr-binary-streaming branch from 63c7aeb to 157d477 Compare June 22, 2021 17:47
@dcr-stripe
Copy link
Contributor Author

PTAL @richardm-stripe - incorporated @tmaxwell-stripe 's feedback to make request_stream require a block.

@dcr-stripe dcr-stripe merged commit 59eb8d0 into master Jun 24, 2021
@remi-stripe remi-stripe deleted the dcr-binary-streaming branch September 28, 2023 23:07
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