immutable specs plus dup cleanup

3 files changed, 164 insertions, 12 deletions

diff --git a/lib/chef/node/attribute_trait/decorator.rb b/lib/chef/node/attribute_trait/decorator.rb
index 7f1f0ff726..c8510a4c69 100644
--- a/lib/chef/node/attribute_trait/decorator.rb
+++ b/lib/chef/node/attribute_trait/decorator.rb
@@ -65,8 +65,7 @@ class Chef
 
         def method_missing(method, *args, &block)
           if wrapped_object.respond_to?(method, false)
-            # we can't define_method here because then we'll always respond_to? the
-            # method and in some cases we mutate and no longer respond_to? something
+            # cannot define_method here
             wrapped_object.public_send(method, *args, &block)
           else
             super
@@ -74,10 +73,15 @@ class Chef
         end
 
         def respond_to?(method, include_private = false)
+          # since we define these methods, :respond_to_missing? doesn't work.
           return false if is_a?(Array) && method == :to_hash
           return false if is_a?(Hash) && method == :to_ary
           return false if is_a?(Array) && method == :each_pair
-          wrapped_object.respond_to?(method, include_private) || super
+          wrapped_object.respond_to?(method, false) || super
+        end
+
+        def respond_to_missing?(method, include_private = false)
+          wrapped_object.respond_to?(method, false) || super
         end
 
         # avoid method_missing perf hit
@@ -182,15 +186,9 @@ class Chef
 
         def dup
           if is_a?(Array)
-            Array.new(map { |e|
-              safe_dup(e)
-            })
+            map(&method(:safe_dup))
           elsif is_a?(Hash)
-            h = Hash.new
-            each do |k, v|
-              h[safe_dup(k)] = safe_dup(v)
-            end
-            h
+            Hash[map { |k, v| [ safe_dup(k), safe_dup(v) ] } ]
           else
             safe_dup(wrapped_object)
           end
diff --git a/lib/chef/node/attribute_trait/immutable.rb b/lib/chef/node/attribute_trait/immutable.rb
index 7b74bd5a9a..16c8636403 100644
--- a/lib/chef/node/attribute_trait/immutable.rb
+++ b/lib/chef/node/attribute_trait/immutable.rb
@@ -34,7 +34,7 @@ class Chef
           :sort!,
           :sort_by!,
           :uniq!,
-          :unshift
+          :unshift,
         ]
 
         MUTATOR_METHODS.each do |method|
