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

Streaming requests #1207

Merged
merged 4 commits into from
Jun 25, 2021
Merged

Streaming requests #1207

merged 4 commits into from
Jun 25, 2021

Conversation

richardm-stripe
Copy link
Contributor

@richardm-stripe richardm-stripe commented May 12, 2021

Notify

r? @dcr-stripe
cc @stripe/api-libraries

Summary

Adds ApiResource.requestStream, which is like ApiResource.request except does not buffer the response body into memory and makes no attempt to deserialize the body. Instead, returns an InputStream.

Motivation: a /pdf endpoint is coming soon to the Stripe API, which returns the bytes of a pdf. Users will want to e.g. pipe these bytes to a file, or pipe them to an email API. This is unlike all previous Stripe methods, which return JSON and are buffered into memory and parsed.

Similar to https://github.com/stripe/stripe-node/pull/1157/files

I ran japi-compliance-checker and got the following
report -- no incompatible changes.

image

@richardm-stripe richardm-stripe marked this pull request as draft May 12, 2021 03:51
@richardm-stripe richardm-stripe force-pushed the richardm-binary-streaming branch from a953964 to 304aa7e Compare May 13, 2021 19:55
this.bodyStream = bodyStream;
}

public StripeResponse unstream() throws IOException {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bikeshedding on this name encouraged.

Copy link
Contributor

Choose a reason for hiding this comment

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

readStream? readFromStream?

Copy link
Contributor

@dcr-stripe dcr-stripe left a comment

Choose a reason for hiding this comment

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

I think this makes sense overall - thanks for putting this together! My biggest feedback would be around the duplicated implementation for the response interface. Probably worth sharing that for now. Otherwise I think this all looks good!

public StripeResponse requestWithTelemetry(StripeRequest request) throws StripeException {
public StripeResponseStream requestStream(StripeRequest request) throws StripeException {
throw new UnsupportedOperationException(
"streamingRequest is unimplemented for this HttpClient");
Copy link
Contributor

Choose a reason for hiding this comment

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

streamingRequest -> requestStream?

src/main/java/com/stripe/net/HttpClient.java Outdated Show resolved Hide resolved
this.bodyStream = bodyStream;
}

public StripeResponse unstream() throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

readStream? readFromStream?

import java.time.Instant;

/** Common interface representing an HTTP response from Stripe. */
interface StripeResponseInterface {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like the majority of this implementation would be shared right (ie. all the header extraction)? If so, I think we could make some abstract class with the shared implementation to avoid duplicating the code and have both implementations extend this instead of (or along with) the interface. This might be nice for testing as well.

@richardm-stripe richardm-stripe force-pushed the richardm-binary-streaming branch from 92c9ff3 to 9b16a15 Compare June 7, 2021 15:52
@richardm-stripe richardm-stripe force-pushed the richardm-binary-streaming branch from 5310d7e to 001fdc0 Compare June 7, 2021 15:58
@richardm-stripe richardm-stripe marked this pull request as ready for review June 24, 2021 16:31
@richardm-stripe richardm-stripe changed the title [WIP] binary streaming Streaming requests Jun 24, 2021
Copy link
Contributor

@dcr-stripe dcr-stripe left a comment

Choose a reason for hiding this comment

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

LG! Just one naming nit!

import java.io.IOException;
import java.io.InputStream;

public class StripeResponseStream extends AbstractStripeResponse<InputStream> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: StripeResponseStream -> StripeStreamResponse for consistency with other libraries?

@richardm-stripe richardm-stripe merged commit ecb791f into master Jun 25, 2021
@richardm-stripe richardm-stripe deleted the richardm-binary-streaming branch June 25, 2021 17:59
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