Merge pull request #1118 from opscode/CHEF-4631

CHEF-4596 / CHEF-4631 : Pick the array that is higher in the precedence order instead of merging during deep merge

6 files changed, 559 insertions, 231 deletions

diff --git a/chef/lib/chef/config.rb b/chef/lib/chef/config.rb
index 01fbbe14fd..51160d8cac 100644
--- a/chef/lib/chef/config.rb
+++ b/chef/lib/chef/config.rb
@@ -204,7 +204,7 @@ class Chef
     client_fork false
     enable_reporting true
     enable_reporting_url_fatals false
-    
+
     # Set these to enable SSL authentication / mutual-authentication
     # with the server
     ssl_client_cert nil
@@ -326,5 +326,9 @@ class Chef
     # returns a platform specific path to the user home dir
     windows_home_path = ENV['SYSTEMDRIVE'] + ENV['HOMEPATH'] if ENV['SYSTEMDRIVE'] && ENV['HOMEPATH']
     user_home (ENV['HOME'] || windows_home_path || ENV['USERPROFILE'])
+
+    # CHEF-4631
+    # Enables the concatanation behavior instead of merging
+    deep_merge_array_concat true
   end
 end
diff --git a/chef/lib/chef/mixin/deep_merge.rb b/chef/lib/chef/mixin/deep_merge.rb
index dd6e946ba1..8073352ee6 100644
--- a/chef/lib/chef/mixin/deep_merge.rb
+++ b/chef/lib/chef/mixin/deep_merge.rb
@@ -8,9 +8,9 @@
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
 # You may obtain a copy of the License at
-# 
+#
 #     http://www.apache.org/licenses/LICENSE-2.0
-# 
+#
 # Unless required by applicable law or agreed to in writing, software
 # distributed under the License is distributed on an "AS IS" BASIS,
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -35,17 +35,24 @@ class Chef
         DeepMerge.deep_merge(second, first, {:preserve_unmergeables => false})
       end
 
+      def horizontal_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, {:preserve_unmergeables => false, :horizontal_precedence => true})
+      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, {:knockout_prefix => "!merge", :preserve_unmergeables => false})
+        DeepMerge.deep_merge(second, first, {:preserve_unmergeables => false, :knockout_prefix => "!merge", :horizontal_precedence => true})
       end
-    
+
       class InvalidParameter < StandardError; end
-      
+
       # Deep Merge core documentation.
       # deep_merge! method permits merging of arbitrary child elements. The two top level
       # elements must be hashes. These hashes can contain unlimited (to stack limit) levels
@@ -60,7 +67,7 @@ class Chef
       #   Results: {:x => [1,2,3,4,5,'6'], :y => 2}
       # By default, "deep_merge!" will overwrite any unmergeables and merge everything else.
       # To avoid this, use "deep_merge" (no bang/exclamation mark)
-      # 
+      #
       # Options:
       #   Options are specified in the last parameter passed, which should be in hash format:
       #   hash.deep_merge!({:x => [1,2]}, {:knockout_prefix => '!merge'})
@@ -78,7 +85,7 @@ class Chef
       #      Set to true to get console output of merge process for debugging
       #
       # Selected Options Details:
-      # :knockout_prefix => The purpose of this is to provide a way to remove elements 
+      # :knockout_prefix => The purpose of this is to provide a way to remove elements
       #   from existing Hash by specifying them in a special way in incoming hash
       #    source = {:x => ['!merge:1', '2']}
       #    dest   = {:x => ['1', '3']}
@@ -96,9 +103,9 @@ class Chef
       #   dest   = {:x => ['5','6','7,8']}
       #   dest.deep_merge!(source, {:unpack_arrays => ','})
       #   Results: {:x => ['1','2','3','4','5','6','7','8'}
-      #   Why: If receiving data from an HTML form, this makes it easy for a checkbox 
+      #   Why: If receiving data from an HTML form, this makes it easy for a checkbox
       #    to pass multiple values from within a single HTML element
-      # 
+      #
       # There are many tests for this library - and you can learn more about the features
       # and usages of deep_merge! by just browsing the test examples
       def deep_merge!(source, dest, options = {})
@@ -131,13 +138,15 @@ class Chef
                 dest[src_key] = deep_merge!(src_value, dest[src_key], options.merge(:debug_indent => di + '  '))
               else # dest[src_key] doesn't exist so we want to create and overwrite it (but we do this via deep_merge!)
                 puts "#{di} ==>merging over: #{src_key.inspect} => #{src_value.inspect}" if merge_debug
-                # note: we rescue here b/c some classes respond to "dup" but don't implement it (Numeric, TrueClass, FalseClass, NilClass among maybe others)
-                begin
-                  src_dup = src_value.dup # we dup src_value if possible because we're going to merge into it (since dest is empty)
-                rescue TypeError
-                  src_dup = src_value
-                end
-                dest[src_key] = deep_merge!(src_value, src_dup, options.merge(:debug_indent => di + '  '))
+                # NOTE: We are doing this via deep_merge! because the
+                # src_value can still contain merge directives such as
+                # knockout_prefix.
+                # Historically we have been dup'ing the src_value here
+                # and merging src_value with src_value.dup. This logic
+                # is not applicable anymore because it results in
+                # duplicates when merging two array values with
+                # :horizontal_precedence = true.
+                dest[src_key] = deep_merge!(src_value, { }, options.merge(:debug_indent => di + '  '))
               end
             else # dest isn't a hash, so we overwrite it completely (if permitted)
               if overwrite_unmergeable
@@ -181,7 +190,30 @@ class Chef
               puts if merge_debug
             end
             puts "#{di} merging arrays: #{source.inspect} :: #{dest.inspect}" if merge_debug
-            dest = dest | source
+            # Behavior of merging arrays has changed with CHEF-4631.
+            # Old behavior was to deduplicate and merge the
+            # arrays. New behavior is to concatanate the arrays if the
+            # merge is happening on the same attribute precedence
+            # level and pick the value from the higher precedence
+            # level if merge is being done across the precedence
+            # levels.
+            # Old behavior can still be used by setting
+            # :deep_merge_array_concat to false in config.
+            if Chef::Config[:deep_merge_array_concat]
+              # If :horizontal_precedence is set, this means we are
+              # merging two arrays at the same precendence level so
+              # concatanate them. Otherwise this is a merge across
+              # precedence levels which means we will pick the one
+              # from higher precedence level.
+              if options[:horizontal_precedence]
+                dest += source
+              else
+                dest = source
+              end
+            else
+              # Pre CHEF-4631 behavior for array merging
+              dest = dest | source
+            end
             dest.sort! if sort_merged_arrays
           elsif overwrite_unmergeable
             puts "#{di} overwriting dest: #{source.inspect} -over-> #{dest.inspect}" if merge_debug
@@ -194,7 +226,7 @@ class Chef
         puts "#{di}Returning #{dest.inspect}" if merge_debug
         dest
       end # deep_merge!
-     
+
       # allows deep_merge! to uniformly handle overwriting of unmergeable entities
       def overwrite_unmergeables(source, dest, options)
         merge_debug = options[:merge_debug] || false
@@ -229,7 +261,7 @@ class Chef
       def deep_merge(source, dest, options = {})
         deep_merge!(source.dup, dest.dup, options)
       end
-     
+
       def clear_or_nil(obj)
         if obj.respond_to?(:clear)
           obj.clear
@@ -238,9 +270,9 @@ class Chef
         end
         obj
       end
-     
+
     end
-     
+
   end
 end
 
diff --git a/chef/lib/chef/node.rb b/chef/lib/chef/node.rb
index 76eb593838..96da8090b8 100644
--- a/chef/lib/chef/node.rb
+++ b/chef/lib/chef/node.rb
@@ -391,7 +391,7 @@ class Chef
     def consume_attributes(attrs)
       normal_attrs_to_merge = consume_run_list(attrs)
       Chef::Log.debug("Applying attributes from json file")
