Refactor HasManyThroughAssociation to inherit from HasManyAssociation. Association callbacks and <association>_ids= now work with hm:t. Closes #11516 [rubyruy]

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

6 files changed, 67 insertions, 87 deletions

diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index c5cf06cf10..0acc63bd69 100755
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -44,6 +44,11 @@ module ActiveRecord
     end
   end
 
+  class HasManyThroughCantAssociateThroughHasManyReflection < ActiveRecordError #:nodoc:
+    def initialize(owner, reflection)
+      super("Cannot modify association '#{owner.class.name}##{reflection.name}' because the source reflection class '#{reflection.source_reflection.class_name}' is associated to '#{reflection.through_reflection.class_name}' via :#{reflection.source_reflection.macro}.")
+    end
+  end
   class HasManyThroughCantAssociateNewRecords < ActiveRecordError #:nodoc:
     def initialize(owner, reflection)
       super("Cannot associate new records through '#{owner.class.name}##{reflection.name}' on '#{reflection.source_reflection.class_name rescue nil}##{reflection.source_reflection.name rescue nil}'. Both records must have an id in order to create the has_many :through record associating them.")
@@ -125,27 +130,27 @@ module ActiveRecord
     #   generated methods                 | habtm | has_many | :through
     #   ----------------------------------+-------+----------+----------
     #   #others                           |   X   |    X     |    X
-    #   #others=(other,other,...)         |   X   |    X     |
+    #   #others=(other,other,...)         |   X   |    X     |    X
     #   #other_ids                        |   X   |    X     |    X
-    #   #other_ids=(id,id,...)            |   X   |    X     |
+    #   #other_ids=(id,id,...)            |   X   |    X     |    X
     #   #others<<                         |   X   |    X     |    X
     #   #others.push                      |   X   |    X     |    X
     #   #others.concat                    |   X   |    X     |    X
-    #   #others.build(attributes={})      |   X   |    X     |
-    #   #others.create(attributes={})     |   X   |    X     |
+    #   #others.build(attributes={})      |   X   |    X     |    X
+    #   #others.create(attributes={})     |   X   |    X     |    X
     #   #others.create!(attributes={})    |   X   |    X     |    X
     #   #others.size                      |   X   |    X     |    X
     #   #others.length                    |   X   |    X     |    X
     #   #others.count                     |   X   |    X     |    X
     #   #others.sum(args*,&block)         |   X   |    X     |    X
     #   #others.empty?                    |   X   |    X     |    X
-    #   #others.clear                     |   X   |    X     |
+    #   #others.clear                     |   X   |    X     |    X
     #   #others.delete(other,other,...)   |   X   |    X     |    X
     #   #others.delete_all                |   X   |    X     |
     #   #others.destroy_all               |   X   |    X     |    X
     #   #others.find(*args)               |   X   |    X     |    X
     #   #others.find_first                |   X   |          |
-    #   #others.uniq                      |   X   |    X     |
+    #   #others.uniq                      |   X   |    X     |    X
     #   #others.reset                     |   X   |    X     |    X
     #
     # == Cardinality and associations
@@ -650,7 +655,8 @@ module ActiveRecord
       # * <tt>:dependent</tt>   - if set to <tt>:destroy</tt> all the associated objects are destroyed
       #   alongside this object by calling their destroy method.  If set to <tt>:delete_all</tt> all associated
       #   objects are deleted *without* calling their destroy method.  If set to <tt>:nullify</tt> all associated
-      #   objects' foreign keys are set to +NULL+ *without* calling their save callbacks.
+      #   objects' foreign keys are set to +NULL+ *without* calling their save callbacks. *Warning:* This option is ignored when also using
+      #   the <tt>through</tt> option.
       # * <tt>:finder_sql</tt>  - specify a complete SQL statement to fetch the association. This is a good way to go for complex
       #   associations that depend on multiple tables. Note: When this option is used, +find_in_collection+ is _not_ added.
       # * <tt>:counter_sql</tt>  - specify a complete SQL statement to fetch the size of the association. If <tt>:finder_sql</tt> is
