diff options
8 files changed, 94 insertions, 45 deletions
diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index f9a9c58b60..8a3331a0ff 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -1,5 +1,7 @@ *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] * Alias association #build to #new so it behaves predictably. #8787 [lifofifo] diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index 5598d6daa7..5e2e2e0189 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -85,19 +85,15 @@ module ActiveRecord end 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 - record + if attrs.is_a?(Array) + attrs.collect { |attr| create(attr) } + else + create_record(attrs) { |record| record.save } + end end 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 - record + create_record(attrs) { |record| record.save! } end # Returns the size of the collection by executing a SELECT COUNT(*) query if the collection hasn't been loaded and @@ -189,6 +185,27 @@ module ActiveRecord end 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| case callback 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 d67207fa4d..13c99455c0 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 @@ -8,33 +8,15 @@ module ActiveRecord def build(attributes = {}) load_target - record = @reflection.klass.new(attributes) - @target << record - record + build_record(attributes) end def create(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) - insert_record(record) unless @owner.new_record? - record - end + create_record(attributes) { |record| insert_record(record) } end def create!(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) - insert_record(record, true) unless @owner.new_record? - record - end + create_record(attributes) { |record| insert_record(record, true) } end def find_first @@ -160,6 +142,19 @@ module ActiveRecord def finding_with_ambiguous_select?(select_clause) !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 end diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index 282b43a2b7..b1b9895832 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -10,13 +10,7 @@ module ActiveRecord if attributes.is_a?(Array) attributes.collect { |attr| build(attr) } else - record = @reflection.klass.new(attributes) - set_belongs_to_association_for(record) - - @target ||= [] unless loaded? - @target << record - - record + build_record(attributes) { |record| set_belongs_to_association_for(record) } end end diff --git a/activerecord/test/associations/callbacks_test.rb b/activerecord/test/associations/callbacks_test.rb index afb1b8e84d..4b6b7f4123 100644 --- a/activerecord/test/associations/callbacks_test.rb +++ b/activerecord/test/associations/callbacks_test.rb @@ -60,6 +60,29 @@ class AssociationCallbacksTest < Test::Unit::TestCase "after_adding#{@thinking.id}", "after_adding_proc#{@thinking.id}"], @david.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_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) ar = projects(:active_record) @@ -98,6 +121,17 @@ class AssociationCallbacksTest < Test::Unit::TestCase assert_equal log_array, activerecord.developers_log.sort 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) begin diff --git a/activerecord/test/associations_test.rb b/activerecord/test/associations_test.rb index 9e4d0f2711..f912816f33 100755 --- a/activerecord/test/associations_test.rb +++ b/activerecord/test/associations_test.rb @@ -595,6 +595,13 @@ class HasManyAssociationsTest < Test::Unit::TestCase end 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 Developer.new("name" => "Aredridel").projects.create! diff --git a/activerecord/test/fixtures/author.rb b/activerecord/test/fixtures/author.rb index fa710bab9c..5ef0c6cfe9 100644 --- a/activerecord/test/fixtures/author.rb +++ b/activerecord/test/fixtures/author.rb @@ -33,13 +33,13 @@ class Author < ActiveRecord::Base :before_remove => :log_before_removing, :after_remove => :log_after_removing has_many :posts_with_proc_callbacks, :class_name => "Post", - :before_add => Proc.new {|o, r| o.post_log << "before_adding#{r.id}"}, - :after_add => Proc.new {|o, r| o.post_log << "after_adding#{r.id}"}, + :before_add => Proc.new {|o, r| o.post_log << "before_adding#{r.id || '<new>'}"}, + :after_add => Proc.new {|o, r| o.post_log << "after_adding#{r.id || '<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", - :before_add => [:log_before_adding, Proc.new {|o, r| o.post_log << "before_adding_proc#{r.id}"}], - :after_add => [:log_after_adding, Proc.new {|o, r| o.post_log << "after_adding_proc#{r.id}"}] + :before_add => [:log_before_adding, Proc.new {|o, r| o.post_log << "before_adding_proc#{r.id || '<new>'}"}], + :after_add => [:log_after_adding, Proc.new {|o, r| o.post_log << "after_adding_proc#{r.id || '<new>'}"}] has_many :unchangable_posts, :class_name => "Post", :before_add => :raise_exception, :after_add => :log_after_adding has_many :categorizations @@ -74,7 +74,7 @@ class Author < ActiveRecord::Base private def log_before_adding(object) - @post_log << "before_adding#{object.id}" + @post_log << "before_adding#{object.id || '<new>'}" end def log_after_adding(object) diff --git a/activerecord/test/fixtures/project.rb b/activerecord/test/fixtures/project.rb index 2079dc9b58..2538b99d00 100644 --- a/activerecord/test/fixtures/project.rb +++ b/activerecord/test/fixtures/project.rb @@ -7,8 +7,8 @@ class Project < ActiveRecord::Base has_and_belongs_to_many :salaried_developers, :class_name => "Developer", :conditions => "salary > 0" 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}" - has_and_belongs_to_many :developers_with_callbacks, :class_name => "Developer", :before_add => Proc.new {|o, r| o.developers_log << "before_adding#{r.id}"}, - :after_add => Proc.new {|o, r| o.developers_log << "after_adding#{r.id}"}, + has_and_belongs_to_many :developers_with_callbacks, :class_name => "Developer", :before_add => Proc.new {|o, r| o.developers_log << "before_adding#{r.id || '<new>'}"}, + :after_add => Proc.new {|o, r| o.developers_log << "after_adding#{r.id || '<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}"} |