-      @normal_attrs = Chef::Mixin::DeepMerge.merge(@normal_attrs,normal_attrs_to_merge)
+      @normal_attrs = Chef::Mixin::DeepMerge.horizontal_merge(@normal_attrs,normal_attrs_to_merge)
       self.tags # make sure they're defined
     end
 
@@ -450,11 +450,11 @@ class Chef
     def apply_expansion_attributes(expansion)
       load_chef_environment_object = (chef_environment == "_default" ? nil : Chef::Environment.load(chef_environment))
       environment_default_attrs = load_chef_environment_object.nil? ? {} : load_chef_environment_object.default_attributes
-      default_before_roles = Chef::Mixin::DeepMerge.merge(default_attrs, environment_default_attrs)
-      @default_attrs = Chef::Mixin::DeepMerge.merge(default_before_roles, expansion.default_attrs)
+      default_before_roles = Chef::Mixin::DeepMerge.horizontal_merge(default_attrs, environment_default_attrs)
+      @default_attrs = Chef::Mixin::DeepMerge.horizontal_merge(default_before_roles, expansion.default_attrs)
       environment_override_attrs = load_chef_environment_object.nil? ? {} : load_chef_environment_object.override_attributes
-      overrides_before_environments = Chef::Mixin::DeepMerge.merge(override_attrs, expansion.override_attrs)
-      @override_attrs = Chef::Mixin::DeepMerge.merge(overrides_before_environments, environment_override_attrs)
+      overrides_before_environments = Chef::Mixin::DeepMerge.horizontal_merge(override_attrs, expansion.override_attrs)
+      @override_attrs = Chef::Mixin::DeepMerge.horizontal_merge(overrides_before_environments, environment_override_attrs)
     end
 
     # Transform the node to a Hash
diff --git a/chef/spec/unit/mixin/deep_merge_spec.rb b/chef/spec/unit/mixin/deep_merge_spec.rb
index 21be4974c6..a4853d38e2 100644
--- a/chef/spec/unit/mixin/deep_merge_spec.rb
+++ b/chef/spec/unit/mixin/deep_merge_spec.rb
@@ -75,81 +75,290 @@ describe Chef::Mixin::DeepMerge, "deep_merge!" do
     hash_dst.should == hash_src
   end
 
-  it "tests hashes holding array" do
-    hash_src = {"property" => ["1","3"]}
-    hash_dst = {"property" => ["2","4"]}
-    @dm.deep_merge!(hash_src, hash_dst)
-    hash_dst.should == {"property" => ["2","4","1","3"]}
-  end
-
-  it "tests hashes holding array (sorted)" do
-    hash_src = {"property" => ["1","3"]}
-    hash_dst = {"property" => ["2","4"]}
-    @dm.deep_merge!(hash_src, hash_dst, {:sort_merged_arrays => true})
-    hash_dst.should == {"property" => ["1","2","3","4"]}
-  end
-
-  it "tests hashes holding hashes holding arrays (array with duplicate elements is merged with dest then src" do
-    hash_src = {"property" => {"bedroom_count" => ["1", "2"], "bathroom_count" => ["1", "4+"]}}
-    hash_dst = {"property" => {"bedroom_count" => ["3", "2"], "bathroom_count" => ["2"]}}
-    @dm.deep_merge!(hash_src, hash_dst)
-    hash_dst.should == {"property" => {"bedroom_count" => ["3","2","1"], "bathroom_count" => ["2", "1", "4+"]}}
-  end
-
-  it "tests hash holding hash holding array v string (string is overwritten by array)" do
-    hash_src = {"property" => {"bedroom_count" => ["1", "2"], "bathroom_count" => ["1", "4+"]}}
-    hash_dst = {"property" => {"bedroom_count" => "3", "bathroom_count" => ["2"]}}
-    @dm.deep_merge!(hash_src, hash_dst)
-    hash_dst.should == {"property" => {"bedroom_count" => ["1", "2"], "bathroom_count" => ["2","1","4+"]}}
-  end
-
-  it "tests hash holding hash holding array v string (string is NOT overwritten by array)" do
-    hash_src = {"property" => {"bedroom_count" => ["1", "2"], "bathroom_count" => ["1", "4+"]}}
-    hash_dst = {"property" => {"bedroom_count" => "3", "bathroom_count" => ["2"]}}
-    @dm.deep_merge!(hash_src, hash_dst, {:preserve_unmergeables => true})
-    hash_dst.should == {"property" => {"bedroom_count" => "3", "bathroom_count" => ["2","1","4+"]}}
-  end
-
-  it "tests hash holding hash holding string v array (array is overwritten by string)" do
-    hash_src = {"property" => {"bedroom_count" => "3", "bathroom_count" => ["1", "4+"]}}
-    hash_dst = {"property" => {"bedroom_count" => ["1", "2"], "bathroom_count" => ["2"]}}
-    @dm.deep_merge!(hash_src, hash_dst)
-    hash_dst.should == {"property" => {"bedroom_count" => "3", "bathroom_count" => ["2","1","4+"]}}
-  end
-
-  it "tests hash holding hash holding string v array (array does NOT overwrite string)" do
-    hash_src = {"property" => {"bedroom_count" => "3", "bathroom_count" => ["1", "4+"]}}
-    hash_dst = {"property" => {"bedroom_count" => ["1", "2"], "bathroom_count" => ["2"]}}
-    @dm.deep_merge!(hash_src, hash_dst, {:preserve_unmergeables => true})
-    hash_dst.should == {"property" => {"bedroom_count" => ["1", "2"], "bathroom_count" => ["2","1","4+"]}}
-  end
-
-  it "tests hash holding hash holding hash v array (array is overwritten by hash)" do
-    hash_src = {"property" => {"bedroom_count" => {"king_bed" => 3, "queen_bed" => 1}, "bathroom_count" => ["1", "4+"]}}
-    hash_dst = {"property" => {"bedroom_count" => ["1", "2"], "bathroom_count" => ["2"]}}
-    @dm.deep_merge!(hash_src, hash_dst)
-    hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => 3, "queen_bed" => 1}, "bathroom_count" => ["2","1","4+"]}}
-  end
+  context "while merging arrays" do
+    it ":deep_merge_array_concat should be set by default" do
+      Chef::Config[:deep_merge_array_concat].should == true
+    end
 
