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

Symbolize hash keys inside convert_to_stripe_object_with_params #1152

Conversation

mebezac
Copy link
Contributor

@mebezac mebezac commented Nov 30, 2022

Without this, perfectly good hash representations of Stripe classes get turned into generic Stripe::StripeObjects if they happen to have string keys.

Without this, perfectly good hash representations of Stripe classes
get turned into generic `Stripe::StripeObject`s if they happen to have
string keys.
@CLAassistant
Copy link

CLAassistant commented Nov 30, 2022

CLA assistant check
All committers have signed the CLA.

@pakrym-stripe
Copy link
Contributor

Hi @mebezac, can you describe the scenario where you encounter this issue?

@mebezac
Copy link
Contributor Author

mebezac commented Feb 10, 2023

Hi @mebezac, can you describe the scenario where you encounter this issue?

When we get a webhook from Stripe, we usually pass the data along to a sidekiq worker. We'll quite often pass event.data.object as an argument to the sidekiq worker. Sidekiq serializes all the parameters to JSON in order to store in redis. (https://github.com/mperham/sidekiq/blob/621cab6f756183607a0a5b7f676020d2b9bf05fd/lib/sidekiq/client.rb#L61-L66). That JSON serialization causes all keys to become strings.

Inside jobs, we sometimes we want to use the familiar/convenient methods that a Stripe object gives us. That means we'll take the data (which has been JSON-serialized) and turn it back into a Stripe object using Stripe::Util.convert_to_stripe_object. For now we've just been making sure to .symbolize_keys the data hash before passing it, but I think it would be nice for this library to accept hashes with string keys so that gotcha doesn't trip anyone else up.

Thanks!

@pakrym-stripe
Copy link
Contributor

Does using JSON.parse(json, symbolize_names: true) help you here?

I'm a bit reluctant to put an extra symbolize_names call in the main SDK path as it does a full copy of the entire object graph. This adds performance penalty for every production customer to enable a pretty narrow use-case.

@mebezac
Copy link
Contributor Author

mebezac commented Feb 21, 2023

@pakrym-stripe thanks for the feedback, how about the most recent commit? All that really matters is the object key when looking up the class to construct, construct_from does a symbolize_names and will handle any remaining string keys.

@pakrym-stripe
Copy link
Contributor

The change looks good but the linter is unhappy.

@pakrym-stripe pakrym-stripe merged commit 57a2806 into stripe:master Feb 21, 2023
@pakrym-stripe
Copy link
Contributor

Thank you for contribution @mebezac !

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

Successfully merging this pull request may close these issues.

3 participants