Refactoring: replace the mix of variables like @finder_sql, @counter_sql, etc with just a single scope hash (created on initialization of the proxy). This is now used consistently across all associations. Therefore, all you have to do to ensure finding/counting etc is done correctly is implement the scope correctly.

11 files changed, 146 insertions, 159 deletions

diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb
index cb2d9e0a79..896e18af01 100644
--- a/activerecord/lib/active_record/associations/association_collection.rb
+++ b/activerecord/lib/active_record/associations/association_collection.rb
@@ -19,11 +19,6 @@ module ActiveRecord
     # If you need to work on all current children, new and existing records,
     # +load_target+ and the +loaded+ flag are your friends.
     class AssociationCollection < AssociationProxy #:nodoc:
-      def initialize(owner, reflection)
-        super
-        construct_sql
-      end
-
       delegate :group, :order, :limit, :joins, :where, :preload, :eager_load, :includes, :from, :lock, :readonly, :having, :to => :scoped
 
       def select(select = nil)
@@ -36,7 +31,7 @@ module ActiveRecord
       end
 
       def scoped
-        with_scope(construct_scope) { @reflection.klass.scoped }
+        with_scope(@scope) { @reflection.klass.scoped }
       end
 
       def find(*args)
@@ -58,9 +53,7 @@ module ActiveRecord
           merge_options_from_reflection!(options)
           construct_find_options!(options)
 
-          find_scope = construct_scope[:find].slice(:conditions, :order)
-
-          with_scope(:find => find_scope) do
+          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
@@ -178,17 +171,18 @@ module ActiveRecord
         end
       end
 
-      # Count all records using SQL. If the +:counter_sql+ option is set for the association, it will
-      # be used for the query. If no +:counter_sql+ was supplied, but +:finder_sql+ was set, the
-      # descendant's +construct_sql+ method will have set :counter_sql automatically.
-      # Otherwise, construct options and pass them with scope to the target class's +count+.
+      # Count all records using SQL. If the +:counter_sql+ or +:finder_sql+ option is set for the
+      # association, it will be used for the query. Otherwise, construct options and pass them with
+      # scope to the target class's +count+.
       def count(column_name = nil, options = {})
         column_name, options = nil, column_name if column_name.is_a?(Hash)
 
-        if @reflection.options[:counter_sql] && !options.blank?
-          raise ArgumentError, "If finder_sql/counter_sql is used then options cannot be passed"
-        elsif @reflection.options[:counter_sql]
-          @reflection.klass.count_by_sql(@counter_sql)
+        if @reflection.options[:counter_sql] || @reflection.options[:finder_sql]
+          unless options.blank?
+            raise ArgumentError, "If finder_sql/counter_sql is used then options cannot be passed"
+          end
+          
+          @reflection.klass.count_by_sql(custom_counter_sql)
         else
 
           if @reflection.options[:uniq]
@@ -197,7 +191,7 @@ module ActiveRecord
             options.merge!(:distinct => true)
           end
 
-          value = @reflection.klass.send(:with_scope, construct_scope) { @reflection.klass.count(column_name, options) }
+          value = @reflection.klass.send(:with_scope, @scope) { @reflection.klass.count(column_name, options) }
 
           limit  = @reflection.options[:limit]
           offset = @reflection.options[:offset]
@@ -377,18 +371,6 @@ module ActiveRecord
         def construct_find_options!(options)
         end
 
-        def construct_counter_sql
-          if @reflection.options[:counter_sql]
-            @counter_sql = interpolate_sql(@reflection.options[:counter_sql])
-          elsif @reflection.options[:finder_sql]
-            # replace the SELECT clause with COUNT(*), preserving any hints within /* ... */
-            @reflection.options[:counter_sql] = @reflection.options[:finder_sql].sub(/SELECT\b(\/\*.*?\*\/ )?(.*)\bFROM\b/im) { "SELECT #{$1}COUNT(*) FROM" }
-            @counter_sql = interpolate_sql(@reflection.options[:counter_sql])
-          else
-            @counter_sql = @finder_sql
-          end
-        end
-
         def load_target
           if !@owner.new_record? || foreign_key_present
             begin
