From 59c6bd77abb071423daea0b62c4d97a54e0aa26a Mon Sep 17 00:00:00 2001 From: Lamont Granquist Date: Mon, 13 Mar 2017 15:56:57 -0700 Subject: [PATCH 1/2] Chef-13: remove method_missing access to node object. So again the reasons why we break this: - node.class vs. node['class'] kinds of problems - adding node#foo to Chef::Node as an extension to the API and breaking node.foo meaning node["foo"] (we can't make exensions to that class without possibly breaking someone) - crazy things like the old CHEF-3799 issue in the old ticketing system where IO#puts in ruby blindly calls #to_ary on stuff and expects it to raise -- whereas we would potentially autovivify a 'to_ary' hash key and return nil which breaks the world. This also has caused issues with the hashie gem and they've gone to spamming warnings by default to try to deal with it: https://github.com/berkshelf/berkshelf/issues/1665 Signed-off-by: Lamont Granquist --- lib/chef/node/attribute.rb | 21 +----------- lib/chef/node/attribute_collections.rb | 20 +---------- lib/chef/node/immutable_collections.rb | 20 +---------- spec/unit/node/attribute_spec.rb | 33 +++--------------- spec/unit/node_spec.rb | 47 +++----------------------- 5 files changed, 13 insertions(+), 128 deletions(-) diff --git a/lib/chef/node/attribute.rb b/lib/chef/node/attribute.rb index 4febd47b449..761694e0105 100644 --- a/lib/chef/node/attribute.rb +++ b/lib/chef/node/attribute.rb @@ -1,7 +1,7 @@ #-- # Author:: Adam Jacob () # Author:: AJ Christensen () -# 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 index 694b5fbc3a1..a31b2d2b9b2 100644 --- a/lib/chef/node/attribute_collections.rb +++ b/lib/chef/node/attribute_collections.rb @@ -1,6 +1,6 @@ #-- # Author:: Daniel DeLeo () -# 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 index dad712e078e..12ee2e5dd86 100644 --- 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 index a3e62ff9397..e9515ff3b4e 100644 --- a/spec/unit/node/attribute_spec.rb +++ b/spec/unit/node/attribute_spec.rb @@ -1,7 +1,7 @@ # # Author:: Adam Jacob () # Author:: AJ Christensen () -# 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 index a8f50de0467..40780e523bf 100644 --- 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 From 122a38ba58c7d7286d37ea366d45dc7d1034d78b Mon Sep 17 00:00:00 2001 From: Lamont Granquist Date: Mon, 13 Mar 2017 16:21:30 -0700 Subject: [PATCH 2/2] update RELEASE_NOTES for node method_missing removal Signed-off-by: Lamont Granquist --- RELEASE_NOTES.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index db8732c32bd..4a5c0b8f8c3 100644 --- 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. +