Dup keys in OrderedHash to prevent them from being modified [#1676 state:resolved]

Signed-off-by: Frederick Cheung <frederick.cheung@gmail.com>

activesupport/lib/active_support/ordered_hash.rb

@@ -10,10 +10,14 @@ module ActiveSupport
         @keys = []
       end
 
+      def initialize_copy(other)
+        super
+        # make a deep copy of keys
+        @keys = other.keys
+      end
+
       def []=(key, value)
-        if !has_key?(key)
-          @keys << key
-        end
+        @keys << key if !has_key?(key)
         super
       end
 
@@ -24,6 +28,12 @@ module ActiveSupport
         end
         super
       end
+      
+      def delete_if
+        super
+        sync_keys!
+        self
+      end
 
       def reject!
         super
@@ -36,7 +46,7 @@ module ActiveSupport
       end
 
       def keys
-        @keys
+        @keys.dup
       end
 
       def values
@@ -56,7 +66,7 @@ module ActiveSupport
       end
 
       def each
-        keys.each {|key| yield [key, self[key]]}
+        @keys.each {|key| yield [key, self[key]]}
       end
 
       alias_method :each_pair, :each
@@ -73,13 +83,16 @@ module ActiveSupport
         [k, v]
       end
 
-      def merge(other_hash)
-        result = dup
-        other_hash.each {|k,v| result[k]=v}
-        result
+      def merge!(other_hash)
+        other_hash.each {|k,v| self[k] = v }
+        self
       end
 
-      private
+      def merge(other_hash)
+        dup.merge!(other_hash)
+      end
+
+    private
 
       def sync_keys!
         @keys.delete_if {|k| !has_key?(k)}

activesupport/test/ordered_hash_test.rb

@@ -98,7 +98,8 @@ class OrderedHashTest < Test::Unit::TestCase
   end
 
   def test_delete_if
-    (copy = @ordered_hash.dup).delete('pink')
+    copy = @ordered_hash.dup
+    copy.delete('pink')
     assert_equal copy, @ordered_hash.delete_if { |k, _| k == 'pink' }
     assert !@ordered_hash.keys.include?('pink')
   end
@@ -115,6 +116,7 @@ class OrderedHashTest < Test::Unit::TestCase
     new_ordered_hash = @ordered_hash.reject { |k, _| k == 'pink' }
     assert_equal copy, @ordered_hash
     assert !new_ordered_hash.keys.include?('pink')
+    assert @ordered_hash.keys.include?('pink')
   end
 
   def test_clear
@@ -140,4 +142,10 @@ class OrderedHashTest < Test::Unit::TestCase
     assert_equal [@keys.first, @values.first], pair
     assert !@ordered_hash.keys.include?(pair.first)
   end
+  
+  def test_keys
+    original = @ordered_hash.keys.dup
+    @ordered_hash.keys.pop
+    assert_equal original, @ordered_hash.keys
+  end
 end