From 193ed4891c556bb8bb75561cc40a64d8cf87883d Mon Sep 17 00:00:00 2001 From: Tim Riley Date: Sun, 16 Sep 2018 21:04:31 +1000 Subject: [PATCH] Only assign nil values in initialize if ivar not previously defined This improves compatibility with objects initialized in unconventional ways --- lib/dry/auto_inject/strategies/hash.rb | 2 +- lib/dry/auto_inject/strategies/kwargs.rb | 7 ++- .../existing_ivars_before_initialize_spec.rb | 45 +++++++++++++++++++ .../existing_ivars_before_initialize_spec.rb | 45 +++++++++++++++++++ 4 files changed, 96 insertions(+), 3 deletions(-) create mode 100644 spec/integration/hash/inheritance/existing_ivars_before_initialize_spec.rb create mode 100644 spec/integration/kwargs/inheritance/existing_ivars_before_initialize_spec.rb diff --git a/lib/dry/auto_inject/strategies/hash.rb b/lib/dry/auto_inject/strategies/hash.rb index 905f6ff..c1effd0 100644 --- a/lib/dry/auto_inject/strategies/hash.rb +++ b/lib/dry/auto_inject/strategies/hash.rb @@ -27,7 +27,7 @@ def define_initialize(klass) instance_mod.class_eval <<-RUBY, __FILE__, __LINE__ + 1 def initialize(options) - #{dependency_map.names.map { |name| "@#{name} = options[:#{name}]" }.join("\n")} + #{dependency_map.names.map { |name| "@#{name} = options[:#{name}] unless !options.key?(#{name}) && instance_variable_defined?(:'@#{name}')" }.join("\n")} super(#{super_params}) end RUBY diff --git a/lib/dry/auto_inject/strategies/kwargs.rb b/lib/dry/auto_inject/strategies/kwargs.rb index 56f14e3..ef66d8d 100644 --- a/lib/dry/auto_inject/strategies/kwargs.rb +++ b/lib/dry/auto_inject/strategies/kwargs.rb @@ -40,7 +40,7 @@ def define_initialize_with_keywords instance_mod.class_eval <<-RUBY, __FILE__, __LINE__ + 1 def initialize(#{initialize_params}) - #{dependency_map.names.map { |name| "@#{name} = #{name}" }.join("\n")} + #{dependency_map.names.map { |name| "@#{name} = #{name} unless #{name}.nil? && instance_variable_defined?(:'@#{name}')" }.join("\n")} super() end RUBY @@ -56,7 +56,10 @@ def define_initialize_with_splat(super_method) instance_mod.class_exec(dependency_map) do |dependency_map| define_method :initialize do |*args, **kwargs| dependency_map.names.each do |name| - instance_variable_set :"@#{name}", kwargs[name] + # Assign instance variables, but only if the ivar is not + # previously defined (this improves compatibility with objects + # initialized in unconventional ways) + instance_variable_set :"@#{name}", kwargs[name] unless kwargs[name].nil? && instance_variable_defined?(:"@#{name}") end super_kwargs = kwargs.each_with_object({}) { |(key, _), hsh| diff --git a/spec/integration/hash/inheritance/existing_ivars_before_initialize_spec.rb b/spec/integration/hash/inheritance/existing_ivars_before_initialize_spec.rb new file mode 100644 index 0000000..ef214ae --- /dev/null +++ b/spec/integration/hash/inheritance/existing_ivars_before_initialize_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +RSpec.describe "kwargs / inheritance / instance variables set before #initialize" do + before do + module Test + AutoInject = Dry::AutoInject(configuration: "configuration", another_dep: "another_dep") + end + end + + let(:framework_class) { + Class.new do + def self.new(configuration:, **args) + allocate.tap do |obj| + obj.instance_variable_set :@configuration, configuration + obj.send :initialize, **args + end + end + end + } + + let(:parent_class) { + Class.new(framework_class) do + include Test::AutoInject.hash[:configuration] + end + } + + let(:child_class) { + Class.new(parent_class) do + include Test::AutoInject.hash[:another_dep] + end + + } + + it "does not assign nil value from missing dependency arg to its instance variable if it is already defined" do + child = child_class.new + expect(child.configuration).to eq "configuration" + expect(child.another_dep).to eq "another_dep" + end + + it "does assign an explicitly provided non-nil dependency to iits instance variable, even if it is already defined" do + child = child_class.new(configuration: "child configuration") + expect(child.configuration).to eq "child configuration" + expect(child.another_dep).to eq "another_dep" + end +end diff --git a/spec/integration/kwargs/inheritance/existing_ivars_before_initialize_spec.rb b/spec/integration/kwargs/inheritance/existing_ivars_before_initialize_spec.rb new file mode 100644 index 0000000..7ff7b18 --- /dev/null +++ b/spec/integration/kwargs/inheritance/existing_ivars_before_initialize_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +RSpec.describe "kwargs / inheritance / instance variables set before #initialize" do + before do + module Test + AutoInject = Dry::AutoInject(configuration: "configuration", another_dep: "another_dep") + end + end + + let(:framework_class) { + Class.new do + def self.new(configuration:, **args) + allocate.tap do |obj| + obj.instance_variable_set :@configuration, configuration + obj.send :initialize, **args + end + end + end + } + + let(:parent_class) { + Class.new(framework_class) do + include Test::AutoInject[:configuration] + end + } + + let(:child_class) { + Class.new(parent_class) do + include Test::AutoInject[:another_dep] + end + + } + + it "does not assign nil value from missing dependency arg to its instance variable if it is already defined" do + child = child_class.new + expect(child.configuration).to eq "configuration" + expect(child.another_dep).to eq "another_dep" + end + + it "does assign an explicitly provided non-nil dependency to iits instance variable, even if it is already defined" do + child = child_class.new(configuration: "child configuration") + expect(child.configuration).to eq "child configuration" + expect(child.another_dep).to eq "another_dep" + end +end