Merge pull request #5895 from chef/lcg/remove-node-method-missing

Chef-13: remove method_missing access to node object.
chef · Mar 14, 2017 · 7b65a18 · 7b65a18
2 parents ed4cf4b + 122a38b
commit 7b65a18
Show file tree

Hide file tree

Showing 6 changed files with 19 additions and 128 deletions.
diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
@@ -53,3 +53,9 @@ The remediation is removing the self-dependency `depends` line in the metadata.
 
 Retained only for the service resource (where it makes some sense) and for the mount resource.
 
+### Removed deprecated `method_missing` access from the Chef::Node object
+
+Previously, the syntax `node.foo.bar` could be used to mean `node["foo"]["bar"]`, but this API had sharp edges where methods collided
+with the core ruby Object class (e.g. `node.class`) and where it collided with our own ability to extend the `Chef::Node` API.  This
+method access has been deprecated for some time, and has been removed in Chef-13.
+
diff --git a/lib/chef/node/attribute.rb b/lib/chef/node/attribute.rb
@@ -1,7 +1,7 @@
 #--
 # Author:: Adam Jacob (<adam@chef.io>)
 # Author:: AJ Christensen (<aj@chef.io>)
-# Copyright:: Copyright 2008-2016, Chef Software, Inc.
+# Copyright:: Copyright 2008-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -476,25 +476,6 @@ def unlink!(level, *path)
 
       alias :each_attribute :each
 
-      def method_missing(symbol, *args)
-        if symbol == :to_ary
-          merged_attributes.send(symbol, *args)
-        elsif args.empty?
-          Chef.deprecated(:attributes, %q{method access to node attributes (node.foo.bar) is deprecated and will be removed in Chef 13, please use bracket syntax (node["foo"]["bar"])})
-          if key?(symbol)
-            self[symbol]
-          else
-            raise NoMethodError, "Undefined method or attribute `#{symbol}' on `node'"
-          end
-        elsif symbol.to_s =~ /=$/
-          Chef.deprecated(:attributes, %q{method setting of node attributes (node.foo="bar") is deprecated and will be removed in Chef 13, please use bracket syntax (node["foo"]="bar")})
-          key_to_set = symbol.to_s[/^(.+)=$/, 1]
-          self[key_to_set] = (args.length == 1 ? args[0] : args)
-        else
-          raise NoMethodError, "Undefined node attribute or method `#{symbol}' on `node'"
-        end
-      end
-
       def to_s
         merged_attributes.to_s
       end

diff --git a/lib/chef/node/attribute_collections.rb b/lib/chef/node/attribute_collections.rb
@@ -1,6 +1,6 @@
 #--
 # Author:: Daniel DeLeo (<dan@chef.io>)
-# Copyright:: Copyright 2012-2016, Chef Software, Inc.
+# Copyright:: Copyright 2012-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -180,24 +180,6 @@ def []=(key, value)
 
       alias :attribute? :has_key?
 
-      def method_missing(symbol, *args)
-        # Calling `puts arg` implicitly calls #to_ary on `arg`. If `arg` does
-        # not implement #to_ary, ruby recognizes it as a single argument, and
-        # if it returns an Array, then ruby prints each element. If we don't
-        # account for that here, we'll auto-vivify a VividMash for the key
-        # :to_ary which creates an unwanted key and raises a TypeError.
-        if symbol == :to_ary
-          super
-        elsif args.empty?
-          self[symbol]
-        elsif symbol.to_s =~ /=$/
-          key_to_set = symbol.to_s[/^(.+)=$/, 1]
-          self[key_to_set] = (args.length == 1 ? args[0] : args)
-        else
-          raise NoMethodError, "Undefined node attribute or method `#{symbol}' on `node'. To set an attribute, use `#{symbol}=value' instead."
-        end
-      end
-
       def convert_key(key)
         super
       end

diff --git a/lib/chef/node/immutable_collections.rb b/lib/chef/node/immutable_collections.rb
@@ -1,5 +1,5 @@
 #--
-# Copyright:: Copyright 2012-2016, Chef Software, Inc.
+# Copyright:: Copyright 2012-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -125,24 +125,6 @@ def public_method_that_only_deep_merge_should_use(key, value)
 
       alias :attribute? :has_key?
 
-      def method_missing(symbol, *args)
-        if symbol == :to_ary
-          super
-        elsif args.empty?
-          if key?(symbol)
-            self[symbol]
-          else
-            raise NoMethodError, "Undefined method or attribute `#{symbol}' on `node'"
-          end
-          # This will raise a ImmutableAttributeModification error:
-        elsif symbol.to_s =~ /=$/
-          key_to_set = symbol.to_s[/^(.+)=$/, 1]
-          self[key_to_set] = (args.length == 1 ? args[0] : args)
-        else
-          raise NoMethodError, "Undefined node attribute or method `#{symbol}' on `node'"
-        end
-      end
-
       # Mash uses #convert_value to mashify values on input.
       # Since we're handling this ourselves, override it to be a no-op
       def convert_value(value)

