Refactor headers (#667)

* Unrelated: Clarify README a bit * Refactor headers * Make errors show up better in tests
stripe · Jul 31, 2019 · 45bdd55 · 45bdd55
1 parent 8b369e2
commit 45bdd55
Show file tree

Hide file tree

Showing 5 changed files with 117 additions and 82 deletions.
diff --git 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

diff --git 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
@@ -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
@@ -17,25 +17,25 @@ 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
       );
       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');
     });
   });
@@ -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);
         });
       });
 

diff --git 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();
     });
   });