@@ -434,9 +416,9 @@ module ActiveRecord
           elsif @reflection.klass.scopes[method]
             @_named_scopes_cache ||= {}
             @_named_scopes_cache[method] ||= {}
-            @_named_scopes_cache[method][args] ||= with_scope(construct_scope) { @reflection.klass.send(method, *args) }
+            @_named_scopes_cache[method][args] ||= with_scope(@scope) { @reflection.klass.send(method, *args) }
           else
-            with_scope(construct_scope) do
+            with_scope(@scope) do
               if block_given?
                 @reflection.klass.send(method, *args) { |*block_args| yield(*block_args) }
               else
@@ -446,9 +428,19 @@ module ActiveRecord
           end
         end
 
-        # overloaded in derived Association classes to provide useful scoping depending on association type.
-        def construct_scope
-          {}
+        def custom_counter_sql
+          if @reflection.options[:counter_sql]
+            counter_sql = @reflection.options[:counter_sql]
+          else
+            # replace the SELECT clause with COUNT(*), preserving any hints within /* ... */
+            counter_sql = @reflection.options[:finder_sql].sub(/SELECT\b(\/\*.*?\*\/ )?(.*)\bFROM\b/im) { "SELECT #{$1}COUNT(*) FROM" }
+          end
+          
+          interpolate_sql(counter_sql)
+        end
+        
+        def custom_finder_sql
+          interpolate_sql(@reflection.options[:finder_sql])
         end
 
         def reset_target!
@@ -462,7 +454,7 @@ module ActiveRecord
         def find_target
           records =
             if @reflection.options[:finder_sql]
-              @reflection.klass.find_by_sql(@finder_sql)
+              @reflection.klass.find_by_sql(custom_finder_sql)
             else
               find(:all)
             end
@@ -494,7 +486,7 @@ module ActiveRecord
           ensure_owner_is_not_new
 
           scoped_where = scoped.where_values_hash
-          create_scope = scoped_where ? construct_scope[:create].merge(scoped_where) : construct_scope[:create]
+          create_scope = scoped_where ? @scope[:create].merge(scoped_where) : @scope[:create]
           record = @reflection.klass.send(:with_scope, :create => create_scope) do
             @reflection.build_association(attrs)
           end
diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb
index f333f4d603..0c12c3737d 100644
--- a/activerecord/lib/active_record/associations/association_proxy.rb
+++ b/activerecord/lib/active_record/associations/association_proxy.rb
@@ -61,6 +61,7 @@ module ActiveRecord
         reflection.check_validity!
         Array.wrap(reflection.options[:extend]).each { |ext| proxy_extend(ext) }
         reset
+        construct_scope
       end
 
       # Returns the owner of the proxy.
@@ -203,6 +204,24 @@ module ActiveRecord
           @reflection.klass.send :with_scope, *args, &block
         end
 
+        # Construct the scope used for find/create queries on the target
+        def construct_scope
+          @scope = {
+            :find   => construct_find_scope,
+            :create => construct_create_scope
+          }
+        end
+        
+        # Implemented by subclasses
+        def construct_find_scope
+          raise NotImplementedError
+        end
+        
+        # Implemented by (some) subclasses
+        def construct_create_scope
+          {}
+        end
+
       private
         # Forwards any missing method call to the \target.
         def method_missing(method, *args)
diff --git a/activerecord/lib/active_record/associations/belongs_to_association.rb b/activerecord/lib/active_record/associations/belongs_to_association.rb
index 2eb56e5cd3..34b6cd5576 100644
--- a/activerecord/lib/active_record/associations/belongs_to_association.rb
+++ b/activerecord/lib/active_record/associations/belongs_to_association.rb
@@ -50,19 +50,21 @@ module ActiveRecord
                           "find"
                         end
 
