Merge pull request #390 from stripe/brandur-refactor-serialize-params

Refactor `serialize_params` under `StripeObject`

lib/stripe/account.rb

@@ -52,15 +52,16 @@ module Stripe
     #
     # We're trying to get this overturned on the server side, but for now,
     # patch in a special allowance.
-    def self.serialize_params(obj, original_value=nil)
+    def serialize_params(options = {})
-      update_hash = StripeObject.serialize_params(obj, original_value)
+      serialize_params_account(self, super)
-      case obj
+    end
-      when StripeObject
+
-        obj_values = obj.instance_variable_get(:@values)
+    def serialize_params_account(obj, update_hash)
-        obj_values.each do |k, v|
+      if entity = @values[:legal_entity]
-          if k == :additional_owners && v.is_a?(Array)
+        if owners = entity[:additional_owners]
-            update_hash[k] = serialize_additional_owners(obj, v)
+          entity_update = update_hash[:legal_entity] ||= {}
-          end
+          entity_update[:additional_owners] =
+            serialize_additional_owners(entity, owners)
         end
       end
       update_hash
@@ -89,9 +90,9 @@ module Stripe
 
     private
 
-    def self.serialize_additional_owners(obj, value)
+    def serialize_additional_owners(legal_entity, additional_owners)
-      original_value = obj.instance_variable_get(:@original_values)[:additional_owners]
+      original_value = legal_entity.instance_variable_get(:@original_values)[:additional_owners]
-      if original_value && original_value.length > value.length
+      if original_value && original_value.length > additional_owners.length
         # url params provide no mechanism for deleting an item in an array,
         # just overwriting the whole array or adding new items. So let's not
         # allow deleting without a full overwrite until we have a solution.
@@ -101,10 +102,17 @@ module Stripe
       end
 
       update_hash = {}
-      value.each_with_index do |v, i|
+      additional_owners.each_with_index do |v, i|
-        update = StripeObject.serialize_params(v)
+        # We will almost always see a StripeObject except in the case of a Hash
-        if update != {} && (!original_value || update != original_value[i])
+        # that's been appended to an array of `additional_owners`. We may be
-          update_hash[i.to_s] = update
+        # able to normalize that ugliness by using an array proxy object with
+        # StripeObjects that can detect appends and replace a hash with a
+        # StripeObject.
+        update = v.is_a?(StripeObject) ? v.serialize_params : v
+
+        if update != {} && (!original_value ||
+          update != legal_entity.serialize_params_value(original_value[i], nil, false, true))
+            update_hash[i.to_s] = update
         end
       end
       update_hash

lib/stripe/api_operations/update.rb

@@ -25,16 +25,15 @@ module Stripe
         # Now remove any parameters that look like object attributes.
         params = params.reject { |k, _| respond_to?(k) }
 
-        values = self.class.serialize_params(self).merge(params)
+        values = self.serialize_params(self).merge(params)
 
-        if values.length > 0
+        # note that id gets removed here our call to #url above has already
-          # note that id gets removed here our call to #url above has already
+        # generated a uri for this object with an identifier baked in
-          # generated a uri for this object with an identifier baked in
+        values.delete(:id)
-          values.delete(:id)
+
+        response, opts = request(:post, req_url, values)
+        initialize_from(response, opts)
 
-          response, opts = request(:post, req_url, values)
-          initialize_from(response, opts)
-        end
         self
       end
 

lib/stripe/stripe_object.rb

@@ -72,13 +72,21 @@ module Stripe
     #
     # * +values+ - Hash of values to use to update the current attributes of
     #   the object.
+    # * +:opts+ Options for StripeObject like an API key.
     #
     # ==== Options
     #
-    # * +:opts+ Options for StripeObject like an API key.
+    # * +:dirty+ Whether values should be initiated as "dirty" (unsaved) and
-    def update_attributes(values, opts = {})
+    #   which applies only to new StripeObjects being ininiated under this
+    #   StripeObject. Defaults to true.
+    def update_attributes(values, opts = {}, method_options = {})
+      # Default to true. TODO: Convert to optional arguments after we're off
+      # 1.9 which will make this quite a bit more clear.
+      dirty = method_options.fetch(:dirty, true)
+
       values.each do |k, v|
         @values[k] = Util.convert_to_stripe_object(v, opts)
+        dirty_value!(@values[k]) if dirty
         @unsaved_values.add(k)
       end
     end
@@ -142,73 +150,51 @@ module Stripe
       end
     end
 
