Remove duplicate code from associations. [Pratik]

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@9231 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

6 files changed, 72 insertions, 164 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index 474b1499d3..90173a4bc1 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Remove duplicate code from associations. [Pratik]
+
 * Refactor HasManyThroughAssociation to inherit from HasManyAssociation. Association callbacks and <association>_ids= now work with hm:t. #11516 [rubyruy]
 
 * Ensure HABTM#create and HABTM#build do not load entire association. [Pratik]
diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb
index 73f22cb0fa..9c724d2117 100644
--- a/activerecord/lib/active_record/associations/association_collection.rb
+++ b/activerecord/lib/active_record/associations/association_collection.rb
@@ -3,6 +3,51 @@ require 'set'
 module ActiveRecord
   module Associations
     class AssociationCollection < AssociationProxy #:nodoc:
+      def initialize(owner, reflection)
+        super
+        construct_sql
+      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(&: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
+        else
+          conditions = "#{@finder_sql}"
+          if sanitized_conditions = sanitize_sql(options[:conditions])
+            conditions << " AND (#{sanitized_conditions})"
+          end
+          
+          options[:conditions] = conditions
+
+          if options[:order] && @reflection.options[:order]
+            options[:order] = "#{options[:order]}, #{@reflection.options[:order]}"
+          elsif @reflection.options[:order]
+            options[:order] = @reflection.options[:order]
+          end
+          
+          # Build options specific to association
+          construct_find_options!(options)
+          
+          merge_options_from_reflection!(options)
+          
+          # Pass through args exactly as we received them.
+          args << options
+          @reflection.klass.find(*args)
+        end
+      end
+      
       def to_ary
         load_target
         @target.to_ary
@@ -30,10 +75,9 @@ module ActiveRecord
         @owner.transaction do
           flatten_deeper(records).each do |record|
             raise_on_type_mismatch(record)
-            callback(:before_add, record)
-            result &&= insert_record(record) unless @owner.new_record?
-            @target << record
-            callback(:after_add, record)
+            add_record_to_target_with_callbacks(record) do |r|
+              result &&= insert_record(record) unless @owner.new_record?
+            end
           end
         end
 
@@ -63,18 +107,13 @@ module ActiveRecord
       def delete(*records)
         records = flatten_deeper(records)
         records.each { |record| raise_on_type_mismatch(record) }
-        records.reject! do |record|
-          if record.new_record?
-            callback(:before_remove, record)
-            @target.delete(record)
-            callback(:after_remove, record)
-          end
-        end
-        return if records.empty?
         
         @owner.transaction do
           records.each { |record| callback(:before_remove, record) }
-          delete_records(records)
+          
+          old_records = records.reject {|r| r.new_record? }
+          delete_records(old_records) if old_records.any?
+          
           records.each do |record|
             @target.delete(record)
             callback(:after_remove, record)
@@ -180,6 +219,9 @@ module ActiveRecord
       end
 
       protected
+        def construct_find_options!(options)
+        end
+        
         def load_target
           if !@owner.new_record? || foreign_key_present
             begin
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 8ce5b83831..d4143e0645 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
@@ -1,11 +1,6 @@
 module ActiveRecord
   module Associations
     class HasAndBelongsToManyAssociation < AssociationCollection #:nodoc:
-      def initialize(owner, reflection)
-        super
-        construct_sql
-      end
-
       def create(attributes = {})
         create_record(attributes) { |record| insert_record(record) }
       end
@@ -14,53 +9,13 @@ module ActiveRecord
         create_record(attributes) { |record| insert_record(record, true) }
       end
 
-      def find_first
-        load_target.first
-      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
-
-          if ids.size == 1
-            id = ids.first.to_i
-            record = load_target.detect { |r| id == r.id }
-            expects_array ? [record] : record
-          else
-            load_target.select { |r| ids.include?(r.id) }
-          end
-        else
-          conditions = "#{@finder_sql}"
-
-          if sanitized_conditions = sanitize_sql(options[:conditions])
-            conditions << " AND (#{sanitized_conditions})"
-          end
-
-          options[:conditions] = conditions
+      protected
+        def construct_find_options!(options)
           options[:joins]      = @join_sql
           options[:readonly]   = finding_with_ambiguous_select?(options[:select] || @reflection.options[:select])
-
-          if options[:order] && @reflection.options[:order]
-            options[:order] = "#{options[:order]}, #{@reflection.options[:order]}"
-          elsif @reflection.options[:order]
-            options[:order] = @reflection.options[:order]
-          end
-
-          merge_options_from_reflection!(options)
-
-          options[:select] ||= (@reflection.options[:select] || '*')
-
-          # Pass through args exactly as we received them.
-          args << options
-          @reflection.klass.find(*args)
+          options[:select]   ||= (@reflection.options[:select] || '*')
         end
-      end
-
-      protected
+        
         def count_records
           load_target.size
         end
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index 6cf10c2192..963a938485 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -1,11 +1,6 @@
 module ActiveRecord
   module Associations
     class HasManyAssociation < AssociationCollection #:nodoc:
-      def initialize(owner, reflection)
-        super
-        construct_sql
-      end
-
       # Count the number of associated records. All arguments are optional.
       def count(*args)
         if @reflection.options[:counter_sql]
@@ -23,42 +18,6 @@ module ActiveRecord
         end
       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(&: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
-        else
-          conditions = "#{@finder_sql}"
-          if sanitized_conditions = sanitize_sql(options[:conditions])
-            conditions << " AND (#{sanitized_conditions})"
-          end
-          options[:conditions] = conditions
-
-          if options[:order] && @reflection.options[:order]
-            options[:order] = "#{options[:order]}, #{@reflection.options[:order]}"
-          elsif @reflection.options[:order]
-            options[:order] = @reflection.options[:order]
-          end
-
-          merge_options_from_reflection!(options)
-
-          # Pass through args exactly as we received them.
-          args << options
-          @reflection.klass.find(*args)
-        end
-      end
-
       protected
         def count_records
           count = if has_cached_counter?
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 23cead3ca6..ebea313c18 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -2,37 +2,8 @@ module ActiveRecord
   module Associations
     class HasManyThroughAssociation < HasManyAssociation #:nodoc:
       def initialize(owner, reflection)
-        super
         reflection.check_validity!
-        @finder_sql = construct_conditions
-      end
-
-
-      def find(*args)
-        options = args.extract_options!
-
-        conditions = "#{@finder_sql}"
-        if sanitized_conditions = sanitize_sql(options[:conditions])
-          conditions << " AND (#{sanitized_conditions})"
-        end
-        options[:conditions] = conditions
-
-        if options[:order] && @reflection.options[:order]
-          options[:order] = "#{options[:order]}, #{@reflection.options[:order]}"
-        elsif @reflection.options[:order]
-          options[:order] = @reflection.options[:order]
-        end
-
-        options[:select]  = construct_select(options[:select])
-        options[:from]  ||= construct_from
-        options[:joins]   = construct_joins(options[:joins])
-        options[:include] = @reflection.source_reflection.options[:include] if options[:include].nil?
-
-        merge_options_from_reflection!(options)
-
-        # Pass through args exactly as we received them.
-        args << options
-        @reflection.klass.find(*args)
+        super
       end
 
       alias_method :new, :build
@@ -59,15 +30,6 @@ module ActiveRecord
         return @target.size if loaded?
         return count
       end
-
-      # Calculate sum using SQL, not Enumerable
-      def sum(*args)
-        if block_given?
-          calculate(:sum, *args) { |*block_args| yield(*block_args) }
-        else
-          calculate(:sum, *args)
-        end
-      end
       
       def count(*args)
         column_name, options = @reflection.klass.send(:construct_count_options_from_args, *args)
@@ -79,8 +41,14 @@ module ActiveRecord
         @reflection.klass.send(:with_scope, construct_scope) { @reflection.klass.count(column_name, options) } 
       end
 
-
       protected
+        def construct_find_options!(options)
+          options[:select]  = construct_select(options[:select])
+          options[:from]  ||= construct_from
+          options[:joins]   = construct_joins(options[:joins])
+          options[:include] = @reflection.source_reflection.options[:include] if options[:include].nil?
+        end
+        
         def insert_record(record, force=true)
           if record.new_record?
             if force
@@ -101,26 +69,6 @@ module ActiveRecord
           end
         end
 
-        def method_missing(method, *args)
-          if @target.respond_to?(method) || (!@reflection.klass.respond_to?(method) && Class.respond_to?(method))
-            if block_given?
-              super { |*block_args| yield(*block_args) }
-            else
-              super
-            end
-          elsif @reflection.klass.scopes.include?(method)
-            @reflection.klass.scopes[method].call(self, *args)
-          else
-            with_scope construct_scope do
-              if block_given?
-                @reflection.klass.send(method, *args) { |*block_args| yield(*block_args) }
-              else
-                @reflection.klass.send(method, *args)
-              end
-            end
-          end
-        end
-
         def find_target
           @reflection.klass.find(:all,
             :select     => construct_select,
@@ -237,6 +185,8 @@ module ActiveRecord
 
               @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
 
           if @reflection.options[:counter_sql]
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 ecf9d8208d..c846956e1f 100644
--- a/activerecord/lib/active_record/associations/has_one_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_one_through_association.rb
@@ -1,6 +1,6 @@
 module ActiveRecord
   module Associations
-    class HasOneThroughAssociation < ActiveRecord::Associations::HasManyThroughAssociation
+    class HasOneThroughAssociation < HasManyThroughAssociation
       
       def create_through_record(new_value) #nodoc:
         klass = @reflection.through_reflection.klass