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

Metadata with the key class can't be properly marshaled #969

Closed
thomas07vt opened this issue Apr 1, 2021 · 2 comments · Fixed by #970
Closed

Metadata with the key class can't be properly marshaled #969

thomas07vt opened this issue Apr 1, 2021 · 2 comments · Fixed by #970

Comments

@thomas07vt
Copy link

thomas07vt commented Apr 1, 2021

Ruby Version: ruby 2.6.0
Stripe Gem Version: 5.28.0

If a Stripe object (such as a charge) has metadata with a key of class then Stripe can't properly marshal the objects. In my case I am writing to the Rails cache which is backed by Redis. It is able to write the data to cache correctly but when I read from the cache, it throws an error. For instance if the metadata is something like this:

{ class: "someschoolclassname" }

Then when I try to read hydrate the marshaled data I get this error:

NoMethodError: undefined method `construct_from' for "someschoolclassname":String 

Here is the back trace:

"/app/vendor/bundle/ruby/2.6.0/gems/stripe-5.28.0/lib/stripe/stripe_object.rb" line 528 in deep_copy
"/app/vendor/bundle/ruby/2.6.0/gems/stripe-5.28.0/lib/stripe/stripe_object.rb" line 524 in block in deep_copy
"/app/vendor/bundle/ruby/2.6.0/gems/stripe-5.28.0/lib/stripe/stripe_object.rb" line 523 in each
"/app/vendor/bundle/ruby/2.6.0/gems/stripe-5.28.0/lib/stripe/stripe_object.rb" line 523 in each_with_object
"/app/vendor/bundle/ruby/2.6.0/gems/stripe-5.28.0/lib/stripe/stripe_object.rb" line 523 in deep_copy
"/app/vendor/bundle/ruby/2.6.0/gems/stripe-5.28.0/lib/stripe/stripe_object.rb" line 413 in initialize_from
"/app/vendor/bundle/ruby/2.6.0/gems/stripe-5.28.0/lib/stripe/stripe_object.rb" line 233 in marshal_load

So basically it is initially a StripeObject, then on line 423 of lib/stripe/stripe_object.rb it adds the class accessor, which messes with the construct_from method.

At least from what I can gather.

@thomas07vt thomas07vt changed the title Metadata with the key class can't be properly marshalled Metadata with the key class can't be properly marshaled Apr 1, 2021
brandur-stripe pushed a commit that referenced this issue Apr 1, 2021
When populating `StripeObject`s, we add accessors to them so that people
can access fields like `obj.currency`.

This was probably only meant to apply to API resources, but through
what might have been an accident of history, we've also traditionally
unmarshaled any hash that comes back from the API as a `StripeObject`,
including `metadata` fields. This allows some convenience because users
can access values like `obj.metadata.my_field`, but is also obviously a
minefield for potential problems.

In issue #969, what's essentially happening is that because there's a
metadata field named `class`, we've overwritten the object's normal
`class` method with our own custom one that accesses the metadata value.
Amazingly, the object can still marshal/unmarshal mostly properly, but
fails on this line as we try to access `obj.class` and that turns out to
be a metadata value instead of a class:

``` ruby
when StripeObject
  obj.class.construct_from(
    ...
```

Here I solve the problem by banning accessors added with the name
`class`. This has a slight risk of backward incompatibility in that
users that previously had metadata named "class" will now have to use
square bracket accessors instead like `obj.metadata[:class]`, but
honestly, I just can't see anything good in allowing "class" to be used
as an accessor.

An alternative solution might be to alias `class` in `StripeObject` and
then make sure we always use that in places like `initialize_from` and
`deep_copy`.

The best long term solution would be to stop add accessors to metadata
objects. This just seems like a bad idea given that there are still
myriads of Ruby built-ins that could potentially be overwritten. This is
definitely a considerably-sized breaking change though, so we'd have to
do it on a major.
@brandur-stripe
Copy link
Contributor

Started a fix in #970.

brandur-stripe pushed a commit that referenced this issue Apr 2, 2021
#970)

When populating `StripeObject`s, we add accessors to them so that people
can access fields like `obj.currency`.

This was probably only meant to apply to API resources, but through
what might have been an accident of history, we've also traditionally
unmarshaled any hash that comes back from the API as a `StripeObject`,
including `metadata` fields. This allows some convenience because users
can access values like `obj.metadata.my_field`, but is also obviously a
minefield for potential problems.

In issue #969, what's essentially happening is that because there's a
metadata field named `class`, we've overwritten the object's normal
`class` method with our own custom one that accesses the metadata value.
Amazingly, the object can still marshal/unmarshal mostly properly, but
fails on this line as we try to access `obj.class` and that turns out to
be a metadata value instead of a class:

``` ruby
when StripeObject
  obj.class.construct_from(
    ...
```

Here I solve the problem by banning accessors added with the name
`class`. This has a slight risk of backward incompatibility in that
users that previously had metadata named "class" will now have to use
square bracket accessors instead like `obj.metadata[:class]`, but
honestly, I just can't see anything good in allowing "class" to be used
as an accessor.

An alternative solution might be to alias `class` in `StripeObject` and
then make sure we always use that in places like `initialize_from` and
`deep_copy`.

The best long term solution would be to stop add accessors to metadata
objects. This just seems like a bad idea given that there are still
myriads of Ruby built-ins that could potentially be overwritten. This is
definitely a considerably-sized breaking change though, so we'd have to
do it on a major.
@brandur-stripe
Copy link
Contributor

Fix released in 5.31.0.

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 a pull request may close this issue.

2 participants