Merge pull request stripe#586 from stripe/brandur-remove-marshal

Implement deep copy for StripeObject and remove marshal/unmarshal

.rubocop_todo.yml

@@ -35,10 +35,10 @@ Metrics/LineLength:
 Metrics/MethodLength:
   Max: 47
 
-# Offense count: 1
+# Offense count: 2
 # Configuration parameters: CountComments.
 Metrics/ModuleLength:
-  Max: 290
+  Max: 423
 
 # Offense count: 6
 # Configuration parameters: CountKeywordArgs.
@@ -51,11 +51,6 @@ Metrics/PerceivedComplexity:
   Exclude:
     - 'lib/stripe/stripe_object.rb'
 
-# Offense count: 1
-Security/MarshalLoad:
-  Exclude:
-    - 'lib/stripe/stripe_object.rb'
-
 # Offense count: 2
 Style/ClassVars:
   Exclude:

lib/stripe/api_operations/request.rb

@@ -2,8 +2,6 @@ module Stripe
   module APIOperations
     module Request
       module ClassMethods
-        OPTS_KEYS_TO_PERSIST = Set[:api_key, :api_base, :client, :stripe_account, :stripe_version]
-
         def request(method, url, params = {}, opts = {})
           warn_on_opts_in_params(params)
 
@@ -25,19 +23,16 @@ def request(method, url, params = {}, opts = {})
           # Hash#select returns an array before 1.9
           opts_to_persist = {}
           opts.each do |k, v|
-            opts_to_persist[k] = v if OPTS_KEYS_TO_PERSIST.include?(k)
+            opts_to_persist[k] = v if Util::OPTS_KEYS_TO_PERSIST.include?(k)
           end
 
           [resp, opts_to_persist]
         end
 
         private
 
-        KNOWN_OPTS = Set[:api_key, :idempotency_key, :stripe_account, :stripe_version]
-        private_constant :KNOWN_OPTS
-
         def warn_on_opts_in_params(params)
-          KNOWN_OPTS.each do |opt|
+          Util::OPTS_USER_SPECIFIED.each do |opt|
             if params.key?(opt)
               $stderr.puts("WARNING: #{opt} should be in opts instead of params.")
             end

lib/stripe/stripe_object.rb

@@ -133,20 +133,6 @@ def each(&blk)
       @values.each(&blk)
     end
 
-    def _dump(_level)
-      # The StripeClient instance in @opts is not serializable and is not
-      # really a property of the StripeObject, so we exclude it when
-      # dumping
-      opts = @opts.clone
-      opts.delete(:client)
-      Marshal.dump([@values, opts])
-    end
-
-    def self._load(args)
-      values, opts = Marshal.load(args)
-      construct_from(values, opts)
-    end
-
     # Sets all keys within the StripeObject as unsaved so that they will be
     # included with an update when #serialize_params is called. This method is
     # also recursive, so any StripeObjects contained as values or which are
@@ -305,7 +291,9 @@ def respond_to_missing?(symbol, include_private = false)
     #   remove accessors.
     def initialize_from(values, opts, partial = false)
       @opts = Util.normalize_opts(opts)
-      @original_values = Marshal.load(Marshal.dump(values)) # deep copy
+
+      # the `#send` is here so that we can keep this method private
+      @original_values = self.class.send(:deep_copy, values)
 
       removed = partial ? Set.new : Set.new(@values.keys - values.keys)
       added = Set.new(values.keys - @values.keys)
@@ -405,6 +393,30 @@ def serialize_params_value(value, original, unsaved, force, key: nil)
 
     private
 
+    # Produces a deep copy of the given object including support for arrays,
+    # hashes, and StripeObjects.
+    def self.deep_copy(obj)
+      case obj
+      when Array
+        obj.map { |e| deep_copy(e) }
+      when Hash
+        obj.each_with_object({}) do |(k, v), copy|
+          copy[k] = deep_copy(v)
+          copy
+        end
+      when StripeObject
+        StripeObject.construct_from(
+          deep_copy(obj.instance_variable_get(:@values)),
+          obj.instance_variable_get(:@opts).select do |k, _v|
+            Util::OPTS_COPYABLE.include?(k)
+          end
+        )
+      else
+        obj
+      end
+    end
+    private_class_method :deep_copy
+
     def dirty_value!(value)
       case value
       when Array

lib/stripe/util.rb

@@ -2,6 +2,22 @@
 
 module Stripe
   module Util
+    # Options that a user is allowed to specify.
+    OPTS_USER_SPECIFIED = Set[
+      :api_key,
+      :idempotency_key,
+      :stripe_account,
+      :stripe_version
+    ].freeze
+
+    # Options that should be copyable from one StripeObject to another
+    # including options that may be internal.
+    OPTS_COPYABLE = (OPTS_USER_SPECIFIED + Set[:api_base]).freeze
+
+    # Options that should be persisted between API requests. This includes
+    # client, which is an object containing an HTTP client to reuse.
+    OPTS_KEYS_TO_PERSIST = (OPTS_USER_SPECIFIED + Set[:client]).freeze
+
     def self.objects_to_ids(h)
       case h
       when APIResource

test/stripe/stripe_object_test.rb

@@ -35,19 +35,75 @@ class StripeObjectTest < Test::Unit::TestCase
       assert_equal "Stripe", obj[:name]
     end
 
-    should "marshal a stripe object correctly" do
-      obj = Stripe::StripeObject.construct_from(
-        { id: 1, name: "Stripe" },
-        api_key: "apikey",
-        # StripeClient is not serializable and is expected to be removed
-        # when marshalling StripeObjects
-        client: StripeClient.active_client
-      )
-      m = Marshal.load(Marshal.dump(obj))
-      assert_equal 1, m.id
-      assert_equal "Stripe", m.name
-      expected_hash = { api_key: "apikey" }
-      assert_equal expected_hash, m.instance_variable_get("@opts")
+    context "#deep_copy" do
+      should "produce a deep copy" do
+        opts = {
+          api_base: Stripe.api_base,
+          api_key: "apikey",
+        }
+        values = {
+          id: 1,
+          name: "Stripe",
+          arr: [
+            StripeObject.construct_from({ id: "index0" }, opts),
+            "index1",
+            2,
+          ],
+          map: {
+            :"0" => StripeObject.construct_from({ id: "index0" }, opts),
+            :"1" => "index1",
+            :"2" => 2,
+          },
+        }
+
+        # it's not good to test methods with `#send` like this, but I've done
+        # it in the interest of trying to keep `.deep_copy` as internal as
+        # possible
+        copy_values = Stripe::StripeObject.send(:deep_copy, values)
+
+        # we can't compare the hashes directly because they have embedded
+        # objects which are different from each other
+        assert_equal values[:id], copy_values[:id]
+        assert_equal values[:name], copy_values[:name]
+
+        assert_equal values[:arr].length, copy_values[:arr].length
+
+        # internal values of the copied StripeObject should be the same
+        # (including opts), but the object itself should be new (hence the
+        # refutation of equality on #object_id)
+        assert_equal values[:arr][0][:id], copy_values[:arr][0][:id]
+        refute_equal values[:arr][0].object_id, copy_values[:arr][0].object_id
+        assert_equal values[:arr][0].instance_variable_get(:@opts),
+                     copy_values[:arr][0].instance_variable_get(:@opts)
+
+        # scalars however, can be compared
+        assert_equal values[:arr][1], copy_values[:arr][1]
+        assert_equal values[:arr][2], copy_values[:arr][2]
+
+        # and a similar story with the hash
+        assert_equal values[:map].keys, copy_values[:map].keys
+        assert_equal values[:map][:"0"][:id], copy_values[:map][:"0"][:id]
+        refute_equal values[:map][:"0"].object_id, copy_values[:map][:"0"].object_id
+        assert_equal values[:map][:"0"].instance_variable_get(:@opts),
+                     copy_values[:map][:"0"].instance_variable_get(:@opts)
+        assert_equal values[:map][:"1"], copy_values[:map][:"1"]
+        assert_equal values[:map][:"2"], copy_values[:map][:"2"]
+      end
+
+      should "not copy a client" do
+        opts = {
+          api_key: "apikey",
+          client: StripeClient.active_client,
+        }
+        values = { id: 1, name: "Stripe" }
+
+        obj = Stripe::StripeObject.construct_from(values, opts)
+        copy_obj = Stripe::StripeObject.send(:deep_copy, obj)
+
+        assert_equal values, copy_obj.instance_variable_get(:@values)
+        assert_equal opts.reject { |k, _v| k == :client },
+                     copy_obj.instance_variable_get(:@opts)
+      end
     end
 
     should "recursively call to_hash on its values" do