diff options
author | Sean Griffin <sean@seantheprogrammer.com> | 2016-08-31 08:54:38 -0400 |
---|---|---|
committer | Sean Griffin <sean@seantheprogrammer.com> | 2016-08-31 09:26:25 -0400 |
commit | caa178c178468f7adcc5f4c597280e6362d9e175 (patch) | |
tree | 92e31877f28485d694d5e55bc315ef36b924ca37 | |
parent | 0ca595ea6e3a9523b7902b05305437054de67e43 (diff) | |
download | rails-caa178c178468f7adcc5f4c597280e6362d9e175.tar.gz rails-caa178c178468f7adcc5f4c597280e6362d9e175.tar.bz2 rails-caa178c178468f7adcc5f4c597280e6362d9e175.zip |
Ensure that inverse associations are set before running callbacks
If a parent association was accessed in an `after_find` or
`after_initialize` callback, it would always end up loading the
association, and then immediately overwriting the association we just
loaded. If this occurred in a way that the parent's `current_scope` was
set to eager load the child, this would result in an infinite loop and
eventually overflow the stack.
For records that are created with `.new`, we have a mechanism to
perform an action before the callbacks are run. I've introduced the same
code path for records created with `instantiate`, and updated all code
which sets inverse instances on newly loaded associations to use this
block instead.
Fixes #26320.
14 files changed, 77 insertions, 24 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 7838fe7167..f62d29b5bf 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,10 @@ +* Inverse association instances will now be set before `after_find` or + `after_initialize` callbacks are run. + + Fixes #26320. + + *Sean Griffin* + * Remove unnecessarily association load when a `belongs_to` association has already been loaded then the foreign key is changed directly and the record saved. diff --git a/activerecord/lib/active_record/association_relation.rb b/activerecord/lib/active_record/association_relation.rb index 2da2d968b9..de2d03cd0b 100644 --- a/activerecord/lib/active_record/association_relation.rb +++ b/activerecord/lib/active_record/association_relation.rb @@ -29,7 +29,10 @@ module ActiveRecord private def exec_queries - super.each { |r| @association.set_inverse_instance r } + super do |r| + @association.set_inverse_instance r + yield r if block_given? + end end end end diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb index 0f51b35164..0c911a5396 100644 --- a/activerecord/lib/active_record/associations/collection_association.rb +++ b/activerecord/lib/active_record/associations/collection_association.rb @@ -390,9 +390,9 @@ module ActiveRecord end binds = AssociationScope.get_bind_values(owner, reflection.chain) - records = sc.execute(binds, klass, conn) - records.each { |record| set_inverse_instance(record) } - records + sc.execute(binds, klass, conn) do |record| + set_inverse_instance(record) + end end # We have some records loaded from the database (persisted) and some that are diff --git a/activerecord/lib/active_record/associations/join_dependency.rb b/activerecord/lib/active_record/associations/join_dependency.rb index 62acad0eda..02f0721bed 100644 --- a/activerecord/lib/active_record/associations/join_dependency.rb +++ b/activerecord/lib/active_record/associations/join_dependency.rb @@ -286,17 +286,19 @@ module ActiveRecord end def construct_model(record, node, row, model_cache, id, aliases) - model = model_cache[node][id] ||= node.instantiate(row, - aliases.column_aliases(node)) other = record.association(node.reflection.name) + model = model_cache[node][id] ||= + node.instantiate(row, aliases.column_aliases(node)) do |m| + other.set_inverse_instance(m) + end + if node.reflection.collection? other.target.push(model) else other.target = model end - other.set_inverse_instance(model) model end end diff --git a/activerecord/lib/active_record/associations/join_dependency/join_part.rb b/activerecord/lib/active_record/associations/join_dependency/join_part.rb index 551087f822..61cec5403a 100644 --- a/activerecord/lib/active_record/associations/join_dependency/join_part.rb +++ b/activerecord/lib/active_record/associations/join_dependency/join_part.rb @@ -62,8 +62,8 @@ module ActiveRecord hash end - def instantiate(row, aliases) - base_klass.instantiate(extract_record(row, aliases)) + def instantiate(row, aliases, &block) + base_klass.instantiate(extract_record(row, aliases), &block) end end end diff --git a/activerecord/lib/active_record/associations/preloader/association.rb b/activerecord/lib/active_record/associations/preloader/association.rb index 4bb627f399..07407700cd 100644 --- a/activerecord/lib/active_record/associations/preloader/association.rb +++ b/activerecord/lib/active_record/associations/preloader/association.rb @@ -62,7 +62,12 @@ module ActiveRecord private def associated_records_by_owner(preloader) - records = load_records + records = load_records do |record| + owner = owners_by_key[convert_key(record[association_key_name])] + association = owner.association(reflection.name) + association.set_inverse_instance(record) + end + owners.each_with_object({}) do |owner, result| result[owner] = records[convert_key(owner[owner_key_name])] || [] end @@ -79,6 +84,15 @@ module ActiveRecord @owner_keys end + def owners_by_key + unless defined?(@owners_by_key) + @owners_by_key = owners.each_with_object({}) do |owner, h| + h[convert_key(owner[owner_key_name])] = owner + end + end + @owners_by_key + end + def key_conversion_required? @key_conversion_required ||= association_key_type != owner_key_type end @@ -99,13 +113,13 @@ module ActiveRecord @model.type_for_attribute(owner_key_name.to_s).type end - def load_records + def load_records(&block) return {} if owner_keys.empty? # Some databases impose a limit on the number of ids in a list (in Oracle it's 1000) # Make several smaller queries if necessary or make one query if the adapter supports it slices = owner_keys.each_slice(klass.connection.in_clause_length || owner_keys.size) @preloaded_records = slices.flat_map do |slice| - records_for(slice) + records_for(slice).load(&block) end @preloaded_records.group_by do |record| convert_key(record[association_key_name]) diff --git a/activerecord/lib/active_record/associations/preloader/collection_association.rb b/activerecord/lib/active_record/associations/preloader/collection_association.rb index 24b8e01029..26690bf16d 100644 --- a/activerecord/lib/active_record/associations/preloader/collection_association.rb +++ b/activerecord/lib/active_record/associations/preloader/collection_association.rb @@ -9,7 +9,6 @@ module ActiveRecord association = owner.association(reflection.name) association.loaded! association.target.concat(records) - records.each { |record| association.set_inverse_instance(record) } end end end diff --git a/activerecord/lib/active_record/associations/preloader/singular_association.rb b/activerecord/lib/active_record/associations/preloader/singular_association.rb index 0888d383a6..5c5828262e 100644 --- a/activerecord/lib/active_record/associations/preloader/singular_association.rb +++ b/activerecord/lib/active_record/associations/preloader/singular_association.rb @@ -10,7 +10,6 @@ module ActiveRecord association = owner.association(reflection.name) association.target = record - association.set_inverse_instance(record) if record end end end diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index aef4761be4..2725c85446 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -366,6 +366,8 @@ module ActiveRecord self.class.define_attribute_methods + yield self if block_given? + _run_find_callbacks _run_initialize_callbacks diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb index a6615f3774..a04ef2e263 100644 --- a/activerecord/lib/active_record/persistence.rb +++ b/activerecord/lib/active_record/persistence.rb @@ -63,10 +63,10 @@ module ActiveRecord # # See <tt>ActiveRecord::Inheritance#discriminate_class_for_record</tt> to see # how this "single-table" inheritance mapping is implemented. - def instantiate(attributes, column_types = {}) + def instantiate(attributes, column_types = {}, &block) klass = discriminate_class_for_record(attributes) attributes = klass.attributes_builder.build_from_database(attributes, column_types) - klass.allocate.init_with("attributes" => attributes, "new_record" => false) + klass.allocate.init_with("attributes" => attributes, "new_record" => false, &block) end private diff --git a/activerecord/lib/active_record/querying.rb b/activerecord/lib/active_record/querying.rb index dd7d650207..36689f6559 100644 --- a/activerecord/lib/active_record/querying.rb +++ b/activerecord/lib/active_record/querying.rb @@ -35,7 +35,7 @@ module ActiveRecord # # Post.find_by_sql ["SELECT title FROM posts WHERE author = ? AND created > ?", author_id, start_date] # Post.find_by_sql ["SELECT body FROM comments WHERE author = :user_id OR approved_by = :user_id", { :user_id => user_id }] - def find_by_sql(sql, binds = [], preparable: nil) + def find_by_sql(sql, binds = [], preparable: nil, &block) result_set = connection.select_all(sanitize_sql(sql), "#{name} Load", binds, preparable: preparable) column_types = result_set.column_types.dup columns_hash.each_key { |k| column_types.delete k } @@ -47,7 +47,7 @@ module ActiveRecord } message_bus.instrument("instantiation.active_record", payload) do - result_set.map { |record| instantiate(record, column_types) } + result_set.map { |record| instantiate(record, column_types, &block) } end end diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index d7de1032b6..6d571cf026 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -562,8 +562,8 @@ module ActiveRecord # return value is the relation itself, not the records. # # Post.where(published: true).load # => #<ActiveRecord::Relation> - def load - exec_queries unless loaded? + def load(&block) + exec_queries(&block) unless loaded? self end @@ -678,8 +678,8 @@ module ActiveRecord private - def exec_queries - @records = eager_loading? ? find_with_associations.freeze : @klass.find_by_sql(arel, bound_attributes).freeze + def exec_queries(&block) + @records = eager_loading? ? find_with_associations.freeze : @klass.find_by_sql(arel, bound_attributes, &block).freeze preload = preload_values preload += includes_values unless eager_loading? diff --git a/activerecord/lib/active_record/statement_cache.rb b/activerecord/lib/active_record/statement_cache.rb index fd67032235..d19bb96ede 100644 --- a/activerecord/lib/active_record/statement_cache.rb +++ b/activerecord/lib/active_record/statement_cache.rb @@ -99,12 +99,12 @@ module ActiveRecord @bind_map = bind_map end - def execute(params, klass, connection) + def execute(params, klass, connection, &block) bind_values = bind_map.bind params sql = query_builder.sql_for bind_values, connection - klass.find_by_sql(sql, bind_values, preparable: true) + klass.find_by_sql(sql, bind_values, preparable: true, &block) end alias :call :execute end diff --git a/activerecord/test/cases/associations/inverse_associations_test.rb b/activerecord/test/cases/associations/inverse_associations_test.rb index 0b23cea420..6fe6ee6783 100644 --- a/activerecord/test/cases/associations/inverse_associations_test.rb +++ b/activerecord/test/cases/associations/inverse_associations_test.rb @@ -494,6 +494,33 @@ class InverseHasManyTests < ActiveRecord::TestCase assert !man.persisted? end + + def test_inverse_instance_should_be_set_before_find_callbacks_are_run + reset_callbacks(Interest, :find) do + Interest.after_find { raise unless association(:man).loaded? && man.present? } + + assert Man.first.interests.reload.any? + assert Man.includes(:interests).first.interests.any? + assert Man.joins(:interests).includes(:interests).first.interests.any? + end + end + + def test_inverse_instance_should_be_set_before_initialize_callbacks_are_run + reset_callbacks(Interest, :initialize) do + Interest.after_initialize { raise unless association(:man).loaded? && man.present? } + + assert Man.first.interests.reload.any? + assert Man.includes(:interests).first.interests.any? + assert Man.joins(:interests).includes(:interests).first.interests.any? + end + end + + def reset_callbacks(target, type) + old_callbacks = target.send(:get_callbacks, type).deep_dup + yield + ensure + target.send(:set_callbacks, type, old_callbacks) if old_callbacks + end end class InverseBelongsToTests < ActiveRecord::TestCase |