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

Merge pull request #25337 from sgrif/sg-changes-in-callbacks

Deprecate the behavior of AR::Dirty inside of after_(create|update|save) callbacks
This commit is contained in:
Sean Griffin 2016-11-01 13:17:58 -04:00 committed by GitHub
commit 29b3b5dd8e
18 changed files with 362 additions and 53 deletions

View file

@ -166,7 +166,7 @@ module ActiveRecord
def initialize_attributes(record, except_from_scope_attributes = nil) #:nodoc:
except_from_scope_attributes ||= {}
skip_assign = [reflection.foreign_key, reflection.type].compact
assigned_keys = record.changed
assigned_keys = record.changed_attribute_names_to_save
assigned_keys += except_from_scope_attributes.keys.map(&:to_s)
attributes = create_scope.except(*(assigned_keys - skip_assign))
record.assign_attributes(attributes)

View file

@ -35,17 +35,17 @@ module ActiveRecord::Associations::Builder # :nodoc:
@_after_create_counter_called = false
elsif (@_after_replace_counter_called ||= false)
@_after_replace_counter_called = false
elsif attribute_changed?(foreign_key) && !new_record?
elsif saved_change_to_attribute?(foreign_key) && !new_record?
if reflection.polymorphic?
model = attribute(reflection.foreign_type).try(:constantize)
model_was = attribute_was(reflection.foreign_type).try(:constantize)
model = attribute_in_database(reflection.foreign_type).try(:constantize)
model_was = attribute_before_last_save(reflection.foreign_type).try(:constantize)
else
model = reflection.klass
model_was = reflection.klass
end
foreign_key_was = attribute_was foreign_key
foreign_key = attribute foreign_key
foreign_key_was = attribute_before_last_save foreign_key
foreign_key = attribute_in_database foreign_key
if foreign_key && model.respond_to?(:increment_counter)
model.increment_counter(cache_column, foreign_key)
@ -70,14 +70,16 @@ module ActiveRecord::Associations::Builder # :nodoc:
klass.attr_readonly cache_column if klass && klass.respond_to?(:attr_readonly)
end
def self.touch_record(o, foreign_key, name, touch, touch_method) # :nodoc:
old_foreign_id = o.changed_attributes[foreign_key]
def self.touch_record(o, changes, foreign_key, name, touch, touch_method) # :nodoc:
old_foreign_id = changes[foreign_key] && changes[foreign_key].first
if old_foreign_id
association = o.association(name)
reflection = association.reflection
if reflection.polymorphic?
klass = o.public_send("#{reflection.foreign_type}_was").constantize
foreign_type = reflection.foreign_type
klass = changes[foreign_type] && changes[foreign_type].first || o.public_send(foreign_type)
klass = klass.constantize
else
klass = association.klass
end
@ -107,13 +109,13 @@ module ActiveRecord::Associations::Builder # :nodoc:
n = reflection.name
touch = reflection.options[:touch]
callback = lambda { |record|
BelongsTo.touch_record(record, foreign_key, n, touch, belongs_to_touch_method)
}
callback = lambda { |changes_method| lambda { |record|
BelongsTo.touch_record(record, record.send(changes_method), foreign_key, n, touch, belongs_to_touch_method)
}}
model.after_save callback, if: :changed?
model.after_touch callback
model.after_destroy callback
model.after_save callback.(:saved_changes), if: :saved_changes?
model.after_touch callback.(:changes_to_save)
model.after_destroy callback.(:changes_to_save)
end
def self.add_destroy_callbacks(model, reflection)

View file

@ -375,7 +375,7 @@ module ActiveRecord
persisted.map! do |record|
if mem_record = memory.delete(record)
((record.attribute_names & mem_record.attribute_names) - mem_record.changes.keys).each do |name|
((record.attribute_names & mem_record.attribute_names) - mem_record.changed_attribute_names_to_save).each do |name|
mem_record[name] = record[name]
end

