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

Stripe.Util.map_keys_to_atoms error on unknown keys #516

Closed
kerryjj opened this issue Jun 19, 2019 · 3 comments
Closed

Stripe.Util.map_keys_to_atoms error on unknown keys #516

kerryjj opened this issue Jun 19, 2019 · 3 comments

Comments

@kerryjj
Copy link
Contributor

kerryjj commented Jun 19, 2019

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.

@begedin
Copy link
Contributor

begedin commented Jun 22, 2019

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.

@github-actions
Copy link

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.

Copy link

github-actions bot commented Nov 7, 2023

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 github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants