Add records to has_many :through using <<, push, and concat by creating the...

Add records to has_many :through using <<, push, and concat by creating the association record. Raise if base or associate are new records since both ids are required to create the association. #build raises since you can't associate an unsaved record. #create! takes an attributes hash and creates the associated record and its association in a transaction.

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

activerecord/CHANGELOG

 *SVN*
 
+* Add records to has_many :through using <<, push, and concat by creating the association record. Raise if base or associate are new records since both ids are required to create the association. #build raises since you can't associate an unsaved record. #create! takes an attributes hash and creates the associated record and its association in a transaction. [Jeremy Kemper]
+
+    # Create a tagging to associate the post and tag.
+    post.tags << Tag.find_by_name('old')
+    post.tags.create! :name => 'general'
+
+    # Would have been:
+    post.taggings.create!(:tag => Tag.find_by_name('finally')
+    transaction do
+      post.taggings.create!(:tag => Tag.create!(:name => 'general'))
+    end
+
 * Cache nil results for :included has_one associations also.  #5787 [Michael Schoen]
 
 * Fixed a bug which would cause .save to fail after trying to access a empty has_one association on a unsaved record. [Tobias Luetke]

activerecord/lib/active_record/associations.rb

@@ -38,6 +38,12 @@ def initialize(reflection)
     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.")
+    end
+  end
+
   class EagerLoadPolymorphicError < ActiveRecordError #:nodoc:
     def initialize(reflection)
       super("Can not eagerly load the polymorphic association #{reflection.name.inspect}")

activerecord/lib/active_record/associations/has_many_through_association.rb

@@ -22,12 +22,12 @@ def find(*args)
         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.
@@ -40,19 +40,41 @@ def reset
         @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.
       def <<(*args)
-        raise ActiveRecord::ReadOnlyAssociation.new(@reflection)
+        return if args.empty?
+        through = @reflection.through_reflection
+        raise ActiveRecord::HasManyThroughCantAssociateNewRecords.new(@owner, through) if @owner.new_record?
+
+        klass = through.klass
+        klass.transaction do
+          args.each do |associate|
+            raise ActiveRecord::HasManyThroughCantAssociateNewRecords.new(@owner, through) unless associate.respond_to?(:new_record?) && !associate.new_record?
+            klass.with_scope(:create => construct_association_attributes(through)) { klass.create! }
+          end
+        end
       end
 
-      [:push, :concat, :create, :build].each do |method|
-        alias_method method, :<<
+      [:push, :concat].each { |method| alias_method method, :<< }
+
+      def build(attrs = nil)
+        raise ActiveRecord::HasManyThroughCantAssociateNewRecords.new(@owner, @reflection.through_reflection)
       end
-      
+
+      def create!(attrs = nil)
+        @reflection.klass.transaction do
+          self << @reflection.klass.with_scope(:create => attrs) { @reflection.klass.create! }
+        end
+      end
+
       # Calculate sum using SQL, not Enumerable
       def sum(*args, &block)
         calculate(:sum, *args, &block)
       end
-      
+
       protected
         def method_missing(method, *args, &block)
           if @target.respond_to?(method) || (!@reflection.klass.respond_to?(method) && Class.respond_to?(method))
@@ -63,12 +85,12 @@ def method_missing(method, *args, &block)
         end
 
         def find_target
-          records = @reflection.klass.find(:all, 
+          records = @reflection.klass.find(:all,
             :select     => construct_select,
             :conditions => construct_conditions,
             :from       => construct_from,
             :joins      => construct_joins,
-            :order      => @reflection.options[:order], 
+            :order      => @reflection.options[:order],
             :limit      => @reflection.options[:limit],
             :group      => @reflection.options[:group],
             :include    => @reflection.options[:include] || @reflection.source_reflection.options[:include]
@@ -77,27 +99,44 @@ def find_target
           @reflection.options[:uniq] ? records.to_set.to_a : records
         end
 
-        def construct_conditions
-          conditions = if @reflection.through_reflection.options[:as]
-              "#{@reflection.through_reflection.table_name}.#{@reflection.through_reflection.options[:as]}_id = #{@owner.quoted_id} " + 
-              "AND #{@reflection.through_reflection.table_name}.#{@reflection.through_reflection.options[:as]}_type = #{@owner.class.quote @owner.class.base_class.name.to_s}"
+        def construct_association_attributes(reflection)
+          if as = reflection.options[:as]
+            { "#{as}_id" => @owner.id,
+              "#{as}_type" => @owner.class.base_class.name.to_s }
+          else
+            { reflection.primary_key_name => @owner.id }
+          end
+        end
+
+        def construct_quoted_association_attributes(reflection)
+          if as = reflection.options[:as]
+            { "#{as}_id" => @owner.quoted_id,
+              "#{as}_type" => reflection.klass.quote(
+                @owner.class.base_class.name.to_s,
+                reflection.klass.columns_hash["#{as}_type"]) }
           else
-            "#{@reflection.through_reflection.table_name}.#{@reflection.through_reflection.primary_key_name} = #{@owner.quoted_id}"
+            { reflection.primary_key_name => @owner.quoted_id }
+          end
+        end
+
+        def construct_conditions
+          table_name = @reflection.through_reflection.table_name
+          conditions = construct_quoted_association_attributes(@reflection.through_reflection).map do |attr, value|
+            "#{table_name}.#{attr} = #{value}"
           end
-          conditions << " AND (#{sql_conditions})" if sql_conditions
-          
-          return conditions
+          conditions << sql_conditions if sql_conditions
+          "(" + conditions.join(') AND (') + ")"
         end
 
         def construct_from
           @reflection.table_name
         end
-        
+
         def construct_select(custom_select = nil)
-          selected = custom_select || @reflection.options[:select] || "#{@reflection.table_name}.*"          
+          selected = custom_select || @reflection.options[:select] || "#{@reflection.table_name}.*"
         end
-        
-        def construct_joins(custom_joins = nil)          
+
+        def construct_joins(custom_joins = nil)
           polymorphic_join = nil
           if @reflection.through_reflection.options[:as] || @reflection.source_reflection.macro == :belongs_to
             reflection_primary_key = @reflection.klass.primary_key
@@ -120,14 +159,15 @@ def construct_joins(custom_joins = nil)
             polymorphic_join
           ]
         end
-        
+
         def construct_scope
-          {
-            :find   => { :from => construct_from, :conditions => construct_conditions, :joins => construct_joins, :select => construct_select },
-            :create => { @reflection.primary_key_name => @owner.id }
-          }
+          { :create => construct_association_attributes(@reflection),
+            :find   => { :from        => construct_from,
+                         :conditions  => construct_conditions,
+                         :joins       => construct_joins,
+                         :select      => construct_select } }
         end
-        
+
         def construct_sql
           case
             when @reflection.options[:finder_sql]
@@ -147,14 +187,14 @@ def construct_sql
             @counter_sql = @finder_sql
           end
         end
-        
+
         def conditions
           @conditions ||= [
             (interpolate_sql(@reflection.active_record.send(:sanitize_sql, @reflection.options[:conditions])) if @reflection.options[:conditions]),
             (interpolate_sql(@reflection.active_record.send(:sanitize_sql, @reflection.through_reflection.options[:conditions])) if @reflection.through_reflection.options[:conditions])
           ].compact.collect { |condition| "(#{condition})" }.join(' AND ') unless (!@reflection.options[:conditions] && !@reflection.through_reflection.options[:conditions])
         end
-        
+
         alias_method :sql_conditions, :conditions
     end
   end

activerecord/test/associations_join_model_test.rb

@@ -363,14 +363,24 @@ def test_has_many_through_uses_correct_attributes
     assert_nil posts(:thinking).tags.find_by_name("General").attributes["tag_id"]
   end
 
-  def test_raise_error_when_adding_to_has_many_through
-    assert_raise(ActiveRecord::ReadOnlyAssociation) { posts(:thinking).tags <<     tags(:general)  }
-    assert_raise(ActiveRecord::ReadOnlyAssociation) { posts(:thinking).tags.push   tags(:general)  }
-    assert_raise(ActiveRecord::ReadOnlyAssociation) { posts(:thinking).tags.concat tags(:general)  }
-    assert_raise(ActiveRecord::ReadOnlyAssociation) { posts(:thinking).tags.build(:name => 'foo')  }
-    assert_raise(ActiveRecord::ReadOnlyAssociation) { posts(:thinking).tags.create(:name => 'foo') }
+  def test_raise_error_when_adding_new_record_to_has_many_through
+    assert_raise(ActiveRecord::HasManyThroughCantAssociateNewRecords) { posts(:thinking).tags << tags(:general).clone }
+    assert_raise(ActiveRecord::HasManyThroughCantAssociateNewRecords) { posts(:thinking).clone.tags << tags(:general) }
+    assert_raise(ActiveRecord::HasManyThroughCantAssociateNewRecords) { posts(:thinking).tags.build }
   end
-  
+
+  def test_create_associate_when_adding_to_has_many_through
+    count = Tagging.count
+    assert_nothing_raised { posts(:thinking).tags << tags(:general) }
+    assert_equal(count + 1, Tagging.count)
+
+    assert_nothing_raised { posts(:thinking).tags.create!(:name => 'foo') }
+    assert_equal(count + 2, Tagging.count)
+
+    assert_nothing_raised { posts(:thinking).tags.concat(Tag.create!(:name => 'abc'), Tag.create!(:name => 'def')) }
+    assert_equal(count + 4, Tagging.count)
+  end
+
   def test_has_many_through_sum_uses_calculations
     assert_nothing_raised { authors(:david).comments.sum(:post_id) }
   end