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.

14 files changed, 90 insertions, 114 deletions

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])
diff --git a/activerecord/test/cases/associations/join_model_test.rb b/activerecord/test/cases/associations/join_model_test.rb
index 263c90097f..7e9c190f42 100644
--- a/activerecord/test/cases/associations/join_model_test.rb
+++ b/activerecord/test/cases/associations/join_model_test.rb
@@ -85,13 +85,6 @@ class AssociationsJoinModelTest < ActiveRecord::TestCase
     end
   end
 
-  def test_polymorphic_has_many_going_through_join_model_with_disabled_include
-    assert_equal tags(:general), tag = posts(:welcome).tags.add_joins_and_select.first
-    assert_queries 1 do
-      tag.tagging
-    end
-  end
-
   def test_polymorphic_has_many_going_through_join_model_with_custom_select_and_joins
     assert_equal tags(:general), tag = posts(:welcome).tags.add_joins_and_select.first
     tag.author_id
diff --git a/activerecord/test/cases/readonly_test.rb b/activerecord/test/cases/readonly_test.rb
index a1eb96ef09..8d694900ff 100644
--- a/activerecord/test/cases/readonly_test.rb
+++ b/activerecord/test/cases/readonly_test.rb
@@ -6,11 +6,6 @@ require 'models/project'
 require 'models/reader'
 require 'models/person'
 
-# Dummy class methods to test implicit association scoping.
-def Comment.foo() find :first end
-def Project.foo() find :first end
-
-
 class ReadOnlyTest < ActiveRecord::TestCase
   fixtures :posts, :comments, :developers, :projects, :developers_projects, :people, :readers
 
@@ -114,7 +109,13 @@ class ReadOnlyTest < ActiveRecord::TestCase
   end
 
   def test_association_collection_method_missing_scoping_not_readonly
-    assert !Developer.find(1).projects.foo.readonly?
-    assert !Post.find(1).comments.foo.readonly?
+    developer = Developer.find(1)
+    project   = Post.find(1)
+
+    assert !developer.projects.all_as_method.first.readonly?
+    assert !developer.projects.all_as_scope.first.readonly?
+
+    assert !project.comments.all_as_method.first.readonly?
+    assert !project.comments.all_as_scope.first.readonly?
   end
 end
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index cd774ce343..4de180880c 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -806,4 +806,8 @@ class RelationTest < ActiveRecord::TestCase
     assert_equal [rails_author], [rails_author] & relation
     assert_equal [rails_author], relation & [rails_author]
   end
+
+  def test_removing_limit_with_options
+    assert_not_equal 1, Post.limit(1).all(:limit => nil).count
+  end
 end
diff --git a/activerecord/test/models/comment.rb b/activerecord/test/models/comment.rb
index 88061b2145..a9aa0afced 100644
--- a/activerecord/test/models/comment.rb
+++ b/activerecord/test/models/comment.rb
@@ -15,6 +15,11 @@ class Comment < ActiveRecord::Base
   def self.search_by_type(q)
     self.find(:all, :conditions => ["#{QUOTED_TYPE} = ?", q])
   end
+
+  def self.all_as_method
+    all
+  end
+  scope :all_as_scope, {}
 end
 
 class SpecialComment < Comment
diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb
index b51f7ff48f..1c95d30d6b 100644
--- a/activerecord/test/models/post.rb
+++ b/activerecord/test/models/post.rb
@@ -51,7 +51,7 @@ class Post < ActiveRecord::Base
   has_many :taggings, :as => :taggable
   has_many :tags, :through => :taggings do
     def add_joins_and_select
-      find :all, :select => 'tags.*, authors.id as author_id', :include => false,
+      find :all, :select => 'tags.*, authors.id as author_id',
         :joins => 'left outer join posts on taggings.taggable_id = posts.id left outer join authors on posts.author_id = authors.id'
     end
   end
diff --git a/activerecord/test/models/project.rb b/activerecord/test/models/project.rb
index 416032cb75..8a53a8f803 100644
--- a/activerecord/test/models/project.rb
+++ b/activerecord/test/models/project.rb
@@ -28,6 +28,10 @@ class Project < ActiveRecord::Base
     @developers_log = []
   end
 
+  def self.all_as_method
+    all
+  end
+  scope :all_as_scope, {}
 end
 
 class SpecialProject < Project