Reserve some critical field names when adding StripeObject accessors (

#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.

lib/stripe/stripe_object.rb

@@ -267,6 +267,27 @@ def self.protected_fields
       []
     end
 
+    # When designing APIs, we now make a conscious effort server-side to avoid
+    # naming fields after important built-ins in various languages (e.g. class,
+    # method, etc.).
+    #
+    # However, a long time ago we made the mistake (either consciously or by
+    # accident) of initializing our `metadata` fields as instances of
+    # `StripeObject`, and metadata can have a wide range of different keys
+    # defined in it. This is somewhat a convenient in that it allows users to
+    # access data like `obj.metadata.my_field`, but is almost certainly not
+    # worth the cost.
+    #
+    # Naming metadata fields bad things like `class` causes `initialize_from`
+    # to produce strange results, so we ban known offenders here.
+    #
+    # In a future major version we should consider leaving `metadata` as a hash
+    # and forcing people to access it with `obj.metadata[:my_field]` because
+    # the potential for trouble is just too high. For now, reserve names.
+    RESERVED_FIELD_NAMES = [
+      :class,
+    ].freeze
+
     protected def metaclass
       class << self; self; end
     end
@@ -277,6 +298,7 @@ class << self; self; end
 
       metaclass.instance_eval do
         keys.each do |k|
+          next if RESERVED_FIELD_NAMES.include?(k)
           next if protected_fields.include?(k)
           next if @@permanent_attributes.include?(k)
 
@@ -312,6 +334,7 @@ class << self; self; end
 
       metaclass.instance_eval do
         keys.each do |k|
+          next if RESERVED_FIELD_NAMES.include?(k)
           next if protected_fields.include?(k)
           next if @@permanent_attributes.include?(k)
 

test/stripe/stripe_object_test.rb

@@ -496,5 +496,15 @@ class WithAdditiveObjectParam < Stripe::StripeObject
         assert obj.method(:id).is_a?(Method)
       end
     end
+
+    should "ignore properties that are reserved names" do
+      obj = Stripe::StripeObject.construct_from(metadata: { class: "something" })
+
+      # See comment on `StripeObject::RESERVED_FIELD_NAMES`
+      assert_equal Stripe::StripeObject, obj.metadata.class
+
+      # Value still accessible with hash syntax
+      assert_equal "something", obj.metadata[:class]
+    end
   end
 end