Changeset 7935

Timestamp:

10/16/07 05:07:58 (11 months ago)

Author:

bitsweat

Message:

Refactor association create and build so before & after callbacks behave consistently. Closes #8854.

Files:

• trunk/activerecord/CHANGELOG (modified) (1 diff)
• trunk/activerecord/lib/active_record/associations/association_collection.rb (modified) (2 diffs)
• trunk/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb (modified) (2 diffs)
• trunk/activerecord/lib/active_record/associations/has_many_association.rb (modified) (1 diff)
• trunk/activerecord/test/associations_test.rb (modified) (1 diff)
• trunk/activerecord/test/associations/callbacks_test.rb (modified) (2 diffs)
• trunk/activerecord/test/fixtures/author.rb (modified) (2 diffs)
• trunk/activerecord/test/fixtures/project.rb (modified) (1 diff)

trunk/activerecord/CHANGELOG

@@ -1 +1 @@
-*SVN*
+
+* Refactor association create and build so before & after callbacks behave consistently.  #8854 [lifofifo, mortent]
-
-* Quote table names. Defaults to column quoting.  #4593 [Justin Lynn, gwcoffey, eadz, Dmitry V. Sabanin, Jeremy Kemper]

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

@@ -86 +86 @@
-      
-      def create(attrs = {})
-ensure_owner_is_not_new
-record = @reflection.klass.send(:with_scope, :create => construct_scope[:create]) { @reflection.klass.create(attrs) }                
-@target ||= [] unless loaded?
-@target << record 
-recor
+if attrs.is_a?(Array)
+  attrs.collect { |attr| create(attr) }
+else
+  create_record(attrs) { |record| record.save }
+en
-      end
-
-      def create!(attrs = {})
-
-
-
-
-
+
-      end
-
@@ -190 +186 @@
-
-      private
+
+        def create_record(attrs, &block)
+          ensure_owner_is_not_new
+          record = @reflection.klass.send(:with_scope, :create => construct_scope[:create]) { @reflection.klass.new(attrs) }
+          add_record_to_target_with_callbacks(record, &block)
+        end
+
+        def build_record(attrs, &block)
+          record = @reflection.klass.new(attrs)
+          add_record_to_target_with_callbacks(record, &block)
+        end
+
+        def add_record_to_target_with_callbacks(record)
+          callback(:before_add, record)
+          yield(record) if block_given?
+          @target ||= [] unless loaded?
+          @target << record
+          callback(:after_add, record)
+          record
+        end
+
-        def callback(method, record)
-          callbacks_for(method).each do |callback|

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

@@ -9 +9 @@
-      def build(attributes = {})
-        load_target
-
-
-
+
-      end
-
-      def create(attributes = {})
-
-
-
-
-
-
-
-
-
+
-      end
-      
-      def create!(attributes = {})
-
-
-
-
-
-
-
-
-
+
-      end
-
@@ -161 +143 @@
-          !select_clause && @owner.connection.columns(@reflection.options[:join_table], "Join Table Columns").size != 2
-        end
+
+      private
+        def create_record(attributes)
+          # Can't use Base.create because the foreign key may be a protected attribute.
+          ensure_owner_is_not_new
+          if attributes.is_a?(Array)
+            attributes.collect { |attr| create(attr) }
+          else
+            record = build(attributes)
+            yield(record)
+            record
+          end
+        end
-    end
-  end

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

@@ -11 +11 @@
-          attributes.collect { |attr| build(attr) }
-        else
-
-
-
-
-
-
-
+
-        end
-      end

trunk/activerecord/test/associations_test.rb

@@ -596 +596 @@
-  end
-  
+  def test_create_with_bang_on_has_many_raises_when_record_not_saved
+    assert_raises(ActiveRecord::RecordInvalid) do
+      firm = Firm.find(:first)
+      firm.plain_clients.create!
+    end
+  end
+
-  def test_create_with_bang_on_habtm_when_parent_is_new_raises
-    assert_raises(ActiveRecord::RecordNotSaved) do 

trunk/activerecord/test/associations/callbacks_test.rb

@@ -61 +61 @@
-  end
-
+  def test_has_many_callbacks_with_create
+    morten = Author.create :name => "Morten"
+    post = morten.posts_with_proc_callbacks.create! :title => "Hello", :body => "How are you doing?"
+    assert_equal ["before_adding<new>", "after_adding#{post.id}"], morten.post_log
+  end
+
+  def test_has_many_callbacks_with_create!
+    morten = Author.create! :name => "Morten"
+    post = morten.posts_with_proc_callbacks.create :title => "Hello", :body => "How are you doing?"
+    assert_equal ["before_adding<new>", "after_adding#{post.id}"], morten.post_log
+  end
+
+  def test_has_many_callbacks_for_save_on_parent
+    jack = Author.new :name => "Jack"
+    post = jack.posts_with_callbacks.build :title => "Call me back!", :body => "Before you wake up and after you sleep"
+
+    callback_log = ["before_adding<new>", "after_adding#{jack.posts_with_callbacks.first.id}"]
+    assert_equal callback_log, jack.post_log
+    assert jack.save
+    assert_equal 1, jack.posts_with_callbacks.count
+    assert_equal callback_log, jack.post_log
+  end
+
-  def test_has_and_belongs_to_many_add_callback
-    david = developers(:david)
@@ -99 +122 @@
-  end
-
+  def test_has_many_and_belongs_to_many_callbacks_for_save_on_parent
+    project = Project.new :name => "Callbacks"
+    project.developers_with_callbacks.build :name => "Jack", :salary => 95000
+
+    callback_log = ["before_adding<new>", "after_adding<new>"]
+    assert_equal callback_log, project.developers_log
+    assert project.save
+    assert_equal 1, project.developers_with_callbacks.count
+    assert_equal callback_log, project.developers_log
+  end
+
-  def test_dont_add_if_before_callback_raises_exception
-    assert !@david.unchangable_posts.include?(@authorless)

trunk/activerecord/test/fixtures/author.rb

@@ -34 +34 @@
-           :after_remove  => :log_after_removing
-  has_many :posts_with_proc_callbacks, :class_name => "Post",
-
-
+ || '<new>'
+ || '<new>'
-           :before_remove => Proc.new {|o, r| o.post_log << "before_removing#{r.id}"},
-           :after_remove  => Proc.new {|o, r| o.post_log << "after_removing#{r.id}"}
-  has_many :posts_with_multiple_callbacks, :class_name => "Post",
-
-
+ || '<new>'
+ || '<new>'
-  has_many :unchangable_posts, :class_name => "Post", :before_add => :raise_exception, :after_add => :log_after_adding
-
@@ -75 +75 @@
-  private
-    def log_before_adding(object)
-
+ || '<new>'
-    end
-

trunk/activerecord/test/fixtures/project.rb

@@ -8 +8 @@
-  has_and_belongs_to_many :developers_with_finder_sql, :class_name => "Developer", :finder_sql => 'SELECT t.*, j.* FROM developers_projects j, developers t WHERE t.id = j.developer_id AND j.project_id = #{id}'
-  has_and_belongs_to_many :developers_by_sql, :class_name => "Developer", :delete_sql => "DELETE FROM developers_projects WHERE project_id = \#{id} AND developer_id = \#{record.id}"
-
-}"}, 
+ || '<new>'
+ || '<new>'}"},
-                            :before_remove => Proc.new {|o, r| o.developers_log << "before_removing#{r.id}"},
-                            :after_remove => Proc.new {|o, r| o.developers_log << "after_removing#{r.id}"}