diff --git a/spec/unit/node/attribute_trait/immutable.rb b/spec/unit/node/attribute_trait/immutable.rb
new file mode 100644
index 0000000000..0dee400568
--- /dev/null
+++ b/spec/unit/node/attribute_trait/immutable.rb
@@ -0,0 +1,154 @@
+
+require 'spec_helper'
+require 'bigdecimal'
+
+describe Chef::Node::AttributeTrait::Immutable do
+  class Test
+    include Chef::Node::AttributeTrait::Decorator
+    include Chef::Node::AttributeTrait::Immutable
+  end
+
+  let(:test) { t = Test.new }
+
+  context "#dup" do
+    it "deep dup's correctly to a mutable Array from an Array" do
+      test.wrapped_object = [1,[2,3]]
+      dup = test.dup
+      expect(dup).to eql([1,[2,3]])
+      expect(dup).to be_instance_of(Array)
+      expect(dup[1]).to be_instance_of(Array)
+      dup[1][2] = 4
+      expect(dup).to eql([1,[2,3,4]])
+      expect(test).to eql([1,[2,3]])
+    end
+
+    it "deep dup's correctly to a mutable Hash from a Hash" do
+      test.wrapped_object = { a: { b: 'b' } }
+      dup = test.dup
+      expect(dup).to eql({ a: { b: 'b' } })
+      expect(dup).to be_instance_of(Hash)
+      expect(dup[:a]).to be_instance_of(Hash)
+      dup[:a][:c] = 'c'
+      expect(dup).to eql({ a: { b: 'b', c: 'c' } })
+      expect(test).to eql({ a: { b: 'b' } })
+    end
+
+    it "handles undupable values without an Exception" do
+      test.wrapped_object = {
+        nil: nil,
+        false: 'false',
+        true: 'true',
+        fixnum: '1',
+        float: '1.1',
+        symbol: :foo,
+        method: method(:puts),
+        big_decimal: BigDecimal.new("1.2"),
+      }
+      test.dup
+    end
+  end
+
+  context "#to_hash" do
+    context "when the wrapped_object is an Array" do
+      before { test.wrapped_object =  [ 1, 2 ] }
+
+      it "throws NoMethodError" do
+        expect { test.to_hash }.to raise_error(NoMethodError)
+      end
+
+      it "does not respond_to #to_hash" do
+        expect(test.respond_to?(:to_hash)).to be false
+      end
+
+      it "INCORRECTLY responds to #method" do
+        # this isn't fixable since we don't use method_missing
+        expect(test.method(:to_hash)).to be_kind_of(Method)
+      end
+    end
+
+    context "when the wrapped_object is a Hash" do
+      before { test.wrapped_object = { a: 'a' } }
+
+      it "will respond_to #to_hash" do
+        expect(test.respond_to?(:to_hash)).to be true
+      end
+
+      it "responds to #method" do
+        expect(test.method(:to_hash)).to be_kind_of(Method)
+      end
+
+      it "converts to a bare object" do
+        expect(test.to_hash).to be_instance_of(Hash)
+      end
+    end
+  end
+
+  context "#to_ary" do
+    context "when the wrapped_object is a Hash" do
+      before { test.wrapped_object = { a: 'a' } }
+
+      it "throws NoMethodError" do
+        expect { test.to_ary }.to raise_error(NoMethodError)
+      end
+
+      it "does not respond_to #to_ary" do
+        expect(test.respond_to?(:to_ary)).to be false
+      end
+
+      it "INCORRECTLY responds to #method" do
+        # this isn't fixable since we don't use method_missing
+        expect(test.method(:to_ary)).to be_kind_of(Method)
+      end
+    end
+
+    context "when the wrapped_object is an Array" do
+      before { test.wrapped_object =  [ 1, 2 ] }
+
+      it "will respond_to #to_ary" do
+        expect(test.respond_to?(:to_ary)).to be true
+      end
+
+      it "responds to #method" do
+        expect(test.method(:to_ary)).to be_kind_of(Method)
+      end
+
+      it "converts to a bare object" do
+        expect(test.to_ary).to be_instance_of(Array)
+      end
+    end
+  end
+
+  context "mutator methods raise immutable errors" do
+    context "as a Hash" do
+      before { test.wrapped_object = {} }
+
+      Test::MUTATOR_METHODS.each do |method|
+        it "raises ImmutableAttributeModification on #{method}" do
+          expect { test.public_send(method) }.to raise_error(Chef::Exceptions::ImmutableAttributeModification)
+        end
+        it "respond_to?(:#{method}) is true" do
+          expect(test.respond_to?(method)).to be true
+        end
+        it "returns a method(:#{method})" do
+          expect(test.method(method)).to be_instance_of(Method)
+        end
+      end
+    end
+
+    context "as an Array" do
+      before { test.wrapped_object = [] }
+
+      Test::MUTATOR_METHODS.each do |method|
+        it "raises ImmutableAttributeModification on #{method}" do
+          expect { test.public_send(method) }.to raise_error(Chef::Exceptions::ImmutableAttributeModification)
+        end
+        it "respond_to?(:#{method}) is true" do
+          expect(test.respond_to?(method)).to be true
+        end
+        it "returns a method(:#{method})" do
+          expect(test.method(method)).to be_instance_of(Method)
+        end
+      end
+    end
+  end
+end