View file

@ -35,7 +35,7 @@ module ActiveRecord
return target unless target || record
assigning_another_record = target != record
if assigning_another_record || record.changed?
if assigning_another_record || record.has_changes_to_save?
save &&= owner.persisted?
transaction_if(save) do

View file

@ -1,3 +1,4 @@
# frozen_string_literal: true
require "active_support/core_ext/module/attribute_accessors"
require "active_record/attribute_mutation_tracker"
@ -15,6 +16,18 @@ module ActiveRecord
class_attribute :partial_writes, instance_writer: false
self.partial_writes = true
after_create { changes_internally_applied }
after_update { changes_internally_applied }
# Attribute methods for "changed in last call to save?"
attribute_method_affix(prefix: "saved_change_to_", suffix: "?")
attribute_method_prefix("saved_change_to_")
attribute_method_suffix("_before_last_save")
# Attribute methods for "will change if I call save?"
attribute_method_affix(prefix: "will_save_change_to_", suffix: "?")
attribute_method_suffix("_change_to_be_saved", "_in_database")
end
# Attempts to +save+ the record and clears changed attributes if successful.
@ -35,8 +48,8 @@ module ActiveRecord
# <tt>reload</tt> the record and clears changed attributes.
def reload(*)
super.tap do
@mutation_tracker = nil
@previous_mutation_tracker = nil
clear_mutation_trackers
@changed_attributes = HashWithIndifferentAccess.new
end
end
@ -46,19 +59,26 @@ module ActiveRecord
@attributes = self.class._default_attributes.map do |attr|
attr.with_value_from_user(@attributes.fetch_value(attr.name))
end
@mutation_tracker = nil
clear_mutation_trackers
end
def changes_internally_applied # :nodoc:
@mutations_before_last_save = mutation_tracker
forget_attribute_assignments
@mutations_from_database = AttributeMutationTracker.new(@attributes)
end
def changes_applied
@previous_mutation_tracker = mutation_tracker
@changed_attributes = HashWithIndifferentAccess.new
store_original_attributes
clear_mutation_trackers
end
def clear_changes_information
@previous_mutation_tracker = nil
@changed_attributes = HashWithIndifferentAccess.new
store_original_attributes
forget_attribute_assignments
clear_mutation_trackers
end
def raw_write_attribute(attr_name, *)
@ -80,17 +100,27 @@ module ActiveRecord
if defined?(@cached_changed_attributes)
@cached_changed_attributes
else
emit_warning_if_needed("changed_attributes", "attributes_in_database")
super.reverse_merge(mutation_tracker.changed_values).freeze
end
end
def changes
cache_changed_attributes do
emit_warning_if_needed("changes", "changes_to_save")
super
end
end
def previous_changes
unless previous_mutation_tracker.equal?(mutations_before_last_save)
ActiveSupport::Deprecation.warn(<<-EOW.strip_heredoc)
The behavior of `previous_changes` inside of after callbacks is
deprecated without replacement. In the next release of Rails,
this method inside of `after_save` will return the changes that
were just saved.
EOW
end
previous_mutation_tracker.changes
end
@ -98,6 +128,109 @@ module ActiveRecord
mutation_tracker.changed_in_place?(attr_name)
end
# Did this attribute change when we last saved? This method can be invoked
# as `saved_change_to_name?` instead of `saved_change_to_attribute?("name")`.
# Behaves similarly to +attribute_changed?+. This method is useful in
# after callbacks to determine if the call to save changed a certain
# attribute.
#
# ==== Options
#
# +from+ When passed, this method will return false unless the original
# value is equal to the given option
#
# +to+ When passed, this method will return false unless the value was
# changed to the given value
def saved_change_to_attribute?(attr_name, **options)
mutations_before_last_save.changed?(attr_name, **options)
end
# Returns the change to an attribute during the last save. If the
# attribute was changed, the result will be an array containing the
# original value and the saved value.
#
# Behaves similarly to +attribute_change+. This method is useful in after
# callbacks, to see the change in an attribute that just occurred
#
# This method can be invoked as `saved_change_to_name` in instead of
# `saved_change_to_attribute("name")`
def saved_change_to_attribute(attr_name)
mutations_before_last_save.change_to_attribute(attr_name)
end
# Returns the original value of an attribute before the last save.
# Behaves similarly to +attribute_was+. This method is useful in after
# callbacks to get the original value of an attribute before the save that
# just occurred
def attribute_before_last_save(attr_name)
mutations_before_last_save.original_value(attr_name)
end
# Did the last call to `save` have any changes to change?
def saved_changes?
mutations_before_last_save.any_changes?
end
# Returns a hash containing all the changes that were just saved.
def saved_changes
mutations_before_last_save.changes
end
# Alias for `attribute_changed?`
def will_save_change_to_attribute?(attr_name, **options)
mutations_from_database.changed?(attr_name, **options)
end
# Alias for `attribute_change`
def attribute_change_to_be_saved(attr_name)
mutations_from_database.change_to_attribute(attr_name)
end
# Alias for `attribute_was`
def attribute_in_database(attr_name)
mutations_from_database.original_value(attr_name)
end
# Alias for `changed?`
def has_changes_to_save?
mutations_from_database.any_changes?
end
# Alias for `changes`
def changes_to_save
mutations_from_database.changes
end
# Alias for `changed`
def changed_attribute_names_to_save
changes_to_save.keys
end
# Alias for `changed_attributes`
def attributes_in_database
changes_to_save.transform_values(&:first)
end
def attribute_was(*)
emit_warning_if_needed("attribute_was", "attribute_in_database")
super
end
def attribute_change(*)
emit_warning_if_needed("attribute_change", "attribute_change_to_be_saved")
super
end
def attribute_changed?(*)
emit_warning_if_needed("attribute_changed?", "will_save_change_to_attribute?")
super
end
def changed(*)
emit_warning_if_needed("changed", "changed_attribute_names_to_save")
super
end
private
def mutation_tracker
@ -107,12 +240,37 @@ module ActiveRecord
@mutation_tracker ||= AttributeMutationTracker.new(@attributes)
end
def emit_warning_if_needed(method_name, new_method_name)
unless mutation_tracker.equal?(mutations_from_database)
ActiveSupport::Deprecation.warn(<<-EOW.squish)
The behavior of `#{method_name}` inside of after callbacks will
be changing in the next version of Rails. The new return value will reflect the
behavior of calling the method after `save` returned (e.g. the opposite of what
it returns now). To maintain the current behavior, use `#{new_method_name}`
instead.
EOW
end
end
def mutations_from_database
unless defined?(@mutations_from_database)
@mutations_from_database = nil
end
@mutations_from_database ||= mutation_tracker
end
def changes_include?(attr_name)
super || mutation_tracker.changed?(attr_name)
end
def clear_attribute_change(attr_name)
mutation_tracker.forget_change(attr_name)
mutations_from_database.forget_change(attr_name)
end
def attribute_will_change!(attr_name)
super
mutations_from_database.force_change(attr_name)
end
def _update_record(*)
@ -124,18 +282,27 @@ module ActiveRecord
end
def keys_for_partial_write
changed & self.class.column_names
changed_attribute_names_to_save & self.class.column_names
end
def store_original_attributes
def forget_attribute_assignments
@attributes = @attributes.map(&:forgetting_assignment)
end
def clear_mutation_trackers
@mutation_tracker = nil
@mutations_from_database = nil
@mutations_before_last_save = nil
end
def previous_mutation_tracker
@previous_mutation_tracker ||= NullMutationTracker.instance
end
def mutations_before_last_save
@mutations_before_last_save ||= previous_mutation_tracker
end
def cache_changed_attributes
@cached_changed_attributes = changed_attributes
yield