diff --git a/spec/unit/node/attribute_spec.rb b/spec/unit/node/attribute_spec.rb
@@ -1,7 +1,7 @@
 #
 # Author:: Adam Jacob (<adam@chef.io>)
 # Author:: AJ Christensen (<aj@chef.io>)
-# Copyright:: Copyright 2008-2016, Chef Software Inc.
+# Copyright:: Copyright 2008-2017, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -490,11 +490,6 @@
       expect(@attributes.has_key?("does_not_exist_at_all")).to eq(false)
     end
 
-    it "should return true if an attribute exists but is set to nil using dot notation" do
-      Chef::Config[:treat_deprecation_warnings_as_errors] = false
-      expect(@attributes.music.deeper.has_key?("gates_of_ishtar")).to eq(true)
-    end
-
     it "should return true if an attribute exists but is set to false" do
       @attributes.has_key?("music")
       expect(@attributes["music"].has_key?("apophis")).to eq(true)
@@ -530,19 +525,6 @@
 
   end
 
-  describe "method_missing" do
-    it "should behave like a [] lookup" do
-      Chef::Config[:treat_deprecation_warnings_as_errors] = false
-      expect(@attributes.music.mastodon).to eq("rocks")
-    end
-
-    it "should allow the last method to set a value if it has an = sign on the end" do
-      Chef::Config[:treat_deprecation_warnings_as_errors] = false
-      @attributes.normal.music.mastodon = %w{dream still shining}
-      expect(@attributes.normal.music.mastodon).to eq(%w{dream still shining})
-    end
-  end
-
   describe "keys" do
     before(:each) do
       @attributes = Chef::Node::Attribute.new(
@@ -1109,25 +1091,25 @@ def silence
   describe "when setting a component attribute to a new value" do
     it "converts the input in to a VividMash tree (default)" do
       @attributes.default = {}
-      @attributes.default.foo = "bar"
+      @attributes.default["foo"] = "bar"
       expect(@attributes.merged_attributes[:foo]).to eq("bar")
     end
 
     it "converts the input in to a VividMash tree (normal)" do
       @attributes.normal = {}
-      @attributes.normal.foo = "bar"
+      @attributes.normal["foo"] = "bar"
       expect(@attributes.merged_attributes[:foo]).to eq("bar")
     end
 
     it "converts the input in to a VividMash tree (override)" do
       @attributes.override = {}
-      @attributes.override.foo = "bar"
+      @attributes.override["foo"] = "bar"
       expect(@attributes.merged_attributes[:foo]).to eq("bar")
     end
 
     it "converts the input in to a VividMash tree (automatic)" do
       @attributes.automatic = {}
-      @attributes.automatic.foo = "bar"
+      @attributes.automatic["foo"] = "bar"
       expect(@attributes.merged_attributes[:foo]).to eq("bar")
     end
   end
@@ -1170,11 +1152,6 @@ def silence
     it "raises an error when using []=" do
       expect { @attributes[:new_key] = "new value" }.to raise_error(Chef::Exceptions::ImmutableAttributeModification)
     end
-
-    it "raises an error when using `attr=value`" do
-      Chef::Config[:treat_deprecation_warnings_as_errors] = false
-      expect { @attributes.new_key = "new value" }.to raise_error(Chef::Exceptions::ImmutableAttributeModification)
-    end
   end
 
   describe "deeply converting values" do

diff --git a/spec/unit/node_spec.rb b/spec/unit/node_spec.rb
@@ -238,43 +238,25 @@
       expect(node["battles"]["people"].attribute?("snozzberry")).to eq(false)
     end
 
-    it "does not allow you to set an attribute via method_missing" do
-      Chef::Config[:treat_deprecation_warnings_as_errors] = false
-      expect { node.sunshine = "is bright" }.to raise_error(Chef::Exceptions::ImmutableAttributeModification)
-    end
-
     it "does not allow modification of node attributes via hash methods" do
-      Chef::Config[:treat_deprecation_warnings_as_errors] = false
       node.default["h4sh"] = { foo: "bar" }
       expect { node["h4sh"].delete("foo") }.to raise_error(Chef::Exceptions::ImmutableAttributeModification)
-      expect { node.h4sh.delete("foo") }.to raise_error(Chef::Exceptions::ImmutableAttributeModification)
     end
 
     it "does not allow modification of node attributes via array methods" do
       Chef::Config[:treat_deprecation_warnings_as_errors] = false
       node.default["array"] = []
       expect { node["array"] << "boom" }.to raise_error(Chef::Exceptions::ImmutableAttributeModification)
-      expect { node.array << "boom" }.to raise_error(Chef::Exceptions::ImmutableAttributeModification)
     end
 
     it "returns merged immutable attributes for arrays" do
-      Chef::Config[:treat_deprecation_warnings_as_errors] = false
       node.default["array"] = []
       expect( node["array"].class ).to eql(Chef::Node::ImmutableArray)
-      expect( node.array.class ).to eql(Chef::Node::ImmutableArray)
     end
 
     it "returns merged immutable attributes for hashes" do
-      Chef::Config[:treat_deprecation_warnings_as_errors] = false
       node.default["h4sh"] = {}
       expect( node["h4sh"].class ).to eql(Chef::Node::ImmutableMash)
-      expect( node.h4sh.class ).to eql(Chef::Node::ImmutableMash)
-    end
-
-    it "should allow you get get an attribute via method_missing" do
-      Chef::Config[:treat_deprecation_warnings_as_errors] = false
-      node.default.sunshine = "is bright"
-      expect(node.sunshine).to eql("is bright")
     end
 
     describe "normal attributes" do
@@ -321,12 +303,6 @@
         expect(node[:snoopy][:is_a_puppy]).to eq(true)
       end
 
-      it "auto-vivifies attributes created via method syntax" do
-        Chef::Config[:treat_deprecation_warnings_as_errors] = false
-        node.normal.fuu.bahrr.baz = "qux"
-        expect(node.fuu.bahrr.baz).to eq("qux")
-      end
-
       it "should let you use tag as a convience method for the tags attribute" do
         node.normal["tags"] = %w{one two}
         node.tag("three", "four")
@@ -407,12 +383,6 @@
         expect(node["a"]["r1"]["g"]["u"]).to eql("u1")
       end
 
-      it "auto-vivifies attributes created via method syntax" do
-        Chef::Config[:treat_deprecation_warnings_as_errors] = false
-        node.default.fuu.bahrr.baz = "qux"
-        expect(node.fuu.bahrr.baz).to eq("qux")
-      end
-
       it "default_unless correctly resets the deep merge cache" do
         node.normal["tags"] = []  # this sets our top-level breadcrumb
         node.default_unless["foo"]["bar"] = "NK-19V"
@@ -469,12 +439,6 @@
         node.override[:snoopy][:is_a_puppy] = true
         expect(node[:snoopy][:is_a_puppy]).to eq(true)
       end
-
-      it "auto-vivifies attributes created via method syntax" do
-        Chef::Config[:treat_deprecation_warnings_as_errors] = false
-        node.override.fuu.bahrr.baz = "qux"
-        expect(node.fuu.bahrr.baz).to eq("qux")
-      end
     end
 
     describe "globally deleting attributes" do
@@ -811,10 +775,9 @@
     #
     describe "deep merge attribute cache edge conditions" do
       it "does not error with complicated attribute substitution" do
-        Chef::Config[:treat_deprecation_warnings_as_errors] = false
         node.default["chef_attribute_hell"]["attr1"] = "attribute1"
-        node.default["chef_attribute_hell"]["attr2"] = "#{node.chef_attribute_hell.attr1}/attr2"
-        expect { node.default["chef_attribute_hell"]["attr3"] = "#{node.chef_attribute_hell.attr2}/attr3" }.not_to raise_error
+        node.default["chef_attribute_hell"]["attr2"] = "#{node[:chef_attribute_hell][:attr1]}/attr2"
+        expect { node.default["chef_attribute_hell"]["attr3"] = "#{node[:chef_attribute_hell][:attr2]}/attr3" }.not_to raise_error
       end
 
       it "caches both strings and symbols correctly" do
@@ -829,7 +792,7 @@
         Chef::Config[:treat_deprecation_warnings_as_errors] = false
         node.default["passenger"]["version"]     = "4.0.57"
         node.default["passenger"]["root_path"]   = "passenger-#{node['passenger']['version']}"
-        node.default["passenger"]["root_path_2"] = "passenger-#{node.passenger['version']}"
+        node.default["passenger"]["root_path_2"] = "passenger-#{node[:passenger]['version']}"
         expect(node["passenger"]["root_path_2"]).to eql("passenger-4.0.57")
         expect(node[:passenger]["root_path_2"]).to eql("passenger-4.0.57")
       end
@@ -841,8 +804,8 @@
     end
 
     it "should allow you to iterate over attributes with each_attribute" do
-      node.default.sunshine = "is bright"
-      node.default.canada = "is a nice place"
+      node.default["sunshine"] = "is bright"
+      node.default["canada"] = "is a nice place"
       seen_attributes = Hash.new
       node.each_attribute do |a, v|
         seen_attributes[a] = v