Subscription update method mutates items

[jperasmus]

Hi there,

Thanks for this awesome library and for Stripe in general. A-grade 👏

Using:

node v10.9.0
stripe v6.15.0

When performing an update of a subscription's items I've discovered that the stripe.subscriptions.update() method mutates the payload provided to it by transforming the items from an array to an object with the array indexes as the object keys. This can lead to very unexpected behaviour when reusing the same payload after the update method was invoked.

I've managed to trace the problem to this utility method:

/**
   * 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[param] = utils.arrayToObject(data[param]);
    }
    return data;
},

Ideally a new transformed data object should be returned instead of mutating it. Something like:

const cloneDeep = require('lodash.cloneDeep');

/**
   * 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, payload) {
    const data = cloneDeep(payload);
    if (data[param] !== undefined) {
      data[param] = utils.arrayToObject(data[param]);
    }
    return data;
},

The problem would be for any of the other methods that use this utility method and not just for stripe.subscriptions.update().

Is this something you would be interested in fixing? I would be happy to create a PR.

[ob-stripe]

Hey @jperasmus, thanks for the detailed report!

I agree that this sounds like unexpected behavior and should be fixed, but my Node.js expertise is pretty limited. I'm not thrilled about pulling in a new dependency just for this though, even if it's a lodash module.

cc @rattrayalex-stripe @jlomas-stripe what do you think?

[jperasmus]

Hi @ob-stripe

Thanks for the reply. The reason I used the lodash cloneDeep as an example is that in the same file I saw the lodash.isplainobject package used in the same way.

JavaScript, unfortunately, doesn't currently have natively functionality to deeply clone an object, so you could either create a small utility yourself or use something like lodash's cloneDeep that has been battle tested. By using lodash's method you know it would cover most edge cases that might be missed in your own implementation.

In terms of impact on the bundle size, this might give you more insights to help with the decision: https://bundlephobia.com/[email protected]

It is definitely not the smallest package. Other smaller options might be https://bundlephobia.com/[email protected]

Another quick and dirty way would be to stringify and parse it again, but that can strip out any unserializable properties like functions: JSON.parse(JSON.stringify(original)) - would not necessarily recommend it.

[rattrayalex-stripe]

Thanks for reporting @jperasmus , and for the suggested fix! I'll try to take a look at this today.