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

Reserve some critical field names when adding StripeObject accessors #970

Merged
merged 1 commit into from
Apr 2, 2021

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Apr 1, 2021

When populating StripeObjects, 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:

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.

Fixes #969.

r? @richardm-stripe @remi-stripe

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.
@remi-stripe
Copy link
Contributor

The PR looks great and I like that you only special-cased class for now since it's the only one reported. I'll defer to @richardm-stripe for the official approval though

@remi-stripe remi-stripe removed their assignment Apr 1, 2021
@brandur-stripe
Copy link
Contributor

Thanks guys!

@brandur-stripe brandur-stripe merged commit b9c7afd into master Apr 2, 2021
@remi-stripe remi-stripe deleted the brandur-reserved-fields branch September 28, 2023 23:07
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.

Metadata with the key class can't be properly marshaled
4 participants