From 3103296a61709e808aa89c3d37cf22bcdbc5a675 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 2 Jan 2011 20:33:18 +0000 Subject: Let AssociationCollection#find use #scoped to do its finding. Note that I am removing test_polymorphic_has_many_going_through_join_model_with_disabled_include, since this specifies different behaviour for an association than for a regular scope. It seems reasonable to expect scopes and association proxies to behave in roughly the same way rather than having subtle differences. --- .../associations/association_collection.rb | 81 +++++++++++----------- .../associations/association_proxy.rb | 14 ---- .../has_and_belongs_to_many_association.rb | 34 +++++---- .../associations/has_many_association.rb | 10 --- .../associations/has_many_through_association.rb | 5 -- .../associations/through_association.rb | 19 ++--- .../lib/active_record/nested_attributes.rb | 2 +- .../lib/active_record/relation/spawn_methods.rb | 2 +- 8 files changed, 68 insertions(+), 99 deletions(-) (limited to 'activerecord/lib') diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index be5d264a8f..8b0017e7bf 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -32,37 +32,10 @@ module ActiveRecord end def find(*args) - options = args.extract_options! - - # If using a custom finder_sql, scan the entire collection. if @reflection.options[:finder_sql] - expects_array = args.first.kind_of?(Array) - ids = args.flatten.compact.uniq.map { |arg| arg.to_i } - - if ids.size == 1 - id = ids.first - record = load_target.detect { |r| id == r.id } - expects_array ? [ record ] : record - else - load_target.select { |r| ids.include?(r.id) } - end + find_by_scan(*args) else - merge_options_from_reflection!(options) - construct_find_options!(options) - - with_scope(:find => @scope[:find].slice(:conditions, :order)) do - relation = @reflection.klass.send(:construct_finder_arel, options, @reflection.klass.send(:current_scoped_methods)) - - case args.first - when :first, :last - relation.send(args.first) - when :all - records = relation.all - @reflection.options[:uniq] ? uniq(records) : records - else - relation.find(*args) - end - end + find_by_sql(*args) end end @@ -360,7 +333,25 @@ module ActiveRecord end protected - def construct_find_options!(options) + + def construct_find_scope + { + :conditions => construct_conditions, + :select => construct_select, + :readonly => @reflection.options[:readonly], + :order => @reflection.options[:order], + :limit => @reflection.options[:limit], + :include => @reflection.options[:include], + :joins => @reflection.options[:joins], + :group => @reflection.options[:group], + :having => @reflection.options[:having], + :offset => @reflection.options[:offset] + } + end + + def construct_select + @reflection.options[:select] || + @reflection.options[:uniq] && "DISTINCT #{@reflection.quoted_table_name}.*" end def load_target @@ -394,7 +385,7 @@ module ActiveRecord target end - def method_missing(method, *args) + def method_missing(method, *args, &block) match = DynamicFinderMatch.match(method) if match && match.creator? attributes = match.attribute_names @@ -406,15 +397,9 @@ module ActiveRecord elsif @reflection.klass.scopes[method] @_scopes_cache ||= {} @_scopes_cache[method] ||= {} - @_scopes_cache[method][args] ||= with_scope(@scope) { @reflection.klass.send(method, *args) } + @_scopes_cache[method][args] ||= scoped.readonly(nil).send(method, *args) else - with_scope(@scope) do - if block_given? - @reflection.klass.send(method, *args) { |*block_args| yield(*block_args) } - else - @reflection.klass.send(method, *args) - end - end + scoped.readonly(nil).send(method, *args, &block) end end @@ -547,6 +532,24 @@ module ActiveRecord @target.include?(record) end end + + # If using a custom finder_sql, #find scans the entire collection. + def find_by_scan(*args) + expects_array = args.first.kind_of?(Array) + ids = args.flatten.compact.uniq.map { |arg| arg.to_i } + + if ids.size == 1 + id = ids.first + record = load_target.detect { |r| id == r.id } + expects_array ? [ record ] : record + else + load_target.select { |r| ids.include?(r.id) } + end + end + + def find_by_sql(*args) + scoped.find(*args) + end end end end diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb index e86f4c0283..ab42d0f215 100644 --- a/activerecord/lib/active_record/associations/association_proxy.rb +++ b/activerecord/lib/active_record/associations/association_proxy.rb @@ -182,20 +182,6 @@ module ActiveRecord @reflection.klass.send(:sanitize_sql, sql, table_name) end - # Merges into +options+ the ones coming from the reflection. - def merge_options_from_reflection!(options) - options.reverse_merge!( - :group => @reflection.options[:group], - :having => @reflection.options[:having], - :limit => @reflection.options[:limit], - :offset => @reflection.options[:offset], - :joins => @reflection.options[:joins], - :include => @reflection.options[:include], - :select => @reflection.options[:select], - :readonly => @reflection.options[:readonly] - ) - end - # Forwards +with_scope+ to the reflection. def with_scope(*args, &block) target_klass.send :with_scope, *args, &block diff --git a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb index 5336b6cc28..3abe3c2dae 100644 --- a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb +++ b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb @@ -2,6 +2,7 @@ module ActiveRecord # = Active Record Has And Belongs To Many Association module Associations class HasAndBelongsToManyAssociation < AssociationCollection #:nodoc: + def columns @reflection.columns(@reflection.options[:join_table], "#{@reflection.options[:join_table]} Columns") end @@ -15,11 +16,6 @@ module ActiveRecord end protected - def construct_find_options!(options) - options[:joins] = Arel::SqlLiteral.new(@scope[:find][:joins]) - options[:readonly] = finding_with_ambiguous_select?(options[:select] || @reflection.options[:select]) - options[:select] ||= (@reflection.options[:select] || Arel::SqlLiteral.new('*')) - end def count_records load_target.size @@ -84,22 +80,23 @@ module ActiveRecord end def construct_find_scope - { - :conditions => construct_conditions, - :joins => construct_joins, - :readonly => false, - :order => @reflection.options[:order], - :include => @reflection.options[:include], - :limit => @reflection.options[:limit] - } + super.merge( + :joins => construct_joins, + :readonly => ambiguous_select?(@reflection.options[:select]), + :select => @reflection.options[:select] || Arel.star + ) end # Join tables with additional columns on top of the two foreign keys must be considered # ambiguous unless a select clause has been explicitly defined. Otherwise you can get # broken records back, if, for example, the join column also has an id column. This will # then overwrite the id column of the records coming back. - def finding_with_ambiguous_select?(select_clause) - !select_clause && columns.size != 2 + def ambiguous_select?(select) + extra_join_columns? && select.nil? + end + + def extra_join_columns? + columns.size > 2 end private @@ -114,6 +111,13 @@ module ActiveRecord def invertible_for?(record) false end + + def find_by_sql(*args) + options = args.extract_options! + ambiguous = ambiguous_select?(@reflection.options[:select] || options[:select]) + + scoped.readonly(ambiguous).find(*(args << options)) + end end end end diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index 0d044b28e4..ca02aa8612 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -71,16 +71,6 @@ module ActiveRecord end end - def construct_find_scope - { - :conditions => construct_conditions, - :readonly => false, - :order => @reflection.options[:order], - :limit => @reflection.options[:limit], - :include => @reflection.options[:include] - } - end - def construct_create_scope construct_owner_attributes end diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb index d432e8486d..400db6baf1 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -38,11 +38,6 @@ module ActiveRecord end end - def construct_find_options!(options) - options[:joins] = [construct_joins] + Array.wrap(options[:joins]) - options[:include] = @reflection.source_reflection.options[:include] if options[:include].nil? && @reflection.source_reflection.options[:include] - end - def insert_record(record, force = true, validate = true) if record.new_record? return false unless save_record(record, force, validate) diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb index 2b28dda363..b2f1f941c0 100644 --- a/activerecord/lib/active_record/associations/through_association.rb +++ b/activerecord/lib/active_record/associations/through_association.rb @@ -13,15 +13,11 @@ module ActiveRecord protected def construct_find_scope - { - :conditions => construct_conditions, - :joins => construct_joins, - :include => @reflection.options[:include] || @reflection.source_reflection.options[:include], - :select => construct_select, - :order => @reflection.options[:order], - :limit => @reflection.options[:limit], - :readonly => @reflection.options[:readonly] - } + super.merge( + :joins => construct_joins, + :include => @reflection.options[:include] || + @reflection.source_reflection.options[:include] + ) end # This scope affects the creation of the associated records (not the join records). At the @@ -44,11 +40,6 @@ module ActiveRecord super(aliased_through_table, @reflection.through_reflection) end - def construct_select - @reflection.options[:select] || - @reflection.options[:uniq] && "DISTINCT #{@reflection.quoted_table_name}.*" - end - def construct_joins right = aliased_through_table left = @reflection.klass.arel_table diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb index 16023defe3..f3a32e85e6 100644 --- a/activerecord/lib/active_record/nested_attributes.rb +++ b/activerecord/lib/active_record/nested_attributes.rb @@ -393,7 +393,7 @@ module ActiveRecord association.to_a else attribute_ids = attributes_collection.map {|a| a['id'] || a[:id] }.compact - attribute_ids.empty? ? [] : association.all(:conditions => {association.primary_key => attribute_ids}) + attribute_ids.empty? ? [] : association.all(:conditions => {association.klass.primary_key => attribute_ids}) end attributes_collection.each do |attributes| diff --git a/activerecord/lib/active_record/relation/spawn_methods.rb b/activerecord/lib/active_record/relation/spawn_methods.rb index e1f7fa2949..a8fb1bccf4 100644 --- a/activerecord/lib/active_record/relation/spawn_methods.rb +++ b/activerecord/lib/active_record/relation/spawn_methods.rb @@ -102,7 +102,7 @@ module ActiveRecord options.assert_valid_keys(VALID_FIND_OPTIONS) finders = options.dup - finders.delete_if { |key, value| value.nil? } + finders.delete_if { |key, value| value.nil? && key != :limit } ([:joins, :select, :group, :order, :having, :limit, :offset, :from, :lock, :readonly] & finders.keys).each do |finder| relation = relation.send(finder, finders[finder]) -- cgit v1.2.3