Added actual database-changing behavior to collection assigment for has_many and has_and_belongs_to_many #1425 [Sebastian Kanthak]

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

4 files changed, 102 insertions, 4 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index b91fb6213a..107d1cb698 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -1,5 +1,18 @@
 *SVN*
 
+* Added actual database-changing behavior to collection assigment for has_many and has_and_belongs_to_many #1425 [Sebastian Kanthak].
+  Example:
+
+    david.projects = [Project.find(1), Project.new("name" => "ActionWebSearch")]
+    david.save
+  
+  If david.projects already contain the project with ID 1, this is left unchanged. Any other projects are dropped. And the new
+  project is saved when david.save is called.
+  
+  Also included is a way to do assignments through IDs, which is perfect for checkbox updating, so you get to do:
+  
+    david.project_ids = [1, 5, 7]
+
 * Corrected typo in find SQL for has_and_belongs_to_many.  #1312 [ben@bensinclair.com]
 
 * Added ActiveRecord::Recursion for guarding against recursive saves
diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index 93960f87e4..65549aca0c 100755
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -200,6 +200,8 @@ module ActiveRecord
       # * <tt>collection<<(object, ...)</tt> - adds one or more objects to the collection by setting their foreign keys to the collection's primary key.
       # * <tt>collection.delete(object, ...)</tt> - removes one or more objects from the collection by setting their foreign keys to NULL.  
       #   This will also destroy the objects if they're declared as belongs_to and dependent on this model.
+      # * <tt>collection=objects</tt> - replaces the collections content by deleting and adding objects as appropriate.
+      # * <tt>collection_singular_ids=ids</tt> - replace the collection by the objects identified by the primary keys in +ids+
       # * <tt>collection.clear</tt> - removes every object from the collection. This does not destroy the objects.
       # * <tt>collection.empty?</tt> - returns true if there are no associated objects.
       # * <tt>collection.size</tt> - returns the number of associated objects.
@@ -215,6 +217,8 @@ module ActiveRecord
       # * <tt>Firm#clients</tt> (similar to <tt>Clients.find :all, :conditions => "firm_id = #{id}"</tt>)
       # * <tt>Firm#clients<<</tt>
       # * <tt>Firm#clients.delete</tt>
+      # * <tt>Firm#clients=</tt>
+      # * <tt>Firm#client_ids=</tt>
       # * <tt>Firm#clients.clear</tt>
       # * <tt>Firm#clients.empty?</tt> (similar to <tt>firm.clients.size == 0</tt>)
       # * <tt>Firm#clients.size</tt> (similar to <tt>Client.count "firm_id = #{id}"</tt>)
@@ -463,6 +467,8 @@ module ActiveRecord
       #   (collection.concat_with_attributes is an alias to this method).
       # * <tt>collection.delete(object, ...)</tt> - removes one or more objects from the collection by removing their associations from the join table.  
       #   This does not destroy the objects.
+      # * <tt>collection=objects</tt> - replaces the collections content by deleting and adding objects as appropriate.
+      # * <tt>collection_singular_ids=ids</tt> - replace the collection by the objects identified by the primary keys in +ids+
       # * <tt>collection.clear</tt> - removes every object from the collection. This does not destroy the objects.
       # * <tt>collection.empty?</tt> - returns true if there are no associated objects.
       # * <tt>collection.size</tt> - returns the number of associated objects.
@@ -474,6 +480,8 @@ module ActiveRecord
       # * <tt>Developer#projects<<</tt>
       # * <tt>Developer#projects.push_with_attributes</tt>
       # * <tt>Developer#projects.delete</tt>
+      # * <tt>Developer#projects=</tt>
+      # * <tt>Developer#project_ids=</tt>
       # * <tt>Developer#projects.clear</tt>
       # * <tt>Developer#projects.empty?</tt>
       # * <tt>Developer#projects.size</tt>
@@ -634,6 +642,10 @@ module ActiveRecord
             association.replace(new_value)
             association
           end
+
+          define_method("#{Inflector.singularize(association_name)}_ids=") do |new_value|
+            send("#{association_name}=", association_class_name.constantize.find(new_value))
+          end
         end
 
         def require_association_class(class_name)
@@ -687,7 +699,11 @@ module ActiveRecord
               instance_variable_set("@#{association_name}", association)
             end
 
