1
0
Fork 0
mirror of https://github.com/rails/rails.git synced 2022-11-09 12:12:34 -05:00

Fix dirty tracking after rollback.

Currently the rollback only restores primary key value, `new_record?`,
`destroyed?`, and `frozen?`. Since the `save` clears current dirty
attribute states, retrying save after rollback will causes no change
saved if partial writes is enabled (by default).

This makes `remember_transaction_record_state` remembers original values
then restores dirty attribute states after rollback.

Fixes #15018.
Fixes #30167.
Fixes #33868.
Fixes #33443.
Closes #33444.
Closes #34504.
This commit is contained in:
Ryuta Kamizono 2018-05-19 11:46:03 +09:00
parent 20b94af9eb
commit 63ff495bdf
9 changed files with 92 additions and 25 deletions

View file

@ -1,3 +1,9 @@
* Fix dirty tracking after rollback.
Fixes #15018, #30167, #33868.
*Ryuta Kamizono*
* Fix dirty tracking for `touch` to track saved changes.
Fixes #33429.

View file

@ -46,6 +46,7 @@ module ActiveRecord
# task.read_attribute_before_type_cast('completed_on') # => "2012-10-21"
# task.read_attribute_before_type_cast(:completed_on) # => "2012-10-21"
def read_attribute_before_type_cast(attr_name)
sync_with_transaction_state
@attributes[attr_name.to_s].value_before_type_cast
end
@ -60,6 +61,7 @@ module ActiveRecord
# task.attributes_before_type_cast
# # => {"id"=>nil, "title"=>nil, "is_done"=>true, "completed_on"=>"2012-10-21", "created_at"=>nil, "updated_at"=>nil}
def attributes_before_type_cast
sync_with_transaction_state
@attributes.values_before_type_cast
end
@ -71,6 +73,7 @@ module ActiveRecord
end
def attribute_came_from_user?(attribute_name)
sync_with_transaction_state
@attributes[attribute_name].came_from_user?
end
end

View file

@ -156,6 +156,16 @@ module ActiveRecord
end
private
def mutations_from_database
sync_with_transaction_state
super
end
def mutations_before_last_save
sync_with_transaction_state
super
end
def write_attribute_without_type_cast(attr_name, value)
name = attr_name.to_s
if self.class.attribute_alias?(name)

View file

@ -16,39 +16,33 @@ module ActiveRecord
# Returns the primary key column's value.
def id
sync_with_transaction_state
primary_key = self.class.primary_key
_read_attribute(primary_key) if primary_key
end
# Sets the primary key column's value.
def id=(value)
sync_with_transaction_state
primary_key = self.class.primary_key
_write_attribute(primary_key, value) if primary_key
end
# Queries the primary key column's value.
def id?
sync_with_transaction_state
query_attribute(self.class.primary_key)
end
# Returns the primary key column's value before type cast.
def id_before_type_cast
sync_with_transaction_state
read_attribute_before_type_cast(self.class.primary_key)
end
# Returns the primary key column's previous value.
def id_was
sync_with_transaction_state
attribute_was(self.class.primary_key)
end
# Returns the primary key column's value from the database.
def id_in_database
sync_with_transaction_state
attribute_in_database(self.class.primary_key)
end

View file

@ -9,14 +9,11 @@ module ActiveRecord
private
def define_method_attribute(name)
sync_with_transaction_state = "sync_with_transaction_state" if name == primary_key
ActiveModel::AttributeMethods::AttrNames.define_attribute_accessor_method(
generated_attribute_methods, name
) do |temp_method_name, attr_name_expr|
generated_attribute_methods.module_eval <<-RUBY, __FILE__, __LINE__ + 1
def #{temp_method_name}
#{sync_with_transaction_state}
name = #{attr_name_expr}
_read_attribute(name) { |n| missing_attribute(n, caller) }
end
@ -36,13 +33,13 @@ module ActiveRecord
primary_key = self.class.primary_key
name = primary_key if name == "id" && primary_key
sync_with_transaction_state if name == primary_key
_read_attribute(name, &block)
end
# This method exists to avoid the expensive primary_key check internally, without
# breaking compatibility with the read_attribute API
def _read_attribute(attr_name, &block) # :nodoc
sync_with_transaction_state
@attributes.fetch_value(attr_name.to_s, &block)
end

View file

