From 3103296a61709e808aa89c3d37cf22bcdbc5a675 Mon Sep 17 00:00:00 2001
From: Jon Leighton <j@jonathanleighton.com>
Date: Sun, 2 Jan 2011 20:33:18 +0000
Subject: [PATCH] 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 ---
 .../has_many_through_association.rb           |  5 --
 .../associations/through_association.rb       | 19 ++---
 .../lib/active_record/nested_attributes.rb    |  2 +-
 .../active_record/relation/spawn_methods.rb   |  2 +-
 .../cases/associations/join_model_test.rb     |  7 --
 activerecord/test/cases/readonly_test.rb      | 15 ++--
 activerecord/test/cases/relations_test.rb     |  4 +
 activerecord/test/models/comment.rb           |  5 ++
 activerecord/test/models/post.rb              |  2 +-
 activerecord/test/models/project.rb           |  4 +
 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 be5d264a8fa..8b0017e7bfc 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 e86f4c02835..ab42d0f2153 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 5336b6cc28b..3abe3c2dae3 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 0d044b28e40..ca02aa86125 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 d432e8486d4..400db6baf13 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 2b28dda363f..b2f1f941c06 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 16023defe38..f3a32e85e6e 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 e1f7fa29491..a8fb1bccf46 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 263c90097f6..7e9c190f42f 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 a1eb96ef091..8d694900ff4 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 cd774ce3437..4de180880cf 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 88061b21457..a9aa0afcedd 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 b51f7ff48f9..1c95d30d6bf 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 416032cb756..8a53a8f8036 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