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

Check for @values in StripeObject#method_not_found #122

Merged
merged 1 commit into from
Jul 9, 2014

Conversation

mcolyer
Copy link
Contributor

@mcolyer mcolyer commented Feb 24, 2014

Previous to 1.10.0 StripeObjects could be deserialized from YAML without issue. I believe #99 introduced an issue where @values.has_key? is called before @values is initialized which prevents proper deserialization from occurring.

This is the backtrace, I got and I've verified this change resolves the issue:

rbenv/versions/2.1.0/lib/ruby/gems/2.1.0/gems/stripe-1.10.1/lib/stripe/stripe_object.rb:184:in `respond_to_missing?'
rbenv/versions/2.1.0/lib/ruby/2.1.0/psych/visitors/to_ruby.rb:343:in `respond_to?'
rbenv/versions/2.1.0/lib/ruby/2.1.0/psych/visitors/to_ruby.rb:343:in `init_with'
rbenv/versions/2.1.0/lib/ruby/2.1.0/psych/visitors/to_ruby.rb:336:in `revive'
rbenv/versions/2.1.0/lib/ruby/2.1.0/psych/visitors/to_ruby.rb:201:in `visit_Psych_Nodes_Mapping'

If a StripeObject is being deserialized by psych, @values.has_key? is
called before @values is initialized which prevents proper
deserialization from occurring. Checking for existence first resolves
the issue.
@bikramwp
Copy link

👍 This works for me. When is this pull request likely to be merged?

@synth
Copy link

synth commented May 13, 2014

+1 hitting this as well.

@jeberly
Copy link

jeberly commented May 23, 2014

+1 happening to me as well

@jeberly
Copy link

jeberly commented May 23, 2014

fwiw, I got around this in rails by switching from yaml to json store.

eg. serialize :charge_response, JSON

@descentintomael
Copy link

+1, getting this too.

@barkerja
Copy link

Same here. 👍

@theworkerant
Copy link

+1 got this as well

@amfeng
Copy link
Contributor

amfeng commented Jul 8, 2014

Hi all, sorry for the absurdly late response here.

Huh, that's weird. I wonder if there's something buggy happening upstream that we'll just be covering up by adding the nil check -- will investigate.

@amfeng
Copy link
Contributor

amfeng commented Jul 9, 2014

Ok, after digging into this it looks like this is because init_with in Psych calls allocate[0] instead of new, which doesn't end up actually initializing any of the state or @values that we need and breaks when it later tries to duck type[1] the object.

Not really sure there's a way to get around this, and Psych fills in all of the instance variables that would otherwise be set by initialize so everything will still work as expected. I'm a little nervous about the nil check possibly hiding bad initialization behavior in the future, but this seems like an appropriate fix for now.

0: https://github.com/tenderlove/psych/blob/master/lib/psych/visitors/to_ruby.rb#L339
1: https://github.com/tenderlove/psych/blob/master/lib/psych/visitors/to_ruby.rb#L347

amfeng added a commit that referenced this pull request Jul 9, 2014
Check for `@values` in StripeObject#method_not_found.

This is necessary because we check for `@values` in `respond_to_missing?` now, but this isn't necessarily always set. For example, in deserializing from YAML, Psych creates the object using `allocate` and not `new`, which ends up skipping any initializing of instance variables. See #122 for more details.
@amfeng amfeng merged commit 8bb6acf into stripe:master Jul 9, 2014
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.

9 participants