From 800f0989a645d270348a245b1aa18004171b54d2 Mon Sep 17 00:00:00 2001 From: Alex Rattray Date: Tue, 30 Jul 2019 17:33:37 -0700 Subject: [PATCH 1/3] Unrelated: Clarify README a bit --- README.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index c81a75e42f..ad6cd67ee6 100644 --- a/README.md +++ b/README.md @@ -175,7 +175,7 @@ This will retry requests `n` times with exponential backoff if they fail due to [Idempotency keys](https://stripe.com/docs/api/idempotent_requests) are added where appropriate to prevent duplication. ```js - // Retry a request twice before giving up +// Retry a request twice before giving up stripe.setMaxNetworkRetries(2); ``` @@ -410,16 +410,16 @@ $ yarn test If you do not have `yarn` installed, you can get it with `npm install --global yarn`. -Run a single test suite: +Run a single test suite without a coverage report: ```bash -$ yarn mocha test/Error.spec.js +$ yarn mocha-only test/Error.spec.js ``` -Run a single test (case sensitive): +Run a single test (case sensitive) in watch mode: ```bash -$ yarn mocha test/Error.spec.js --grep 'Populates with type' +$ yarn mocha-only test/Error.spec.js --grep 'Populates with type' --watch ``` If you wish, you may run tests using your Stripe _Test_ API key by setting the From 4f2bc28159f8eabe285539d957ecc9421abcd5db Mon Sep 17 00:00:00 2001 From: Alex Rattray Date: Tue, 30 Jul 2019 17:50:24 -0700 Subject: [PATCH 2/3] Refactor headers --- lib/StripeResource.js | 114 +++++++++++++++++++----------------- lib/utils.js | 41 +++++++++++-- test/StripeResource.spec.js | 10 ++-- test/utils.spec.js | 8 +-- 4 files changed, 104 insertions(+), 69 deletions(-) diff --git a/lib/StripeResource.js b/lib/StripeResource.js index 2deaf1671f..cb25c7eade 100644 --- a/lib/StripeResource.js +++ b/lib/StripeResource.js @@ -123,7 +123,7 @@ StripeResource.prototype = { const requestDurationMs = Date.now() - req._requestStart; - const responseEvent = utils.removeEmpty({ + const responseEvent = utils.removeNullish({ api_version: headers['stripe-version'], account: headers['stripe-account'], idempotency_key: headers['idempotency-key'], @@ -262,38 +262,58 @@ StripeResource.prototype = { return sleepSeconds * 1000; }, - _defaultHeaders(auth, contentLength, apiVersion) { - let userAgentString = `Stripe/v1 NodeBindings/${this._stripe.getConstant( - 'PACKAGE_VERSION' - )}`; - - if (this._stripe._appInfo) { - userAgentString += ` ${this._stripe.getAppInfoAsString()}`; + _defaultIdempotencyKey(method) { + // If this is a POST and we allow multiple retries, ensure an idempotency key. + if (method === 'POST' && this._stripe.getMaxNetworkRetries() > 0) { + return uuid(); } + return null; + }, - const headers = { + _makeHeaders( + auth, + contentLength, + apiVersion, + clientUserAgent, + method, + userSuppliedHeaders + ) { + const defaultHeaders = { // Use specified auth token or use default from this stripe instance: Authorization: auth ? `Bearer ${auth}` : this._stripe.getApiField('auth'), Accept: 'application/json', 'Content-Type': 'application/x-www-form-urlencoded', 'Content-Length': contentLength, - 'User-Agent': userAgentString, + 'User-Agent': this._getUserAgentString(), + 'X-Stripe-Client-User-Agent': clientUserAgent, + 'X-Stripe-Client-Telemetry': this._getTelemetryHeader(), + 'Stripe-Version': apiVersion, + 'Idempotency-Key': this._defaultIdempotencyKey(method), }; - if (apiVersion) { - headers['Stripe-Version'] = apiVersion; - } + return Object.assign( + utils.removeNullish(defaultHeaders), + // If the user supplied, say 'idempotency-key', override instead of appending by ensuring caps are the same. + utils.normalizeHeaders(userSuppliedHeaders) + ); + }, - return headers; + _getUserAgentString() { + const packageVersion = this._stripe.getConstant('PACKAGE_VERSION'); + const appInfo = this._stripe._appInfo + ? this._stripe.getAppInfoAsString() + : ''; + + return `Stripe/v1 NodeBindings/${packageVersion} ${appInfo}`.trim(); }, - _addTelemetryHeader(headers) { + _getTelemetryHeader() { if ( this._stripe.getTelemetryEnabled() && this._stripe._prevRequestMetrics.length > 0 ) { const metrics = this._stripe._prevRequestMetrics.shift(); - headers['X-Stripe-Client-Telemetry'] = JSON.stringify({ + return JSON.stringify({ last_request_metrics: metrics, }); } @@ -320,6 +340,16 @@ StripeResource.prototype = { _request(method, host, path, data, auth, options, callback) { let requestData; + const retryRequest = (requestFn, apiVersion, headers, requestRetries) => { + return setTimeout( + requestFn, + this._getSleepTimeInMS(requestRetries), + apiVersion, + headers, + requestRetries + 1 + ); + }; + const makeRequest = (apiVersion, headers, numRetries) => { const timeout = this._stripe.getApiField('timeout'); const isInsecureConnection = @@ -339,15 +369,7 @@ StripeResource.prototype = { ciphers: 'DEFAULT:!aNULL:!eNULL:!LOW:!EXPORT:!SSLv2:!MD5', }); - // If this is a POST and we allow multiple retries, set a idempotency key if one is not - // already provided. - if (method === 'POST' && this._stripe.getMaxNetworkRetries() > 0) { - if (!headers.hasOwnProperty('Idempotency-Key')) { - headers['Idempotency-Key'] = uuid(); - } - } - - const requestEvent = utils.removeEmpty({ + const requestEvent = utils.removeNullish({ api_version: apiVersion, account: headers['Stripe-Account'], idempotency_key: headers['Idempotency-Key'], @@ -399,27 +421,23 @@ StripeResource.prototype = { }); }; - const makeRequestWithData = (error, data) => { + const prepareAndMakeRequest = (error, data) => { if (error) { return callback(error); } - const apiVersion = this._stripe.getApiField('version'); requestData = data; - const headers = this._defaultHeaders( - auth, - requestData.length, - apiVersion - ); - - this._stripe.getClientUserAgent((cua) => { - headers['X-Stripe-Client-User-Agent'] = cua; - - if (options.headers) { - Object.assign(headers, options.headers); - } - this._addTelemetryHeader(headers); + this._stripe.getClientUserAgent((clientUserAgent) => { + const apiVersion = this._stripe.getApiField('version'); + const headers = this._makeHeaders( + auth, + requestData.length, + apiVersion, + clientUserAgent, + method, + options.headers + ); makeRequest(apiVersion, headers); }); @@ -430,23 +448,11 @@ StripeResource.prototype = { method, data, options.headers, - makeRequestWithData + prepareAndMakeRequest ); } else { - makeRequestWithData(null, utils.stringifyRequestData(data || {})); + prepareAndMakeRequest(null, utils.stringifyRequestData(data || {})); } - - const retryRequest = (requestFn, apiVersion, headers, requestRetries) => { - requestRetries += 1; - - return setTimeout( - requestFn, - this._getSleepTimeInMS(requestRetries), - apiVersion, - headers, - requestRetries - ); - }; }, }; diff --git a/lib/utils.js b/lib/utils.js index 96b24987d1..f62c7e83eb 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -202,18 +202,47 @@ const utils = (module.exports = { /** * Remove empty values from an object */ - removeEmpty: (obj) => { + removeNullish: (obj) => { if (typeof obj !== 'object') { throw new Error('Argument must be an object'); } - Object.keys(obj).forEach((key) => { - if (obj[key] === null || obj[key] === undefined) { - delete obj[key]; + return Object.keys(obj).reduce((result, key) => { + if (obj[key] != null) { + result[key] = obj[key]; } - }); + return result; + }, {}); + }, + + /** + * Normalize standard HTTP Headers: + * {'foo-bar': 'hi'} + * becomes + * {'Foo-Bar': 'hi'} + */ + normalizeHeaders: (obj) => { + if (!(obj && typeof obj === 'object')) { + return obj; + } - return obj; + return Object.keys(obj).reduce((result, header) => { + result[utils.normalizeHeader(header)] = obj[header]; + return result; + }, {}); + }, + + /** + * Stolen from https://github.com/marten-de-vries/header-case-normalizer/blob/master/index.js#L36-L41 + * without the exceptions which are irrelevant to us. + */ + normalizeHeader: (header) => { + return header + .split('-') + .map( + (text) => text.charAt(0).toUpperCase() + text.substr(1).toLowerCase() + ) + .join('-'); }, /** diff --git a/test/StripeResource.spec.js b/test/StripeResource.spec.js index 7fac0c5cb4..a6b64c8563 100644 --- a/test/StripeResource.spec.js +++ b/test/StripeResource.spec.js @@ -17,13 +17,13 @@ describe('StripeResource', () => { }); }); - describe('_defaultHeaders', () => { + describe('_makeHeaders', () => { it('sets the Authorization header with Bearer auth using the global API key', () => { - const headers = stripe.invoices._defaultHeaders(null, 0, null); + const headers = stripe.invoices._makeHeaders(null, 0, null); expect(headers.Authorization).to.equal('Bearer fakeAuthToken'); }); it('sets the Authorization header with Bearer auth using the specified API key', () => { - const headers = stripe.invoices._defaultHeaders( + const headers = stripe.invoices._makeHeaders( 'anotherFakeAuthToken', 0, null @@ -31,11 +31,11 @@ describe('StripeResource', () => { expect(headers.Authorization).to.equal('Bearer anotherFakeAuthToken'); }); it('sets the Stripe-Version header if an API version is provided', () => { - const headers = stripe.invoices._defaultHeaders(null, 0, '1970-01-01'); + const headers = stripe.invoices._makeHeaders(null, 0, '1970-01-01'); expect(headers['Stripe-Version']).to.equal('1970-01-01'); }); it('does not the set the Stripe-Version header if no API version is provided', () => { - const headers = stripe.invoices._defaultHeaders(null, 0, null); + const headers = stripe.invoices._makeHeaders(null, 0, null); expect(headers).to.not.include.keys('Stripe-Version'); }); }); diff --git a/test/utils.spec.js b/test/utils.spec.js index 0582fdd039..4156e7244b 100644 --- a/test/utils.spec.js +++ b/test/utils.spec.js @@ -312,10 +312,10 @@ describe('utils', () => { }); }); - describe('removeEmpty', () => { + describe('removeNullish', () => { it('removes empty properties and leaves non-empty ones', () => { expect( - utils.removeEmpty({ + utils.removeNullish({ cat: 3, dog: false, rabbit: undefined, @@ -327,9 +327,9 @@ describe('utils', () => { }); }); - it('throws an error if not given two things to compare', () => { + it('throws an error if not given an object', () => { expect(() => { - utils.removeEmpty('potato'); + utils.removeNullish('potato'); }).to.throw(); }); }); From d50bcaec693e50c0a2cdcf83236b4f9c07692449 Mon Sep 17 00:00:00 2001 From: Alex Rattray Date: Tue, 30 Jul 2019 19:55:52 -0700 Subject: [PATCH 3/3] Make errors show up better in tests --- test/StripeResource.spec.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/StripeResource.spec.js b/test/StripeResource.spec.js index a6b64c8563..94dadc8681 100644 --- a/test/StripeResource.spec.js +++ b/test/StripeResource.spec.js @@ -67,7 +67,7 @@ describe('StripeResource', () => { .reply(200, '{}'); realStripe.invoices.retrieveUpcoming(options.data, (err, response) => { - done(); + done(err); scope.done(); }); }); @@ -92,7 +92,7 @@ describe('StripeResource', () => { 'sub_123', options.data, (err, response) => { - done(); + done(err); scope.done(); } ); @@ -175,7 +175,7 @@ describe('StripeResource', () => { realStripe.charges.create(options.data, (err, charge) => { expect(charge.id).to.equal('ch_123'); - done(); + done(err); }); }); @@ -198,7 +198,7 @@ describe('StripeResource', () => { realStripe.charges.create(options.data, (err, charge) => { expect(charge.id).to.equal('ch_123'); - done(); + done(err); }); }); @@ -272,7 +272,7 @@ describe('StripeResource', () => { realStripe.charges.create(options.data, (err, charge) => { expect(charge.id).to.equal('ch_123'); - done(); + done(err); }); }); @@ -295,7 +295,7 @@ describe('StripeResource', () => { realStripe.charges.retrieve('ch_123', (err, charge) => { expect(charge.id).to.equal('ch_123'); - done(); + done(err); }); }); @@ -322,9 +322,9 @@ describe('StripeResource', () => { realStripe.setMaxNetworkRetries(1); - realStripe.charges.create(options.data, () => { + realStripe.charges.create(options.data, (err) => { expect(headers).to.have.property('idempotency-key'); - done(); + done(err); }); });