Skip to content
Browse files

Merge pull request #15674 from sgrif/sg-mutable-attributes

Detect in-place changes on mutable AR attributes
  • Loading branch information...
2 parents 1dcb8e9 + 4bf8ffc commit 49fee3d271e52a44a7bc7fcbbcb00792b613b7df @matthewd matthewd committed
View
6 activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+* `ActiveRecord::Dirty` now detects in-place changes to mutable values.
+ Serialized attributes on ActiveRecord models will no longer save when
+ unchanged. Fixes #8328.
+
+ Sean Griffin
+
* Fixed automatic maintaining test schema to properly handle sql structure
schema format.
View
83 activerecord/lib/active_record/attribute_methods/dirty.rb
@@ -38,12 +38,39 @@ def reload(*)
end
end
- def initialize_dup(other) # :nodoc:
- super
- init_changed_attributes
- end
+ def initialize_dup(other) # :nodoc:
+ super
+ init_changed_attributes
+ end
+
+ def changed?
+ super || changed_in_place.any?
+ end
+
+ def changed
+ 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
+ end
+
+ def reset_changes
+ super
+ original_raw_attributes.clear
+ end
+
+ private
- private
def initialize_internals_callback
super
init_changed_attributes
@@ -65,11 +92,20 @@ def write_attribute(attr, value)
old_value = old_attribute_value(attr)
- result = super(attr, value)
+ result = super
+ store_original_raw_attribute(attr)
save_changed_attribute(attr, old_value)
result
end
+ def raw_write_attribute(attr, value)
+ attr = attr.to_s
+
+ result = super
+ original_raw_attributes[attr] = value
+ result
+ end
+
def save_changed_attribute(attr, old_value)
if attribute_changed?(attr)
changed_attributes.delete(attr) unless _field_changed?(attr, old_value)
@@ -105,6 +141,41 @@ def _field_changed?(attr, old_value)
raw_value = read_attribute_before_type_cast(attr)
column_for_attribute(attr).changed?(old_value, new_value, raw_value)
end
+
+ def changed_in_place
+ self.class.attribute_names.select do |attr_name|
+ changed_in_place?(attr_name)
+ end
+ end
+
+ def changed_in_place?(attr_name)
+ type = type_for_attribute(attr_name)
+ old_value = original_raw_attribute(attr_name)
+ value = read_attribute(attr_name)
+ type.changed_in_place?(old_value, value)
+ end
+
+ def original_raw_attribute(attr_name)
+ original_raw_attributes.fetch(attr_name) do
+ read_attribute_before_type_cast(attr_name)
+ end
+ end
+
+ def original_raw_attributes
+ @original_raw_attributes ||= {}
+ end
+
+ def store_original_raw_attribute(attr_name)
+ type = type_for_attribute(attr_name)
+ value = type.type_cast_for_database(read_attribute(attr_name))
+ original_raw_attributes[attr_name] = value
+ end
+
+ def store_original_raw_attributes
+ attribute_names.each do |attr|
+ store_original_raw_attribute(attr)
+ end
+ end
end
end
end
View
17 activerecord/lib/active_record/attribute_methods/serialization.rb
@@ -50,8 +50,6 @@ module ClassMethods
# serialize :preferences, Hash
# end
def serialize(attr_name, class_name_or_coder = Object)
- include Behavior
-
coder = if [:load, :dump].all? { |x| class_name_or_coder.respond_to?(x) }
class_name_or_coder
else
@@ -67,21 +65,6 @@ def serialize(attr_name, class_name_or_coder = Object)
self.serialized_attributes = serialized_attributes.merge(attr_name.to_s => coder)
end
end
-
-
- # This is only added to the model when serialize is called, which
- # ensures we do not make things slower when serialization is not used.
- module Behavior # :nodoc:
- extend ActiveSupport::Concern
-
- def should_record_timestamps?
- super || (self.record_timestamps && (attributes.keys & self.class.serialized_attributes.keys).present?)
- end
-
- def keys_for_partial_write
- super | (attributes.keys & self.class.serialized_attributes.keys)
- end
- end
end
end
end
View
6 activerecord/lib/active_record/connection_adapters/postgresql/oid/hstore.rb
@@ -3,14 +3,12 @@ module ConnectionAdapters
module PostgreSQL
module OID # :nodoc:
class Hstore < Type::Value
+ include Type::Mutable
+
def type
:hstore
end
- def type_cast_from_user(value)
- type_cast_from_database(type_cast_for_database(value))
- end
-
def type_cast_from_database(value)
ConnectionAdapters::PostgreSQLColumn.string_to_hstore(value)
end
View
6 activerecord/lib/active_record/connection_adapters/postgresql/oid/json.rb
@@ -3,14 +3,12 @@ module ConnectionAdapters
module PostgreSQL
module OID # :nodoc:
class Json < Type::Value
+ include Type::Mutable
+
def type
:json
end
- def type_cast_from_user(value)
- type_cast_from_database(type_cast_for_database(value))
- end
-
def type_cast_from_database(value)
ConnectionAdapters::PostgreSQLColumn.string_to_json(value)
end
View
1 activerecord/lib/active_record/type.rb
@@ -1,3 +1,4 @@
+require 'active_record/type/mutable'
require 'active_record/type/numeric'
require 'active_record/type/time_value'
require 'active_record/type/value'
View
16 activerecord/lib/active_record/type/mutable.rb
@@ -0,0 +1,16 @@
+module ActiveRecord
+ module Type
+ module Mutable
+ def type_cast_from_user(value)
+ type_cast_from_database(type_cast_for_database(value))
+ end
+
+ # +raw_old_value+ will be the `_before_type_cast` version of the
+ # value (likely a string). +new_value+ will be the current, type
+ # cast value.
+ def changed_in_place?(raw_old_value, new_value) # :nodoc:
+ raw_old_value != type_cast_for_database(new_value)
+ end
+ end
+ end
+end
View
6 activerecord/lib/active_record/type/serialized.rb
@@ -1,6 +1,8 @@
module ActiveRecord
module Type
class Serialized < SimpleDelegator # :nodoc:
+ include Mutable
+
attr_reader :subtype, :coder
def initialize(subtype, coder)
@@ -17,10 +19,6 @@ def type_cast_from_database(value)
end
end
- def type_cast_from_user(value)
- type_cast_from_database(type_cast_for_database(value))
- end
-
def type_cast_for_database(value)
return if value.nil?
unless is_default_value?(value)
View
4 activerecord/lib/active_record/type/value.rb
@@ -60,6 +60,10 @@ def changed?(old_value, new_value, _new_value_before_type_cast) # :nodoc:
old_value != new_value
end
+ def changed_in_place?(*) # :nodoc:
+ false
+ end
+
private
# Takes an input from the database, or from attribute setters,
# and casts it to a type appropriate for this object. This method
View
9 activerecord/test/cases/adapters/postgresql/hstore_test.rb
@@ -163,6 +163,15 @@ def test_yaml_round_trip_with_store_accessors
assert_equal "GMT", y.timezone
end
+ def test_changes_in_place
+ hstore = Hstore.create!(settings: { 'one' => 'two' })
+ hstore.settings['three'] = 'four'
+ hstore.save!
+ hstore.reload
+
+ assert_equal 'four', hstore.settings['three']
+ end
+
def test_gen1
assert_equal(%q(" "=>""), @column.class.hstore_to_string({' '=>''}))
end
View
20 activerecord/test/cases/adapters/postgresql/json_test.rb
@@ -165,4 +165,24 @@ def test_update_all
JsonDataType.update_all payload: { }
assert_equal({ }, json.reload.payload)
end
+
+ def test_changes_in_place
+ json = JsonDataType.new
+ assert_not json.changed?
+
+ json.payload = { 'one' => 'two' }
+ assert json.changed?
+ assert json.payload_changed?
+
+ json.save!
+ assert_not json.changed?
+
+ json.payload['three'] = 'four'
+ assert json.payload_changed?
+
+ json.save!
+ json.reload
+
+ assert json.payload['three'] = 'four'
+ end
end
View
11 activerecord/test/cases/dirty_test.rb
@@ -445,11 +445,20 @@ def test_reverted_changes_are_not_dirty_going_from_nil_to_value_and_back
def test_save_should_store_serialized_attributes_even_with_partial_writes
with_partial_writes(Topic) do
topic = Topic.create!(:content => {:a => "a"})
+
+ assert_not topic.changed?
+
topic.content[:b] = "b"
- #assert topic.changed? # Known bug, will fail
+
+ assert topic.changed?
+
topic.save!
+
+ assert_not topic.changed?
assert_equal "b", topic.content[:b]
+
topic.reload
+
assert_equal "b", topic.content[:b]
end
end

0 comments on commit 49fee3d

Please sign in to comment.
Something went wrong with that request. Please try again.