Always encode arrays as integer-indexed hashes

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({

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

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
   */

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"
   },

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());

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,
           },
         });

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,
             },
-          },
+          ],
         },
       });
     });

test/utils.spec.js

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