Repository: stripe/stripe-node
Pull request #1157 (2021-05-11 14:50:15)

Binary streaming
## Notify
r? @dcr-stripe 
cc @stripe/api-libraries  

## Summary
This PR adds support for defining 'streaming' stripe methods on the Stripe client.  Unlike existing methods, 'streaming' methods do not buffer the response body into memory and try to parse it into json -- they pass the underlying `http.Request` object to the caller, so that the caller can pipe it to a file or socket without having to buffer it.

The motivation for this is an upcoming `/pdf` method that will return a pdf instead of JSON.

Also incidentally adds the ability to set "host" per-request, this was useful in my testing.

richardm-stripe: r? @dcr-stripe - the problem was that there were test cases that were supposed to trigger timeouts by never calling `res.end()`. In these cases, the "finish" or "close" events wouldn't fire. There's not really a cute way to automatically close the server in these cases after the test case is finished, so I had the helper expose a `closeServer` function to trigger manually on the test case, for now.
https://github.com/stripe/stripe-node/pull/1157/commits/2923bde5ce413ebfccf2ac0e21049bccde7ae1b3

Patch:
diff --git a/lib/StripeResource.js b/lib/StripeResource.js
index 8550721a..7fb628dd 100644
--- a/lib/StripeResource.js
+++ b/lib/StripeResource.js
@@ -102,7 +102,65 @@ StripeResource.prototype = {
     };
   },
 
