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]

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