diff --git a/lib/resources/Invoices.js b/lib/resources/Invoices.js index db304e947d..bcd9645cd7 100644 --- a/lib/resources/Invoices.js +++ b/lib/resources/Invoices.js @@ -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({ diff --git a/lib/resources/Subscriptions.js b/lib/resources/Subscriptions.js index 5932dec833..a014c5d85a 100644 --- a/lib/resources/Subscriptions.js +++ b/lib/resources/Subscriptions.js @@ -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 diff --git a/lib/utils.js b/lib/utils.js index 0c7b7766ae..73629b914b 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -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, ']'); }, /** @@ -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 */ diff --git a/package.json b/package.json index d48f7043c1..d9adc2d1ec 100644 --- a/package.json +++ b/package.json @@ -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" }, diff --git a/test/StripeResource.spec.js b/test/StripeResource.spec.js index 4f095b9fe8..ccfca3becf 100644 --- a/test/StripeResource.spec.js +++ b/test/StripeResource.spec.js @@ -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', + }; + + 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()); diff --git a/test/resources/Invoices.spec.js b/test/resources/Invoices.spec.js index 0750debb53..17236f65ee 100644 --- a/test/resources/Invoices.spec.js +++ b/test/resources/Invoices.spec.js @@ -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: {}, }); @@ -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}, + ], subscription_prorate: true, }, }); diff --git a/test/resources/Subscriptions.spec.js b/test/resources/Subscriptions.spec.js index 8cf1661a6b..8e732dab61 100644 --- a/test/resources/Subscriptions.spec.js +++ b/test/resources/Subscriptions.spec.js @@ -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, }, - }, + ], }, }); }); @@ -105,12 +105,12 @@ describe('subscriptions Resource', function() { url: '/v1/subscriptions', headers: {}, data: { - items: { - '0': { - 'plan': 'foo', - 'quantity': 2, + items: [ + { + plan: 'foo', + quantity: 2, }, - }, + ], }, }); }); diff --git a/test/utils.spec.js b/test/utils.spec.js index b252f34854..2cb89db83e 100644 --- a/test/utils.spec.js +++ b/test/utils.spec.js @@ -34,7 +34,7 @@ describe('utils', function() { }); it('Handles deeply nested object', function() { - expect(decodeURI(utils.stringifyRequestData({ + expect(utils.stringifyRequestData({ a: { b: { c: { @@ -42,25 +42,25 @@ describe('utils', function() { }, }, }, - }))).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'); }) 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'); }) it('Creates a string from an object, handling shallow nested objects', function() { @@ -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'); - }); - }); }); describe('protoExtend', function() { @@ -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);