Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor headers #667

Merged
merged 3 commits into from
Jul 31, 2019
Merged

Refactor headers #667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
800f098
Unrelated: Clarify README a bit
rattrayalex-stripe Jul 31, 2019
4f2bc28
Refactor headers
rattrayalex-stripe Jul 31, 2019
d50bcae
Make errors show up better in tests
rattrayalex-stripe Jul 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
```

Expand Down Expand Up @@ -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
Expand Down
114 changes: 60 additions & 54 deletions lib/StripeResource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'],
Expand Down Expand Up @@ -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),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we pass userSuppliedheaders into defaultIdempotencyKey and use whatever the user passed in (if they passed something in and after normalizing their input). It'll save us having to run uuid() on every POST request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code was uglier, and uuid() is fast:

> now = Date.now(); uuid(); Date.now() - now
2
> now = Date.now(); uuid(); Date.now() - now
0

We'd have to normalize the user headers first, and override in two places, which rubbed me the wrong way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eg; it'd be

const cleanedUserHeaders = normalize(userHeaders)
const defaultHeaders = {
//...
  'Idempotency-Key': cleanedUserHeaders['Idempotency-Key'] || this._defaultIdempotencyKey(method)
}
Object.assign(defaultHeaders, cleanedUserHeaders) // why override twice?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uuid is fast, so agreed that this isn't a big deal 👍

Copy link
Contributor

@tinovyatkin tinovyatkin Jul 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rattrayalex-stripe My 5 cents: uuid is a huge dependency (install size) for just one call across the library. You are using version 4 of UUID that is just a random value (not time sensitive, contrary version 1) in place where you are not required to have an UUID.
Doing something like Date.now() + crypto.randomBytes(40).toString('hex') is safer and faster for this use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof! Good catch, Tino. I'll remove that in a follow-up (please let me do it 😄 I have opinions on how I want it to happen).

};

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,
});
}
Expand All @@ -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 =
Expand All @@ -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'],
Expand Down Expand Up @@ -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);
});
Expand All @@ -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
);
};
},
};

Expand Down
41 changes: 35 additions & 6 deletions lib/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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('-');
},

/**
Expand Down
26 changes: 13 additions & 13 deletions test/StripeResource.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
});
Expand Down Expand Up @@ -67,7 +67,7 @@ describe('StripeResource', () => {
.reply(200, '{}');

realStripe.invoices.retrieveUpcoming(options.data, (err, response) => {
done();
done(err);
scope.done();
});
});
Expand All @@ -92,7 +92,7 @@ describe('StripeResource', () => {
'sub_123',
options.data,
(err, response) => {
done();
done(err);
scope.done();
}
);
Expand Down Expand Up @@ -175,7 +175,7 @@ describe('StripeResource', () => {

realStripe.charges.create(options.data, (err, charge) => {
expect(charge.id).to.equal('ch_123');
done();
done(err);
});
});

Expand All @@ -198,7 +198,7 @@ describe('StripeResource', () => {

realStripe.charges.create(options.data, (err, charge) => {
expect(charge.id).to.equal('ch_123');
done();
done(err);
});
});

Expand Down Expand Up @@ -272,7 +272,7 @@ describe('StripeResource', () => {

realStripe.charges.create(options.data, (err, charge) => {
expect(charge.id).to.equal('ch_123');
done();
done(err);
});
});

Expand All @@ -295,7 +295,7 @@ describe('StripeResource', () => {

realStripe.charges.retrieve('ch_123', (err, charge) => {
expect(charge.id).to.equal('ch_123');
done();
done(err);
});
});

Expand All @@ -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);
});
});

Expand Down
8 changes: 4 additions & 4 deletions test/utils.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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();
});
});
Expand Down