-            association.send(constructor, attributees, replace_existing)
+            if association_proxy_class == HasOneAssociation
+              association.send(constructor, attributees, replace_existing)
+            else
+              association.send(constructor, attributees)
+            end
           end
         end
 
diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb
index 1eac8a2a0b..4495d92151 100644
--- a/activerecord/lib/active_record/associations/association_collection.rb
+++ b/activerecord/lib/active_record/associations/association_collection.rb
@@ -1,3 +1,5 @@
+require 'set'
+
 module ActiveRecord
   module Associations
     class AssociationCollection < AssociationProxy #:nodoc:
@@ -83,11 +85,19 @@ module ActiveRecord
         collection.inject([]) { |uniq_records, record| uniq_records << record unless uniq_records.include?(record); uniq_records }
       end
 
+      # Replace this collection with +other_array+
+      # This will perform a diff and delete/add only records that have changed.
       def replace(other_array)
-        other_array.each{ |val| raise_on_type_mismatch(val) }
+        other_array.each { |val| raise_on_type_mismatch(val) }
+
+        load_target
+        other   = other_array.size < 100 ? other_array : other_array.to_set
+        current = @target.size < 100 ? @target : @target.to_set
 
-        @target = other_array
-        @loaded = true
+        @owner.transaction do
+          delete(@target.select { |v| !other.include?(v) })
+          concat(other_array.select { |v| !current.include?(v) })
+        end
       end
 
       private
diff --git a/activerecord/test/associations_test.rb b/activerecord/test/associations_test.rb
index 80b984722a..02212a862b 100755
--- a/activerecord/test/associations_test.rb
+++ b/activerecord/test/associations_test.rb
@@ -596,6 +596,42 @@ class HasManyAssociationsTest < Test::Unit::TestCase
     firm = companies(:first_firm)
     assert_equal 2, firm.clients.find(:all).length
   end
+
+  def test_replace_with_less
+    firm = Firm.find_first
+    firm.clients = [companies(:first_client)]
+    assert firm.save, "Could not save firm"
+    firm.reload
+    assert_equal 1, firm.clients.length
+  end
+  
+  def test_replace_with_new
+    firm = Firm.find_first
+    new_client = Client.new("name" => "New Client")
+    firm.clients = [companies(:second_client),new_client]
+    firm.save
+    firm.reload
+    assert_equal 2, firm.clients.length
+    assert !firm.clients.include?(:first_client)
+  end
+  
+  def test_replace_on_new_object
+    firm = Firm.new("name" => "New Firm")
+    firm.clients = [companies(:second_client), Client.new("name" => "New Client")]
+    assert firm.save
+    firm.reload
+    assert_equal 2, firm.clients.length
+    assert firm.clients.include?(Client.find_by_name("New Client"))
+  end
+  
+  def test_assign_ids
+    firm = Firm.new("name" => "Apple")
+    firm.client_ids = [companies(:first_client).id, companies(:second_client).id]
+    firm.save
+    firm.reload
+    assert_equal 2, firm.clients.length
+    assert firm.clients.include?(companies(:second_client))
+  end
 end
 
 class BelongsToAssociationsTest < Test::Unit::TestCase
@@ -734,6 +770,7 @@ class BelongsToAssociationsTest < Test::Unit::TestCase
     apple.clients.to_s 
     assert_equal 1, apple.clients.size, "Should not use the cached number, but go to the database"
   end
+  
 end
 
 
@@ -972,4 +1009,26 @@ class HasAndBelongsToManyAssociationsTest < Test::Unit::TestCase
     assert_equal developers(:david), projects(:active_record).developers.find(:first, :conditions => "salary < 10000")
     assert_equal developers(:jamis), projects(:active_record).developers.find(:first, :order => "salary DESC")
   end
+  
+  def test_replace_with_less
+    david = developers(:david)
+    david.projects = [projects(:action_controller)]
+    assert david.save
+    assert_equal 1, david.projects.length
+  end
+
+  def test_replace_with_new
+    david = developers(:david)
+    david.projects = [projects(:action_controller), Project.new("name" => "ActionWebSearch")]
+    david.save
+    assert_equal 2, david.projects.length
+    assert !david.projects.include?(projects(:active_record))
+  end
+  
+  def test_replace_on_new_object
+    new_developer = Developer.new("name" => "Matz")
+    new_developer.projects = [projects(:action_controller), Project.new("name" => "ActionWebSearch")]
+    new_developer.save
+    assert_equal 2, new_developer.projects.length
+  end
 end