Skip to content

Commit

Permalink
stripe-node v13 release (#1808)
Browse files Browse the repository at this point in the history 
* Fix retry override behavior

* Add failing test

* (next major) one network retry by default

* adjust tests

* Cleanup

* Remove subscriptions.del test (#1869)

Remove subscriptions del test

---------

Co-authored-by: anniel-stripe <97691964+anniel-stripe@users.noreply.github.com>
  • Loading branch information
@richardm-stripe @anniel-stripe
richardm-stripe and anniel-stripe committed Aug 16, 2023
1 parent ca229a6 commit 07a8d2a
Show file tree
Hide file tree
Showing 9 changed files with 95 additions and 44 deletions.
12 changes: 8 additions & 4 deletions README.md
View file
Expand Up @@ -186,7 +186,7 @@ const stripe = Stripe('sk_test_...', {
| Option | Default | Description |
| ------------------- | ------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `apiVersion` | `null` | Stripe API version to be used. If not set, stripe-node will use the latest version at the time of release. |
| `maxNetworkRetries` | 0 | The amount of times a request should be [retried](#network-retries). |
| `maxNetworkRetries` | 1 | The amount of times a request should be [retried](#network-retries). |
| `httpAgent` | `null` | [Proxy](#configuring-a-proxy) agent to be used by the library. |
| `timeout` | 80000 | [Maximum time each request can take in ms.](#configuring-timeout) |
| `host` | `'api.stripe.com'` | Host that requests are made to. |
Expand Down Expand Up @@ -253,10 +253,14 @@ if (process.env.http_proxy) {

### Network retries

Automatic network retries can be enabled with the `maxNetworkRetries` config option.
This will retry requests `n` times with exponential backoff if they fail due to an intermittent network problem.
[Idempotency keys](https://stripe.com/docs/api/idempotent_requests) are added where appropriate to prevent duplication.
As of [v13](https://github.com/stripe/stripe-node/releases/tag/v13.0.0) stripe-node will automatically do one reattempt for failed requests that are safe to retry. Automatic network retries can be disabled by setting the `maxNetworkRetries` config option to `0`. You can also set a higher number to reattempt multiple times, with exponential backoff. [Idempotency keys](https://stripe.com/docs/api/idempotent_requests) are added where appropriate to prevent duplication.

```js
const stripe = Stripe('sk_test_...', {
maxNetworkRetries: 0, // Disable retries
});

```
```js
const stripe = Stripe('sk_test_...', {
maxNetworkRetries: 2, // Retry a request twice before giving up
Expand Down
3 changes: 1 addition & 2 deletions src/RequestSender.ts
View file
Expand Up @@ -287,7 +287,7 @@ export class RequestSender {

// Max retries can be set on a per request basis. Favor those over the global setting
_getMaxNetworkRetries(settings: RequestSettings = {}): number {
return settings.maxNetworkRetries &&
return settings.maxNetworkRetries !== undefined &&
Number.isInteger(settings.maxNetworkRetries)
? settings.maxNetworkRetries
: this._stripe.getMaxNetworkRetries();
Expand Down Expand Up @@ -474,7 +474,6 @@ export class RequestSender {
const requestRetries = numRetries || 0;

const maxRetries = this._getMaxNetworkRetries(options.settings || {});

this._stripe._emitter.emit('request', requestEvent);

req
Expand Down
2 changes: 1 addition & 1 deletion src/stripe.core.ts
View file
Expand Up @@ -113,7 +113,7 @@ export function createStripe(
maxNetworkRetries: validateInteger(
'maxNetworkRetries',
props.maxNetworkRetries,
0
1
),
agent: agent,
httpClient:
Expand Down
66 changes: 59 additions & 7 deletions test/RequestSender.spec.ts
View file
Expand Up @@ -92,7 +92,7 @@ describe('RequestSender', () => {
// Use a real instance of stripe as we're mocking the http.request responses.
const realStripe = require('../src/stripe.cjs.node.js')('sk_test_xyz');

after(() => {
afterEach(() => {
nock.cleanAll();
});

Expand Down Expand Up @@ -337,7 +337,9 @@ describe('RequestSender', () => {

describe('Retry Network Requests', () => {
// Use a real instance of stripe as we're mocking the http.request responses.
const realStripe = require('../src/stripe.cjs.node.js')('sk_test_xyz');
const realStripe = require('../src/stripe.cjs.node.js')(FAKE_API_KEY, {
maxNetworkRetries: 0,
});

// Override the sleep timer to speed up tests
realStripe.charges._getSleepTimeInMS = () => 0;
Expand All @@ -359,10 +361,6 @@ describe('RequestSender', () => {
stripe._setApiNumberField('maxNetworkRetries', 0);
});

after(() => {
nock.cleanAll();
});

describe('_request', () => {
it('throws an error on connection failure', (done) => {
// Mock the connection error.
Expand All @@ -378,7 +376,7 @@ describe('RequestSender', () => {

it('throws an error on connection timeout', (done) => {
return getTestServerStripe(
{timeout: 10},
{timeout: 10, maxNetworkRetries: 0},
(req, res) => {
// Do nothing. This will trigger a timeout.
},
Expand Down Expand Up @@ -603,6 +601,60 @@ describe('RequestSender', () => {
});
});

it('should give precedence to request-level (1) vs client-level maxNetworkRetries (0)', (done) => {
let nReceivedRequests = 0;
nock(`https://${options.host}`)
.post(options.path, options.params)
.reply((_1, _2, callback) => {
nReceivedRequests += 1;
callback(new Error('bad stuff'));
})
.post(options.path, options.params)
.reply((_1, _2, callback) => {
nReceivedRequests += 1;
callback(new Error('worse stuff'));
});

realStripe._setApiNumberField('maxNetworkRetries', 0);

realStripe.charges.create(
options.data,
{maxNetworkRetries: 1},
(err) => {
const errorMessage = RequestSender._generateConnectionErrorMessage(
1
);
expect(err.message).to.equal(errorMessage);
expect(err.detail.message).to.deep.equal('worse stuff');
expect(nReceivedRequests).to.equal(2);
done();
}
);
});

it('should give precedence to request-level (0) vs client-level maxNetworkRetries (1)', (done) => {
nock(`https://${options.host}`)
.post(options.path, options.params)
.reply((_1, _2, callback) => {
callback(new Error('bad stuff'));
});

realStripe._setApiNumberField('maxNetworkRetries', 1);

realStripe.charges.create(
options.data,
{maxNetworkRetries: 0},
(err) => {
expect(err.detail.message).to.deep.equal('bad stuff');
const errorMessage = RequestSender._generateConnectionErrorMessage(
0
);
expect(err.message).to.equal(errorMessage);
done();
}
);
});

it('should retry on a 409 error', (done) => {
nock(`https://${options.host}`)
.post(options.path, options.params)
Expand Down
23 changes: 14 additions & 9 deletions test/StripeResource.spec.ts
View file
Expand Up @@ -211,7 +211,7 @@ describe('StripeResource', () => {
});

it('failure', (callback) => {
const handleRequest = (req, res) => {
const handleRequest = (_req, res, nPreviousRequests) => {
setTimeout(() => res.writeHead(500));
setTimeout(
() =>
Expand All @@ -221,6 +221,8 @@ describe('StripeResource', () => {
10
);
setTimeout(() => res.end(), 20);
// fail twice, then close the server
return {shouldStayOpen: nPreviousRequests < 1};
};

getTestServerStripe({}, handleRequest, (err, stripe, closeServer) => {
Expand All @@ -230,17 +232,20 @@ describe('StripeResource', () => {

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();
});
return foos.pdf(
{id: 'foo_123'},
{host: 'localhost', maxNetworkRetries: 1},
(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();
}
);
});
});
});

describe('makeRequest args', () => {
it('does not mutate user-supplied deprecated opts', () => {
const args = [
Expand Down
5 changes: 3 additions & 2 deletions test/resources/Quotes.spec.js
View file
Expand Up @@ -169,7 +169,7 @@ describe('Quotes Resource', () => {
});

it('failure', (callback) => {
const handleRequest = (req, res) => {
const handleRequest = (req, res, nPreviousRequests) => {
setTimeout(() => res.writeHead(500));
setTimeout(
() =>
Expand All @@ -179,6 +179,7 @@ describe('Quotes Resource', () => {
10
);
setTimeout(() => res.end(), 20);
return {shouldStayOpen: nPreviousRequests < 1};
};

testUtils.getTestServerStripe(
Expand All @@ -191,7 +192,7 @@ describe('Quotes Resource', () => {

return stripe.quotes.pdf(
'foo_123',
{host: 'localhost'},
{host: 'localhost', maxNetworkRetries: 1},
(err, res) => {
closeServer();
expect(err).to.exist;
Expand Down
13 changes: 0 additions & 13 deletions test/resources/Subscriptions.spec.js
View file
Expand Up @@ -17,19 +17,6 @@ describe('subscriptions Resource', () => {
});
});

describe('del', () => {
it('Sends the correct request', () => {
stripe.subscriptions.del('test_sub');
expect(stripe.LAST_REQUEST).to.deep.equal({
method: 'DELETE',
url: '/v1/subscriptions/test_sub',
headers: {},
data: {},
settings: {},
});
});
});

describe('update', () => {
it('Sends the correct request', () => {
stripe.subscriptions.update('test_sub', {
Expand Down
6 changes: 3 additions & 3 deletions test/stripe.spec.ts
View file
Expand Up @@ -586,12 +586,12 @@ describe('Stripe Module', function() {
});

describe('when passed in via the config object', () => {
it('should default to 0 if a non-integer is passed', () => {
it('should default to 1 if a non-integer is passed', () => {
const newStripe = Stripe(FAKE_API_KEY, {
maxNetworkRetries: 'foo',
});

expect(newStripe.getMaxNetworkRetries()).to.equal(0);
expect(newStripe.getMaxNetworkRetries()).to.equal(1);

expect(() => {
Stripe(FAKE_API_KEY, {
Expand All @@ -613,7 +613,7 @@ describe('Stripe Module', function() {
it('should use the default', () => {
const newStripe = Stripe(FAKE_API_KEY);

expect(newStripe.getMaxNetworkRetries()).to.equal(0);
expect(newStripe.getMaxNetworkRetries()).to.equal(1);
});
});
});
Expand Down
9 changes: 6 additions & 3 deletions test/testUtils.ts
View file
Expand Up @@ -27,16 +27,19 @@ export const getTestServerStripe = (
clientOptions: RequestSettings,
handler: (
req: http.IncomingMessage,
res: http.ServerResponse
) => {shouldStayOpen?: true} | null,
res: http.ServerResponse,
nPreviousRequests: number
) => {shouldStayOpen?: boolean} | null,
callback: (
err: Error | null,
stripe: StripeClient,
closeServer: () => void
) => void
): void => {
let nPreviousRequests = 0;
const server = http.createServer((req, res) => {
const {shouldStayOpen} = handler(req, res) || {};
const {shouldStayOpen} = handler(req, res, nPreviousRequests) || {};
nPreviousRequests += 1;
if (!shouldStayOpen) {
res.on('close', () => {
server.close();
Expand Down

0 comments on commit 07a8d2a

Please sign in to comment.