-    def serialize_nested_object(key)
+    # Sets all keys within the StripeObject as unsaved so that they will be
-      new_value = @values[key]
+    # included with an update when #serialize_params is called. This method is
-      if new_value.is_a?(APIResource)
+    # also recursive, so any StripeObjects contained as values or which are
-        return {}
+    # values in a tenant array are also marked as dirty.
-      end
+    def dirty!
-
+      @unsaved_values = Set.new(@values.keys)
-      if @unsaved_values.include?(key)
+      @values.each do |k, v|
-        # the object has been reassigned
+        dirty_value!(v)
-        # e.g. as object.key = {foo => bar}
-        update = new_value
-        update = update.to_hash if update.is_a?(StripeObject)
-        new_keys = update.keys.map(&:to_sym)
-
-        # remove keys at the server, but not known locally
-        if @original_values[key]
-          keys_to_unset = @original_values[key].keys - new_keys
-          keys_to_unset.each {|key| update[key] = ''}
-        end
-
-        update
-      else
-        # can be serialized normally
-        self.class.serialize_params(new_value)
       end
     end
 
-    def self.serialize_params(obj, original_value=nil)
+    def serialize_params(options = {})
-      case obj
+      update_hash = {}
-      when nil
+
-        ''
+      @values.each do |k, v|
-      when Array
+        # There are a few reasons that we may want to add in a parameter for
-        update = obj.map { |v| serialize_params(v) }
+        # update:
-        if original_value != update
+        #
-          update
+        #   1. The `force` option has been set.
-        else
+        #   2. We know that it was modified.
-          nil
+        #   3. Its value is a StripeObject. A StripeObject may contain modified
+        #      values within in that its parent StripeObject doesn't know about.
+        #
+        unsaved = @unsaved_values.include?(k)
+        if options[:force] || unsaved || v.is_a?(StripeObject)
+          update_hash[k.to_sym] =
+            serialize_params_value(@values[k], @original_values[k], unsaved, options[:force])
         end
-      when StripeObject
-        unsaved_keys = obj.instance_variable_get(:@unsaved_values)
-        obj_values = obj.instance_variable_get(:@values)
-        update_hash = {}
-
-        unsaved_keys.each do |k|
-          update_hash[k] = serialize_params(obj_values[k])
-        end
-
-        obj_values.each do |k, v|
-          if v.is_a?(Array)
-            original_value = obj.instance_variable_get(:@original_values)[k]
-
-            # the conditional here tests whether the old and new values are
-            # different (and therefore needs an update), or the same (meaning
-            # we can leave it out of the request)
-            if updated = serialize_params(v, original_value)
-              update_hash[k] = updated
-            else
-              update_hash.delete(k)
-            end
-          elsif v.is_a?(StripeObject) || v.is_a?(Hash)
-            update_hash[k] = obj.serialize_nested_object(k)
-          end
-        end
-
-        update_hash
-      else
-        obj
       end
+
+      # a `nil` that makes it out of `#serialize_params_value` signals an empty
+      # value that we shouldn't appear in the serialized form of the object
+      update_hash.reject! { |_, v| v == nil }
+
+      update_hash
+    end
+
+    class << self
+      # This class method has been deprecated in favor of the instance method
+      # of the same name.
+      def serialize_params(obj, options = {})
+        obj.serialize_params(options)
+      end
+      extend Gem::Deprecate
+      deprecate :serialize_params, "#serialize_params", 2016, 9
     end
 
     protected
@@ -249,7 +235,8 @@ module Stripe
                 "We interpret empty strings as nil in requests. " \
                 "You may set #{self}.#{k} = nil to delete the property.")
             end
-            @values[k] = v
+            @values[k] = Util.convert_to_stripe_object(v, @opts)
+            dirty_value!(@values[k])
             @unsaved_values.add(k)
           end
 
@@ -328,7 +315,7 @@ module Stripe
         @unsaved_values.delete(k)
       end
 
-      update_attributes(values, opts)
+      update_attributes(values, opts, :dirty => false)
       values.each do |k, _|
         @transient_values.delete(k)
         @unsaved_values.delete(k)
@@ -336,5 +323,83 @@ module Stripe
 
       self
     end