-  it "tests hash holding hash holding hash v array (array is NOT overwritten by hash)" do
-    hash_src = {"property" => {"bedroom_count" => {"king_bed" => 3, "queen_bed" => 1}, "bathroom_count" => ["1", "4+"]}}
-    hash_dst = {"property" => {"bedroom_count" => ["1", "2"], "bathroom_count" => ["2"]}}
-    @dm.deep_merge!(hash_src, hash_dst, {:preserve_unmergeables => true})
-    hash_dst.should == {"property" => {"bedroom_count" => ["1", "2"], "bathroom_count" => ["2","1","4+"]}}
-  end
+    context "when :deep_merge_array_concat is set" do
+      before do
+        Chef::Config.stub!(:[]).with(:deep_merge_array_concat).and_return(true)
+      end
+
+      it "tests hashes holding array" do
+        hash_src = {"property" => ["1","3"]}
+        hash_dst = {"property" => ["2","4"]}
+        @dm.deep_merge!(hash_src, hash_dst)
+        hash_dst.should == {"property" => ["1","3"]}
+      end
+
+      it "tests hashes holding array (sorted)" do
+        hash_src = {"property" => ["3","1"]}
+        hash_dst = {"property" => ["2","4"]}
+        @dm.deep_merge!(hash_src, hash_dst, {:sort_merged_arrays => true})
+        hash_dst.should == {"property" => ["1","3"]}
+      end
+
+      it "tests hashes holding hashes holding arrays (array with duplicate elements are preserved)" do
+        hash_src = {"property" => {"bedroom_count" => ["1", "2", "2"], "bathroom_count" => ["1", "4+"]}}
+        hash_dst = {"property" => {"bedroom_count" => ["3", "2"], "bathroom_count" => ["2"]}}
+        @dm.deep_merge!(hash_src, hash_dst)
+        hash_dst.should == {"property" => {"bedroom_count" => ["1","2","2"], "bathroom_count" => ["1", "4+"]}}
+      end
+
+      it "tests 3 hash layers holding arrays of int (src arrays are picked)" do
+        hash_src = {"property" => {"bedroom_count" => {"king_bed" => [3], "queen_bed" => [1]}, "bathroom_count" => ["1", "4+"]}}
+        hash_dst = {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
+        @dm.deep_merge!(hash_src, hash_dst)
+        hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => [3], "queen_bed" => [1]}, "bathroom_count" => ["1","4+"]}}
+      end
+
+      it "tests 3 hash layers holding arrays of int (src arrays are picked) but second hash's array is overwritten" do
+        hash_src = {"property" => {"bedroom_count" => {"king_bed" => [3], "queen_bed" => [1]}, "bathroom_count" => "1"}}
+        hash_dst = {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
+        @dm.deep_merge!(hash_src, hash_dst)
+        hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => [3], "queen_bed" => [1]}, "bathroom_count" => "1"}}
+      end
+
+      it "tests 3 hash layers holding arrays of int (src arrays are picked) but second hash's array is NOT overwritten" do
+        hash_src = {"property" => {"bedroom_count" => {"king_bed" => [3], "queen_bed" => [1]}, "bathroom_count" => "1"}}
+        hash_dst = {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
+        @dm.deep_merge!(hash_src, hash_dst, {:preserve_unmergeables => true})
+        hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => [3], "queen_bed" => [1]}, "bathroom_count" => ["2"]}}
+      end
+
+      it "tests 3 hash layers holding arrays of int, but one holds int. This one overwrites, but src are picked for the rest" do
+        hash_src = {"property" => {"bedroom_count" => {"king_bed" => 3, "queen_bed" => [1]}, "bathroom_count" => ["1"]}}
+        hash_dst = {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
+        @dm.deep_merge!(hash_src, hash_dst)
+        hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => 3, "queen_bed" => [1]}, "bathroom_count" => ["1"]}}
+      end
+
+      it "tests 3 hash layers holding arrays of int, but source is incomplete." do
+        hash_src = {"property" => {"bedroom_count" => {"king_bed" => [3]}, "bathroom_count" => ["1"]}}
+        hash_dst = {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
+        @dm.deep_merge!(hash_src, hash_dst)
+        hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => [3], "queen_bed" => [4]}, "bathroom_count" => ["1"]}}
+      end
+
+      it "tests 3 hash layers holding arrays of int, but source is shorter and has new 2nd level ints." do
+        hash_src = {"property" => {"bedroom_count" => {2=>3, "king_bed" => [3]}, "bathroom_count" => ["1"]}}
+        hash_dst = {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
+        @dm.deep_merge!(hash_src, hash_dst)
+        hash_dst.should == {"property" => {"bedroom_count" => {2=>3, "king_bed" => [3], "queen_bed" => [4]}, "bathroom_count" => ["1"]}}
+      end
+
+      # KNOCKOUT_PREFIX testing
+      # the next few tests are looking for correct behavior from specific real-world params/session merges
+      # using the custom modifiers built for param/session merges
+
+      [nil, ","].each do |ko_split|
+        it "tests typical params/session style hash with knockout_merge elements" do
+          hash_src = {"property"=>{"bedroom_count"=>[@field_ko_prefix+":1", "2", "3"]}}
+          hash_dst = {"property"=>{"bedroom_count"=>["1", "2", "3"]}}
+          @dm.deep_merge!(hash_src, hash_dst, {:knockout_prefix => @field_ko_prefix, :unpack_arrays => ko_split})
+          hash_dst.should == {"property"=>{"bedroom_count"=>["2", "3"]}}
+        end
+
+        it "tests typical params/session style hash with knockout_merge elements" do
+          hash_src = {"property"=>{"bedroom_count"=>[@field_ko_prefix+":1", "2", "3"]}}
+          hash_dst = {"property"=>{"bedroom_count"=>["3"]}}
+          @dm.deep_merge!(hash_src, hash_dst, {:knockout_prefix => @field_ko_prefix, :unpack_arrays => ko_split})
+          hash_dst.should == {"property"=>{"bedroom_count"=>["2","3"]}}
+        end
+
+        it "tests typical params/session style hash with knockout_merge elements" do
+          hash_src = {"property"=>{"bedroom_count"=>[@field_ko_prefix+":1", "2", "3"]}}
+          hash_dst = {"property"=>{"bedroom_count"=>["4"]}}
+          @dm.deep_merge!(hash_src, hash_dst, {:knockout_prefix => @field_ko_prefix, :unpack_arrays => ko_split})
+          hash_dst.should == {"property"=>{"bedroom_count"=>["2","3"]}}
+        end
+
+        it "tests typical params/session style hash with knockout_merge elements" do
+          hash_src = {"property"=>{"bedroom_count"=>[@field_ko_prefix+":1", "2", "3"]}}
+          hash_dst = {"property"=>{"bedroom_count"=>[@field_ko_prefix+":1", "4"]}}
+          @dm.deep_merge!(hash_src, hash_dst, {:knockout_prefix => @field_ko_prefix, :unpack_arrays => ko_split})
+          hash_dst.should == {"property"=>{"bedroom_count"=>["2","3"]}}
+        end
+
+        it "tests typical params/session style hash with knockout_merge elements" do
+          hash_src = {"amenity"=>{"id"=>[@field_ko_prefix+":1", @field_ko_prefix+":2", "3", "4"]}}
+          hash_dst = {"amenity"=>{"id"=>["1", "2"]}}
+          @dm.deep_merge!(hash_src, hash_dst, {:knockout_prefix => @field_ko_prefix, :unpack_arrays => ko_split})
+          hash_dst.should == {"amenity"=>{"id"=>["3","4"]}}
+        end
+      end
+
+      it "tests same as previous but without ko_split value, this merge should fail" do
+        hash_src = {"amenity"=>{"id"=>[@field_ko_prefix+":1,"+@field_ko_prefix+":2", "3,4"]}}
+        hash_dst = {"amenity"=>{"id"=>["1", "2"]}}
+        @dm.deep_merge!(hash_src, hash_dst, {:knockout_prefix => @field_ko_prefix})
+        hash_dst.should == {"amenity"=>{"id"=>["3,4"]}}
+      end
+
+      it "tests edge test: make sure that when we turn off knockout_prefix that all values are processed correctly" do
+        hash_src = {"region" => {'ids' => ["7", @field_ko_prefix, "2", "6,8"]}}
+        hash_dst = {"region"=>{"ids"=>["1", "2", "3", "4"], 'id'=>'11'}}
+        @dm.deep_merge!(hash_src, hash_dst, {:unpack_arrays => ","})
+        hash_dst.should == {'region' => {'ids' => ["7", @field_ko_prefix, "2", "6", "8"], 'id'=>'11'}}
+      end
+
+      it "tests edge test 2: make sure that when we turn off source array split that all values are processed correctly" do
+        hash_src = {"region" => {'ids' => ["7", "3", @field_ko_prefix, "6,8"]}}
+        hash_dst = {"region"=>{"ids"=>["1", "2", "3", "4"], 'id'=>'11'}}
+        @dm.deep_merge!(hash_src, hash_dst)
+        hash_dst.should == {'region' => {'ids' => ["7", "3", @field_ko_prefix, "6,8"], 'id'=>'11'}}
+      end
+
+      it "tests hash of array of hashes" do
+        hash_src = {"item" => [{"1" => "3"}, {"2" => "4"}]}
+        hash_dst = {"item" => [{"3" => "5"}]}
+        @dm.deep_merge!(hash_src, hash_dst)
+        hash_dst.should == {"item" => [{"1" => "3"}, {"2" => "4"}]}
+      end
 
-  it "tests 3 hash layers holding integers (integers are overwritten by source)" do
-    hash_src = {"property" => {"bedroom_count" => {"king_bed" => 3, "queen_bed" => 1}, "bathroom_count" => ["1", "4+"]}}
-    hash_dst = {"property" => {"bedroom_count" => {"king_bed" => 2, "queen_bed" => 4}, "bathroom_count" => ["2"]}}
-    @dm.deep_merge!(hash_src, hash_dst)
-    hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => 3, "queen_bed" => 1}, "bathroom_count" => ["2","1","4+"]}}
-  end
+    end
 
