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

Always encode arrays as integer-indexed hashes #565

Merged
merged 1 commit into from
Feb 14, 2019
Merged

Always encode arrays as integer-indexed hashes #565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 0 additions & 4 deletions lib/resources/Invoices.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,11 @@ module.exports = StripeResource.extend({
if (urlData.invoiceOptions && typeof urlData.invoiceOptions === 'string') {
return url + '&subscription=' + urlData.invoiceOptions;
} else if (urlData.invoiceOptions && typeof urlData.invoiceOptions === 'object') {
if (urlData.invoiceOptions.subscription_items !== undefined) {
urlData.invoiceOptions.subscription_items = utils.arrayToObject(urlData.invoiceOptions.subscription_items);
}
return url + '&' + utils.stringifyRequestData(urlData.invoiceOptions);
}
return url;
},
urlParams: ['customerId', 'optional!invoiceOptions'],
encode: utils.encodeParamWithIntegerIndexes.bind(null, 'subscription_items'),
}),

sendInvoice: stripeMethod({
Expand Down
15 changes: 1 addition & 14 deletions lib/resources/Subscriptions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,12 @@
'use strict';

var StripeResource = require('../StripeResource');
var utils = require('../utils');
var stripeMethod = StripeResource.method;

module.exports = StripeResource.extend({

path: 'subscriptions',
includeBasic: ['list', 'retrieve', 'del',],

create: stripeMethod({
method: 'POST',
encode: utils.encodeParamWithIntegerIndexes.bind(null, 'items'),
}),

update: stripeMethod({
method: 'POST',
path: '{id}',
urlParams: ['id'],
encode: utils.encodeParamWithIntegerIndexes.bind(null, 'items'),
}),
includeBasic: ['create', 'list', 'retrieve', 'update', 'del',],

/**
* Subscription: Discount methods
Expand Down
33 changes: 5 additions & 28 deletions lib/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,11 @@ var utils = module.exports = {
* (forming the conventional key 'parent[child]=value')
*/
stringifyRequestData: function(data) {
return qs.stringify(data, {arrayFormat: 'brackets'});
return qs.stringify(data)
// Don't use strict form encoding by changing the square bracket control
// characters back to their literals. This is fine by the server, and
// makes these parameter strings easier to read.
.replace(/%5B/g, '[').replace(/%5D/g, ']');
},

/**
Expand Down Expand Up @@ -146,33 +150,6 @@ var utils = module.exports = {
return Constructor;
},

/**
* Encodes a particular param of data, whose value is an array, as an
* object with integer string attributes. Returns the entirety of data
* with just that param modified.
*/
encodeParamWithIntegerIndexes: function(param, data) {
if (data[param] !== undefined) {
data = Object.assign({}, data);
data[param] = utils.arrayToObject(data[param]);
}
return data;
},

/**
* Convert an array into an object with integer string attributes
*/
arrayToObject: function(arr) {
if (Array.isArray(arr)) {
var obj = {};
arr.map(function(item, i) {
obj[i.toString()] = item;
});
return obj;
}
return arr;
},

/**
* Secure compare, from https://github.com/freewil/scmp
*/
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
},
"dependencies": {
"lodash.isplainobject": "^4.0.6",
"qs": "~6.5.1",
"qs": "^6.6.0",
"safe-buffer": "^5.1.1",
"uuid": "^3.3.2"
},
Expand Down
58 changes: 58 additions & 0 deletions test/StripeResource.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,64 @@ describe('StripeResource', function() {
});
});

describe('Parameter encoding', function() {
// Use a real instance of stripe as we're mocking the http.request responses.
var realStripe = require('../lib/stripe')(utils.getUserStripeKey());

after(function() {
nock.cleanAll();
})

describe('_request', function() {
it('encodes the query string in GET requests', function(done) {
var options = {
host: stripe.getConstant('DEFAULT_HOST'),
path: '/v1/invoices/upcoming',
data: {
subscription_items: [
{plan: 'foo', quantity: 2},
{id: 'si_123', deleted: true},
],
},
};

const scope = nock('https://' + options.host)
.get(options.path)
.query(Object.assign({customer: 'cus_123'}, options.data))
.reply(200, '{}');

realStripe.invoices.retrieveUpcoming('cus_123', options.data, function(err, response) {
done();
scope.done();
});
});

it('encodes the body in POST requests', function(done) {
var options = {
host: stripe.getConstant('DEFAULT_HOST'),
path: '/v1/subscriptions/sub_123',
data: {
customer: 'cus_123',
items: [
{plan: 'foo', quantity: 2},
{id: 'si_123', deleted: true},
],
},
body: 'customer=cus_123&items[0][plan]=foo&items[0][quantity]=2&items[1][id]=si_123&items[1][deleted]=true',
ob-stripe marked this conversation as resolved.
Show resolved Hide resolved
};

const scope = nock('https://' + options.host)
.post(options.path, options.body)
.reply(200, '{}');

realStripe.subscriptions.update('sub_123', options.data, function(err, response) {
done();
scope.done();
});
});
});
});

describe('Retry Network Requests', function() {
// Use a real instance of stripe as we're mocking the http.request responses.
var realStripe = require('../lib/stripe')(utils.getUserStripeKey());
Expand Down
19 changes: 6 additions & 13 deletions test/resources/Invoices.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ describe('Invoices Resource', function() {
expect(stripe.LAST_REQUEST).to.deep.equal({
method: 'GET',
url: '/v1/invoices/upcoming?customer=cus_123&' +
'subscription_items%5B0%5D%5Bplan%5D=potato&subscription_items%5B1%5D%5Bplan%5D=rutabaga',
'subscription_items[0][plan]=potato&subscription_items[1][plan]=rutabaga',
headers: {},
data: {},
});
Expand All @@ -137,18 +137,11 @@ describe('Invoices Resource', function() {
url: '/v1/invoices/upcoming?customer=cus_123&subscription=sub_123',
headers: {},
data: {
subscription_items: {
0: {
plan: 'potato',
},
1: {
plan: 'rutabaga',
},
2: {
deleted: true,
id: 'SOME_ID',
},
},
subscription_items: [
{plan: 'potato'},
{plan: 'rutabaga'},
{id: 'SOME_ID', deleted: true},
],
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm somewhat worried about our test coverage reduction here... is there anything in our test suite to verify that these POST bodies are form-encoded with subscription_items[0][plan] instead of subscription_items[][plan] ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair! Looking into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so this is difficult to test in the current state. The URL-encoding of the request parameters happens in the original _request method, but getSpyableStripe() returns an instance of stripe where _request is patched to memorize the method parameters instead of actually sending the request.

We've recently added nock for testing the telemetry stuff, so we could probably write some new tests that use it to check the actual body of our HTTP requests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think nock sounds like the right tool for this job, should be pretty straightforward - fft DM me or paul if you need help with it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I lied, we didn't add nock at all. No idea why I thought that was the case!

Nevertheless, I tried writing a test with nock to test StripeResource._request, but I can't seem to have nock intercept the request -- for some reason the request is always sent to the real API server. Mind giving it a try @rattrayalex-stripe?

Copy link
Contributor

Choose a reason for hiding this comment

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

We add nock in #559 😄

subscription_prorate: true,
},
});
Expand Down
20 changes: 10 additions & 10 deletions test/resources/Subscriptions.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,12 @@ describe('subscriptions Resource', function() {
url: '/v1/subscriptions/test_sub',
headers: {},
data: {
items: {
'0': {
'plan': 'foo',
'quantity': 2,
items: [
{
plan: 'foo',
quantity: 2,
},
},
],
},
});
});
Expand All @@ -105,12 +105,12 @@ describe('subscriptions Resource', function() {
url: '/v1/subscriptions',
headers: {},
data: {
items: {
'0': {
'plan': 'foo',
'quantity': 2,
items: [
{
plan: 'foo',
quantity: 2,
},
},
],
},
});
});
Expand Down
70 changes: 10 additions & 60 deletions test/utils.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,33 +34,33 @@ describe('utils', function() {
});

it('Handles deeply nested object', function() {
expect(decodeURI(utils.stringifyRequestData({
expect(utils.stringifyRequestData({
a: {
b: {
c: {
d: 2,
},
},
},
}))).to.equal('a[b][c][d]=2');
})).to.equal('a[b][c][d]=2');
});

it('Handles arrays of objects', function() {
expect(decodeURI(utils.stringifyRequestData({
expect(utils.stringifyRequestData({
a: [
{b: 'c'},
{b: 'd'},
],
}))).to.equal('a[][b]=c&a[][b]=d');
})).to.equal('a[0][b]=c&a[1][b]=d');
ob-stripe marked this conversation as resolved.
Show resolved Hide resolved
})

it('Handles indexed arrays', function() {
expect(decodeURI(utils.stringifyRequestData({
expect(utils.stringifyRequestData({
a: {
0: 'c',
1: 'd',
0: {b: 'c'},
1: {b: 'd'},
},
}))).to.equal('a[0]=c&a[1]=d');
})).to.equal('a[0][b]=c&a[1][b]=d');
ob-stripe marked this conversation as resolved.
Show resolved Hide resolved
})

it('Creates a string from an object, handling shallow nested objects', function() {
Expand All @@ -76,18 +76,10 @@ describe('utils', function() {
'test=1',
'foo=baz',
'somethingElse=%3A%3A%22%22%25%26',
'nested%5B1%5D=2', // Unencoded: nested[1]=2
'nested%5Ba%20n%20o%20t%20h%20e%20r%5D=',
'nested[1]=2',
'nested[a%20n%20o%20t%20h%20e%20r]=',
].join('&'));
});

describe('Stripe-specific cases', function() {
it('Handles the `expand` array correctly (producing the form `expand[]=_` for each item', function() {
expect(decodeURI(utils.stringifyRequestData({
expand: ['a', 'foo', 'a.b.c'],
}))).to.equal('expand[]=a&expand[]=foo&expand[]=a.b.c');
});
});
ob-stripe marked this conversation as resolved.
Show resolved Hide resolved
});

describe('protoExtend', function() {
Expand Down Expand Up @@ -246,48 +238,6 @@ describe('utils', function() {
});
});

describe('encodeParamWithIntegerIndexes', function() {
it('handles param not existing in data', function() {
var data = {'someParam': ['foo']};
expect(utils.encodeParamWithIntegerIndexes('anotherParam', data)).to.deep.equal(data);
});

it('encodes just the specified param with integer indexes', function() {
var data = {'paramToEncode': ['value1', 'value2'], 'anotherParam': ['foo']};
var expectedData = {'paramToEncode': {'0': 'value1', '1': 'value2'}, 'anotherParam': ['foo']};
expect(utils.encodeParamWithIntegerIndexes('paramToEncode', data)).to.deep.equal(expectedData);
});

it('encodes just the specified param with integer indexes when used via partial application', function() {
var data = {'paramToEncode': ['value1', 'value2'], 'anotherParam': ['foo']};
var expectedData = {'paramToEncode': {'0': 'value1', '1': 'value2'}, 'anotherParam': ['foo']};
var partial = utils.encodeParamWithIntegerIndexes.bind(null, 'paramToEncode');
expect(partial(data)).to.deep.equal(expectedData);
});

it('does not mutate input variables', function() {
var data = {'paramToEncode': ['value1']};
var expectedData = {'paramToEncode': {'0': 'value1'}};
expect(utils.encodeParamWithIntegerIndexes('paramToEncode', data)).to.deep.equal(expectedData);
expect(data).not.to.deep.equal(expectedData);
expect(Array.isArray(data.paramToEncode)).to.equal(true);
});
});

describe('arrayToObject', function() {
it('handles an empty array', function() {
expect(utils.arrayToObject([])).to.deep.equal({});
});
it('handles an array of integers', function() {
var arr = [1, 3];
expect(utils.arrayToObject(arr)).to.deep.equal({'0': 1, '1': 3});
});
it('ignores passes non-array data through', function() {
var arr = '3';
expect(utils.arrayToObject(arr)).to.deep.equal('3');
});
});

describe('secureCompare', function() {
it('returns true given two equal things', function() {
expect(utils.secureCompare('potato', 'potato')).to.equal(true);
Expand Down