Changeset 9230

Timestamp:

04/06/08 00:27:12 (8 months ago)

Author:

pratik

Message:

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

Files:

• trunk/activerecord/CHANGELOG (modified) (1 diff)
• trunk/activerecord/lib/active_record/associations.rb (modified) (6 diffs)
• trunk/activerecord/lib/active_record/associations/association_collection.rb (modified) (2 diffs)
• trunk/activerecord/lib/active_record/associations/association_proxy.rb (modified) (1 diff)
• trunk/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb (modified) (1 diff)
• trunk/activerecord/lib/active_record/associations/has_many_association.rb (modified) (1 diff)
• trunk/activerecord/lib/active_record/associations/has_many_through_association.rb (modified) (5 diffs)
• trunk/activerecord/test/cases/associations_test.rb (modified) (2 diffs)
• trunk/activerecord/test/cases/associations/join_model_test.rb (modified) (1 diff)
• trunk/activerecord/test/models/post.rb (modified) (1 diff)

trunk/activerecord/CHANGELOG

@@ -1 +1 @@
-*SVN*
+
+* 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]

trunk/activerecord/lib/active_record/associations.rb

@@ -45 +45 @@
-  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)
@@ -126 +131 @@
-    #   ----------------------------------+-------+----------+----------
-    #   #others                           |   X   |    X     |    X
-
+    X
-    #   #other_ids                        |   X   |    X     |    X
-
+    X
-    #   #others<<                         |   X   |    X     |    X
-    #   #others.push                      |   X   |    X     |    X
-    #   #others.concat                    |   X   |    X     |    X
-
-
+    X
+    X
-    #   #others.create!(attributes={})    |   X   |    X     |    X
-    #   #others.size                      |   X   |    X     |    X
@@ -140 +145 @@
-    #   #others.sum(args*,&block)         |   X   |    X     |    X
-    #   #others.empty?                    |   X   |    X     |    X
-
+    X
-    #   #others.delete(other,other,...)   |   X   |    X     |    X
-    #   #others.delete_all                |   X   |    X     |
@@ -146 +151 @@
-    #   #others.find(*args)               |   X   |    X     |    X
-    #   #others.find_first                |   X   |          |
-
+    X
-    #   #others.reset                     |   X   |    X     |    X
-    #
@@ -651 +656 @@
-      #   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
-
+
+
-      # * <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.
@@ -694 +700 @@
-        configure_dependency_for_has_many(reflection)
-
+        add_multiple_associated_save_callbacks(reflection.name)
+        add_association_callbacks(reflection.name, reflection.options)
+
-        if options[:through]
-, false
+
-        else
-          add_multiple_associated_save_callbacks(reflection.name)
-          add_association_callbacks(reflection.name, reflection.options)
-          collection_accessor_methods(reflection, HasManyAssociation)
-        end

trunk/activerecord/lib/active_record/associations/association_collection.rb

@@ -14 +14 @@
-      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.
@@ -56 +64 @@
-        records = flatten_deeper(records)
-        records.each { |record| raise_on_type_mismatch(record) }
-
+
+
+
+
+
+
+
-        return if records.empty?
-        

trunk/activerecord/lib/active_record/associations/association_proxy.rb

@@ -8 +8 @@
-    #     BelongsToPolymorphicAssociation
-    #     AssociationCollection
+    #       HasAndBelongsToManyAssociation
-    #       HasManyAssociation
-
-
-
+
+
-    #
-    # Association proxies in Active Record are middlemen between the object that

trunk/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb

@@ -5 +5 @@
-        super
-        construct_sql
-      end
-
-      def build(attributes = {})
-        build_record(attributes)
-      end
-

trunk/activerecord/lib/active_record/associations/has_many_association.rb

@@ -5 +5 @@
-        super
-        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
-

trunk/activerecord/lib/active_record/associations/has_many_through_association.rb

