diff options
13 files changed, 109 insertions, 73 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 7404c1bd8f..8d900d9669 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* Fix eager loading/preloading association with scope including joins. + + Fixes #28324. + + *Ryuta Kamizono* + * Fix transactions to apply state to child transactions Previously if you had a nested transaction and the outer transaction was rolledback the record from the diff --git a/activerecord/lib/active_record/associations/join_dependency/join_association.rb b/activerecord/lib/active_record/associations/join_dependency/join_association.rb index d0c1848079..b14ddfeeeb 100644 --- a/activerecord/lib/active_record/associations/join_dependency/join_association.rb +++ b/activerecord/lib/active_record/associations/join_dependency/join_association.rb @@ -34,13 +34,19 @@ module ActiveRecord table = tables.shift klass = reflection.klass - join_scope = reflection.join_scope(table, foreign_table, foreign_klass) - - binds.concat join_scope.bound_attributes - constraint = join_scope.arel.constraints + constraint = reflection.build_join_constraint(table, foreign_table) joins << table.create_join(table, table.create_on(constraint), join_type) + join_scope = reflection.join_scope(table, foreign_klass) + + if join_scope.arel.constraints.any? + binds.concat join_scope.bound_attributes + joins.concat join_scope.arel.join_sources + right = joins.last.right + right.expr = right.expr.and(join_scope.arel.constraints) + end + # The current table in this iteration becomes the foreign table in the next foreign_table, foreign_klass = table, klass end diff --git a/activerecord/lib/active_record/associations/preloader.rb b/activerecord/lib/active_record/associations/preloader.rb index 208d1b2670..a18994cec4 100644 --- a/activerecord/lib/active_record/associations/preloader.rb +++ b/activerecord/lib/active_record/associations/preloader.rb @@ -54,8 +54,6 @@ module ActiveRecord autoload :BelongsTo, "active_record/associations/preloader/belongs_to" end - NULL_RELATION = Struct.new(:values, :where_clause, :joins_values).new({}, Relation::WhereClause.empty, []) - # Eager loads the named associations for the given Active Record record(s). # # In this description, 'association name' shall refer to the name passed @@ -93,7 +91,6 @@ module ActiveRecord def preload(records, associations, preload_scope = nil) records = Array.wrap(records).compact.uniq associations = Array.wrap(associations) - preload_scope = preload_scope || NULL_RELATION if records.empty? [] diff --git a/activerecord/lib/active_record/associations/preloader/association.rb b/activerecord/lib/active_record/associations/preloader/association.rb index 63ef3f2d8c..85343040db 100644 --- a/activerecord/lib/active_record/associations/preloader/association.rb +++ b/activerecord/lib/active_record/associations/preloader/association.rb @@ -11,7 +11,6 @@ module ActiveRecord @reflection = reflection @preload_scope = preload_scope @model = owners.first && owners.first.class - @scope = nil @preloaded_records = [] end @@ -23,29 +22,11 @@ module ActiveRecord raise NotImplementedError end - def scope - @scope ||= build_scope - end - - def records_for(ids) - scope.where(association_key_name => ids) - end - - def table - klass.arel_table - end - # The name of the key on the associated records def association_key_name raise NotImplementedError end - # This is overridden by HABTM as the condition should be on the foreign_key column in - # the join table - def association_key - klass.arel_attribute(association_key_name, table) - end - # The name of the key on the model which declares the association def owner_key_name raise NotImplementedError @@ -114,54 +95,35 @@ module ActiveRecord # 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).load(&block) + records_for(slice, &block) end @preloaded_records.group_by do |record| convert_key(record[association_key_name]) end end + def records_for(ids, &block) + scope.where(association_key_name => ids).load(&block) + end + + def scope + @scope ||= build_scope + end + def reflection_scope @reflection_scope ||= reflection.scope_for(klass) end def build_scope - scope = klass.unscoped - - values = reflection_scope.values - preload_values = preload_scope.values - - scope.where_clause = reflection_scope.where_clause + preload_scope.where_clause - scope.references_values = Array(values[:references]) + Array(preload_values[:references]) - - if preload_values[:select] || values[:select] - scope._select!(preload_values[:select] || values[:select]) - end - scope.includes! preload_values[:includes] || values[:includes] - if preload_scope.joins_values.any? - scope.joins!(preload_scope.joins_values) - else - scope.joins!(reflection_scope.joins_values) - end - - if order_values = preload_values[:order] || values[:order] - scope.order!(order_values) - end - - if preload_values[:reordering] || values[:reordering] - scope.reordering_value = true - end - - if preload_values[:readonly] || values[:readonly] - scope.readonly! - end + scope = klass.default_scoped - if options[:as] - scope.where!(klass.table_name => { reflection.type => model.base_class.sti_name }) + if reflection.type + scope.where!(reflection.type => model.base_class.sti_name) end - scope.unscope_values = Array(values[:unscope]) + Array(preload_values[:unscope]) - klass.default_scoped.merge(scope) + scope.merge!(reflection_scope) + scope.merge!(preload_scope) if preload_scope + scope end end end diff --git a/activerecord/lib/active_record/associations/preloader/through_association.rb b/activerecord/lib/active_record/associations/preloader/through_association.rb index 34587fd3f5..0999746cd5 100644 --- a/activerecord/lib/active_record/associations/preloader/through_association.rb +++ b/activerecord/lib/active_record/associations/preloader/through_association.rb @@ -79,17 +79,24 @@ module ActiveRecord def through_scope scope = through_reflection.klass.unscoped + values = reflection_scope.values if options[:source_type] scope.where! reflection.foreign_type => options[:source_type] else unless reflection_scope.where_clause.empty? - scope.includes_values = Array(reflection_scope.values[:includes] || options[:source]) + scope.includes_values = Array(values[:includes] || options[:source]) scope.where_clause = reflection_scope.where_clause + if joins = values[:joins] + scope.joins!(source_reflection.name => joins) + end + if left_outer_joins = values[:left_outer_joins] + scope.left_outer_joins!(source_reflection.name => left_outer_joins) + end end - scope.references! reflection_scope.values[:references] - if scope.eager_loading? && order_values = reflection_scope.values[:order] + scope.references! values[:references] + if scope.eager_loading? && order_values = values[:order] scope = scope.order(order_values) end end diff --git a/activerecord/lib/active_record/attribute_mutation_tracker.rb b/activerecord/lib/active_record/attribute_mutation_tracker.rb index 4de993e169..a01a58f8a5 100644 --- a/activerecord/lib/active_record/attribute_mutation_tracker.rb +++ b/activerecord/lib/active_record/attribute_mutation_tracker.rb @@ -26,6 +26,7 @@ module ActiveRecord end def change_to_attribute(attr_name) + attr_name = attr_name.to_s if changed?(attr_name) [attributes[attr_name].original_value, attributes.fetch_value(attr_name)] end @@ -44,7 +45,7 @@ module ActiveRecord end def changed_in_place?(attr_name) - attributes[attr_name].changed_in_place? + attributes[attr_name.to_s].changed_in_place? end def forget_change(attr_name) @@ -54,7 +55,7 @@ module ActiveRecord end def original_value(attr_name) - attributes[attr_name].original_value + attributes[attr_name.to_s].original_value end def force_change(attr_name) diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 8ff2f50fdb..a453ca55c7 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -185,19 +185,23 @@ module ActiveRecord end deprecate :scope_chain - def join_scope(table, foreign_table, foreign_klass) - predicate_builder = predicate_builder(table) - scope_chain_items = join_scopes(table, predicate_builder) - klass_scope = klass_join_scope(table, predicate_builder) - + def build_join_constraint(table, foreign_table) key = join_keys.key foreign_key = join_keys.foreign_key - klass_scope.where!(table[key].eq(foreign_table[foreign_key])) + constraint = table[key].eq(foreign_table[foreign_key]) if klass.finder_needs_type_condition? - klass_scope.where!(klass.send(:type_condition, table)) + table.create_and([constraint, klass.send(:type_condition, table)]) + else + constraint end + end + + def join_scope(table, foreign_klass) + predicate_builder = predicate_builder(table) + scope_chain_items = join_scopes(table, predicate_builder) + klass_scope = klass_join_scope(table, predicate_builder) if type klass_scope.where!(type => foreign_klass.base_class.sti_name) diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb index 55b294cfaa..c0bab19e82 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -68,6 +68,11 @@ class EagerAssociationTest < ActiveRecord::TestCase "expected to find only david's posts" end + def test_loading_with_scope_including_joins + assert_equal clubs(:boring_club), Member.preload(:general_club).find(1).general_club + assert_equal clubs(:boring_club), Member.eager_load(:general_club).find(1).general_club + end + def test_with_ordering list = Post.all.merge!(includes: :comments, order: "posts.id DESC").to_a [:other_by_mary, :other_by_bob, :misc_by_mary, :misc_by_bob, :eager_other, diff --git a/activerecord/test/models/club.rb b/activerecord/test/models/club.rb index 49d7b24a3b..3d441b1d48 100644 --- a/activerecord/test/models/club.rb +++ b/activerecord/test/models/club.rb @@ -8,6 +8,8 @@ class Club < ActiveRecord::Base has_many :favourites, -> { where(memberships: { favourite: true }) }, through: :memberships, source: :member + scope :general, -> { left_joins(:category).where(categories: { name: "General" }) } + private def private_method diff --git a/activerecord/test/models/member.rb b/activerecord/test/models/member.rb index 36f2461b84..b9597c6b9a 100644 --- a/activerecord/test/models/member.rb +++ b/activerecord/test/models/member.rb @@ -22,6 +22,7 @@ class Member < ActiveRecord::Base has_many :organization_member_details_2, through: :organization, source: :member_details has_one :club_category, through: :club, source: :category + has_one :general_club, -> { general }, through: :current_membership, source: :club has_many :current_memberships, -> { where favourite: true } has_many :clubs, through: :current_memberships diff --git a/activesupport/lib/active_support/core_ext/module/delegation.rb b/activesupport/lib/active_support/core_ext/module/delegation.rb index 04f34e553d..13bb8070ae 100644 --- a/activesupport/lib/active_support/core_ext/module/delegation.rb +++ b/activesupport/lib/active_support/core_ext/module/delegation.rb @@ -174,7 +174,7 @@ class Module to = to.to_s to = "self.#{to}" if DELEGATION_RESERVED_METHOD_NAMES.include?(to) - methods.each do |method| + methods.map do |method| # Attribute writer methods only accept one argument. Makes sure []= # methods still accept two arguments. definition = /[^\]]=$/.match?(method) ? "arg" : "*args, &block" diff --git a/activesupport/test/core_ext/module_test.rb b/activesupport/test/core_ext/module_test.rb index a4d4444d69..68bd5233bf 100644 --- a/activesupport/test/core_ext/module_test.rb +++ b/activesupport/test/core_ext/module_test.rb @@ -392,4 +392,42 @@ class ModuleTest < ActiveSupport::TestCase event = Event.new(Tester.new) assert_equal 1, event.foo end + + def test_private_delegate + location = Class.new do + def initialize(place) + @place = place + end + + private(*delegate(:street, :city, to: :@place)) + end + + place = location.new(Somewhere.new("Such street", "Sad city")) + + assert_not place.respond_to?(:street) + assert_not place.respond_to?(:city) + + assert place.respond_to?(:street, true) # Asking for private method + assert place.respond_to?(:city, true) + end + + def test_private_delegate_prefixed + location = Class.new do + def initialize(place) + @place = place + end + + private(*delegate(:street, :city, to: :@place, prefix: :the)) + end + + place = location.new(Somewhere.new("Such street", "Sad city")) + + assert_not place.respond_to?(:street) + assert_not place.respond_to?(:city) + + assert_not place.respond_to?(:the_street) + assert place.respond_to?(:the_street, true) + assert_not place.respond_to?(:the_city) + assert place.respond_to?(:the_city, true) + end end diff --git a/railties/test/application/configuration_test.rb b/railties/test/application/configuration_test.rb index 9fad1a4df6..983ea5c3e6 100644 --- a/railties/test/application/configuration_test.rb +++ b/railties/test/application/configuration_test.rb @@ -1131,6 +1131,7 @@ module ApplicationTests app "development" ActionController::Base.object_id # force lazy load hooks to run + assert_equal :raise, ActionController::Parameters.action_on_unpermitted_parameters post "/posts", post: { "title" => "zomg" } @@ -1141,6 +1142,7 @@ module ApplicationTests app "development" ActionController::Base.object_id # force lazy load hooks to run + assert_equal %w(controller action), ActionController::Parameters.always_permitted_parameters end @@ -1152,6 +1154,7 @@ module ApplicationTests app "development" ActionController::Base.object_id # force lazy load hooks to run + assert_equal %w( controller action format ), ActionController::Parameters.always_permitted_parameters end @@ -1175,6 +1178,7 @@ module ApplicationTests app "development" ActionController::Base.object_id # force lazy load hooks to run + assert_equal :raise, ActionController::Parameters.action_on_unpermitted_parameters post "/posts", post: { "title" => "zomg" }, format: "json" @@ -1185,6 +1189,7 @@ module ApplicationTests app "development" ActionController::Base.object_id # force lazy load hooks to run + assert_equal :log, ActionController::Parameters.action_on_unpermitted_parameters end @@ -1192,6 +1197,7 @@ module ApplicationTests app "test" ActionController::Base.object_id # force lazy load hooks to run + assert_equal :log, ActionController::Parameters.action_on_unpermitted_parameters end @@ -1199,6 +1205,7 @@ module ApplicationTests app "production" ActionController::Base.object_id # force lazy load hooks to run + assert_equal false, ActionController::Parameters.action_on_unpermitted_parameters end |