+
+    def serialize_params_value(value, original, unsaved, force)
+      case value
+      when nil
+        ''
+
+      # The logic here is that essentially any object embedded in another
+      # object that had a `type` is actually an API resource of a different
+      # type that's been included in the response. These other resources must
+      # be updated from their proper endpoints, and therefore they are not
+      # included when serializing even if they've been modified.
+      when APIResource
+        nil
+
+      when Array
+        update = value.map { |v| serialize_params_value(v, nil, true, force) }
+
+        # This prevents an array that's unchanged from being resent.
+        if update != serialize_params_value(original, nil, true, force)
+          update
+        else
+          nil
+        end
+
+      # Handle a Hash for now, but in the long run we should be able to
+      # eliminate all places where hashes are stored as values internally by
+      # making sure any time one is set, we convert it to a StripeObject. This
+      # will simplify our model by making data within an object more
+      # consistent.
+      #
+      # For now, you can still run into a hash if someone appends one to an
+      # existing array being held by a StripeObject. This could happen for
+      # example by appending a new hash onto `additional_owners` for an
+      # account.
+      when Hash
+        Util.convert_to_stripe_object(value, @opts).serialize_params
+
+      when StripeObject
+        update = value.serialize_params(:force => force)
+
+        # If the entire object was replaced, then we need blank each field of
+        # the old object that held a value. The new serialized values will
+        # override any of these empty values.
+        update = empty_values(original).merge(update) if original && unsaved
+
+        update
+
+      else
+        value
+      end
+    end
+
+    private
+
+    def dirty_value!(value)
+      case value
+      when Array
+        value.map { |v| dirty_value!(v) }
+      when StripeObject
+        value.dirty!
+      end
+    end
+
+    # Returns a hash of empty values for all the values that are in the given
+    # StripeObject.
+    def empty_values(obj)
+      values = case obj
+      when Hash         then obj
+      when StripeObject then obj.instance_variable_get(:@values)
+      else
+        raise ArgumentError, "#empty_values got unexpected object type: #{obj.class.name}"
+      end
+
+      values.inject({}) do |update, (k, _)|
+        update[k] = ''
+        update
+      end
+    end
   end
 end

test/stripe/account_test.rb

@@ -155,8 +155,8 @@ module Stripe
     should "#serialize_params an a new additional_owners" do
       obj = Stripe::Util.convert_to_stripe_object({
         :object => "account",
-        :legal_entity => {
+        :legal_entity => Stripe::StripeObject.construct_from({
-        },
+        }),
       }, {})
       obj.legal_entity.additional_owners = [
         { :first_name => "Joe" },
@@ -171,7 +171,7 @@ module Stripe
           }
         }
       }
-      assert_equal(expected, obj.class.serialize_params(obj))
+      assert_equal(expected, obj.serialize_params)
     end
 
     should "#serialize_params on an partially changed additional_owners" do
@@ -197,7 +197,7 @@ module Stripe
           }
         }
       }
-      assert_equal(expected, obj.class.serialize_params(obj))
+      assert_equal(expected, obj.serialize_params)
     end
 
     should "#serialize_params on an unchanged additional_owners" do
@@ -220,7 +220,7 @@ module Stripe
           :additional_owners => {}
         }
       }
-      assert_equal(expected, obj.class.serialize_params(obj))
+      assert_equal(expected, obj.serialize_params)
     end
 
     # Note that the empty string that we send for this one has a special
@@ -246,7 +246,7 @@ module Stripe
           :additional_owners => ""
         }
       }
-      assert_equal(expected, obj.class.serialize_params(obj))
+      assert_equal(expected, obj.serialize_params)
     end
   end
 end

test/stripe/stripe_object_test.rb

@@ -121,20 +121,20 @@ module Stripe
 
     should "#serialize_params on an empty object" do
       obj = Stripe::StripeObject.construct_from({})
-      assert_equal({}, Stripe::StripeObject.serialize_params(obj))
+      assert_equal({}, obj.serialize_params)
     end
 
     should "#serialize_params on a new object with a subobject" do
       obj = Stripe::StripeObject.new
       obj.metadata = { :foo => "bar" }
       assert_equal({ :metadata => { :foo => "bar" } },
-        Stripe::StripeObject.serialize_params(obj))
+        obj.serialize_params)
     end
 
     should "#serialize_params on a basic object" do
       obj = Stripe::StripeObject.construct_from({ :foo => nil })
       obj.update_attributes(:foo => "bar")
-      assert_equal({ :foo => "bar" }, Stripe::StripeObject.serialize_params(obj))
+      assert_equal({ :foo => "bar" }, obj.serialize_params)
     end
 
     should "#serialize_params on a more complex object" do
@@ -146,7 +146,7 @@ module Stripe
       })
       obj.foo.bar = "newbar"
       assert_equal({ :foo => { :bar => "newbar" } },
-        Stripe::StripeObject.serialize_params(obj))
+        obj.serialize_params)
     end
 
     should "#serialize_params on an array" do
@@ -155,7 +155,7 @@ module Stripe
       })
       obj.foo = ["new-value"]
       assert_equal({ :foo => ["new-value"] },
-        Stripe::StripeObject.serialize_params(obj))
+        obj.serialize_params)
     end
 
     should "#serialize_params on an array that shortens" do