@ -13,15 +13,12 @@ module ActiveRecord
private
def define_method_attribute=(name)
sync_with_transaction_state = "sync_with_transaction_state" if name == primary_key
ActiveModel::AttributeMethods::AttrNames.define_attribute_accessor_method(
generated_attribute_methods, name, writer: true,
) do |temp_method_name, attr_name_expr|
generated_attribute_methods.module_eval <<-RUBY, __FILE__, __LINE__ + 1
def #{temp_method_name}(value)
name = #{attr_name_expr}
#{sync_with_transaction_state}
_write_attribute(name, value)
end
RUBY
@ -40,21 +37,21 @@ module ActiveRecord
primary_key = self.class.primary_key
name = primary_key if name == "id" && primary_key
sync_with_transaction_state if name == primary_key
_write_attribute(name, value)
end
# This method exists to avoid the expensive primary_key check internally, without
# breaking compatibility with the write_attribute API
def _write_attribute(attr_name, value) # :nodoc:
sync_with_transaction_state
@attributes.write_from_user(attr_name.to_s, value)
value
end
private
def write_attribute_without_type_cast(attr_name, value)
name = attr_name.to_s
@attributes.write_cast_value(name, value)
sync_with_transaction_state
@attributes.write_cast_value(attr_name.to_s, value)
value
end

View file

@ -583,12 +583,6 @@ module ActiveRecord
def initialize_internals_callback
end
def thaw
if @attributes.frozen?
@attributes = @attributes.dup
end
end
def custom_inspect_method_defined?
self.class.instance_method(:inspect).owner != ActiveRecord::Base.instance_method(:inspect).owner
end

View file

@ -390,6 +390,7 @@ module ActiveRecord
id: id,
new_record: @new_record,
destroyed: @destroyed,
attributes: @attributes,
frozen?: frozen?,
)
@_start_transaction_state[:level] = (@_start_transaction_state[:level] || 0) + 1
@ -422,12 +423,18 @@ module ActiveRecord
transaction_level = (@_start_transaction_state[:level] || 0) - 1
if transaction_level < 1 || force_restore_state
restore_state = @_start_transaction_state
thaw
@new_record = restore_state[:new_record]
@destroyed = restore_state[:destroyed]
@attributes = restore_state[:attributes].map do |attr|
value = @attributes.fetch_value(attr.name)
attr = attr.with_value_from_user(value) if attr.value != value
attr
end
@mutations_from_database = nil
@mutations_before_last_save = nil
pk = self.class.primary_key
if pk && _read_attribute(pk) != restore_state[:id]
_write_attribute(pk, restore_state[:id])
if pk && @attributes.fetch_value(pk) != restore_state[:id]
@attributes.write_from_user(pk, restore_state[:id])
end
freeze if restore_state[:frozen?]
end

View file

@ -18,6 +18,65 @@ class TransactionTest < ActiveRecord::TestCase
@first, @second = Topic.find(1, 2).sort_by(&:id)
end
def test_rollback_dirty_changes
topic = topics(:fifth)
ActiveRecord::Base.transaction do
topic.update(title: "Ruby on Rails")
raise ActiveRecord::Rollback
end
title_change = ["The Fifth Topic of the day", "Ruby on Rails"]
assert_equal title_change, topic.changes["title"]
end
def test_rollback_dirty_changes_multiple_saves
topic = topics(:fifth)
ActiveRecord::Base.transaction do
topic.update(title: "Ruby on Rails")
topic.update(title: "Another Title")
raise ActiveRecord::Rollback
end
title_change = ["The Fifth Topic of the day", "Another Title"]
assert_equal title_change, topic.changes["title"]
end
def test_rollback_dirty_changes_then_retry_save
topic = topics(:fifth)
ActiveRecord::Base.transaction do
topic.update(title: "Ruby on Rails")
raise ActiveRecord::Rollback
end
title_change = ["The Fifth Topic of the day", "Ruby on Rails"]
assert_equal title_change, topic.changes["title"]
assert topic.save
assert_equal title_change, topic.saved_changes["title"]
assert_equal topic.title, topic.reload.title
end
def test_rollback_dirty_changes_then_retry_save_on_new_record
topic = Topic.new(title: "Ruby on Rails")
ActiveRecord::Base.transaction do
topic.save
raise ActiveRecord::Rollback
end
title_change = [nil, "Ruby on Rails"]
assert_equal title_change, topic.changes["title"]
assert topic.save
assert_equal title_change, topic.saved_changes["title"]
assert_equal topic.title, topic.reload.title
end
def test_persisted_in_a_model_with_custom_primary_key_after_failed_save
movie = Movie.create
assert_not_predicate movie, :persisted?