@@ -693,11 +699,12 @@ module ActiveRecord
 
         configure_dependency_for_has_many(reflection)
 
+        add_multiple_associated_save_callbacks(reflection.name)
+        add_association_callbacks(reflection.name, reflection.options)
+
         if options[:through]
-          collection_accessor_methods(reflection, HasManyThroughAssociation, false)
+          collection_accessor_methods(reflection, HasManyThroughAssociation)
         else
-          add_multiple_associated_save_callbacks(reflection.name)
-          add_association_callbacks(reflection.name, reflection.options)
           collection_accessor_methods(reflection, HasManyAssociation)
         end
       end
diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb
index ce1c8a262d..73f22cb0fa 100644
--- a/activerecord/lib/active_record/associations/association_collection.rb
+++ b/activerecord/lib/active_record/associations/association_collection.rb
@@ -13,6 +13,14 @@ module ActiveRecord
         @loaded = false
       end
 
+      def build(attributes = {})
+        if attributes.is_a?(Array)
+          attributes.collect { |attr| build(attr) }
+        else
+          build_record(attributes) { |record| set_belongs_to_association_for(record) }
+        end
+      end
+
       # Add +records+ to this association.  Returns +self+ so method calls may be chained.  
       # Since << flattens its argument list and inserts each record, +push+ and +concat+ behave identically.
       def <<(*records)
@@ -55,7 +63,13 @@ module ActiveRecord
       def delete(*records)
         records = flatten_deeper(records)
         records.each { |record| raise_on_type_mismatch(record) }
