From ff294813d641379941bdad1541db6a4257036ac0 Mon Sep 17 00:00:00 2001 From: Richard Marmorstein Date: Tue, 4 May 2021 17:51:57 -0400 Subject: [PATCH 1/4] Binary streaming --- lib/StripeResource.js | 112 +++++++++++++++++++++++------------- lib/makeRequest.js | 7 ++- lib/utils.js | 4 ++ test/StripeResource.spec.js | 82 ++++++++++++++++++++++++++ 4 files changed, 162 insertions(+), 43 deletions(-) diff --git a/lib/StripeResource.js b/lib/StripeResource.js index 8550721a1b..afb72a235b 100644 --- a/lib/StripeResource.js +++ b/lib/StripeResource.js @@ -102,7 +102,71 @@ StripeResource.prototype = { }; }, - _responseHandler(req, callback) { + _addHeadersDirectlyToResponse(res, headers) { + // 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; + } + }, + + _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 the + * response into memory, or do any parsing into stripe objects or errors, we + * delegate that all of that to the user and pass back the raw http.Response + * object to the callback. + */ + _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 +176,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 +223,7 @@ StripeResource.prototype = { ); } - this._recordRequestMetrics(res.requestId, requestDurationMs); + this._recordRequestMetrics(res.requestId, responseEvent.elapsed); // Expose res object Object.defineProperty(response, 'lastResponse', { @@ -458,7 +487,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 0da41f5b02..8944b30bde 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 5bca895adf..ae4c004730 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; diff --git a/test/StripeResource.spec.js b/test/StripeResource.spec.js index 69adf30938..c15e5d3fca 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', () => { @@ -635,4 +639,82 @@ 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) => { + const foos = makeResourceWithPDFMethod(stripe); + if (err) { + return callback(err); + } + + return foos.pdf({id: 'foo_123'}, {host: 'localhost'}, (err, res) => { + 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) => { + if (err) { + return callback(err); + } + + const foos = makeResourceWithPDFMethod(stripe); + + return foos.pdf({id: 'foo_123'}, {host: 'localhost'}, (err, res) => { + expect(err).to.exist; + expect(err.raw.type).to.equal('api_error'); + expect(err.raw.message).to.equal('this is bad'); + return callback(); + }); + }); + }); + }); }); From e0469afdbbdf2dbbb742afb51c8d12c12b0fcb7a Mon Sep 17 00:00:00 2001 From: Richard Marmorstein Date: Tue, 11 May 2021 19:40:16 -0400 Subject: [PATCH 2/4] More succinct defaults --- lib/StripeResource.js | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/lib/StripeResource.js b/lib/StripeResource.js index afb72a235b..66c44cd153 100644 --- a/lib/StripeResource.js +++ b/lib/StripeResource.js @@ -106,21 +106,9 @@ StripeResource.prototype = { // 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; - } + 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) { From 0dab6ab0098ee9099a0c224bc8baa6fc02792fa1 Mon Sep 17 00:00:00 2001 From: Richard Marmorstein Date: Thu, 24 Jun 2021 12:49:48 -0400 Subject: [PATCH 3/4] Fix comments --- lib/StripeResource.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/StripeResource.js b/lib/StripeResource.js index 66c44cd153..7fb628dd1c 100644 --- a/lib/StripeResource.js +++ b/lib/StripeResource.js @@ -105,6 +105,8 @@ StripeResource.prototype = { _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']; @@ -130,10 +132,14 @@ StripeResource.prototype = { }, /** - * Used by methods with spec.streaming === true. For these methods, we do not buffer the - * response into memory, or do any parsing into stripe objects or errors, we - * delegate that all of that to the user and pass back the raw http.Response - * object to the callback. + * 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) => { From 2923bde5ce413ebfccf2ac0e21049bccde7ae1b3 Mon Sep 17 00:00:00 2001 From: Richard Marmorstein Date: Thu, 24 Jun 2021 16:52:45 -0400 Subject: [PATCH 4/4] Prevent test suite from hanging --- test/StripeResource.spec.js | 74 +++++++++++++++++++++---------------- testUtils/index.js | 8 +++- 2 files changed, 49 insertions(+), 33 deletions(-) diff --git a/test/StripeResource.spec.js b/test/StripeResource.spec.js index c15e5d3fca..18b1f2a585 100644 --- a/test/StripeResource.spec.js +++ b/test/StripeResource.spec.js @@ -195,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(); }); } @@ -216,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(); }); } @@ -665,27 +667,32 @@ describe('StripeResource', () => { setTimeout(() => res.end(), 40); }; - testUtils.getTestServerStripe({}, handleRequest, (err, stripe) => { - const foos = makeResourceWithPDFMethod(stripe); - if (err) { - return callback(err); - } - - return foos.pdf({id: 'foo_123'}, {host: 'localhost'}, (err, res) => { + testUtils.getTestServerStripe( + {}, + handleRequest, + (err, stripe, closeServer) => { + const foos = makeResourceWithPDFMethod(stripe); 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(); + + 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) => { @@ -701,20 +708,25 @@ describe('StripeResource', () => { setTimeout(() => res.end(), 20); }; - testUtils.getTestServerStripe({}, handleRequest, (err, stripe) => { - if (err) { - return callback(err); - } + testUtils.getTestServerStripe( + {}, + handleRequest, + (err, stripe, closeServer) => { + if (err) { + return callback(err); + } - const foos = makeResourceWithPDFMethod(stripe); + const foos = makeResourceWithPDFMethod(stripe); - return foos.pdf({id: 'foo_123'}, {host: 'localhost'}, (err, res) => { - expect(err).to.exist; - expect(err.raw.type).to.equal('api_error'); - expect(err.raw.message).to.equal('this is bad'); - return callback(); - }); - }); + 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 581345be65..e230262334 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(); + }); }); },