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.

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

Do not mutate params when encoding arrays as indexed objects #526

Merged
merged 2 commits into from
Nov 26, 2018

Conversation

rattrayalex-stripe
Copy link
Contributor

Fixes #521

Also, for good measure, does a shallow copy of request data in general – I don't think there were any other situations that would have mutated this today, and it also doesn't protect against deep mutations, but I don't think it can hurt much (I wouldn't guess that the extra copy operations would have a meaningful perf impact).

cc @jperasmus
cc @ob-stripe
cc @enugent-stripe

@jlomas-stripe
Copy link
Contributor

lgtm 👍

@jperasmus
Copy link

jperasmus commented Nov 26, 2018

Thanks for the quick fix @rattrayalex-stripe

I see you went for the shallow copy approach. In this situation, it looks to be a good solution because the encodeParamWithIntegerIndexes utility method only applies the arrayToObject transformation one level deep.

For situations with more deeply nested data objects, it will still be copied over by reference, but luckily won't be mutated here.

One example I can think of right now is when you have a customer object with shipping details including the address object.

Something like:

const customer = await stripe.customers.update(customerId, {
  shipping: {
    address: {
      line1: 'Some street',
      // etc
    }
  }
});

@rattrayalex-stripe rattrayalex-stripe merged commit e40a4ce into master Nov 26, 2018
@rattrayalex-stripe rattrayalex-stripe deleted the ralex/dont-mutate-params branch November 26, 2018 22:05
@rattrayalex-stripe
Copy link
Contributor Author

@jperasmus - yep, sorry, should have addressed directly; for now, a shallow copy is fine since we don't (can't, actually, I think) modify deep properties (please correct me if I'm wrong!).

If/when we add support for "encoding" deeply nested values, it'll require a code change – I'll be sure to check that we're more careful with our inputs then 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants