diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 67bee1e8..35232871 100644 --- a/.rubocop_todo.yml +++ b/.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: diff --git a/lib/stripe/api_operations/request.rb b/lib/stripe/api_operations/request.rb index 2680beea..845924eb 100644 --- a/lib/stripe/api_operations/request.rb +++ b/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,7 +23,7 @@ module Stripe # 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] @@ -33,11 +31,8 @@ module Stripe 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 diff --git a/lib/stripe/stripe_object.rb b/lib/stripe/stripe_object.rb index 35e9fcfa..ed1b30c8 100644 --- a/lib/stripe/stripe_object.rb +++ b/lib/stripe/stripe_object.rb @@ -133,20 +133,6 @@ module Stripe @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 @@ module Stripe # 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 @@ module Stripe 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 diff --git a/lib/stripe/util.rb b/lib/stripe/util.rb index 8acadd35..d8c2a5f2 100644 --- a/lib/stripe/util.rb +++ b/lib/stripe/util.rb @@ -2,6 +2,22 @@ require "cgi" 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 diff --git a/test/stripe/stripe_object_test.rb b/test/stripe/stripe_object_test.rb index a775a028..8e90561f 100644 --- a/test/stripe/stripe_object_test.rb +++ b/test/stripe/stripe_object_test.rb @@ -35,19 +35,75 @@ module Stripe 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