@@ -164,7 +164,7 @@ module Stripe
       })
       obj.foo = ["new-value"]
       assert_equal({ :foo => ["new-value"] },
-        Stripe::StripeObject.serialize_params(obj))
+        obj.serialize_params)
     end
 
     should "#serialize_params on an array that lengthens" do
@@ -173,7 +173,7 @@ module Stripe
       })
       obj.foo = ["new-value"] * 4
       assert_equal({ :foo => ["new-value"] * 4 },
-        Stripe::StripeObject.serialize_params(obj))
+        obj.serialize_params)
     end
 
     should "#serialize_params on an array of hashes" do
@@ -187,12 +187,12 @@ module Stripe
       ]
       obj.foo[0].bar = "baz"
       assert_equal({ :foo => [{ :bar => "baz" }] },
-        Stripe::StripeObject.serialize_params(obj))
+        obj.serialize_params)
     end
 
     should "#serialize_params doesn't include unchanged values" do
       obj = Stripe::StripeObject.construct_from({ :foo => nil })
-      assert_equal({}, Stripe::StripeObject.serialize_params(obj))
+      assert_equal({}, obj.serialize_params)
     end
 
     should "#serialize_params on an array that is unchanged" do
@@ -200,7 +200,7 @@ module Stripe
         :foo => ["0-index", "1-index", "2-index"],
       })
       obj.foo = ["0-index", "1-index", "2-index"]
-      assert_equal({}, Stripe::StripeObject.serialize_params(obj))
+      assert_equal({}, obj.serialize_params)
     end
 
     should "#serialize_params with a StripeObject" do
@@ -211,8 +211,80 @@ module Stripe
       obj.metadata =
         Stripe::StripeObject.construct_from({ :foo => 'bar' })
 
-      serialized = Stripe::StripeObject.serialize_params(obj)
+      serialized = obj.serialize_params
       assert_equal({ :foo => "bar" }, serialized[:metadata])
     end
+
+    should "#serialize_params with a StripeObject that's been replaced" do
+      obj = Stripe::StripeObject.construct_from({
+        :metadata => Stripe::StripeObject.construct_from({ :bar => 'foo' })
+      })
+
+      # Here we replace the object wholesale which means that the client must
+      # be able to blank out the values that were in the old object, but which
+      # are no longer present in the new one.
+      obj.metadata =
+        Stripe::StripeObject.construct_from({ :baz => 'foo' })
+
+      serialized = obj.serialize_params
+      assert_equal({ :bar => "", :baz => 'foo' }, serialized[:metadata])
+    end
+
+    should "#serialize_params with an array of StripeObjects" do
+      obj = Stripe::StripeObject.construct_from({})
+      obj.metadata = [
+        Stripe::StripeObject.construct_from({ :foo => 'bar' })
+      ]
+
+      serialized = obj.serialize_params
+      assert_equal([{ :foo => "bar" }], serialized[:metadata])
+    end
+
+    should "#serialize_params and remove embedded APIResources" do
+      obj = Stripe::StripeObject.construct_from({
+        :customer => Customer.construct_from({})
+      })
+
+      serialized = obj.serialize_params
+      assert_equal({}, serialized)
+    end
+
+    should "#serialize_params takes a force option" do
+      obj = Stripe::StripeObject.construct_from({
+        :id => 'id',
+        :metadata => Stripe::StripeObject.construct_from({ :foo => 'bar' })
+      })
+
+      serialized = obj.serialize_params(:force => true)
+      assert_equal({ :id => 'id', :metadata => { :foo => 'bar' } }, serialized)
+    end
+
+    should "#dirty! forces an object and its subobjects to be saved" do
+      obj = Stripe::StripeObject.construct_from({
+        :id => 'id',
+        :metadata => Stripe::StripeObject.construct_from({ :foo => 'bar' })
+      })
+
+      # note that `force` and `dirty!` are for different things, but are
+      # functionally equivalent
+      obj.dirty!
+
+      serialized = obj.serialize_params
+      assert_equal({ :id => 'id', :metadata => { :foo => 'bar' } }, serialized)
+    end
+
+    should "warn that .serialize_params is deprecated" do
+      old_stderr = $stderr
+      $stderr = StringIO.new
+      begin
+        obj = Stripe::StripeObject.construct_from({})
+        Stripe::StripeObject.serialize_params(obj)
+        message = "NOTE: Stripe::StripeObject.serialize_params is " +
+          "deprecated; use #serialize_params instead"
+        assert_match Regexp.new(message), $stderr.string
+      ensure
+        $stderr = old_stderr
+      end
+    end
   end
 end