View file

@ -45,6 +45,11 @@ module ActiveRecord
attribute_was(self.class.primary_key)
end
def id_in_database
sync_with_transaction_state
attribute_in_database(self.class.primary_key)
end
protected
def attribute_method?(attr_name)
@ -60,7 +65,7 @@ module ActiveRecord
end
end
ID_ATTRIBUTE_METHODS = %w(id id= id? id_before_type_cast id_was).to_set
ID_ATTRIBUTE_METHODS = %w(id id= id? id_before_type_cast id_was id_in_database).to_set
def dangerous_attribute_method?(method_name)
super && !ID_ATTRIBUTE_METHODS.include?(method_name)

View file

@ -1,7 +1,10 @@
module ActiveRecord
class AttributeMutationTracker # :nodoc:
OPTION_NOT_GIVEN = Object.new
def initialize(attributes)
@attributes = attributes
@forced_changes = Set.new
end
def changed_values
@ -14,15 +17,29 @@ module ActiveRecord
def changes
attr_names.each_with_object({}.with_indifferent_access) do |attr_name, result|
if changed?(attr_name)
result[attr_name] = [attributes[attr_name].original_value, attributes.fetch_value(attr_name)]
change = change_to_attribute(attr_name)
if change
result[attr_name] = change
end
end
end
def changed?(attr_name)
def change_to_attribute(attr_name)
if changed?(attr_name)
[attributes[attr_name].original_value, attributes.fetch_value(attr_name)]
end
end
def any_changes?
attr_names.any? { |attr| changed?(attr) }
end
def changed?(attr_name, from: OPTION_NOT_GIVEN, to: OPTION_NOT_GIVEN)
attr_name = attr_name.to_s
attributes[attr_name].changed?
forced_changes.include?(attr_name) ||
attributes[attr_name].changed? &&
(OPTION_NOT_GIVEN == from || attributes[attr_name].original_value == from) &&
(OPTION_NOT_GIVEN == to || attributes[attr_name].value == to)
end
def changed_in_place?(attr_name)
@ -32,11 +49,20 @@ module ActiveRecord
def forget_change(attr_name)
attr_name = attr_name.to_s
attributes[attr_name] = attributes[attr_name].forgetting_assignment
forced_changes.delete(attr_name)
end
def original_value(attr_name)
attributes[attr_name].original_value
end
def force_change(attr_name)
forced_changes << attr_name.to_s
end
protected
attr_reader :attributes
attr_reader :attributes, :forced_changes
private
@ -48,14 +74,21 @@ module ActiveRecord
class NullMutationTracker # :nodoc:
include Singleton
def changed_values
def changed_values(*)
{}
end
def changes
def changes(*)
{}
end
def change_to_attribute(attr_name)
end
def any_changes?(*)
false
end
def changed?(*)
false
end
@ -66,5 +99,8 @@ module ActiveRecord
def forget_change(*)
end
def original_value(*)
end
end
end

