Merge pull request #15788 from sgrif/sg-mutable-strings

Detect in-place modifications on Strings

activerecord/CHANGELOG.md

@@ -1,3 +1,25 @@
+*   Detect in-place modifications on String attributes.
+
+    Before this change user have to mark the attribute as changed to it be persisted
+    in the database. Now it is not required anymore.
+
+    Before:
+
+        user = User.first
+        user.name << ' Griffin'
+        user.name_will_change!
+        user.save
+        user.reload.name # => "Sean Griffin"
+
+    After:
+
+        user = User.first
+        user.name << ' Griffin'
+        user.save
+        user.reload.name # => "Sean Griffin"
+
+    *Sean Griffin*
+
 *   Add `ActiveRecord::Base#validate!` that raises `RecordInvalid` if the record
     is invalid.
 

activerecord/lib/active_record/type/string.rb

@@ -9,13 +9,28 @@ def text?
         true
       end
 
+      def changed_in_place?(raw_old_value, new_value)
+        if new_value.is_a?(::String)
+          raw_old_value != new_value
+        end
+      end
+
+      def type_cast_for_database(value)
+        if value.is_a?(::String)
+          ::String.new(value)
+        else
+          super
+        end
+      end
+
       private
 
       def cast_value(value)
         case value
         when true then "1"
         when false then "0"
-        else value.to_s
+        # String.new is slightly faster than dup
+        else ::String.new(value.to_s)
         end
       end
     end

activerecord/test/cases/dirty_test.rb

@@ -309,16 +309,14 @@ def test_object_should_be_changed_if_any_attribute_is_changed
   def test_attribute_will_change!
     pirate = Pirate.create!(:catchphrase => 'arr')
 
-    pirate.catchphrase << ' matey'
     assert !pirate.catchphrase_changed?
-
     assert pirate.catchphrase_will_change!
     assert pirate.catchphrase_changed?
-    assert_equal ['arr matey', 'arr matey'], pirate.catchphrase_change
+    assert_equal ['arr', 'arr'], pirate.catchphrase_change
 
-    pirate.catchphrase << '!'
+    pirate.catchphrase << ' matey!'
     assert pirate.catchphrase_changed?
-    assert_equal ['arr matey', 'arr matey!'], pirate.catchphrase_change
+    assert_equal ['arr', 'arr matey!'], pirate.catchphrase_change
   end
 
   def test_association_assignment_changes_foreign_key

activerecord/test/cases/type/string_test.rb

@@ -0,0 +1,36 @@
+require 'cases/helper'
+
+module ActiveRecord
+  class StringTypeTest < ActiveRecord::TestCase
+    test "type casting" do
+      type = Type::String.new
+      assert_equal "1", type.type_cast_from_user(true)
+      assert_equal "0", type.type_cast_from_user(false)
+      assert_equal "123", type.type_cast_from_user(123)
+    end
+
+    test "values are duped coming out" do
+      s = "foo"
+      type = Type::String.new
+      assert_not_same s, type.type_cast_from_user(s)
+      assert_not_same s, type.type_cast_from_database(s)
+    end
+
+    test "string mutations are detected" do
+      klass = Class.new(Base)
+      klass.table_name = 'authors'
+
+      author = klass.create!(name: 'Sean')
+      assert_not author.changed?
+
+      author.name << ' Griffin'
+      assert author.name_changed?
+
+      author.save!
+      author.reload
+
+      assert_equal 'Sean Griffin', author.name
+      assert_not author.changed?
+    end
+  end
+end

activerecord/test/cases/types_test.rb

@@ -35,13 +35,6 @@ def test_type_cast_boolean
         assert_equal false, type.type_cast_from_user('SOMETHING RANDOM')
       end
 
-      def test_type_cast_string
-        type = Type::String.new
-        assert_equal "1", type.type_cast_from_user(true)
-        assert_equal "0", type.type_cast_from_user(false)
-        assert_equal "123", type.type_cast_from_user(123)
-      end
-
       def test_type_cast_integer
         type = Type::Integer.new
         assert_equal 1, type.type_cast_from_user(1)