-  it "tests 3 hash layers holding arrays of int (arrays are merged)" do
-    hash_src = {"property" => {"bedroom_count" => {"king_bed" => [3], "queen_bed" => [1]}, "bathroom_count" => ["1", "4+"]}}
-    hash_dst = {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
-    @dm.deep_merge!(hash_src, hash_dst)
-    hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => [2,3], "queen_bed" => [4,1]}, "bathroom_count" => ["2","1","4+"]}}
+    # These tests ensure that deep_merge behavior didn't change when
+    # the config option disables the array merging during deep merge
+    # while fixing CHEF-4631.
+    context "when :deep_merge_array_concat is not set" do
+      before do
+        Chef::Config.stub!(:[]).with(:deep_merge_array_concat).and_return(false)
+      end
+
+      it "tests hashes holding array" do
+        hash_src = {"property" => ["1","3"]}
+        hash_dst = {"property" => ["2","4"]}
+        @dm.deep_merge!(hash_src, hash_dst)
+        hash_dst.should == {"property" => ["2","4","1","3"]}
+      end
+
+      it "tests hashes holding array (sorted)" do
+        hash_src = {"property" => ["1","3"]}
+        hash_dst = {"property" => ["2","4"]}
+        @dm.deep_merge!(hash_src, hash_dst, {:sort_merged_arrays => true})
+        hash_dst.should == {"property" => ["1","2","3","4"]}
+      end
+
+      it "tests hashes holding hashes holding arrays (array with duplicate elements is merged with dest then src" do
+        hash_src = {"property" => {"bedroom_count" => ["1", "2"], "bathroom_count" => ["1", "4+"]}}
+        hash_dst = {"property" => {"bedroom_count" => ["3", "2"], "bathroom_count" => ["2"]}}
+        @dm.deep_merge!(hash_src, hash_dst)
+        hash_dst.should == {"property" => {"bedroom_count" => ["3","2","1"], "bathroom_count" => ["2", "1", "4+"]}}
+      end
+
+      it "tests 3 hash layers holding arrays of int (arrays are merged)" do
+        hash_src = {"property" => {"bedroom_count" => {"king_bed" => [3], "queen_bed" => [1]}, "bathroom_count" => ["1", "4+"]}}
+        hash_dst = {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
+        @dm.deep_merge!(hash_src, hash_dst)
+        hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => [2,3], "queen_bed" => [4,1]}, "bathroom_count" => ["2","1","4+"]}}
+      end
+
+      it "tests 3 hash layers holding arrays of int (arrays are merged) but second hash's array is overwritten" do
+        hash_src = {"property" => {"bedroom_count" => {"king_bed" => [3], "queen_bed" => [1]}, "bathroom_count" => "1"}}
+        hash_dst = {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
+        @dm.deep_merge!(hash_src, hash_dst)
+        hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => [2,3], "queen_bed" => [4,1]}, "bathroom_count" => "1"}}
+      end
+
+      it "tests 3 hash layers holding arrays of int (arrays are merged) but second hash's array is NOT overwritten" do
+        hash_src = {"property" => {"bedroom_count" => {"king_bed" => [3], "queen_bed" => [1]}, "bathroom_count" => "1"}}
+        hash_dst = {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
+        @dm.deep_merge!(hash_src, hash_dst, {:preserve_unmergeables => true})
+        hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => [2,3], "queen_bed" => [4,1]}, "bathroom_count" => ["2"]}}
+      end
+
+      it "tests 3 hash layers holding arrays of int, but one holds int. This one overwrites, but the rest merge" do
+        hash_src = {"property" => {"bedroom_count" => {"king_bed" => 3, "queen_bed" => [1]}, "bathroom_count" => ["1"]}}
+        hash_dst = {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
+        @dm.deep_merge!(hash_src, hash_dst)
+        hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => 3, "queen_bed" => [4,1]}, "bathroom_count" => ["2","1"]}}
+      end
+
+      it "tests 3 hash layers holding arrays of int, but source is incomplete." do
+        hash_src = {"property" => {"bedroom_count" => {"king_bed" => [3]}, "bathroom_count" => ["1"]}}
+        hash_dst = {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
+        @dm.deep_merge!(hash_src, hash_dst)
+        hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => [2,3], "queen_bed" => [4]}, "bathroom_count" => ["2","1"]}}
+      end
+
+      it "tests 3 hash layers holding arrays of int, but source is shorter and has new 2nd level ints." do
+        hash_src = {"property" => {"bedroom_count" => {2=>3, "king_bed" => [3]}, "bathroom_count" => ["1"]}}
+        hash_dst = {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
+        @dm.deep_merge!(hash_src, hash_dst)
+        hash_dst.should == {"property" => {"bedroom_count" => {2=>3, "king_bed" => [2,3], "queen_bed" => [4]}, "bathroom_count" => ["2","1"]}}
+      end
+
+      # KNOCKOUT_PREFIX testing
+      # the next few tests are looking for correct behavior from specific real-world params/session merges
+      # using the custom modifiers built for param/session merges
+
+      [nil, ","].each do |ko_split|
+        it "tests typical params/session style hash with knockout_merge elements" do
+          hash_src = {"property"=>{"bedroom_count"=>[@field_ko_prefix+":1", "2", "3"]}}
+          hash_dst = {"property"=>{"bedroom_count"=>["1", "2", "3"]}}
+          @dm.deep_merge!(hash_src, hash_dst, {:knockout_prefix => @field_ko_prefix, :unpack_arrays => ko_split})
+          hash_dst.should == {"property"=>{"bedroom_count"=>["2", "3"]}}
+        end
+
+        it "tests typical params/session style hash with knockout_merge elements" do
+          hash_src = {"property"=>{"bedroom_count"=>[@field_ko_prefix+":1", "2", "3"]}}
+          hash_dst = {"property"=>{"bedroom_count"=>["3"]}}
+          @dm.deep_merge!(hash_src, hash_dst, {:knockout_prefix => @field_ko_prefix, :unpack_arrays => ko_split})
+          hash_dst.should == {"property"=>{"bedroom_count"=>["3","2"]}}
+        end
+
+        it "tests typical params/session style hash with knockout_merge elements" do
+          hash_src = {"property"=>{"bedroom_count"=>[@field_ko_prefix+":1", "2", "3"]}}
+          hash_dst = {"property"=>{"bedroom_count"=>["4"]}}
+          @dm.deep_merge!(hash_src, hash_dst, {:knockout_prefix => @field_ko_prefix, :unpack_arrays => ko_split})
+          hash_dst.should == {"property"=>{"bedroom_count"=>["4","2","3"]}}
+        end
+
+        it "tests typical params/session style hash with knockout_merge elements" do
+          hash_src = {"property"=>{"bedroom_count"=>[@field_ko_prefix+":1", "2", "3"]}}
+          hash_dst = {"property"=>{"bedroom_count"=>[@field_ko_prefix+":1", "4"]}}
+          @dm.deep_merge!(hash_src, hash_dst, {:knockout_prefix => @field_ko_prefix, :unpack_arrays => ko_split})
+          hash_dst.should == {"property"=>{"bedroom_count"=>["4","2","3"]}}
+        end
+
+        it "tests typical params/session style hash with knockout_merge elements" do
+          hash_src = {"amenity"=>{"id"=>[@field_ko_prefix+":1", @field_ko_prefix+":2", "3", "4"]}}
+          hash_dst = {"amenity"=>{"id"=>["1", "2"]}}
+          @dm.deep_merge!(hash_src, hash_dst, {:knockout_prefix => @field_ko_prefix, :unpack_arrays => ko_split})
+          hash_dst.should == {"amenity"=>{"id"=>["3","4"]}}
+        end
+      end
+
+      it "tests same as previous but without ko_split value, this merge should fail" do
+        hash_src = {"amenity"=>{"id"=>[@field_ko_prefix+":1,"+@field_ko_prefix+":2", "3,4"]}}
+        hash_dst = {"amenity"=>{"id"=>["1", "2"]}}
+        @dm.deep_merge!(hash_src, hash_dst, {:knockout_prefix => @field_ko_prefix})
+        hash_dst.should == {"amenity"=>{"id"=>["1","2","3,4"]}}
+      end
+
+      it "tests edge test: make sure that when we turn off knockout_prefix that all values are processed correctly" do
+        hash_src = {"region" => {'ids' => ["7", @field_ko_prefix, "2", "6,8"]}}
+        hash_dst = {"region"=>{"ids"=>["1", "2", "3", "4"], 'id'=>'11'}}
+        @dm.deep_merge!(hash_src, hash_dst, {:unpack_arrays => ","})
+        hash_dst.should == {'region' => {'ids' => ["1", "2", "3", "4", "7", @field_ko_prefix, "6", "8"], 'id'=>'11'}}
+      end
+
+      it "tests edge test 2: make sure that when we turn off source array split that all values are processed correctly" do
+        hash_src = {"region" => {'ids' => ["7", "3", @field_ko_prefix, "6,8"]}}
+        hash_dst = {"region"=>{"ids"=>["1", "2", "3", "4"], 'id'=>'11'}}
+        @dm.deep_merge!(hash_src, hash_dst)
+        hash_dst.should == {'region' => {'ids' => ["1", "2", "3", "4", "7", @field_ko_prefix, "6,8"], 'id'=>'11'}}
+      end
+
+      it "tests hash of array of hashes" do
+        hash_src = {"item" => [{"1" => "3"}, {"2" => "4"}]}
+        hash_dst = {"item" => [{"3" => "5"}]}
+        @dm.deep_merge!(hash_src, hash_dst)
+        hash_dst.should == {"item" => [{"3" => "5"}, {"1" => "3"}, {"2" => "4"}]}
+      end
+    end
   end
 
   it "tests 1 hash overwriting 3 hash layers holding arrays of int" do
@@ -166,53 +375,67 @@ describe Chef::Mixin::DeepMerge, "deep_merge!" do
     hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
   end
 
-  it "tests 3 hash layers holding arrays of int (arrays are merged) but second hash's array is overwritten" do
-    hash_src = {"property" => {"bedroom_count" => {"king_bed" => [3], "queen_bed" => [1]}, "bathroom_count" => "1"}}
+  it "tests 3 hash layers holding arrays of int, but source is empty" do
+    hash_src = {}
     hash_dst = {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
     @dm.deep_merge!(hash_src, hash_dst)
-    hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => [2,3], "queen_bed" => [4,1]}, "bathroom_count" => "1"}}
+    hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
   end
 
