Merge branch 'master' of github.com:rails/rails

10 files changed, 85 insertions, 23 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 02660ae4c2..b09d9e336b 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,16 @@
+*   Fixed an issue where custom accessor methods (such as those generated by
+    `enum`) with the same name as a global method are incorrectly overridden
+    when subclassing.
+
+    Fixes #16288.
+
+    *Godfrey Chan*
+
+*   `*_was` and `changes` now work correctly for in-place attribute changes as
+    well.
+
+    *Sean Griffin*
+
 *   Fix regression on after_commit that didnt fire when having nested transactions.
 
     Fixes #16425
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index 79c3d2b0f5..1413efaf7f 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -103,7 +103,7 @@ module ActiveRecord
           if has_cached_counter?(reflection)
             counter = cached_counter_attribute_name(reflection)
             owner[counter] += difference
-            owner.changed_attributes.delete(counter) # eww
+            owner.send(:clear_attribute_changes, counter) # eww
           end
         end
 
diff --git a/activerecord/lib/active_record/attribute.rb b/activerecord/lib/active_record/attribute.rb
index 15cbbcff68..8cc1904575 100644
--- a/activerecord/lib/active_record/attribute.rb
+++ b/activerecord/lib/active_record/attribute.rb
@@ -30,10 +30,14 @@ module ActiveRecord
 
     def value
       # `defined?` is cheaper than `||=` when we get back falsy values
-      @value = type_cast(value_before_type_cast) unless defined?(@value)
+      @value = original_value unless defined?(@value)
       @value
     end
 
+    def original_value
+      type_cast(value_before_type_cast)
+    end
+
     def value_for_database
       type.type_cast_for_database(value)
     end
@@ -54,7 +58,7 @@ module ActiveRecord
       self.class.from_database(name, value, type)
     end
 
-    def type_cast
+    def type_cast(*)
       raise NotImplementedError
     end
 
diff --git a/activerecord/lib/active_record/attribute_methods.rb b/activerecord/lib/active_record/attribute_methods.rb
index 09e2faee86..1ff28ceccc 100644
--- a/activerecord/lib/active_record/attribute_methods.rb
+++ b/activerecord/lib/active_record/attribute_methods.rb
@@ -57,6 +57,8 @@ module ActiveRecord
       end
     end
 
+    class GeneratedAttributeMethods < Module; end # :nodoc:
+
     module ClassMethods
       def inherited(child_class) #:nodoc:
         child_class.initialize_generated_modules
@@ -64,7 +66,7 @@ module ActiveRecord
       end
 
       def initialize_generated_modules # :nodoc:
-        @generated_attribute_methods = Module.new { extend Mutex_m }
+        @generated_attribute_methods = GeneratedAttributeMethods.new { extend Mutex_m }
         @attribute_methods_generated = false
         include @generated_attribute_methods
       end
@@ -113,10 +115,11 @@ module ActiveRecord
         if superclass == Base
           super
         else
-          # If B < A and A defines its own attribute method, then we don't want to overwrite that.
-          defined = method_defined_within?(method_name, superclass, superclass.generated_attribute_methods)
-          base_defined = Base.method_defined?(method_name) || Base.private_method_defined?(method_name)
-          defined && !base_defined || super
+          # If ThisClass < ... < SomeSuperClass < ... < Base and SomeSuperClass
+          # defines its own attribute method, then we don't want to overwrite that.
+          defined = method_defined_within?(method_name, superclass, Base) &&
+            ! superclass.instance_method(method_name).owner.is_a?(GeneratedAttributeMethods)
+          defined || super
         end
       end
 
diff --git a/activerecord/lib/active_record/attribute_methods/dirty.rb b/activerecord/lib/active_record/attribute_methods/dirty.rb
index b58295a106..d3f4e51c33 100644
--- a/activerecord/lib/active_record/attribute_methods/dirty.rb
+++ b/activerecord/lib/active_record/attribute_methods/dirty.rb
@@ -51,14 +51,6 @@ module ActiveRecord
         super | changed_in_place
       end
 
-      def attribute_changed?(attr_name, options = {})
-        result = super
-        # We can't change "from" something in place. Only setters can define
-        # "from" and "to"
-        result ||= changed_in_place?(attr_name) unless options.key?(:from)
-        result
-      end
-
       def changes_applied
         super
         store_original_raw_attributes
@@ -69,12 +61,16 @@ module ActiveRecord
         original_raw_attributes.clear
       end
 
+      def changed_attributes
+        super.reverse_merge(attributes_changed_in_place).freeze
+      end
+
       private
 
       def calculate_changes_from_defaults
         @changed_attributes = nil
         self.class.column_defaults.each do |attr, orig_value|
-          changed_attributes[attr] = orig_value if _field_changed?(attr, orig_value)
+          set_attribute_was(attr, orig_value) if _field_changed?(attr, orig_value)
         end
       end
 
@@ -100,9 +96,9 @@ module ActiveRecord
 
       def save_changed_attribute(attr, old_value)
         if attribute_changed?(attr)