-  _responseHandler(req, callback) {
+  _addHeadersDirectlyToResponse(res, headers) {
+    // For convenience, make some headers easily accessible on
+    // lastResponse.
+
+    // NOTE: Stripe responds with lowercase header names/keys.
+    res.requestId = headers['request-id'];
+    res.stripeAccount = res.stripeAccount || headers['stripe-account'];
+    res.apiVersion = res.apiVersion || headers['stripe-version'];
+    res.idempotencyKey = res.idempotencyKey || headers['idempotency-key'];
+  },
+
+  _makeResponseEvent(req, res, headers) {
+    const requestEndTime = Date.now();
+    const requestDurationMs = requestEndTime - req._requestStart;
+
+    return utils.removeNullish({
+      api_version: headers['stripe-version'],
+      account: headers['stripe-account'],
+      idempotency_key: headers['idempotency-key'],
+      method: req._requestEvent.method,
+      path: req._requestEvent.path,
+      status: res.statusCode,
+      request_id: res.requestId,
+      elapsed: requestDurationMs,
+      request_start_time: req._requestStart,
+      request_end_time: requestEndTime,
+    });
+  },
+
+  /**
+   * Used by methods with spec.streaming === true. For these methods, we do not
+   * buffer successful responses into memory or do parse them into stripe
+   * objects, we delegate that all of that to the user and pass back the raw
+   * http.Response object to the callback.
+   *
+   * (Unsuccessful responses shouldn't make it here, they should
+   * still be buffered/parsed and handled by _jsonResponseHandler -- see
+   * makeRequest)
+   */
+  _streamingResponseHandler(req, callback) {
+    return (res) => {
+      const headers = res.headers || {};
+      this._addHeadersDirectlyToResponse(res, headers);
+
+      res.once('end', () => {
+        const responseEvent = this._makeResponseEvent(req, res, headers);
+        this._stripe._emitter.emit('response', responseEvent);
+        this._recordRequestMetrics(res.requestId, responseEvent.elapsed);
+      });
+      return callback(null, res);
+    };
+  },
+
+  /**
+   * Default handler for Stripe responses. Buffers the response into memory,
+   * parses the JSON and returns it (i.e. passes it to the callback) if there
+   * is no "error" field. Otherwise constructs/passes an appropriate Error.
+   */
+  _jsonResponseHandler(req, callback) {
     return (res) => {
       let response = '';
 
@@ -112,43 +170,8 @@ StripeResource.prototype = {
       });
       res.once('end', () => {
         const headers = res.headers || {};
-        // NOTE: Stripe responds with lowercase header names/keys.
-
-        // For convenience, make some headers easily accessible on
-        // lastResponse.
-        res.requestId = headers['request-id'];
-
-        const stripeAccount = headers['stripe-account'];
-        if (stripeAccount) {
-          res.stripeAccount = stripeAccount;
-        }
-
-        const apiVersion = headers['stripe-version'];
-        if (apiVersion) {
-          res.apiVersion = apiVersion;
-        }
-
-        const idempotencyKey = headers['idempotency-key'];
-        if (idempotencyKey) {
-          res.idempotencyKey = idempotencyKey;
-        }
-
-        const requestEndTime = Date.now();
-        const requestDurationMs = requestEndTime - req._requestStart;
-
-        const responseEvent = utils.removeNullish({
-          api_version: headers['stripe-version'],
-          account: headers['stripe-account'],
-          idempotency_key: headers['idempotency-key'],
-          method: req._requestEvent.method,
-          path: req._requestEvent.path,
-          status: res.statusCode,
-          request_id: res.requestId,
-          elapsed: requestDurationMs,
-          request_start_time: req._requestStart,
-          request_end_time: requestEndTime,
-        });
-
+        this._addHeadersDirectlyToResponse(res, headers);
+        const responseEvent = this._makeResponseEvent(req, res, headers);
         this._stripe._emitter.emit('response', responseEvent);
 
         try {
@@ -194,7 +217,7 @@ StripeResource.prototype = {
           );
         }
 
-        this._recordRequestMetrics(res.requestId, requestDurationMs);
+        this._recordRequestMetrics(res.requestId, responseEvent.elapsed);
 
         // Expose res object
         Object.defineProperty(response, 'lastResponse', {
@@ -458,7 +481,10 @@ StripeResource.prototype = {
             ((res || {}).headers || {})['retry-after']
           );
         } else {
-          return this._responseHandler(req, callback)(res);
+          if (options.streaming && res.statusCode <= 400) {
+            return this._streamingResponseHandler(req, callback)(res);
+          }
+          return this._jsonResponseHandler(req, callback)(res);
         }
       });
 
diff --git a/lib/makeRequest.js b/lib/makeRequest.js
index 0da41f5b..8944b30b 100644
--- a/lib/makeRequest.js
+++ b/lib/makeRequest.js
@@ -8,7 +8,6 @@ function getRequestOpts(self, requestArgs, spec, overrideData) {
   const requestMethod = (spec.method || 'GET').toUpperCase();
   const urlParams = spec.urlParams || [];
   const encode = spec.encode || ((data) => data);
-  const host = spec.host;
   const path = self.createResourcePathWithSymbols(spec.path);
 
   // Don't mutate args externally.
@@ -31,7 +30,8 @@ function getRequestOpts(self, requestArgs, spec, overrideData) {
   const dataFromArgs = utils.getDataFromArgs(args);
   const data = encode(Object.assign({}, dataFromArgs, overrideData));
   const options = utils.getOptionsFromArgs(args);
-
+  const host = options.host || spec.host;
+  const streaming = !!spec.streaming;
   // Validate that there are no more args.
   if (args.filter((x) => x != null).length) {
     throw new Error(
@@ -58,6 +58,7 @@ function getRequestOpts(self, requestArgs, spec, overrideData) {
     auth: options.auth,
     headers,
     host,
+    streaming,
     settings: options.settings,
   };
 }
@@ -99,7 +100,7 @@ function makeRequest(self, requestArgs, spec, overrideData) {
       path,
       opts.bodyData,
       opts.auth,
-      {headers, settings},
+      {headers, settings, streaming: opts.streaming},
       requestCallback
     );
   });
diff --git a/lib/utils.js b/lib/utils.js
index 5bca895a..ae4c0047 100644
--- a/lib/utils.js
+++ b/lib/utils.js
@@ -25,6 +25,7 @@ const OPTIONS_KEYS = [
   'apiVersion',
   'maxNetworkRetries',
   'timeout',
+  'host',
 ];
 
 const DEPRECATED_OPTIONS = {
@@ -197,6 +198,9 @@ const utils = (module.exports = {
         if (Number.isInteger(params.timeout)) {
           opts.settings.timeout = params.timeout;
         }
+        if (params.host) {
+          opts.host = params.host;
+        }
       }
     }
     return opts;


Test patch:
diff --git a/test/StripeResource.spec.js b/test/StripeResource.spec.js
index 69adf309..18b1f2a5 100644
--- a/test/StripeResource.spec.js
+++ b/test/StripeResource.spec.js
@@ -6,6 +6,10 @@ const nock = require('nock');
 
 const stripe = require('../testUtils').getSpyableStripe();
 const expect = require('chai').expect;
+const testUtils = require('../testUtils');
+
+const StripeResource = require('../lib/StripeResource');
+const stripeMethod = StripeResource.method;
 
 describe('StripeResource', () => {
   describe('createResourcePathWithSymbols', () => {
@@ -191,12 +195,13 @@ describe('StripeResource', () => {
           (req, res) => {
             // Do nothing. This will trigger a timeout.
           },
-          (err, stripe) => {
+          (err, stripe, closeServer) => {
             if (err) {
               return done(err);
             }
             stripe.charges.create(options.data, (err, result) => {
               expect(err.detail.message).to.deep.equal('ETIMEDOUT');
+              closeServer();
               done();
             });
           }
@@ -212,13 +217,14 @@ describe('StripeResource', () => {
             // Do nothing. This will trigger a timeout.
             return {shouldStayOpen: nRequestsReceived < 3};
           },
-          (err, stripe) => {
+          (err, stripe, closeServer) => {
             if (err) {
               return done(err);
             }
             stripe.charges.create(options.data, (err, result) => {
               expect(err.detail.message).to.deep.equal('ETIMEDOUT');
               expect(nRequestsReceived).to.equal(3);
+              closeServer();
               done();
             });
           }
@@ -635,4 +641,92 @@ describe('StripeResource', () => {
       done();
     });
   });
+
+  describe('streaming', () => {
+    /**
+     * Defines a fake resource with a `pdf` method
+     * with binary streaming enabled.
+     */
+    const makeResourceWithPDFMethod = (stripe) => {
+      return new (StripeResource.extend({
+        path: 'resourceWithPDF',
+
+        pdf: stripeMethod({
+          method: 'GET',
+          host: 'files.stripe.com',
+          streaming: true,
+        }),
+      }))(stripe);
+    };
+
+    it('success', (callback) => {
+      const handleRequest = (req, res) => {
+        setTimeout(() => res.write('pretend'), 10);
+        setTimeout(() => res.write(' this'), 20);
+        setTimeout(() => res.write(' is a pdf'), 30);
+        setTimeout(() => res.end(), 40);
+      };
+
+      testUtils.getTestServerStripe(
+        {},
+        handleRequest,
+        (err, stripe, closeServer) => {
+          const foos = makeResourceWithPDFMethod(stripe);
+          if (err) {
+            return callback(err);
+          }
+
+          return foos.pdf({id: 'foo_123'}, {host: 'localhost'}, (err, res) => {
+            closeServer();
+            if (err) {
+              return callback(err);
+            }
+            const chunks = [];
+            res.on('data', (chunk) => chunks.push(chunk));
+            res.on('error', callback);
+            res.on('end', () => {
+              expect(Buffer.concat(chunks).toString()).to.equal(
+                'pretend this is a pdf'
+              );
+              return callback();
+            });
+          });
+        }
+      );
+    });
+
+    it('failure', (callback) => {
+      const handleRequest = (req, res) => {
+        setTimeout(() => res.writeHead(500));
+        setTimeout(
+          () =>
+            res.write(
+              '{"error": "api_error", "error_description": "this is bad"}'
+            ),
+          10
+        );
+        setTimeout(() => res.end(), 20);
+      };
+
+      testUtils.getTestServerStripe(
+        {},
+        handleRequest,
+        (err, stripe, closeServer) => {
+          if (err) {
+            return callback(err);
+          }
+
+          const foos = makeResourceWithPDFMethod(stripe);
+
+          return foos.pdf({id: 'foo_123'}, {host: 'localhost'}, (err, res) => {
+            closeServer();
+            expect(err).to.exist;
+            expect(err.raw.type).to.equal('api_error');
+            expect(err.raw.message).to.equal('this is bad');
+            return callback();
+          });
+        }
+      );
+    });
+  });
 });
diff --git a/testUtils/index.js b/testUtils/index.js
index 581345be..e2302623 100644
--- a/testUtils/index.js
+++ b/testUtils/index.js
@@ -15,7 +15,9 @@ const utils = (module.exports = {
     const server = http.createServer((req, res) => {
       const {shouldStayOpen} = handler(req, res) || {};
       if (!shouldStayOpen) {
-        res.on('close', () => server.close());
+        res.on('close', () => {
+          server.close();
+        });
       }
     });
     server.listen(0, () => {
@@ -29,7 +31,9 @@ const utils = (module.exports = {
           ...clientOptions,
         }
       );
-      return callback(null, stripe);
+      return callback(null, stripe, () => {
+        server.close();
+      });
     });
   },