-  it "tests 3 hash layers holding arrays of int (arrays are merged) but second hash's array is NOT overwritten" do
-    hash_src = {"property" => {"bedroom_count" => {"king_bed" => [3], "queen_bed" => [1]}, "bathroom_count" => "1"}}
-    hash_dst = {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
-    @dm.deep_merge!(hash_src, hash_dst, {:preserve_unmergeables => true})
-    hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => [2,3], "queen_bed" => [4,1]}, "bathroom_count" => ["2"]}}
+  it "tests 3 hash layers holding arrays of int, but dest is empty" do
+    hash_src = {"property" => {"bedroom_count" => {2=>3, "king_bed" => [3]}, "bathroom_count" => ["1"]}}
+    hash_dst = {}
+    @dm.deep_merge!(hash_src, hash_dst)
+    hash_dst.should == {"property" => {"bedroom_count" => {2=>3, "king_bed" => [3]}, "bathroom_count" => ["1"]}}
   end
 
-  it "tests 3 hash layers holding arrays of int, but one holds int. This one overwrites, but the rest merge" do
-    hash_src = {"property" => {"bedroom_count" => {"king_bed" => 3, "queen_bed" => [1]}, "bathroom_count" => ["1"]}}
-    hash_dst = {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
+  it "tests hash holding hash holding array v string (string is overwritten by array)" do
+    hash_src = {"property" => {"bedroom_count" => ["1", "2"]}}
+    hash_dst = {"property" => {"bedroom_count" => "3"}}
     @dm.deep_merge!(hash_src, hash_dst)
-    hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => 3, "queen_bed" => [4,1]}, "bathroom_count" => ["2","1"]}}
+    hash_dst.should == {"property" => {"bedroom_count" => ["1", "2"]}}
   end
 
-  it "tests 3 hash layers holding arrays of int, but source is incomplete." do
-    hash_src = {"property" => {"bedroom_count" => {"king_bed" => [3]}, "bathroom_count" => ["1"]}}
-    hash_dst = {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
-    @dm.deep_merge!(hash_src, hash_dst)
-    hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => [2,3], "queen_bed" => [4]}, "bathroom_count" => ["2","1"]}}
+  it "tests hash holding hash holding array v string (string is NOT overwritten by array)" do
+    hash_src = {"property" => {"bedroom_count" => ["1", "2"]}}
+    hash_dst = {"property" => {"bedroom_count" => "3"}}
+    @dm.deep_merge!(hash_src, hash_dst, {:preserve_unmergeables => true})
+    hash_dst.should == {"property" => {"bedroom_count" => "3"}}
   end
 
-  it "tests 3 hash layers holding arrays of int, but source is shorter and has new 2nd level ints." do
-    hash_src = {"property" => {"bedroom_count" => {2=>3, "king_bed" => [3]}, "bathroom_count" => ["1"]}}
-    hash_dst = {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
+  it "tests hash holding hash holding string v array (array is overwritten by string)" do
+    hash_src = {"property" => {"bedroom_count" => "3"}}
+    hash_dst = {"property" => {"bedroom_count" => ["1", "2"]}}
     @dm.deep_merge!(hash_src, hash_dst)
-    hash_dst.should == {"property" => {"bedroom_count" => {2=>3, "king_bed" => [2,3], "queen_bed" => [4]}, "bathroom_count" => ["2","1"]}}
+    hash_dst.should == {"property" => {"bedroom_count" => "3"}}
   end
 
-  it "tests 3 hash layers holding arrays of int, but source is empty" do
-    hash_src = {}
-    hash_dst = {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
+  it "tests hash holding hash holding string v array (array does NOT overwrite string)" do
+    hash_src = {"property" => {"bedroom_count" => "3"}}
+    hash_dst = {"property" => {"bedroom_count" => ["1", "2"]}}
+    @dm.deep_merge!(hash_src, hash_dst, {:preserve_unmergeables => true})
+    hash_dst.should == {"property" => {"bedroom_count" => ["1", "2"]}}
+  end
+
+  it "tests hash holding hash holding hash v array (array is overwritten by hash)" do
+    hash_src = {"property" => {"bedroom_count" => {"king_bed" => 3, "queen_bed" => 1}}}
+    hash_dst = {"property" => {"bedroom_count" => ["1", "2"]}}
     @dm.deep_merge!(hash_src, hash_dst)
-    hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => [2], "queen_bed" => [4]}, "bathroom_count" => ["2"]}}
+    hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => 3, "queen_bed" => 1}}}
   end
 