-          changed_attributes.delete(attr) unless _field_changed?(attr, old_value)
+          clear_attribute_changes(attr) unless _field_changed?(attr, old_value)
         else
-          changed_attributes[attr] = old_value if _field_changed?(attr, old_value)
+          set_attribute_was(attr, old_value) if _field_changed?(attr, old_value)
         end
       end
 
@@ -132,6 +128,13 @@ module ActiveRecord
         @attributes[attr].changed_from?(old_value)
       end
 
+      def attributes_changed_in_place
+        changed_in_place.each_with_object({}) do |attr_name, h|
+          orig = @attributes[attr_name].original_value
+          h[attr_name] = orig
+        end
+      end
+
       def changed_in_place
         self.class.attribute_names.select do |attr_name|
           changed_in_place?(attr_name)
diff --git a/activerecord/lib/active_record/enum.rb b/activerecord/lib/active_record/enum.rb
index f0ee433d0b..5958373e88 100644
--- a/activerecord/lib/active_record/enum.rb
+++ b/activerecord/lib/active_record/enum.rb
@@ -145,11 +145,11 @@ module ActiveRecord
                   value = read_attribute(attr_name)
                   if attribute_changed?(attr_name)
                     if mapping[old] == value
-                      changed_attributes.delete(attr_name)
+                      clear_attribute_changes([attr_name])
                     end
                   else
                     if old != value
-                      changed_attributes[attr_name] = mapping.key old
+                      set_attribute_was(attr_name, mapping.key(old))
                     end
                   end
                 else
diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb
index 51b1931ed5..755ff2b2f1 100644
--- a/activerecord/lib/active_record/persistence.rb
+++ b/activerecord/lib/active_record/persistence.rb
@@ -466,7 +466,7 @@ module ActiveRecord
 
         changes[self.class.locking_column] = increment_lock if locking_enabled?
 
-        changed_attributes.except!(*changes.keys)
+        clear_attribute_changes(changes.keys)
         primary_key = self.class.primary_key
         self.class.unscoped.where(primary_key => self[primary_key]).update_all(changes) == 1
       else
diff --git a/activerecord/lib/active_record/timestamp.rb b/activerecord/lib/active_record/timestamp.rb
index 417cd61f0c..936a18d99a 100644
--- a/activerecord/lib/active_record/timestamp.rb
+++ b/activerecord/lib/active_record/timestamp.rb
@@ -114,7 +114,7 @@ module ActiveRecord
     def clear_timestamp_attributes
       all_timestamp_attributes_in_model.each do |attribute_name|
         self[attribute_name] = nil
-        changed_attributes.delete(attribute_name)
+        clear_attribute_changes([attribute_name])
       end
     end
   end
diff --git a/activerecord/test/cases/attribute_methods_test.rb b/activerecord/test/cases/attribute_methods_test.rb
index 535928783f..b4917e727a 100644
--- a/activerecord/test/cases/attribute_methods_test.rb
+++ b/activerecord/test/cases/attribute_methods_test.rb
@@ -810,6 +810,24 @@ class AttributeMethodsTest < ActiveRecord::TestCase
     assert_equal "lol", topic.author_name
   end
 
+  def test_inherited_custom_accessors_with_reserved_names
+    klass = Class.new(ActiveRecord::Base) do
+      self.table_name = 'computers'
+      self.abstract_class = true
+      def system; "omg"; end
+      def system=(val); self.developer = val; end
+    end
+
+    subklass = Class.new(klass)
+    [klass, subklass].each(&:define_attribute_methods)
+
+    computer = subklass.find(1)
+    assert_equal "omg", computer.system
+
+    computer.developer = 99
+    assert_equal 99, computer.developer
+  end
+
   def test_on_the_fly_super_invokable_generated_attribute_methods_via_method_missing
     klass = new_topic_like_ar_class do
       def title
diff --git a/activerecord/test/cases/dirty_test.rb b/activerecord/test/cases/dirty_test.rb
index 69a7f25213..0c77eedb52 100644
--- a/activerecord/test/cases/dirty_test.rb
+++ b/activerecord/test/cases/dirty_test.rb
@@ -661,6 +661,27 @@ class DirtyTest < ActiveRecord::TestCase
     assert_not model.foo_changed?
   end
 
+  test "in place mutation detection" do
+    pirate = Pirate.create!(catchphrase: "arrrr")
+    pirate.catchphrase << " matey!"
+
+    assert pirate.catchphrase_changed?
+    expected_changes = {
+      "catchphrase" => ["arrrr", "arrrr matey!"]
+    }
+    assert_equal(expected_changes, pirate.changes)
+    assert_equal("arrrr", pirate.catchphrase_was)
+    assert pirate.catchphrase_changed?(from: "arrrr")
+    assert_not pirate.catchphrase_changed?(from: "anything else")
+    assert pirate.changed_attributes.include?(:catchphrase)
+
+    pirate.save!
+    pirate.reload
+
+    assert_equal "arrrr matey!", pirate.catchphrase
+    assert_not pirate.changed?
+  end
+
   private
     def with_partial_writes(klass, on = true)
       old = klass.partial_writes?