From 63c73dd0214188dc91442db538e141e30ec3b1b9 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 23 Jan 2011 21:29:36 +0000 Subject: We shouldn't be using scoped.scoping { ... } to build associated records, as this can affect validations/callbacks/etc inside the record itself [#6252 state:resolved] --- .../associations/association_collection.rb | 13 +++++-------- .../active_record/associations/singular_association.rb | 15 +++++++++------ .../cases/associations/has_many_associations_test.rb | 18 +++++++++++++++++- .../cases/associations/has_one_associations_test.rb | 15 +++++++++++++++ activerecord/test/models/bulb.rb | 11 +++++++++-- activerecord/test/models/pirate.rb | 2 ++ 6 files changed, 57 insertions(+), 17 deletions(-) diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index 24fb49a65d..a37c3dd432 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -466,17 +466,14 @@ module ActiveRecord force ? record.save! : record.save(:validate => validate) end - def create_record(attrs, &block) + def create_record(attributes, &block) ensure_owner_is_persisted! - - transaction do - scoped.scoping { build_record(attrs, &block) } - end + transaction { build_record(attributes, &block) } end - def build_record(attrs, &block) - attrs.update(@reflection.options[:conditions]) if @reflection.options[:conditions].is_a?(Hash) - record = @reflection.build_association(attrs) + def build_record(attributes, &block) + attributes = scoped.scope_for_create.merge(attributes) + record = @reflection.build_association(attributes) add_record_to_target_with_callbacks(record, &block) end diff --git a/activerecord/lib/active_record/associations/singular_association.rb b/activerecord/lib/active_record/associations/singular_association.rb index b6f49c6f36..b43b52fc52 100644 --- a/activerecord/lib/active_record/associations/singular_association.rb +++ b/activerecord/lib/active_record/associations/singular_association.rb @@ -2,9 +2,7 @@ module ActiveRecord module Associations class SingularAssociation < AssociationProxy #:nodoc: def create(attributes = {}) - record = scoped.scoping { @reflection.create_association(attributes) } - set_new_record(record) - record + new_record(:create, attributes) end def create!(attributes = {}) @@ -12,9 +10,7 @@ module ActiveRecord end def build(attributes = {}) - record = scoped.scoping { @reflection.build_association(attributes) } - set_new_record(record) - record + new_record(:build, attributes) end private @@ -36,6 +32,13 @@ module ActiveRecord raise_on_type_mismatch(record) if record record end + + def new_record(method, attributes) + attributes = scoped.scope_for_create.merge(attributes || {}) + record = @reflection.send("#{method}_association", attributes) + set_new_record(record) + record + end end end end diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb index 1ce91d7211..2ca412d424 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -43,7 +43,7 @@ end class HasManyAssociationsTest < ActiveRecord::TestCase fixtures :accounts, :categories, :companies, :developers, :projects, :developers_projects, :topics, :authors, :comments, - :people, :posts, :readers, :taggings + :people, :posts, :readers, :taggings, :cars def setup Client.destroyed_client_ids.clear @@ -66,6 +66,22 @@ class HasManyAssociationsTest < ActiveRecord::TestCase assert_equal 'exotic', bulb.name end + # When creating objects on the association, we must not do it within a scope (even though it + # would be convenient), because this would cause that scope to be applied to any callbacks etc. + def test_build_and_create_should_not_happen_within_scope + car = cars(:honda) + original_scoped_methods = Bulb.scoped_methods + + bulb = car.bulbs.build + assert_equal original_scoped_methods, bulb.scoped_methods_after_initialize + + bulb = car.bulbs.create + assert_equal original_scoped_methods, bulb.scoped_methods_after_initialize + + bulb = car.bulbs.create! + assert_equal original_scoped_methods, bulb.scoped_methods_after_initialize + end + def test_no_sql_should_be_fired_if_association_already_loaded Car.create(:name => 'honda') bulbs = Car.first.bulbs diff --git a/activerecord/test/cases/associations/has_one_associations_test.rb b/activerecord/test/cases/associations/has_one_associations_test.rb index d9b6694dd8..dbf6dfe20d 100644 --- a/activerecord/test/cases/associations/has_one_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_associations_test.rb @@ -4,6 +4,7 @@ require 'models/project' require 'models/company' require 'models/ship' require 'models/pirate' +require 'models/bulb' class HasOneAssociationsTest < ActiveRecord::TestCase self.use_transactional_fixtures = false unless supports_savepoints? @@ -167,6 +168,20 @@ class HasOneAssociationsTest < ActiveRecord::TestCase assert_equal account, firm.account end + def test_build_and_create_should_not_happen_within_scope + pirate = pirates(:blackbeard) + original_scoped_methods = Bulb.scoped_methods.dup + + bulb = pirate.build_bulb + assert_equal original_scoped_methods, bulb.scoped_methods_after_initialize + + bulb = pirate.create_bulb + assert_equal original_scoped_methods, bulb.scoped_methods_after_initialize + + bulb = pirate.create_bulb! + assert_equal original_scoped_methods, bulb.scoped_methods_after_initialize + end + def test_create_association firm = Firm.create(:name => "GlobalMegaCorp") account = firm.create_account(:credit_limit => 1000) diff --git a/activerecord/test/models/bulb.rb b/activerecord/test/models/bulb.rb index 9eefc5803a..7178bb0d00 100644 --- a/activerecord/test/models/bulb.rb +++ b/activerecord/test/models/bulb.rb @@ -1,7 +1,14 @@ class Bulb < ActiveRecord::Base - + default_scope :conditions => {:name => 'defaulty' } - + belongs_to :car + attr_reader :scoped_methods_after_initialize + + after_initialize :record_scoped_methods_after_initialize + def record_scoped_methods_after_initialize + @scoped_methods_after_initialize = self.class.scoped_methods.dup + end + end diff --git a/activerecord/test/models/pirate.rb b/activerecord/test/models/pirate.rb index b0490f754e..0d3f62bb33 100644 --- a/activerecord/test/models/pirate.rb +++ b/activerecord/test/models/pirate.rb @@ -34,6 +34,8 @@ class Pirate < ActiveRecord::Base :after_remove => proc {|p,b| p.ship_log << "after_removing_proc_bird_#{b.id}"} has_many :birds_with_reject_all_blank, :class_name => "Bird" + has_one :bulb, :foreign_key => :car_id + accepts_nested_attributes_for :parrots, :birds, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? } accepts_nested_attributes_for :ship, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? } accepts_nested_attributes_for :update_only_ship, :update_only => true -- cgit v1.2.3