View file

@ -267,7 +267,7 @@ module ActiveRecord
# Returns whether or not this record has been changed in any way (including whether
# any of its nested autosave associations are likewise changed)
def changed_for_autosave?
new_record? || changed? || marked_for_destruction? || nested_records_changed_for_autosave?
new_record? || has_changes_to_save? || marked_for_destruction? || nested_records_changed_for_autosave?
end
private
@ -451,7 +451,7 @@ module ActiveRecord
def record_changed?(reflection, record, key)
record.new_record? ||
(record.has_attribute?(reflection.foreign_key) && record[reflection.foreign_key] != key) ||
record.attribute_changed?(reflection.foreign_key)
record.will_save_change_to_attribute?(reflection.foreign_key)
end
# Saves the associated record if it's new or <tt>:autosave</tt> is enabled.

View file

@ -14,6 +14,7 @@ require "active_support/core_ext/module/introspection"
require "active_support/core_ext/object/duplicable"
require "active_support/core_ext/class/subclasses"
require "active_record/attribute_decorators"
require "active_record/define_callbacks"
require "active_record/errors"
require "active_record/log_subscriber"
require "active_record/explain_subscriber"
@ -303,6 +304,7 @@ module ActiveRecord #:nodoc:
include AttributeDecorators
include Locking::Optimistic
include Locking::Pessimistic
include DefineCallbacks
include AttributeMethods
include Callbacks
include Timestamp

View file

@ -265,17 +265,6 @@ module ActiveRecord
:before_destroy, :around_destroy, :after_destroy, :after_commit, :after_rollback
]
module ClassMethods # :nodoc:
include ActiveModel::Callbacks
end
included do
include ActiveModel::Validations::Callbacks
define_model_callbacks :initialize, :find, :touch, only: :after
define_model_callbacks :save, :create, :update, :destroy
end
def destroy #:nodoc:
@_destroy_callback_already_called ||= false
return if @_destroy_callback_already_called

View file

@ -0,0 +1,20 @@
module ActiveRecord
# This module exists because `ActiveRecord::AttributeMethods::Dirty` needs to
# define callbacks, but continue to have its version of `save` be the super
# method of `ActiveRecord::Callbacks`. This will be removed when the removal
# of deprecated code removes this need.
module DefineCallbacks
extend ActiveSupport::Concern
module ClassMethods # :nodoc:
include ActiveModel::Callbacks
end
included do
include ActiveModel::Validations::Callbacks
define_model_callbacks :initialize, :find, :touch, :only => :after
define_model_callbacks :save, :create, :update, :destroy
end
end
end

View file

@ -253,7 +253,11 @@ module ActiveRecord
verify_readonly_attribute(name)
public_send("#{name}=", value)
changed? ? save(validate: false) : true
if has_changes_to_save?
save(validate: false)
else
true
end
end
# Updates the attributes of the model from the passed-in hash and saves the
@ -336,7 +340,7 @@ module ActiveRecord
# record could be saved.
def increment!(attribute, by = 1)
increment(attribute, by)
change = public_send(attribute) - (attribute_was(attribute.to_s) || 0)
change = public_send(attribute) - (attribute_in_database(attribute.to_s) || 0)
self.class.update_counters(id, attribute => change)
clear_attribute_change(attribute) # eww
self
@ -548,7 +552,7 @@ module ActiveRecord
if attributes_values.empty?
0
else
self.class.unscoped._update_record attributes_values, id, id_was
self.class.unscoped._update_record attributes_values, id, id_in_database
end
end

View file

@ -74,7 +74,7 @@ module ActiveRecord
timestamp_attributes_for_update_in_model.each do |column|
column = column.to_s
next if attribute_changed?(column)
next if will_save_change_to_attribute?(column)
write_attribute(column, current_time)
end
end
@ -82,7 +82,7 @@ module ActiveRecord
end
def should_record_timestamps?
record_timestamps && (!partial_writes? || changed?)
record_timestamps && (!partial_writes? || has_changes_to_save?)
end
def timestamp_attributes_for_create_in_model

View file

@ -25,7 +25,7 @@ module ActiveRecord
# touch the parents as we are not calling the after_save callbacks
self.class.reflect_on_all_associations(:belongs_to).each do |r|
if touch = r.options[:touch]
ActiveRecord::Associations::Builder::BelongsTo.touch_record(self, r.foreign_key, r.name, touch, :touch_later)
ActiveRecord::Associations::Builder::BelongsTo.touch_record(self, changes_to_save, r.foreign_key, r.name, touch, :touch_later)
end
end
end

View file