-  it "tests 3 hash layers holding arrays of int, but dest is empty" do
-    hash_src = {"property" => {"bedroom_count" => {2=>3, "king_bed" => [3]}, "bathroom_count" => ["1"]}}
-    hash_dst = {}
+  it "tests hash holding hash holding hash v array (array is NOT overwritten by hash)" do
+    hash_src = {"property" => {"bedroom_count" => {"king_bed" => 3, "queen_bed" => 1}}}
+    hash_dst = {"property" => {"bedroom_count" => ["1", "2"]}}
+    @dm.deep_merge!(hash_src, hash_dst, {:preserve_unmergeables => true})
+    hash_dst.should == {"property" => {"bedroom_count" => ["1", "2"]}}
+  end
+
+  it "tests 3 hash layers holding integers (integers are overwritten by source)" do
+    hash_src = {"property" => {"bedroom_count" => {"king_bed" => 3, "queen_bed" => 1}}}
+    hash_dst = {"property" => {"bedroom_count" => {"king_bed" => 2, "queen_bed" => 4}}}
     @dm.deep_merge!(hash_src, hash_dst)
-    hash_dst.should == {"property" => {"bedroom_count" => {2=>3, "king_bed" => [3]}, "bathroom_count" => ["1"]}}
+    hash_dst.should == {"property" => {"bedroom_count" => {"king_bed" => 3, "queen_bed" => 1}}}
   end
 
   it "tests parameter management for knockout_prefix and overwrite unmergable" do
@@ -268,47 +491,6 @@ describe Chef::Mixin::DeepMerge, "deep_merge!" do
     hash_dst.should == hash_src
   end
 
-  # KNOCKOUT_PREFIX testing
-  # the next few tests are looking for correct behavior from specific real-world params/session merges
-  # using the custom modifiers built for param/session merges
-
-  [nil, ","].each do |ko_split|
-    it "tests typical params/session style hash with knockout_merge elements" do
-      hash_src = {"property"=>{"bedroom_count"=>[@field_ko_prefix+":1", "2", "3"]}}
-      hash_dst = {"property"=>{"bedroom_count"=>["1", "2", "3"]}}
-      @dm.deep_merge!(hash_src, hash_dst, {:knockout_prefix => @field_ko_prefix, :unpack_arrays => ko_split})
-      hash_dst.should == {"property"=>{"bedroom_count"=>["2", "3"]}}
-    end
-
-    it "tests typical params/session style hash with knockout_merge elements" do
-      hash_src = {"property"=>{"bedroom_count"=>[@field_ko_prefix+":1", "2", "3"]}}
-      hash_dst = {"property"=>{"bedroom_count"=>["3"]}}
-      @dm.deep_merge!(hash_src, hash_dst, {:knockout_prefix => @field_ko_prefix, :unpack_arrays => ko_split})
-      hash_dst.should == {"property"=>{"bedroom_count"=>["3","2"]}}
-    end
-
-    it "tests typical params/session style hash with knockout_merge elements" do
-      hash_src = {"property"=>{"bedroom_count"=>[@field_ko_prefix+":1", "2", "3"]}}
-      hash_dst = {"property"=>{"bedroom_count"=>["4"]}}
-      @dm.deep_merge!(hash_src, hash_dst, {:knockout_prefix => @field_ko_prefix, :unpack_arrays => ko_split})
-      hash_dst.should == {"property"=>{"bedroom_count"=>["4","2","3"]}}
-    end
-
-    it "tests typical params/session style hash with knockout_merge elements" do
-      hash_src = {"property"=>{"bedroom_count"=>[@field_ko_prefix+":1", "2", "3"]}}
-      hash_dst = {"property"=>{"bedroom_count"=>[@field_ko_prefix+":1", "4"]}}
-      @dm.deep_merge!(hash_src, hash_dst, {:knockout_prefix => @field_ko_prefix, :unpack_arrays => ko_split})
-      hash_dst.should == {"property"=>{"bedroom_count"=>["4","2","3"]}}
-    end
-
-    it "tests typical params/session style hash with knockout_merge elements" do
-      hash_src = {"amenity"=>{"id"=>[@field_ko_prefix+":1", @field_ko_prefix+":2", "3", "4"]}}
-      hash_dst = {"amenity"=>{"id"=>["1", "2"]}}
-      @dm.deep_merge!(hash_src, hash_dst, {:knockout_prefix => @field_ko_prefix, :unpack_arrays => ko_split})
-      hash_dst.should == {"amenity"=>{"id"=>["3","4"]}}
-    end
-  end
-
   it "tests special params/session style hash with knockout_merge elements in form src: [\"1\",\"2\"] dest:[\"@ko:1,@ko:2\", \"3,4\"]" do
     hash_src = {"amenity"=>{"id"=>[@field_ko_prefix+":1,"+@field_ko_prefix+":2", "3,4"]}}
     hash_dst = {"amenity"=>{"id"=>["1", "2"]}}
@@ -316,13 +498,6 @@ describe Chef::Mixin::DeepMerge, "deep_merge!" do
     hash_dst.should == {"amenity"=>{"id"=>["3","4"]}}
   end
 
-  it "tests same as previous but without ko_split value, this merge should fail" do
-    hash_src = {"amenity"=>{"id"=>[@field_ko_prefix+":1,"+@field_ko_prefix+":2", "3,4"]}}
-    hash_dst = {"amenity"=>{"id"=>["1", "2"]}}
-    @dm.deep_merge!(hash_src, hash_dst, {:knockout_prefix => @field_ko_prefix})
-    hash_dst.should == {"amenity"=>{"id"=>["1","2","3,4"]}}
-  end
-
   it "tests special params/session style hash with knockout_merge elements in form src: [\"1\",\"2\"] dest:[\"@ko:1,@ko:2\", \"3,4\"]" do
     hash_src = {"amenity"=>{"id"=>[@field_ko_prefix+":1,2", "3,4", @field_ko_prefix+":5", "6"]}}
     hash_dst = {"amenity"=>{"id"=>["1", "2"]}}
@@ -501,20 +676,6 @@ describe Chef::Mixin::DeepMerge, "deep_merge!" do
     hash_dst.should == {'region' => {'ids' => ["7", "6"], 'id'=>'11'}}
   end
 
-  it "tests edge test: make sure that when we turn off knockout_prefix that all values are processed correctly" do
-    hash_src = {"region" => {'ids' => ["7", @field_ko_prefix, "2", "6,8"]}}
-    hash_dst = {"region"=>{"ids"=>["1", "2", "3", "4"], 'id'=>'11'}}
-    @dm.deep_merge!(hash_src, hash_dst, {:unpack_arrays => ","})
-    hash_dst.should == {'region' => {'ids' => ["1", "2", "3", "4", "7", @field_ko_prefix, "6", "8"], 'id'=>'11'}}
-  end
-
-  it "tests edge test 2: make sure that when we turn off source array split that all values are processed correctly" do
-    hash_src = {"region" => {'ids' => ["7", "3", @field_ko_prefix, "6,8"]}}
-    hash_dst = {"region"=>{"ids"=>["1", "2", "3", "4"], 'id'=>'11'}}
-    @dm.deep_merge!(hash_src, hash_dst)
-    hash_dst.should == {'region' => {'ids' => ["1", "2", "3", "4", "7", @field_ko_prefix, "6,8"], 'id'=>'11'}}
-  end
-
   it "tests Example: src = {'key' => \"@ko:1\"}, dst = {'key' => \"1\"} -> merges to {'key' => \"\"}" do
     hash_src = {"amenity"=>@field_ko_prefix+":1"}
     hash_dst = {"amenity"=>"1"}