@@ -1 +1 @@
-module ActiveRecord
-  module Associations
-AssociationCollec
+HasManyAssocia
-      def initialize(owner, reflection)
-        super
-        reflection.check_validity!
-        @finder_sql = construct_conditions
-  construct_sql
-
+end
+
-
-      def find(*args)
@@ -36 +36 @@
-      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
-
@@ -100 +41 @@
-        @reflection.klass.transaction do
-          self << (object = @reflection.klass.send(:with_scope, :create => attrs) { @reflection.klass.create! })
+          object
+        end
+      end
+
+      def create(attrs = nil)
+        @reflection.klass.transaction do
+          self << (object = @reflection.klass.send(:with_scope, :create => attrs) { @reflection.klass.create })
-          object
-        end
@@ -132 +80 @@
-      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))
@@ -179 +148 @@
-        # 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]

trunk/activerecord/test/cases/associations_test.rb

@@ -551 +551 @@
-end
-
+class HasManyThroughAssociationsTest < ActiveRecord::TestCase
+  fixtures :posts, :readers, :people
+
+  def test_associate_existing
+    assert_queries(2) { posts(:thinking);people(:david) }
+    
+    assert_queries(1) do
+       posts(:thinking).people << people(:david)
+    end
+    
+    assert_queries(1) do
+      assert posts(:thinking).people.include?(people(:david))
+    end
+    
+    assert posts(:thinking).reload.people(true).include?(people(:david))
+  end
+
+  def test_associating_new
+    assert_queries(1) { posts(:thinking) }
+    new_person = nil # so block binding catches it
+    
+    assert_queries(0) do
+      new_person = Person.new
+    end
+    
+    # Associating new records always saves them
+    # Thus, 1 query for the new person record, 1 query for the new join table record
+    assert_queries(2) do
+      posts(:thinking).people << new_person
+    end
+    
+    assert_queries(1) do
+      assert posts(:thinking).people.include?(new_person)
+    end
+    
+    assert posts(:thinking).reload.people(true).include?(new_person)
+  end
+
+  def test_associate_new_by_building
+    assert_queries(1) { posts(:thinking) }
+    
+    assert_queries(0) do
+      posts(:thinking).people.build(:first_name=>"Bob")
+      posts(:thinking).people.new(:first_name=>"Ted")
+    end
+    
+    # Should only need to load the association once
+    assert_queries(1) do
+      assert posts(:thinking).people.collect(&:first_name).include?("Bob")
+      assert posts(:thinking).people.collect(&:first_name).include?("Ted")
+    end
+    
+    # 2 queries for each new record (1 to save the record itself, 1 for the join model)
+    #    * 2 new records = 4
+    # + 1 query to save the actual post = 5
+    assert_queries(5) do
+      posts(:thinking).save
+    end
+    
+    assert posts(:thinking).reload.people(true).collect(&:first_name).include?("Bob")
+    assert posts(:thinking).reload.people(true).collect(&:first_name).include?("Ted")
+  end
+
+  def test_delete_association
+    assert_queries(2){posts(:welcome);people(:michael); }
+    
+    assert_queries(1) do
+      posts(:welcome).people.delete(people(:michael))
+    end
+    
+    assert_queries(1) do
+      assert posts(:welcome).people.empty?
+    end
+    
+    assert posts(:welcome).reload.people(true).empty?
+  end
+
+  def test_replace_association
+    assert_queries(4){posts(:welcome);people(:david);people(:michael); posts(:welcome).people(true)}
+    
+    # 1 query to delete the existing reader (michael)
+    # 1 query to associate the new reader (david)
+    assert_queries(2) do
+      posts(:welcome).people = [people(:david)]
+    end
+    
+    assert_queries(0){
+      assert posts(:welcome).people.include?(people(:david))
+      assert !posts(:welcome).people.include?(people(:michael))
+    }
+    
+    assert posts(:welcome).reload.people(true).include?(people(:david))
+    assert !posts(:welcome).reload.people(true).include?(people(:michael))
+  end
+
+  def test_associate_with_create
+    assert_queries(1) { posts(:thinking) }
+    
+    # 1 query for the new record, 1 for the join table record
+    # No need to update the actual collection yet!
+    assert_queries(2) do
+      posts(:thinking).people.create(:first_name=>"Jeb")
+    end
+    
+    # *Now* we actually need the collection so it's loaded
+    assert_queries(1) do
+      assert posts(:thinking).people.collect(&:first_name).include?("Jeb")
+    end
+    
+    assert posts(:thinking).reload.people(true).collect(&:first_name).include?("Jeb")
+  end
+
+  def test_clear_associations
+    assert_queries(2) { posts(:welcome);posts(:welcome).people(true) }
+    
+    assert_queries(1) do
+      posts(:welcome).people.clear
+    end
+    
+    assert_queries(0) do
+      assert posts(:welcome).people.empty?
+    end
+    
+    assert posts(:welcome).reload.people(true).empty?
+  end
+
+  def test_association_callback_ordering
+    Post.reset_log
+    log = Post.log
+    post = posts(:thinking)
+
+    post.people_with_callbacks << people(:michael)
+    assert_equal [
+      [:added, :before, "Michael"],
+      [:added, :after, "Michael"]
+    ], log.last(2)
+
+    post.people_with_callbacks.push(people(:david), Person.create!(:first_name => "Bob"), Person.new(:first_name => "Lary"))
+    assert_equal [
+      [:added, :before, "David"],
+      [:added, :after, "David"],
+      [:added, :before, "Bob"],
+      [:added, :after, "Bob"],
+      [:added, :before, "Lary"],
+      [:added, :after, "Lary"]
+    ],log.last(6)
+
+    post.people_with_callbacks.build(:first_name => "Ted")
+    assert_equal [
+      [:added, :before, "Ted"],
+      [:added, :after, "Ted"]
+    ], log.last(2)
+
+    post.people_with_callbacks.create(:first_name => "Sam")
+    assert_equal [
+      [:added, :before, "Sam"],
+      [:added, :after, "Sam"]
+    ], log.last(2)
+
+    post.people_with_callbacks = [people(:michael),people(:david), Person.new(:first_name => "Julian"), Person.create!(:first_name => "Roger")]
+    assert_equal (%w(Ted Bob Sam Lary) * 2).sort, log[-12..-5].collect(&:last).sort
+    assert_equal [
+      [:added, :before, "Julian"],
+      [:added, :after, "Julian"],
+      [:added, :before, "Roger"],
+      [:added, :after, "Roger"]
+    ], log.last(4)
+
+    post.people_with_callbacks.clear
+    assert_equal (%w(Michael David Julian Roger) * 2).sort, log.last(8).collect(&:last).sort
+  end
+end
+
-class HasManyAssociationsTest < ActiveRecord::TestCase
-
+ategories, :c
-           :developers_projects, :topics, :authors, :comments, :author_addresses,
-           :people, :posts
@@ -1292 +1465 @@
-  end
-
-
-
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
-  end
-

trunk/activerecord/test/cases/associations/join_model_test.rb

@@ -444 +444 @@
-  end
-
-
-
-
-
-
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
-  end
-

trunk/activerecord/test/models/post.rb

@@ -47 +47 @@
-  has_many :readers
-  has_many :people, :through => :readers
+  has_many :people_with_callbacks, :source=>:person, :through => :readers,
+              :before_add    => lambda {|owner, reader| log(:added,   :before, reader.first_name) },
+              :after_add     => lambda {|owner, reader| log(:added,   :after,  reader.first_name) },
+              :before_remove => lambda {|owner, reader| log(:removed, :before, reader.first_name) },
+              :after_remove  => lambda {|owner, reader| log(:removed, :after,  reader.first_name) }
+
+  def self.reset_log
+    @log = []
+  end
+  
+  def self.log(message=nil, side=nil, new_record=nil)
+    return @log if message.nil?
+    @log << [message, side, new_record]
+  end
-
-  def self.what_are_you