From c5bd7fe6d0f74c82dd873ace1c75ce49006dfa3e Mon Sep 17 00:00:00 2001 From: robin Date: Wed, 17 Dec 2025 16:27:34 +0900 Subject: [PATCH] Optimize dirty tracking for serialized properties by skipping change marking when values remain unchanged. Add comprehensive test coverage for edge cases. --- lib/coaster/serialized_properties.rb | 42 ++++++++--- test/test_serialized_property.rb | 108 +++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 10 deletions(-) diff --git a/lib/coaster/serialized_properties.rb b/lib/coaster/serialized_properties.rb index 4f2671e..4a0dcc4 100644 --- a/lib/coaster/serialized_properties.rb +++ b/lib/coaster/serialized_properties.rb @@ -27,15 +27,26 @@ def _write_sprop_value(key, value, apply_setter: true) setting = self.class.serialized_property_setting(key) return unless setting col = setting[:column] - send("#{col}_will_change!") hsh = send(col) - if value.nil? + old_value = hsh[key.to_s] + + new_value = if value.nil? + nil + else + apply_setter && setting[:setter] ? setting[:setter].call(value) : value + end + + # Skip dirty marking if value hasn't changed + return old_value if old_value == new_value + + send("#{col}_will_change!") + + if new_value.nil? hsh.delete(key.to_s) else - value = setting[:setter].call(value) if apply_setter && setting[:setter] - hsh[key.to_s] = value + hsh[key.to_s] = new_value end - value + new_value end def write_attribute(attr_name, value) @@ -312,15 +323,26 @@ def _define_serialized_property(serialize_column, key, getter: nil, setter: nil, if is_active_record define_method "#{key}_without_callback=".to_sym do |val| col = serialize_column - send("#{col}_will_change!") hsh = send(col) - if val.nil? + old_value = hsh[key.to_s] + + new_value = if val.nil? + nil + else + setter ? setter.call(val) : val + end + + # Skip dirty marking if value hasn't changed + return old_value if old_value == new_value + + send("#{col}_will_change!") + + if new_value.nil? hsh.delete(key.to_s) else - val = setter.call(val) if setter - hsh[key.to_s] = val + hsh[key.to_s] = new_value end - val + new_value end else define_method "#{key}_without_callback=".to_sym do |val| diff --git a/test/test_serialized_property.rb b/test/test_serialized_property.rb index af5256e..46f60de 100644 --- a/test/test_serialized_property.rb +++ b/test/test_serialized_property.rb @@ -372,5 +372,113 @@ def test_nil_value_with_getter_setter assert_nil student.encrypted_value assert_nil student.data['encrypted_value'] end + + # Tests for dirty tracking optimization (skip dirty marking when value unchanged) + def test_same_value_setter_does_not_mark_dirty + user = User.create(name: 'test_same_value') + user.simple = 'initial_value' + user.save! + + # Reload to clear dirty state + user.reload + assert_equal false, user.changed? + + # Set same value - should NOT mark dirty + user.simple = 'initial_value' + assert_equal false, user.simple_changed?, 'Setting same value should not mark property as changed' + assert_equal false, user.changed?, 'Setting same value should not mark record as changed' + end + + def test_different_value_setter_marks_dirty + user = User.create(name: 'test_different_value') + user.simple = 'initial_value' + user.save! + user.reload + + # Set different value - should mark dirty + user.simple = 'new_value' + assert_equal true, user.simple_changed?, 'Setting different value should mark property as changed' + assert_equal true, user.changed?, 'Setting different value should mark record as changed' + assert_equal ['initial_value', 'new_value'], user.simple_change + end + + def test_nil_to_value_marks_dirty + user = User.create(name: 'test_nil_to_value') + user.save! + user.reload + + assert_nil user.simple + user.simple = 'some_value' + assert_equal true, user.simple_changed? + assert_equal [nil, 'some_value'], user.simple_change + end + + def test_value_to_nil_marks_dirty + user = User.create(name: 'test_value_to_nil') + user.simple = 'some_value' + user.save! + user.reload + + user.simple = nil + assert_equal true, user.simple_changed? + assert_equal ['some_value', nil], user.simple_change + end + + def test_nil_to_nil_does_not_mark_dirty + user = User.create(name: 'test_nil_to_nil') + user.save! + user.reload + + assert_nil user.simple + user.simple = nil + assert_equal false, user.simple_changed?, 'Setting nil to nil should not mark as changed' + assert_equal false, user.changed? + end + + def test_same_value_via_write_attribute_does_not_mark_dirty + user = User.create(name: 'test_write_attr_same') + user.write_attribute(:simple, 'initial') + user.save! + user.reload + + user.write_attribute(:simple, 'initial') + assert_equal false, user.simple_changed? + assert_equal false, user.changed? + end + + def test_same_value_via_bracket_accessor_does_not_mark_dirty + user = User.create(name: 'test_bracket_same') + user[:simple] = 'initial' + user.save! + user.reload + + user[:simple] = 'initial' + assert_equal false, user.simple_changed? + assert_equal false, user.changed? + end + + def test_same_value_with_setter_proc_does_not_mark_dirty + student = Student.create(name: 'test_setter_same') + student.uppercase_name = 'hello' # Stored as 'HELLO' + student.save! + student.reload + + # Setting same raw input - setter transforms to same value + student.uppercase_name = 'hello' + assert_equal false, student.uppercase_name_changed?, 'Same value after setter transform should not mark dirty' + assert_equal false, student.changed? + end + + def test_different_case_with_setter_proc_marks_dirty + student = Student.create(name: 'test_setter_diff') + student.uppercase_name = 'hello' # Stored as 'HELLO' + student.save! + student.reload + + # Setting different value + student.uppercase_name = 'world' # Stored as 'WORLD' + assert_equal true, student.uppercase_name_changed? + assert_equal ['HELLO', 'WORLD'], student.uppercase_name_change + end end end \ No newline at end of file