@@ -662,13 +823,6 @@ describe Chef::Mixin::DeepMerge, "deep_merge!" do
     hash_dst.should == {"query_uuid"=>"a0dc3c84-ec7f-6756-bdb0-fff9157438ab", "url_regions"=>[], "region"=>{"muni_city_id"=>"", "id"=>""}, "property"=>{"property_type_id"=>"", "search_rate_min"=>"", "search_rate_max"=>""}, "task"=>"search", "run_query"=>"Search"}
   end
 
-  it "tests hash of array of hashes" do
-    hash_src = {"item" => [{"1" => "3"}, {"2" => "4"}]}
-    hash_dst = {"item" => [{"3" => "5"}]}
-    @dm.deep_merge!(hash_src, hash_dst)
-    hash_dst.should == {"item" => [{"3" => "5"}, {"1" => "3"}, {"2" => "4"}]}
-  end
-
   # Additions since import
   it "should overwrite true with false when merging boolean values" do
     hash_src = {"valid" => false}
@@ -724,10 +878,66 @@ describe Chef::Mixin::DeepMerge do
       @dm.merge(hash_dst, hash_src).should == {"name" => "value"}
     end
 
-    it "should merge arrays within hashes" do
-      hash_dst = {"property" => ["2","4"]}
-      hash_src = {"property" => ["1","3"]}
-      @dm.merge(hash_dst, hash_src).should == {"property" => ["2","4","1","3"]}
+    context "when :deep_merge_array_concat is set" do
+      before do
+        Chef::Config.stub!(:[]).with(:deep_merge_array_concat).and_return(true)
+      end
+
+      it "should pick src arrays within hashes" do
+        hash_dst = {"property" => ["2","4"]}
+        hash_src = {"property" => ["1","3"]}
+        @dm.merge(hash_dst, hash_src).should == {"property" => ["1","3"]}
+      end
+
+      it "should not modify the source or destination during the merge" do
+        hash_dst = {"property" => ["1","2","3"]}
+        hash_src = {"property" => ["4","5","6"]}
+        ret = @dm.merge(hash_dst, hash_src)
+        hash_dst.should == {"property" => ["1","2","3"]}
+        hash_src.should == {"property" => ["4","5","6"]}
+        ret.should == {"property" => ["4","5","6"]}
+      end
+
+      it "should not knockout matching array value when merging arrays within hashes" do
+        hash_dst = {"property" => ["2","4"]}
+        hash_src = {"property" => ["1","!merge:4"]}
+        hash_src_no_colon = {"property" => ["1","!merge"]}
+        @dm.merge(hash_dst, hash_src).should == {"property" => ["1", "!merge:4"]}
+        @dm.merge(hash_dst, hash_src_no_colon).should == {"property" => ["1", "!merge"]}
+      end
+    end
+
+    # These tests ensure that deep_merge behavior didn't change when
+    # the config option disables the array merging during deep merge
+    # while fixing CHEF-4631.
+    context "when :deep_merge_array_concat is not set" do
+      before do
+        Chef::Config.stub!(:[]).with(:deep_merge_array_concat).and_return(false)
+      end
+
+      it "should merge arrays within hashes" do
+        hash_dst = {"property" => ["2","4"]}
+        hash_src = {"property" => ["1","3"]}
+        @dm.merge(hash_dst, hash_src).should == {"property" => ["2","4","1","3"]}
+      end
+
+      it "should not modify the source or destination during the merge" do
+        hash_dst = {"property" => ["1","2","3"]}
+        hash_src = {"property" => ["4","5","6"]}
+        ret = @dm.merge(hash_dst, hash_src)
+        hash_dst.should == {"property" => ["1","2","3"]}
+        hash_src.should == {"property" => ["4","5","6"]}
+        ret.should == {"property" => ["1","2","3","4","5","6"]}
+      end
+
+      it "should not knockout matching array value when merging arrays within hashes" do
+        hash_dst = {"property" => ["2","4"]}
+        hash_src = {"property" => ["1","!merge:4"]}
+        hash_src_no_colon = {"property" => ["1","!merge"]}
+        @dm.merge(hash_dst, hash_src).should == {"property" => ["2", "4", "1", "!merge:4"]}
+        @dm.merge(hash_dst, hash_src_no_colon).should == {"property" => ["2", "4", "1", "!merge"]}
+      end
+
     end
 
     it "should merge deeply nested hashes" do
@@ -736,22 +946,6 @@ describe Chef::Mixin::DeepMerge do
       @dm.merge(hash_dst, hash_src).should == {"property" => {"values" => {"are" => "stable", "can" => "change", "may" => "rise"}}}
     end
 
-    it "should not modify the source or destination during the merge" do
-      hash_dst = {"property" => ["1","2","3"]}
-      hash_src = {"property" => ["4","5","6"]}
-      ret = @dm.merge(hash_dst, hash_src)
-      hash_dst.should == {"property" => ["1","2","3"]}
-      hash_src.should == {"property" => ["4","5","6"]}
-      ret.should == {"property" => ["1","2","3","4","5","6"]}
-    end
-
-    it "should not knockout matching array value when merging arrays within hashes" do
-      hash_dst = {"property" => ["2","4"]}
-      hash_src = {"property" => ["1","!merge:4"]}
-      hash_src_no_colon = {"property" => ["1","!merge"]}
-      @dm.merge(hash_dst, hash_src).should == {"property" => ["2", "4", "1", "!merge:4"]}
-      @dm.merge(hash_dst, hash_src_no_colon).should == {"property" => ["2", "4", "1", "!merge"]}
-    end
   end
 
   describe "role_merge" do
@@ -782,5 +976,37 @@ describe Chef::Mixin::DeepMerge do
       hash_src = {"property" => ["1","!merge:4"]}
       @dm.role_merge(hash_dst, hash_src).should == {"property" => ["2","1"]}
     end
+
+    it "should concat arrays" do
+      hash_dst = {"property" => ["2","4"]}
+      hash_src = {"property" => ["1","4"]}
+      @dm.role_merge(hash_dst, hash_src).should == {"property" => ["2","4", "1","4"]}
+    end
+  end
+
+  describe "horizontal_merge" do
+    it "should concat arrays" do
+      hash_dst = {"property" => ["2","4"]}
+      hash_src = {"property" => ["1","3"]}
+      @dm.horizontal_merge(hash_dst, hash_src).should == {"property" => ["2","4", "1","3"]}
+    end
+
+    it "should concat arrays without removing same elements" do
+      hash_dst = {"property" => ["2","4"]}
+      hash_src = {"property" => ["1","2"]}
+      @dm.horizontal_merge(hash_dst, hash_src).should == {"property" => ["2","4", "1","2"]}
+    end
+
+    it "should be able to merge array values with an empty hash as source" do
+      hash_dst = {"property" => ["2","4"]}
+      hash_src = {}
+      @dm.horizontal_merge(hash_dst, hash_src).should == {"property" => ["2","4"]}
+    end
+
+    it "should be able to merge array values with an empty hash as dest" do
+      hash_dst = {}
+      hash_src = {"property" => ["2","4"]}
+      @dm.horizontal_merge(hash_dst, hash_src).should == {"property" => ["2","4"]}
+    end
   end
 end