-          options = @reflection.options.dup
-          (options.keys - [:select, :include, :readonly]).each do |key|
-            options.delete key
-          end
-          options[:conditions] = conditions
+          options = @reflection.options.dup.slice(:select, :include, :readonly)
 
-          the_target = @reflection.klass.send(find_method,
-            @owner[@reflection.primary_key_name],
-            options
-          ) if @owner[@reflection.primary_key_name]
+          the_target = with_scope(:find => @scope[:find]) do
+            @reflection.klass.send(find_method,
+              @owner[@reflection.primary_key_name],
+              options
+            ) if @owner[@reflection.primary_key_name]
+          end
           set_inverse_instance(the_target, @owner)
           the_target
         end
+        
+        def construct_find_scope
+          { :conditions => conditions }
+        end
 
         def foreign_key_present
           !@owner[@reflection.primary_key_name].nil?
diff --git a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb
index e429806b0c..a0df860623 100644
--- a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb
+++ b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb
@@ -44,20 +44,20 @@ module ActiveRecord
           end
         end
 
+        def construct_find_scope
+          { :conditions => conditions }
+        end
+
         def find_target
           return nil if association_class.nil?
 
-          target =
-            if @reflection.options[:conditions]
-              association_class.find(
-                @owner[@reflection.primary_key_name],
-                :select     => @reflection.options[:select],
-                :conditions => conditions,
-                :include    => @reflection.options[:include]
-              )
-            else
-              association_class.find(@owner[@reflection.primary_key_name], :select => @reflection.options[:select], :include => @reflection.options[:include])
-            end
+          target = association_class.send(:with_scope, :find => @scope[:find]) do
+            association_class.find(
+              @owner[@reflection.primary_key_name],
+              :select  => @reflection.options[:select],
+              :include => @reflection.options[:include]
+            )
+          end
           set_inverse_instance(target, @owner)
           target
         end
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 eb65234dfb..1fc9aba5cf 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
@@ -24,7 +24,7 @@ module ActiveRecord
 
       protected
         def construct_find_options!(options)
-          options[:joins]      = Arel::SqlLiteral.new @join_sql
+          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
@@ -80,27 +80,26 @@ module ActiveRecord
             ).delete
           end
         end
+        
+        def construct_joins
+          "INNER JOIN #{@owner.connection.quote_table_name @reflection.options[:join_table]} ON #{@reflection.quoted_table_name}.#{@reflection.klass.primary_key} = #{@owner.connection.quote_table_name @reflection.options[:join_table]}.#{@reflection.association_foreign_key}"
+        end
 
-        def construct_sql
-          if @reflection.options[:finder_sql]
-            @finder_sql = interpolate_sql(@reflection.options[:finder_sql])
-          else
-            @finder_sql = "#{@owner.connection.quote_table_name @reflection.options[:join_table]}.#{@reflection.primary_key_name} = #{owner_quoted_id} "
-            @finder_sql << " AND (#{conditions})" if conditions
-          end
-
-          @join_sql = "INNER JOIN #{@owner.connection.quote_table_name @reflection.options[:join_table]} ON #{@reflection.quoted_table_name}.#{@reflection.klass.primary_key} = #{@owner.connection.quote_table_name @reflection.options[:join_table]}.#{@reflection.association_foreign_key}"
-
-          construct_counter_sql
+        def construct_conditions
+          sql = "#{@owner.connection.quote_table_name @reflection.options[:join_table]}.#{@reflection.primary_key_name} = #{owner_quoted_id} "
+          sql << " AND (#{conditions})" if conditions
+          sql
         end
 
-        def construct_scope
-          { :find => {  :conditions => @finder_sql,
-                        :joins => @join_sql,
-                        :readonly => false,
-                        :order => @reflection.options[:order],
-                        :include => @reflection.options[:include],
-                        :limit => @reflection.options[:limit] } }
+        def construct_find_scope
+          {
+            :conditions => construct_conditions,
+            :joins      => construct_joins,
+            :readonly   => false,
+            :order      => @reflection.options[:order],
+            :include    => @reflection.options[:include],
+            :limit      => @reflection.options[:limit]
+          }
         end
 
         # Join tables with additional columns on top of the two foreign keys must be considered
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index 978fc74560..830a82980d 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -6,14 +6,10 @@ module ActiveRecord
     # If the association has a <tt>:through</tt> option further specialization
     # is provided by its child HasManyThroughAssociation.
     class HasManyAssociation < AssociationCollection #:nodoc:
-      def initialize(owner, reflection)
-        @finder_sql = nil
-        super
-      end
       protected
         def owner_quoted_id
           if @reflection.options[:primary_key]
-            quote_value(@owner.send(@reflection.options[:primary_key]))
+            @owner.class.quote_value(@owner.send(@reflection.options[:primary_key]))
           else
             @owner.quoted_id
           end
@@ -35,10 +31,10 @@ module ActiveRecord
         def count_records
           count = if has_cached_counter?
             @owner.send(:read_attribute, cached_counter_attribute_name)
-          elsif @reflection.options[:counter_sql]
-            @reflection.klass.count_by_sql(@counter_sql)
+          elsif @reflection.options[:counter_sql] || @reflection.options[:finder_sql]
+            @reflection.klass.count_by_sql(custom_counter_sql)
           else
-            @reflection.klass.count(:conditions => @counter_sql, :include => @reflection.options[:include])
+            @reflection.klass.count(@scope[:find].slice(:conditions, :joins, :include))
           end
 
           # If there's nothing in the database and @target has no new records
@@ -87,36 +83,32 @@ module ActiveRecord
           false
         end
 
-        def construct_sql
-          case
-            when @reflection.options[:finder_sql]
-              @finder_sql = interpolate_sql(@reflection.options[:finder_sql])
-
-            when @reflection.options[:as]
-              @finder_sql =
-                "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_id = #{owner_quoted_id} AND " +
-                "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote_value(@owner.class.base_class.name.to_s)}"
-              @finder_sql << " AND (#{conditions})" if conditions
-
-            else
-              @finder_sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{owner_quoted_id}"
-              @finder_sql << " AND (#{conditions})" if conditions
+        def construct_conditions
+          if @reflection.options[:as]
+            sql =
+              "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_id = #{owner_quoted_id} AND " +
+              "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote_value(@owner.class.base_class.name.to_s)}"
+          else
+            sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{owner_quoted_id}"
           end
+          sql << " AND (#{conditions})" if conditions
+          sql
+        end
 
-          construct_counter_sql
+        def construct_find_scope
+          {
+            :conditions => construct_conditions,
+            :readonly   => false,
+            :order      => @reflection.options[:order],
+            :limit      => @reflection.options[:limit],
+            :include    => @reflection.options[:include]
+          }
         end
 
-        def construct_scope
+        def construct_create_scope
           create_scoping = {}
           set_belongs_to_association_for(create_scoping)
-          {
-            :find => { :conditions => @finder_sql,
-                       :readonly => false,
-                       :order => @reflection.options[:order],
-                       :limit => @reflection.options[:limit],
-                       :include => @reflection.options[:include]},
-            :create => create_scoping
-          }
+          create_scoping
         end
 
         def we_can_set_the_inverse_on_this?(record)
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 97883d8393..437c8b1fd6 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -82,21 +82,7 @@ module ActiveRecord
 
         def find_target
           return [] unless target_reflection_has_associated_record?
-          with_scope(construct_scope) { @reflection.klass.find(:all) }
-        end
-
-        def construct_sql
-          case
-            when @reflection.options[:finder_sql]
-              @finder_sql = interpolate_sql(@reflection.options[:finder_sql])
-
-              @finder_sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{owner_quoted_id}"
-              @finder_sql << " AND (#{conditions})" if conditions
-            else
-              @finder_sql = construct_conditions
-          end
-
-          construct_counter_sql
+          with_scope(@scope) { @reflection.klass.find(:all) }
         end
 
         def has_cached_counter?
diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb
index a6e6bfa356..17901387e9 100644
--- a/activerecord/lib/active_record/associations/has_one_association.rb
+++ b/activerecord/lib/active_record/associations/has_one_association.rb
@@ -2,11 +2,6 @@ module ActiveRecord
   # = Active Record Belongs To Has One Association
   module Associations
     class HasOneAssociation < AssociationProxy #:nodoc:
-      def initialize(owner, reflection)
-        super
-        construct_sql
-      end
-
       def create(attrs = {}, replace_existing = true)
         new_record(replace_existing) do |reflection|
           attrs = merge_with_conditions(attrs)
@@ -79,33 +74,31 @@ module ActiveRecord
 
       private
         def find_target
-          options = @reflection.options.dup
-          (options.keys - [:select, :order, :include, :readonly]).each do |key|
-            options.delete key
-          end
-          options[:conditions] = @finder_sql
+          options = @reflection.options.dup.slice(:select, :order, :include, :readonly)
 
-          the_target = @reflection.klass.find(:first, options)
+          the_target = with_scope(:find => @scope[:find]) do
+            @reflection.klass.find(:first, options)
+          end
           set_inverse_instance(the_target, @owner)
           the_target
         end
 
-        def construct_sql
-          case
-            when @reflection.options[:as]
-              @finder_sql =
-                "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_id = #{owner_quoted_id} AND " +
-                "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote_value(@owner.class.base_class.name.to_s)}"
-            else
-              @finder_sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{owner_quoted_id}"
+        def construct_find_scope
+          if @reflection.options[:as]
+            sql =
+              "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_id = #{owner_quoted_id} AND " +
+              "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote_value(@owner.class.base_class.name.to_s)}"
+          else
+            sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{owner_quoted_id}"
           end
-          @finder_sql << " AND (#{conditions})" if conditions
+          sql << " AND (#{conditions})" if conditions
+          { :conditions => sql }
         end
 
-        def construct_scope
+        def construct_create_scope
           create_scoping = {}
           set_belongs_to_association_for(create_scoping)
-          { :create => create_scoping }
+          create_scoping
         end
 
         def new_record(replace_existing)
@@ -113,7 +106,7 @@ module ActiveRecord
           # instance. Otherwise, if the target has not previously been loaded
           # elsewhere, the instance we create will get orphaned.
           load_target if replace_existing
-          record = @reflection.klass.send(:with_scope, :create => construct_scope[:create]) do
+          record = @reflection.klass.send(:with_scope, :create => @scope[:create]) do
             yield @reflection
           end
 
diff --git a/activerecord/lib/active_record/associations/has_one_through_association.rb b/activerecord/lib/active_record/associations/has_one_through_association.rb
index fba0a2bfcc..7f28abf464 100644
--- a/activerecord/lib/active_record/associations/has_one_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_one_through_association.rb
@@ -33,7 +33,7 @@ module ActiveRecord
 
     private
       def find_target
-        with_scope(construct_scope) { @reflection.klass.find(:first) }
+        with_scope(@scope) { @reflection.klass.find(:first) }
       end
     end
   end
diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb
index cabb33c4a8..bd8e304e99 100644
--- a/activerecord/lib/active_record/associations/through_association_scope.rb
+++ b/activerecord/lib/active_record/associations/through_association_scope.rb
@@ -5,16 +5,20 @@ module ActiveRecord
 
       protected
 
-      def construct_scope
-        { :create => construct_owner_attributes(@reflection),
-          :find   => { :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],
-           } }
+      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]
+        }
+      end
+      
+      def construct_create_scope
+        construct_owner_attributes(@reflection)
       end
 
       # Build SQL conditions from attributes, qualified by table name.
diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index 4a2c078e91..0b89a49896 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -322,8 +322,8 @@ module ActiveRecord
           end
         end
 
-        # reconstruct the SQL queries now that we know the owner's id
-        association.send(:construct_sql) if association.respond_to?(:construct_sql)
+        # reconstruct the scope now that we know the owner's id
+        association.send(:construct_scope) if association.respond_to?(:construct_scope)
       end
     end