@ -17,7 +17,7 @@ module ActiveRecord
relation = build_relation(finder_class, attribute, value)
if record.persisted?
if finder_class.primary_key
relation = relation.where.not(finder_class.primary_key => record.id_was || record.id)
relation = relation.where.not(finder_class.primary_key => record.id_in_database || record.id)
else
raise UnknownPrimaryKey.new(finder_class, "Can not validate uniqueness for persisted record without primary key.")
end

View file

@ -14,6 +14,7 @@ module ActiveRecord
def self.decorate_matching_attribute_types(*); end
def self.initialize_generated_modules; end
include ActiveRecord::DefineCallbacks
include ActiveRecord::AttributeMethods
def self.attribute_names

View file

@ -726,6 +726,89 @@ class DirtyTest < ActiveRecord::TestCase
assert person.changed?
end
test "saved_change_to_attribute? returns whether a change occurred in the last save" do
person = Person.create!(first_name: "Sean")
assert person.saved_change_to_first_name?
refute person.saved_change_to_gender?
assert person.saved_change_to_first_name?(from: nil, to: "Sean")
assert person.saved_change_to_first_name?(from: nil)
assert person.saved_change_to_first_name?(to: "Sean")
refute person.saved_change_to_first_name?(from: "Jim", to: "Sean")
refute person.saved_change_to_first_name?(from: "Jim")
refute person.saved_change_to_first_name?(to: "Jim")
end
test "saved_change_to_attribute returns the change that occurred in the last save" do
person = Person.create!(first_name: "Sean", gender: "M")
assert_equal [nil, "Sean"], person.saved_change_to_first_name
assert_equal [nil, "M"], person.saved_change_to_gender
person.update(first_name: "Jim")
assert_equal ["Sean", "Jim"], person.saved_change_to_first_name
assert_nil person.saved_change_to_gender
end
test "attribute_before_last_save returns the original value before saving" do
person = Person.create!(first_name: "Sean", gender: "M")
assert_nil person.first_name_before_last_save
assert_nil person.gender_before_last_save
person.first_name = "Jim"
assert_nil person.first_name_before_last_save
assert_nil person.gender_before_last_save
person.save
assert_equal "Sean", person.first_name_before_last_save
assert_equal "M", person.gender_before_last_save
end
test "saved_changes? returns whether the last call to save changed anything" do
person = Person.create!(first_name: "Sean")
assert person.saved_changes?
person.save
refute person.saved_changes?
end
test "saved_changes returns a hash of all the changes that occurred" do
person = Person.create!(first_name: "Sean", gender: "M")
assert_equal [nil, "Sean"], person.saved_changes[:first_name]
assert_equal [nil, "M"], person.saved_changes[:gender]
assert_equal %w(id first_name gender created_at updated_at).sort, person.saved_changes.keys.sort
travel(1.second) do
person.update(first_name: "Jim")
end
assert_equal ["Sean", "Jim"], person.saved_changes[:first_name]
assert_equal %w(first_name lock_version updated_at).sort, person.saved_changes.keys.sort
end
test "changed? in after callbacks returns true but is deprecated" do
klass = Class.new(ActiveRecord::Base) do
self.table_name = "people"
after_save do
ActiveSupport::Deprecation.silence do
raise "changed? should be true" unless changed?
end
raise "has_changes_to_save? should be false" if has_changes_to_save?
end
end
person = klass.create!(first_name: "Sean")
refute person.changed?
end
private
def with_partial_writes(klass, on = true)
old = klass.partial_writes?

View file

@ -22,12 +22,12 @@ class Eye < ActiveRecord::Base
alias trace_after_create2 trace_after_create
def trace_after_update
(@after_update_callbacks_stack ||= []) << iris.changed?
(@after_update_callbacks_stack ||= []) << iris.has_changes_to_save?
end
alias trace_after_update2 trace_after_update
def trace_after_save
(@after_save_callbacks_stack ||= []) << iris.changed?
(@after_save_callbacks_stack ||= []) << iris.has_changes_to_save?
end
alias trace_after_save2 trace_after_save
end