diff --git a/chef/spec/unit/node/attribute_spec.rb b/chef/spec/unit/node/attribute_spec.rb
index a3c5fe749a..a59e7d5a28 100644
--- a/chef/spec/unit/node/attribute_spec.rb
+++ b/chef/spec/unit/node/attribute_spec.rb
@@ -7,9 +7,9 @@
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
 # You may obtain a copy of the License at
-# 
+#
 #     http://www.apache.org/licenses/LICENSE-2.0
-# 
+#
 # Unless required by applicable law or agreed to in writing, software
 # distributed under the License is distributed on an "AS IS" BASIS,
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
@@ -20,9 +20,9 @@
 require 'spec_helper'
 require 'chef/node/attribute'
 
-describe Chef::Node::Attribute do 
+describe Chef::Node::Attribute do
   before(:each) do
-    @attribute_hash = 
+    @attribute_hash =
       {"dmi"=>{},
         "command"=>{"ps"=>"ps -ef"},
         "platform_version"=>"10.5.7",
@@ -193,7 +193,7 @@ describe Chef::Node::Attribute do
     @default_hash = {
       "domain" => "opscode.com",
       "hot" => { "day" => "saturday" },
-      "music" => { 
+      "music" => {
         "jimmy_eat_world" => "is fun!",
         "mastodon" => "rocks",
         "mars_volta" => "is loud and nutty",
@@ -225,7 +225,7 @@ describe Chef::Node::Attribute do
     [ :normal, :default, :override, :automatic ].each do |accessor|
       it "should set #{accessor}" do
         na = Chef::Node::Attribute.new({ :normal => true }, { :default => true }, { :override => true }, { :automatic => true })
-        na.send(accessor).should == { accessor => true } 
+        na.send(accessor).should == { accessor => true }
       end
     end
 
@@ -403,6 +403,21 @@ describe Chef::Node::Attribute do
       hash.class.should == Hash
       hash["day"].should == "sunday"
     end
+
+    # Regression test for CHEF-4631
+    context "when merging array values" do
+      before do
+        @default_attrs = {"foo" => {"bar" => ["default"]}}
+        @override_attrs = {"foo" => {"bar" => ["override"]}}
+
+        #(normal, default, override, automatic, state=[])
+        @attributes = Chef::Node::Attribute.new({ }, @default_attrs, @override_attrs, { })
+      end
+
+      it "should return the override" do
+        @attributes["foo"].to_hash["bar"].should == [ "override" ]
+      end
+    end
   end
 
   describe "has_key?" do
@@ -426,10 +441,10 @@ describe Chef::Node::Attribute do
       @attributes["music"]
       @attributes.has_key?("apophis").should == true
     end
-    
+
     it "should find keys at the current nesting level" do
       @attributes["music"]
-      @attributes.has_key?("mastodon").should == true 
+      @attributes.has_key?("mastodon").should == true
       @attributes.has_key?("whitesnake").should == false
     end
 
@@ -443,7 +458,7 @@ describe Chef::Node::Attribute do
 
     [:include?, :key?, :member?].each do |method|
       it "should alias the method #{method} to itself" do
-        @attributes.should respond_to(method) 
+        @attributes.should respond_to(method)
       end
 
       it "#{method} should behave like has_key?" do
@@ -463,7 +478,7 @@ describe Chef::Node::Attribute do
 
     it "should be looking at the current position of the object" do
       @attributes["music"]
-      @attributes.attribute?("mastodon").should == true 
+      @attributes.attribute?("mastodon").should == true
       @attributes.attribute?("whitesnake").should == false
     end
   end
@@ -516,7 +531,7 @@ describe Chef::Node::Attribute do
       collect.include?("snakes").should == true
       collect.include?("snack").should == true
       collect.include?("place").should == true
-      collect.length.should == 5 
+      collect.length.should == 5
     end
 
     it "should yield lower if we go deeper" do
@@ -527,7 +542,7 @@ describe Chef::Node::Attribute do
       collect.include?("two").should == true
       collect.include?("four").should == true
       collect.include?("six").should == true
-      collect.length.should == 3 
+      collect.length.should == 3
     end
 
     it "should not raise an exception if one of the hashes has a nil value on a deep lookup" do
@@ -601,7 +616,7 @@ describe Chef::Node::Attribute do
       @attributes.each_key do |k|
         collect << k
       end
-     
+
       collect.should include("one")
       collect.should include("snack")
       collect.should include("hut")
@@ -644,7 +659,7 @@ describe Chef::Node::Attribute do
       collect["snack"].should == "cookies"
     end
   end
-  
+
   describe "each_value" do
     before do
       @attributes = Chef::Node::Attribute.new(
@@ -1027,7 +1042,27 @@ describe Chef::Node::Attribute do
     it "returns the automatic (highest precedence) value when deleting a key" do
       @attributes["foo"].delete("bar").should == "automatic_value"
     end
+  end
+
+  describe "regression test for CHEF-4631" do
+    before(:each) do
+      @default_attrs = {"foo" => {"bar" => ["default"]}}
+      @override_attrs = {"foo" => {"bar" => ["override"]}}
+
+      #(normal, default, override, automatic, state=[])
+      @attributes = Chef::Node::Attribute.new({ }, @default_attrs, @override_attrs, { })
+    end
 
+    it "array values in the role default attributes should be concatanated to the nodes default attributes" do
+      @default_attrs = {"foo" => {"bar" => ["1", "2"]}}
+      @attributes = Chef::Node::Attribute.new({ }, @default_attrs, { }, { })
+
+      expansion = mock("Expansion", :default_attrs => { "foo" => {"bar" => ["3", "2"]}}, :override_attributes => { })
+
+    end
+
+    it "array values in the role override attributes should override the nodes default attributes" do
+    end
   end
 
 end
diff --git a/chef/spec/unit/node_spec.rb b/chef/spec/unit/node_spec.rb
index d429fb0b12..2af8e4d97e 100644
--- a/chef/spec/unit/node_spec.rb
+++ b/chef/spec/unit/node_spec.rb
@@ -741,4 +741,35 @@ describe Chef::Node do
 
   end
 
+  describe "regression test for CHEF-4631" do
+    before(:each) do
+      @node.stub!(:chef_environment).and_return("_default")
+    end
+
+    it "array values in the role default attributes should be concatanated to the nodes default attributes" do
+      @node.default_attrs = {"foo" => {"bar" => ["1", "2"]}}
+      expansion = mock("Expansion", :default_attrs => { "foo" => {"bar" => ["3", "2"]}}, :override_attrs => { })
+      @node.apply_expansion_attributes(expansion)
+      @node["foo"]["bar"].should == ["1", "2", "3", "2"]
+      @node["foo"].to_hash["bar"].should == ["1", "2", "3", "2"]
+    end
+
+    it "array values in the role override attributes should override the nodes default attributes" do
+      @node.default_attrs = {"foo" => {"bar" => ["1", "2"]}}
+      expansion = mock("Expansion", :default_attrs => { "foo" => {"bar" => ["3", "2"]}}, :override_attrs => { "foo" => {"bar" => ["5"] } })
+      @node.apply_expansion_attributes(expansion)
+      @node["foo"]["bar"].should == ["5"]
+      @node["foo"].to_hash["bar"].should == ["5"]
+    end
+
+    it "array values in the role override attributes should merge with the nodes override attributes" do
+      @node.default_attrs = {"foo" => {"bar" => ["1", "2"]}}
+      @node.override_attrs = {"foo" => {"bar" => ["5", "0"]}}
+      expansion = mock("Expansion", :default_attrs => { "foo" => {"bar" => ["3", "2"]}}, :override_attrs => { "foo" => {"bar" => ["5"] } })
+      @node.apply_expansion_attributes(expansion)
+      @node["foo"]["bar"].should == ["5", "0", "5"]
+      @node["foo"].to_hash["bar"].should == ["5", "0", "5"]
+    end
+  end
+
 end