-        records.reject! { |record| @target.delete(record) if record.new_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
diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb
index 26274fff93..df21124e92 100644
--- a/activerecord/lib/active_record/associations/association_proxy.rb
+++ b/activerecord/lib/active_record/associations/association_proxy.rb
@@ -7,10 +7,10 @@ module ActiveRecord
     #       HasOneAssociation
     #     BelongsToPolymorphicAssociation
     #     AssociationCollection
-    #       HasManyAssociation
     #       HasAndBelongsToManyAssociation
-    #     HasManyThroughAssociation
-    #       HasOneThroughAssociation
+    #       HasManyAssociation
+    #         HasManyThroughAssociation
+    #            HasOneThroughAssociation
     #
     # Association proxies in Active Record are middlemen between the object that
     # holds the association, known as the <tt>@owner</tt>, and the actual associated
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 0edb2397ee..8ce5b83831 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
@@ -6,10 +6,6 @@ module ActiveRecord
         construct_sql
       end
 
-      def build(attributes = {})
-        build_record(attributes)
-      end
-
       def create(attributes = {})
         create_record(attributes) { |record| insert_record(record) }
       end
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index de6b843098..6cf10c2192 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -6,14 +6,6 @@ module ActiveRecord
         construct_sql
       end
 
-      def build(attributes = {})
-        if attributes.is_a?(Array)
-          attributes.collect { |attr| build(attr) }
-        else
-          build_record(attributes) { |record| set_belongs_to_association_for(record) }
-        end
-      end
-
       # Count the number of associated records. All arguments are optional.
       def count(*args)
         if @reflection.options[:counter_sql]
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 7d418ba701..23cead3ca6 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -1,13 +1,13 @@
 module ActiveRecord
   module Associations
-    class HasManyThroughAssociation < AssociationCollection #:nodoc:
+    class HasManyThroughAssociation < HasManyAssociation #:nodoc:
       def initialize(owner, reflection)
         super
         reflection.check_validity!
         @finder_sql = construct_conditions
-        construct_sql
       end
 
+
       def find(*args)
         options = args.extract_options!
 
@@ -35,65 +35,6 @@ module ActiveRecord
         @reflection.klass.find(*args)
       end
 
-      def reset
-        @target = []
-        @loaded = false
-      end
-
-      # Adds records to the association. The source record and its associates
-      # must have ids in order to create records associating them, so this
-      # will raise ActiveRecord::HasManyThroughCantAssociateNewRecords if
-      # either is a new record.  Calls create! so you can rescue errors.
-      #
-      # The :before_add and :after_add callbacks are not yet supported.
-      def <<(*records)
-        return if records.empty?
-        through = @reflection.through_reflection
-        raise ActiveRecord::HasManyThroughCantAssociateNewRecords.new(@owner, through) if @owner.new_record?
-
-        klass = through.klass
-        klass.transaction do
-          flatten_deeper(records).each do |associate|
-            raise_on_type_mismatch(associate)
-            raise ActiveRecord::HasManyThroughCantAssociateNewRecords.new(@owner, through) unless associate.respond_to?(:new_record?) && !associate.new_record?
-
-            @owner.send(@reflection.through_reflection.name).proxy_target << klass.send(:with_scope, :create => construct_join_attributes(associate)) { klass.create! }
-            @target << associate if loaded?
-          end
-        end
-
-        self
-      end
-
-      [:push, :concat].each { |method| alias_method method, :<< }
-
-      # Removes +records+ from this association.  Does not destroy +records+.
-      def delete(*records)
-        records = flatten_deeper(records)
-        records.each { |associate| raise_on_type_mismatch(associate) }
-
-        through = @reflection.through_reflection
-        raise ActiveRecord::HasManyThroughCantDissociateNewRecords.new(@owner, through) if @owner.new_record?
-
-        load_target
-
-        klass = through.klass
-        klass.transaction do
-          flatten_deeper(records).each do |associate|
-            raise_on_type_mismatch(associate)
-            raise ActiveRecord::HasManyThroughCantDissociateNewRecords.new(@owner, through) unless associate.respond_to?(:new_record?) && !associate.new_record?
-
-            klass.delete_all(construct_join_attributes(associate))
-            @target.delete(associate)
-          end
-        end
-
-        self
-      end
-
-      def build(attrs = nil)
-        raise ActiveRecord::HasManyThroughCantAssociateNewRecords.new(@owner, @reflection.through_reflection)
-      end
       alias_method :new, :build
 
       def create!(attrs = nil)
@@ -103,6 +44,13 @@ module ActiveRecord
         end
       end
 
+      def create(attrs = nil)
+        @reflection.klass.transaction do
+          self << (object = @reflection.klass.send(:with_scope, :create => attrs) { @reflection.klass.create })
+          object
+        end
+      end
+
       # Returns the size of the collection by executing a SELECT COUNT(*) query if the collection hasn't been loaded and
       # calling collection.size if it has. If it's more likely than not that the collection does have a size larger than zero
       # and you need to fetch that collection afterwards, it'll take one less SELECT query if you use length.
@@ -131,7 +79,28 @@ module ActiveRecord
         @reflection.klass.send(:with_scope, construct_scope) { @reflection.klass.count(column_name, options) } 
       end
 
+
       protected
+        def insert_record(record, force=true)
+          if record.new_record?
+            if force
+              record.save!
+            else
+              return false unless record.save
+            end
+          end
+          klass = @reflection.through_reflection.klass
+          @owner.send(@reflection.through_reflection.name).proxy_target << klass.send(:with_scope, :create => construct_join_attributes(record)) { klass.create! }
+        end
+
+        # TODO - add dependent option support
+        def delete_records(records)
+          klass = @reflection.through_reflection.klass
+          records.each do |associate|
+            klass.delete_all(construct_join_attributes(associate))
+          end
+        end
+
         def method_missing(method, *args)
           if @target.respond_to?(method) || (!@reflection.klass.respond_to?(method) && Class.respond_to?(method))
             if block_given?
@@ -178,6 +147,8 @@ module ActiveRecord
 
         # Construct attributes for :through pointing to owner and associate.
         def construct_join_attributes(associate)
+          # TODO: revist this to allow it for deletion, supposing dependent option is supported
+          raise ActiveRecord::HasManyThroughCantAssociateThroughHasManyReflection.new(@owner, @reflection) if @reflection.source_reflection.macro == :has_many
           join_attributes = construct_owner_attributes(@reflection.through_reflection).merge(@reflection.source_reflection.primary_key_name => associate.id)
           if @reflection.options[:source_type]
             join_attributes.merge!(@reflection.source_reflection.options[:foreign_type] => associate.class.base_class.name.to_s)