stripity-stripe icon indicating copy to clipboard operation
stripity-stripe copied to clipboard

Stripe.Util.map_keys_to_atoms error on unknown keys

Open kerryjj opened this issue 5 years ago • 1 comments

If a Stripe API response returns any json key string that isn't in the code or library as an atom already we receive the following error.

nofile :erlang.binary_to_existing_atom/2 stripity_stripe lib/stripe/util.ex:34 anonymous fn/1 in Stripe.Util.map_keys_to_atoms/1 elixir lib/map.ex:210 Map.new_transform/3 stripity_stripe lib/stripe/api.ex:176 Stripe.API.request/5 stripity_stripe lib/stripe/request.ex:193 Stripe.Request.make_request/1

I find that quite often there are keys that are returned that I have no interest in using and no reason to be creating them in atoms beforehand.

Converting to atoms is fine, but I'd like to suggest that we don't require it to be an atom that already exists to do the conversion. i.e. In this case change String.to_existing_atom to use String.to_atom instead.

The String.to_atom docs say this "Warning: this function creates atoms dynamically and atoms are not garbage-collected. Therefore, string should not be an untrusted value, such as input received from a socket or during a web request. Consider using to_existing_atom/1 instead."

Using to_existing_atom makes sense when the source of the strings isn't trusted (like user input). However, in our case we're talking about keys from the Stripe API which I think can be trusted. In fact what we need this piece of code to do is to make sure it converts all key strings that it receives from Stripe API into atoms. I can't think of any case where we wouldn't want this conversion to happen, so I think it's safe to use String.to_atom instead.

Let me know what you all think and also let me know if I'm missing something here.

kerryjj avatar Jun 19 '19 15:06 kerryjj

I'm fine with this change. I believe I may be the author of the original code and in all honesty, the opt for .to_existing_atom came from not fully understanding it, since I was new to elixir at that time.

begedin avatar Jun 22 '19 13:06 begedin

This issue has been automatically marked as "stale:discard". If this issue still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment.

github-actions[bot] avatar Oct 23 '23 02:10 github-actions[bot]

Closing this issue after a prolonged period of inactivity. If this issue is still relevant, feel free to re-open the issue. Thank you!

github-actions[bot] avatar Nov 07 '23 02:11 github-actions[bot]