Repository: stripe/stripe-node
Pull request #699 (2019-09-25 22:07:37)

Add request-specific fields from raw error to top level error
Per request of @remi-stripe (https://jira.corp.stripe.com/secure/RapidBoard.jspa?rapidView=1346&projectKey=DX&view=planning&selectedIssue=DX-4753&quickFilter=3988)
It is sometimes important to access properties such as `error.raw.payment_intent`, `error.raw.payment_method`, etc. But "raw" means "here be dragons, don't rely on anything here". This PR copies these properties from `raw` to the top level error object, to indicate that they are properly part of our interface and remove the need for users to access "raw".




Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1bea931d..e45e59c6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,17 +1,11 @@
 # CHANGELOG
 
-## 7.9.1 - 2019-09-17
-
-- [#692](https://github.com/stripe/stripe-node/pull/692) Retry based on `Stripe-Should-Retry` and `Retry-After` headers
-
 ## 7.9.0 - 2019-09-09
-
-- [#691](https://github.com/stripe/stripe-node/pull/691) GET and DELETE requests data: body->queryParams
-- [#684](https://github.com/stripe/stripe-node/pull/684) Bump eslint-utils from 1.3.1 to 1.4.2
+* [#691](https://github.com/stripe/stripe-node/pull/691) GET and DELETE requests data: body->queryParams
+* [#684](https://github.com/stripe/stripe-node/pull/684) Bump eslint-utils from 1.3.1 to 1.4.2
 
 ## 7.8.0 - 2019-08-12
-
-- [#678](https://github.com/stripe/stripe-node/pull/678) Add `subscriptionItems.createUsageRecord()` method
+* [#678](https://github.com/stripe/stripe-node/pull/678) Add `subscriptionItems.createUsageRecord()` method
 
 ## 7.7.0 - 2019-08-09
 
diff --git a/VERSION b/VERSION
index 3fe624e2..4bc5d618 100644
--- a/VERSION
+++ b/VERSION
@@ -1,1 +1,1 @@
-7.9.1
+7.9.0
diff --git a/lib/Error.js b/lib/Error.js
index 5b07c0b1..d96ce9a1 100644
--- a/lib/Error.js
+++ b/lib/Error.js
@@ -36,6 +36,13 @@ class StripeError extends Error {
     this.requestId = raw.requestId;
     this.statusCode = raw.statusCode;
     this.message = raw.message;
+
+    this.charge = raw.charge;
+    this.decline_code = raw.decline_code;
+    this.payment_intent = raw.payment_intent;
+    this.payment_method = raw.payment_method;
+    this.setup_intent = raw.setup_intent;
+    this.source = raw.source;
   }
 
   /**
diff --git a/lib/StripeResource.js b/lib/StripeResource.js
index 3a2ca84d..a0caaf0e 100644
--- a/lib/StripeResource.js
+++ b/lib/StripeResource.js
@@ -18,7 +18,6 @@ StripeResource.method = require('./StripeMethod');
 StripeResource.BASIC_METHODS = require('./StripeMethod.basic');
 
 StripeResource.MAX_BUFFERED_REQUEST_METRICS = 100;
-const MAX_RETRY_AFTER_WAIT = 60;
 
 /**
  * Encapsulates request logic for a Stripe Resource
@@ -218,7 +217,6 @@ StripeResource.prototype = {
     };
   },
 
-  // For more on when and how to retry API requests, see https://stripe.com/docs/error-handling#safely-retrying-requests-with-idempotency
   _shouldRetry(res, numRetries) {
     // Do not retry if we are out of retries.
     if (numRetries >= this._stripe.getMaxNetworkRetries()) {
@@ -230,33 +228,21 @@ StripeResource.prototype = {
       return true;
     }
 
-    // The API may ask us not to retry (eg; if doing so would be a no-op)
-    // or advise us to retry (eg; in cases of lock timeouts); we defer to that.
-    if (res.headers && res.headers['stripe-should-retry'] === 'false') {
-      return false;
-    }
-    if (res.headers && res.headers['stripe-should-retry'] === 'true') {
-      return true;
-    }
-
-    // Retry on conflict errors.
-    if (res.statusCode === 409) {
+    // Retry on conflict and availability errors.
+    if (res.statusCode === 409 || res.statusCode === 503) {
       return true;
     }
 
-    // Retry on 500, 503, and other internal errors.
-    //
-    // Note that we expect the stripe-should-retry header to be false
-    // in most cases when a 500 is returned, since our idempotency framework
-    // would typically replay it anyway.
-    if (res.statusCode >= 500) {
+    // Retry on 5xx's, except POST's, which our idempotency framework
+    // would just replay as 500's again anyway.
+    if (res.statusCode >= 500 && res.req._requestEvent.method !== 'POST') {
       return true;
     }
 
     return false;
   },
 
-  _getSleepTimeInMS(numRetries, retryAfter = null) {
+  _getSleepTimeInMS(numRetries) {
     const initialNetworkRetryDelay = this._stripe.getInitialNetworkRetryDelay();
     const maxNetworkRetryDelay = this._stripe.getMaxNetworkRetryDelay();
 
@@ -275,11 +261,6 @@ StripeResource.prototype = {
     // But never sleep less than the base sleep seconds.
     sleepSeconds = Math.max(initialNetworkRetryDelay, sleepSeconds);
 
-    // And never sleep less than the time the API asks us to wait, assuming it's a reasonable ask.
-    if (Number.isInteger(retryAfter) && retryAfter <= MAX_RETRY_AFTER_WAIT) {
-      sleepSeconds = Math.max(sleepSeconds, retryAfter);
-    }
-
     return sleepSeconds * 1000;
   },
 
@@ -361,16 +342,10 @@ StripeResource.prototype = {
   _request(method, host, path, data, auth, options, callback) {
     let requestData;
 
-    const retryRequest = (
-      requestFn,
-      apiVersion,
-      headers,
-      requestRetries,
-      retryAfter
-    ) => {
+    const retryRequest = (requestFn, apiVersion, headers, requestRetries) => {
       return setTimeout(
         requestFn,
-        this._getSleepTimeInMS(requestRetries, retryAfter),
+        this._getSleepTimeInMS(requestRetries),
         apiVersion,
         headers,
         requestRetries + 1
@@ -419,13 +394,7 @@ StripeResource.prototype = {
 
       req.once('response', (res) => {
         if (this._shouldRetry(res, requestRetries)) {
-          return retryRequest(
-            makeRequest,
-            apiVersion,
-            headers,
-            requestRetries,
-            ((res || {}).headers || {})['retry-after']
-          );
+          return retryRequest(makeRequest, apiVersion, headers, requestRetries);
         } else {
           return this._responseHandler(req, callback)(res);
         }
@@ -433,13 +402,7 @@ StripeResource.prototype = {
 
       req.on('error', (error) => {
         if (this._shouldRetry(null, requestRetries)) {
-          return retryRequest(
-            makeRequest,
-            apiVersion,
-            headers,
-            requestRetries,
-            null
-          );
+          return retryRequest(makeRequest, apiVersion, headers, requestRetries);
         } else {
           return this._errorHandler(req, requestRetries, callback)(error);
         }
diff --git a/package.json b/package.json
index a7c3ff74..0ca0875b 100644
--- a/package.json
+++ b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "stripe",
-  "version": "7.9.1",
+  "version": "7.9.0",
   "description": "Stripe API wrapper",
   "keywords": [
     "stripe",


Test patch:
diff --git a/test/Error.spec.js b/test/Error.spec.js
index 7b70aa16..e4fb1229 100644
--- a/test/Error.spec.js
+++ b/test/Error.spec.js
@@ -60,6 +60,16 @@ describe('Error', () => {
       ).to.be.instanceOf(Error.StripeIdempotencyError);
     });
 
+    it('copies whitelisted properties', () => {
+      const e = new Error.StripeError({
+        charge: 'foo',
+        unknown_prop: 'bar',
+      });
+      expect(e).to.have.property('charge', 'foo');
+      expect(e).not.to.have.property('unknown_prop', 'bar');
+      expect(e).not.to.have.property('decline_code', 'xyzzy');
+    });
+
     it('Pulls in headers', () => {
       const headers = {'Request-Id': '123'};
       const e = Error.StripeError.generate({
diff --git a/test/StripeResource.spec.js b/test/StripeResource.spec.js
index 8ee82833..6c60c2fd 100644
--- a/test/StripeResource.spec.js
+++ b/test/StripeResource.spec.js
@@ -240,18 +240,14 @@ describe('StripeResource', () => {
         });
       });
 
-      it('should not retry when a header says not to', (done) => {
+      it('should not retry on a 500 error when the method is POST', (done) => {
         nock(`https://${options.host}`)
           .post(options.path, options.params)
-          .reply(
-            500,
-            {
-              error: {
-                type: 'api_error',
-              },
+          .reply(500, {
+            error: {
+              type: 'api_error',
             },
-            {'stripe-should-retry': 'false'}
-          );
+          });
 
         realStripe.setMaxNetworkRetries(1);
 
@@ -261,29 +257,6 @@ describe('StripeResource', () => {
         });
       });
 
-      it('should retry when a header says it should, even on status codes we ordinarily wouldnt', (done) => {
-        nock(`https://${options.host}`)
-          .post(options.path, options.params)
-          .reply(
-            400,
-            {error: {type: 'your_fault'}},
-            {'stripe-should-retry': 'true'}
-          )
-          .post(options.path, options.params)
-          .reply(200, {
-            id: 'ch_123',
-            object: 'charge',
-            amount: 1000,
-          });
-
-        realStripe.setMaxNetworkRetries(1);
-
-        realStripe.charges.create(options.data, (err, charge) => {
-          expect(charge.id).to.equal('ch_123');
-          done(err);
-        });
-      });
-
       it('should handle OAuth errors gracefully', (done) => {
         nock('https://connect.stripe.com')
           .post('/oauth/token')
@@ -498,29 +471,17 @@ describe('StripeResource', () => {
 
     describe('_getSleepTimeInMS', () => {
       it('should not exceed the maximum or minimum values', () => {
+        let sleepSeconds;
         const max = stripe.getMaxNetworkRetryDelay();
         const min = stripe.getInitialNetworkRetryDelay();
 
         for (let i = 0; i < 10; i++) {
-          const sleepSeconds = stripe.invoices._getSleepTimeInMS(i) / 1000;
+          sleepSeconds = stripe.invoices._getSleepTimeInMS(i) / 1000;
 
           expect(sleepSeconds).to.be.at.most(max);
           expect(sleepSeconds).to.be.at.least(min);
         }
       });
-
-      it('should allow a maximum override', () => {
-        const maxSec = stripe.getMaxNetworkRetryDelay();
-        const minMS = stripe.getInitialNetworkRetryDelay() * 1000;
-
-        expect(stripe.invoices._getSleepTimeInMS(3, 0)).to.be.gt(minMS);
-        expect(stripe.invoices._getSleepTimeInMS(2, 3)).to.equal(3000);
-        expect(stripe.invoices._getSleepTimeInMS(0, 3)).to.equal(3000);
-        expect(stripe.invoices._getSleepTimeInMS(0, 0)).to.equal(minMS);
-        expect(stripe.invoices._getSleepTimeInMS(0, maxSec * 2)).to.equal(
-          maxSec * 2 * 1000
-        );
-      });
     });
   });
 });