diff options
author | Serdar Sutay <serdar@opscode.com> | 2014-11-23 20:42:10 -0800 |
---|---|---|
committer | Serdar Sutay <serdar@opscode.com> | 2014-11-23 20:43:52 -0800 |
commit | 772df7b2aa71d9c2908e6e2f2f0a793fa7e1441c (patch) | |
tree | 47ca257d2d0a5f2cfd21ff4233a3d6932dabdc24 | |
parent | ab0503c93b216d3c660a7efc6f72dbefab8250ea (diff) | |
download | chef-772df7b2aa71d9c2908e6e2f2f0a793fa7e1441c.tar.gz |
Merge pull request #2462 from opscode/lcg/remove-knockout-merge
remove old knockout merge exception
Conflicts:
spec/unit/mixin/deep_merge_spec.rb
-rw-r--r-- | lib/chef/mixin/deep_merge.rb | 62 | ||||
-rw-r--r-- | lib/chef/node.rb | 23 | ||||
-rw-r--r-- | lib/chef/node/attribute.rb | 75 | ||||
-rw-r--r-- | lib/chef/node/attribute_collections.rb | 5 | ||||
-rw-r--r-- | lib/chef/node/immutable_collections.rb | 4 | ||||
-rw-r--r-- | lib/chef/provider_resolver.rb | 4 | ||||
-rw-r--r-- | lib/chef/run_list/run_list_expansion.rb | 4 | ||||
-rw-r--r-- | spec/unit/mixin/deep_merge_spec.rb | 26 |
8 files changed, 109 insertions, 94 deletions
diff --git a/lib/chef/mixin/deep_merge.rb b/lib/chef/mixin/deep_merge.rb index a8a4737758..825406a93e 100644 --- a/lib/chef/mixin/deep_merge.rb +++ b/lib/chef/mixin/deep_merge.rb @@ -27,17 +27,6 @@ class Chef # http://trac.misuse.org/science/wiki/DeepMerge module DeepMerge - class InvalidSubtractiveMerge < ArgumentError; end - - - OLD_KNOCKOUT_PREFIX = "!merge:".freeze - - # Regex to match the "knockout prefix" that was used to indicate - # subtractive merging in Chef 10.x and previous. Subtractive merging is - # removed as of Chef 11, but we detect attempted use of it and raise an - # error (see: raise_if_knockout_used!) - OLD_KNOCKOUT_MATCH = %r[!merge].freeze - extend self def merge(first, second) @@ -47,15 +36,6 @@ class Chef DeepMerge.deep_merge(second, first) end - # Inherited roles use the knockout_prefix array subtraction functionality - # This is likely to go away in Chef >= 0.11 - def role_merge(first, second) - first = Mash.new(first) unless first.kind_of?(Mash) - second = Mash.new(second) unless second.kind_of?(Mash) - - DeepMerge.deep_merge(second, first) - end - class InvalidParameter < StandardError; end # Deep Merge core documentation. @@ -78,8 +58,6 @@ class Chef dest = source; return dest end - raise_if_knockout_used!(source) - raise_if_knockout_used!(dest) case source when nil dest @@ -89,7 +67,6 @@ class Chef if dest[src_key] dest[src_key] = deep_merge!(src_value, dest[src_key]) else # dest[src_key] doesn't exist so we take whatever source has - raise_if_knockout_used!(src_value) dest[src_key] = src_value end end @@ -128,11 +105,19 @@ class Chef # If there are two Hashes, recursively merge. if merge_onto.kind_of?(Hash) && merge_with.kind_of?(Hash) merge_with.each do |key, merge_with_value| - merge_onto[key] = if merge_onto.has_key?(key) - hash_only_merge(merge_onto[key], merge_with_value) - else - merge_with_value - end + value = + if merge_onto.has_key?(key) + hash_only_merge(merge_onto[key], merge_with_value) + else + merge_with_value + end + + if merge_onto.respond_to?(:public_method_that_only_deep_merge_should_use) + # we can't call ImmutableMash#[]= because its immutable, but we need to mutate it to build it in-place + merge_onto.public_method_that_only_deep_merge_should_use(key, value) + else + merge_onto[key] = value + end end merge_onto @@ -146,27 +131,6 @@ class Chef end end - # Checks for attempted use of subtractive merge, which was removed for - # Chef 11.0. If subtractive merge use is detected, will raise an - # InvalidSubtractiveMerge exception. - def raise_if_knockout_used!(obj) - if uses_knockout?(obj) - raise InvalidSubtractiveMerge, "subtractive merge with !merge is no longer supported" - end - end - - # Checks for attempted use of subtractive merge in +obj+. - def uses_knockout?(obj) - case obj - when String - obj =~ OLD_KNOCKOUT_MATCH - when Array - obj.any? {|element| element.respond_to?(:gsub) && element =~ OLD_KNOCKOUT_MATCH } - else - false - end - end - def deep_merge(source, dest) deep_merge!(safe_dup(source), safe_dup(dest)) end diff --git a/lib/chef/node.rb b/lib/chef/node.rb index dbb7852586..6055f0e1e9 100644 --- a/lib/chef/node.rb +++ b/lib/chef/node.rb @@ -127,6 +127,7 @@ class Chef # Set a normal attribute of this node, but auto-vivify any Mashes that # might be missing def normal + attributes.top_level_breadcrumb = nil attributes.set_unless_value_present = false attributes.normal end @@ -136,14 +137,17 @@ class Chef # Set a normal attribute of this node, auto-vivifying any mashes that are # missing, but if the final value already exists, don't set it def normal_unless + attributes.top_level_breadcrumb = nil attributes.set_unless_value_present = true attributes.normal end + alias_method :set_unless, :normal_unless # Set a default of this node, but auto-vivify any Mashes that might # be missing def default + attributes.top_level_breadcrumb = nil attributes.set_unless_value_present = false attributes.default end @@ -151,6 +155,7 @@ class Chef # Set a default attribute of this node, auto-vivifying any mashes that are # missing, but if the final value already exists, don't set it def default_unless + attributes.top_level_breadcrumb = nil attributes.set_unless_value_present = true attributes.default end @@ -158,6 +163,7 @@ class Chef # Set an override attribute of this node, but auto-vivify any Mashes that # might be missing def override + attributes.top_level_breadcrumb = nil attributes.set_unless_value_present = false attributes.override end @@ -165,35 +171,30 @@ class Chef # Set an override attribute of this node, auto-vivifying any mashes that # are missing, but if the final value already exists, don't set it def override_unless + attributes.top_level_breadcrumb = nil attributes.set_unless_value_present = true attributes.override end - def override_attrs - attributes.override - end + alias :override_attrs :override + alias :default_attrs :default + alias :normal_attrs :normal def override_attrs=(new_values) attributes.override = new_values end - def default_attrs - attributes.default - end - def default_attrs=(new_values) attributes.default = new_values end - def normal_attrs - attributes.normal - end - def normal_attrs=(new_values) attributes.normal = new_values end def automatic_attrs + attributes.top_level_breadcrumb = nil + attributes.set_unless_value_present = false attributes.automatic end diff --git a/lib/chef/node/attribute.rb b/lib/chef/node/attribute.rb index 6130925aea..3c48f653eb 100644 --- a/lib/chef/node/attribute.rb +++ b/lib/chef/node/attribute.rb @@ -175,6 +175,19 @@ class Chef # return the automatic level attribute component attr_reader :automatic + # This is used to track the top level key as we descend through method chaining into + # a precedence level (e.g. node.default['foo']['bar']['baz']= results in 'foo' here). We + # need this so that when we hit the end of a method chain which results in a mutator method + # that we can invalidate the whole top-level deep merge cache for the top-level key. It is + # the responsibility of the accessor on the Chef::Node object to reset this to nil, and then + # the first VividMash#[] call can ||= and set this to the first key we encounter. + attr_accessor :top_level_breadcrumb + + # Cache of deep merged values by top-level key. This is a simple hash which has keys that are the + # top-level keys of the node object, and we save the computed deep-merge for that key here. There is + # no cache of subtrees. + attr_accessor :deep_merge_cache + def initialize(normal, default, override, automatic) @set_unless_present = false @@ -195,6 +208,8 @@ class Chef @merged_attributes = nil @combined_override = nil @combined_default = nil + @top_level_breadcrumb = nil + @deep_merge_cache = {} end # Debug what's going on with an attribute. +args+ is a path spec to the @@ -230,51 +245,75 @@ class Chef @set_unless_present = setting end + # Invalidate a key in the deep_merge_cache. If called with nil, or no arg, this will invalidate + # the entire deep_merge cache. In the case of the user doing node.default['foo']['bar']['baz']= + # that eventually results in a call to reset_cache('foo') here. A node.default=hash_thing call + # must invalidate the entire cache and re-deep-merge the entire node object. + def reset_cache(path = nil) + if path.nil? + @deep_merge_cache = {} + else + deep_merge_cache.delete(path) + end + end + + alias :reset :reset_cache + # Set the cookbook level default attribute component to +new_data+. def default=(new_data) + reset @default = VividMash.new(self, new_data) end # Set the role level default attribute component to +new_data+ def role_default=(new_data) + reset @role_default = VividMash.new(self, new_data) end # Set the environment level default attribute component to +new_data+ def env_default=(new_data) + reset @env_default = VividMash.new(self, new_data) end # Set the force_default (+default!+) level attributes to +new_data+ def force_default=(new_data) + reset @force_default = VividMash.new(self, new_data) end # Set the normal level attribute component to +new_data+ def normal=(new_data) + reset @normal = VividMash.new(self, new_data) end # Set the cookbook level override attribute component to +new_data+ def override=(new_data) + reset @override = VividMash.new(self, new_data) end # Set the role level override attribute component to +new_data+ def role_override=(new_data) + reset @role_override = VividMash.new(self, new_data) end # Set the environment level override attribute component to +new_data+ def env_override=(new_data) + reset @env_override = VividMash.new(self, new_data) end def force_override=(new_data) + reset @force_override = VividMash.new(self, new_data) end def automatic=(new_data) + reset @automatic = VividMash.new(self, new_data) end @@ -284,6 +323,7 @@ class Chef # clears attributes from all precedence levels def rm(*args) + reset(args[0]) # just easier to compute our retval, rather than collect+merge sub-retvals ret = args.inject(merged_attributes) do |attr, arg| if attr.nil? || !attr.respond_to?(:[]) @@ -314,6 +354,7 @@ class Chef # # equivalent to: force_default!['foo']['bar'].delete('baz') def rm_default(*args) + reset(args[0]) remove_from_precedence_level(force_default!(autovivify: false), *args) end @@ -321,6 +362,7 @@ class Chef # # equivalent to: normal!['foo']['bar'].delete('baz') def rm_normal(*args) + reset(args[0]) remove_from_precedence_level(normal!(autovivify: false), *args) end @@ -328,6 +370,7 @@ class Chef # # equivalent to: force_override!['foo']['bar'].delete('baz') def rm_override(*args) + reset(args[0]) remove_from_precedence_level(force_override!(autovivify: false), *args) end @@ -337,35 +380,51 @@ class Chef # sets default attributes without merging def default!(opts={}) + # FIXME: do not flush whole cache + reset MultiMash.new(self, @default, [], opts) end # sets normal attributes without merging def normal!(opts={}) + # FIXME: do not flush whole cache + reset MultiMash.new(self, @normal, [], opts) end # sets override attributes without merging def override!(opts={}) + # FIXME: do not flush whole cache + reset MultiMash.new(self, @override, [], opts) end # clears from all default precedence levels and then sets force_default def force_default!(opts={}) + # FIXME: do not flush whole cache + reset MultiMash.new(self, @force_default, [@default, @env_default, @role_default], opts) end # clears from all override precedence levels and then sets force_override def force_override!(opts={}) + # FIXME: do not flush whole cache + reset MultiMash.new(self, @force_override, [@override, @env_override, @role_override], opts) end # - # Accessing merged attributes + # Accessing merged attributes. + # + # Note that merged_attributes('foo', 'bar', 'baz') can be called to compute only the + # deep merge of node['foo']['bar']['baz'], but in practice we currently always compute + # all of node['foo'] even if the user only requires node['foo']['bar']['baz']. # def merged_attributes(*path) - immutablize(merge_all(path)) + # immutablize( + merge_all(path) + # ) end def combined_override(*path) @@ -377,7 +436,13 @@ class Chef end def [](key) - merged_attributes(key) + if deep_merge_cache.has_key?(key) + # return the cache of the deep merged values by top-level key + deep_merge_cache[key] + else + # save all the work of computing node[key] + deep_merge_cache[key] = merged_attributes(key) + end end def []=(key, value) @@ -480,7 +545,9 @@ class Chef safe_dup(component) end - components.inject(nil) do |merged, component| + return nil if components.compact.empty? + + components.inject(ImmutableMash.new({})) do |merged, component| Chef::Mixin::DeepMerge.hash_only_merge!(merged, component) end end diff --git a/lib/chef/node/attribute_collections.rb b/lib/chef/node/attribute_collections.rb index 3b19a14d1c..333f4864c6 100644 --- a/lib/chef/node/attribute_collections.rb +++ b/lib/chef/node/attribute_collections.rb @@ -63,6 +63,7 @@ class Chef MUTATOR_METHODS.each do |mutator| class_eval(<<-METHOD_DEFN, __FILE__, __LINE__) def #{mutator}(*args, &block) + root.reset_cache(root.top_level_breadcrumb) super end METHOD_DEFN @@ -127,6 +128,7 @@ class Chef MUTATOR_METHODS.each do |mutator| class_eval(<<-METHOD_DEFN, __FILE__, __LINE__) def #{mutator}(*args, &block) + root.reset_cache(root.top_level_breadcrumb) super end METHOD_DEFN @@ -138,6 +140,7 @@ class Chef end def [](key) + root.top_level_breadcrumb ||= key value = super if !key?(key) value = self.class.new(root) @@ -148,9 +151,11 @@ class Chef end def []=(key, value) + root.top_level_breadcrumb ||= key if set_unless? && key?(key) self[key] else + root.reset_cache(root.top_level_breadcrumb) super end end diff --git a/lib/chef/node/immutable_collections.rb b/lib/chef/node/immutable_collections.rb index af04ef26d4..56b8fed3b7 100644 --- a/lib/chef/node/immutable_collections.rb +++ b/lib/chef/node/immutable_collections.rb @@ -155,6 +155,10 @@ class Chef end end + def public_method_that_only_deep_merge_should_use(key, value) + internal_set(key, immutablize(value)) + end + alias :attribute? :has_key? # Redefine all of the methods that mutate a Hash to raise an error when called. diff --git a/lib/chef/provider_resolver.rb b/lib/chef/provider_resolver.rb index 247102f191..d83a3e0468 100644 --- a/lib/chef/provider_resolver.rb +++ b/lib/chef/provider_resolver.rb @@ -34,7 +34,7 @@ class Chef # return a deterministically sorted list of Chef::Provider subclasses def providers - @providers ||= Chef::Provider.descendants.sort {|a,b| a.to_s <=> b.to_s } + @providers ||= Chef::Provider.descendants end def resolve @@ -48,7 +48,7 @@ class Chef @enabled_handlers ||= providers.select do |klass| klass.provides?(node, resource) - end + end.sort {|a,b| a.to_s <=> b.to_s } end # this cut looks at if the provider can handle the specific resource and action diff --git a/lib/chef/run_list/run_list_expansion.rb b/lib/chef/run_list/run_list_expansion.rb index 73665f39e7..46b45f1d9e 100644 --- a/lib/chef/run_list/run_list_expansion.rb +++ b/lib/chef/run_list/run_list_expansion.rb @@ -96,8 +96,8 @@ class Chef end def apply_role_attributes(role) - @default_attrs = Chef::Mixin::DeepMerge.role_merge(@default_attrs, role.default_attributes) - @override_attrs = Chef::Mixin::DeepMerge.role_merge(@override_attrs, role.override_attributes) + @default_attrs = Chef::Mixin::DeepMerge.merge(@default_attrs, role.default_attributes) + @override_attrs = Chef::Mixin::DeepMerge.merge(@override_attrs, role.override_attributes) end def applied_role?(role_name) diff --git a/spec/unit/mixin/deep_merge_spec.rb b/spec/unit/mixin/deep_merge_spec.rb index 76f5c68a29..d024acccdd 100644 --- a/spec/unit/mixin/deep_merge_spec.rb +++ b/spec/unit/mixin/deep_merge_spec.rb @@ -290,32 +290,6 @@ describe Chef::Mixin::DeepMerge do end - describe "role_merge" do - it "errors out if knockout merge use is detected in an array" do - hash_dst = {"property" => ["2","4"]} - hash_src = {"property" => ["1","!merge:4"]} - lambda {@dm.role_merge(hash_dst, hash_src)}.should raise_error(Chef::Mixin::DeepMerge::InvalidSubtractiveMerge) - end - - it "errors out if knockout merge use is detected in an array (reversed merge order)" do - hash_dst = {"property" => ["1","!merge:4"]} - hash_src = {"property" => ["2","4"]} - lambda {@dm.role_merge(hash_dst, hash_src)}.should raise_error(Chef::Mixin::DeepMerge::InvalidSubtractiveMerge) - end - - it "errors out if knockout merge use is detected in a string" do - hash_dst = {"property" => ["2","4"]} - hash_src = {"property" => "!merge"} - lambda {@dm.role_merge(hash_dst, hash_src)}.should raise_error(Chef::Mixin::DeepMerge::InvalidSubtractiveMerge) - end - - it "errors out if knockout merge use is detected in a string (reversed merge order)" do - hash_dst = {"property" => "!merge"} - hash_src= {"property" => ["2","4"]} - lambda {@dm.role_merge(hash_dst, hash_src)}.should raise_error(Chef::Mixin::DeepMerge::InvalidSubtractiveMerge) - end - end - describe "hash-only merging" do it "merges Hashes like normal deep merge" do merge_ee_hash = {"top_level_a" => {"1_deep_a" => "1-a-merge-ee", "1_deep_b" => "1-deep-b-merge-ee"}, "top_level_b" => "top-level-b-merge-ee"} |