Merge remote branch 'jonleighton/association_fixes'

* jonleighton/association_fixes:
  Add a transaction wrapper in add_to_target. This means that #build will now also use a transaction. IMO this is reasonable given that the before_add and after_add callbacks might do anything, and this great consistency allows us to abstract out the duplicate code from #build and #create.
  Inline ensure_owner_is_persisted! as it is only called from one place
  @target should always be an array
  Rename add_record_to_target_with_callbacks to add_to_target
  Don't pass the block through build_record
  Move create and create! next to build
  Get rid of create_record as it is not only used in one place
  Get rid of AssociationCollection#save_record
  Fix test/cases/connection_pool_test.rb for sqlite3 in-memory db
  Add interpolation of association conditions back in, in the form of proc { ... } rather than instance_eval-ing strings

11 files changed, 93 insertions, 90 deletions

diff --git a/activerecord/lib/active_record/association_preload.rb b/activerecord/lib/active_record/association_preload.rb
index b83c00e9f8..52d2c49836 100644
--- a/activerecord/lib/active_record/association_preload.rb
+++ b/activerecord/lib/active_record/association_preload.rb
@@ -399,10 +399,18 @@ module ActiveRecord
         end
       end
 
+      def process_conditions(conditions, klass = self)
+        if conditions.respond_to?(:to_proc)
+          conditions = instance_eval(&conditions)
+        end
+
+        klass.send(:sanitize_sql, conditions)
+      end
+
       def append_conditions(reflection, preload_options)
         [
-          ("(#{reflection.sanitized_conditions})" if reflection.sanitized_conditions),
-          ("(#{sanitize_sql preload_options[:conditions]})" if preload_options[:conditions]),
+          ('(' + process_conditions(reflection.options[:conditions], reflection.klass) + ')' if reflection.options[:conditions]),
+          ('(' + process_conditions(preload_options[:conditions]) + ')' if preload_options[:conditions]),
         ].compact.map { |x| Arel.sql x }
       end
 
diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb
index 3e3237c348..ca350f51c9 100644
--- a/activerecord/lib/active_record/associations/association_collection.rb
+++ b/activerecord/lib/active_record/associations/association_collection.rb
@@ -56,14 +56,21 @@ module ActiveRecord
       end
 
       def build(attributes = {}, &block)
-        if attributes.is_a?(Array)
-          attributes.collect { |attr| build(attr, &block) }
-        else
-          build_record(attributes) do |record|
-            block.call(record) if block_given?
-            set_owner_attributes(record)
-          end
+        build_or_create(attributes, :build, &block)
+      end
+
+      def create(attributes = {}, &block)
+        unless @owner.persisted?
+          raise ActiveRecord::RecordNotSaved, "You cannot call create unless the parent is saved"
         end
+
+        build_or_create(attributes, :create, &block)
+      end
+
+      def create!(attrs = {}, &block)
+        record = create(attrs, &block)
+        Array.wrap(record).each(&:save!)
+        record
       end
 
       # Add +records+ to this association.  Returns +self+ so method calls may be chained.
@@ -75,7 +82,7 @@ module ActiveRecord
         transaction do
           records.flatten.each do |record|
             raise_on_type_mismatch(record)
-            add_record_to_target_with_callbacks(record) do |r|
+            add_to_target(record) do |r|
               result &&= insert_record(record) unless @owner.new_record?
             end
           end
@@ -191,24 +198,6 @@ module ActiveRecord
         delete_or_destroy(records, :destroy)
       end
 
-      def create(attrs = {})
-        if attrs.is_a?(Array)
-          attrs.collect { |attr| create(attr) }
-        else
-          create_record(attrs) do |record|
-            yield(record) if block_given?
-            insert_record(record, false)
-          end
-        end
-      end
-
-      def create!(attrs = {})
-        create_record(attrs) do |record|
-          yield(record) if block_given?
-          insert_record(record, true)
-        end
-      end
-
       # Returns the size of the collection by executing a SELECT COUNT(*)
       # query if the collection hasn't been loaded, and calling
       # <tt>collection.size</tt> if it has.
@@ -348,17 +337,21 @@ module ActiveRecord
           target
         end
 
-        def add_record_to_target_with_callbacks(record)
-          callback(:before_add, record)
-          yield(record) if block_given?
-          @target ||= [] unless loaded?
-          if @reflection.options[:uniq] && index = @target.index(record)
-            @target[index] = record
-          else
-            @target << record
+        def add_to_target(record)
+          transaction do
+            callback(:before_add, record)
+            yield(record) if block_given?
+
+            if @reflection.options[:uniq] && index = @target.index(record)
+              @target[index] = record
+            else
+              @target << record
+            end
+
+            callback(:after_add, record)
+            set_inverse_instance(record)
           end
-          callback(:after_add, record)
-          set_inverse_instance(record)
+
           record
         end
 
@@ -374,17 +367,15 @@ module ActiveRecord
 
         def custom_counter_sql
           if @reflection.options[:counter_sql]
-            counter_sql = @reflection.options[:counter_sql]
+            interpolate(@reflection.options[:counter_sql])
           else
             # replace the SELECT clause with COUNT(*), preserving any hints within /* ... */
-            counter_sql = @reflection.options[:finder_sql].sub(/SELECT\b(\/\*.*?\*\/ )?(.*)\bFROM\b/im) { "SELECT #{$1}COUNT(*) FROM" }
+            interpolate(@reflection.options[:finder_sql]).sub(/SELECT\b(\/\*.*?\*\/ )?(.*)\bFROM\b/im) { "SELECT #{$1}COUNT(*) FROM" }
           end
-
-          interpolate_sql(counter_sql)
         end
 
         def custom_finder_sql
-          interpolate_sql(@reflection.options[:finder_sql])
+          interpolate(@reflection.options[:finder_sql])
         end
 
         def find_target
@@ -421,26 +412,26 @@ module ActiveRecord
           end + existing
         end
 
-        # Do the relevant stuff to insert the given record into the association collection. The
-        # force param specifies whether or not an exception should be raised on failure. The
-        # validate param specifies whether validation should be performed (if force is false).
-        def insert_record(record, force = true, validate = true)
-          raise NotImplementedError
-        end
+        def build_or_create(attributes, method)
+          records = Array.wrap(attributes).map do |attrs|
+            record = build_record(attrs)
+
+            add_to_target(record) do
+              yield(record) if block_given?
+              insert_record(record) if method == :create
+            end
+          end
 
-        def save_record(record, force, validate)
-          force ? record.save! : record.save(:validate => validate)
+          attributes.is_a?(Array) ? records : records.first
         end
 
-        def create_record(attributes, &block)
-          ensure_owner_is_persisted!
-          transaction { build_record(attributes, &block) }
+        # Do the relevant stuff to insert the given record into the association collection.
+        def insert_record(record, validate = true)
+          raise NotImplementedError
         end
 
-        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)
+        def build_record(attributes)
+          @reflection.build_association(scoped.scope_for_create.merge(attributes))
         end
 
         def delete_or_destroy(records, method)
@@ -482,12 +473,6 @@ module ActiveRecord
           @owner.class.send(full_callback_name.to_sym) || []
         end
 
-        def ensure_owner_is_persisted!
-          unless @owner.persisted?
-            raise ActiveRecord::RecordNotSaved, "You cannot call create unless the parent is saved"
-          end
-        end
-
         # Should we deal with assoc.first or assoc.last by issuing an independent query to
         # the database, or by getting the target, and then taking the first/last item from that?
         #
diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb
index 2832f49c23..fc03a4b619 100644
--- a/activerecord/lib/active_record/associations/association_proxy.rb
+++ b/activerecord/lib/active_record/associations/association_proxy.rb
@@ -184,7 +184,8 @@ module ActiveRecord
         def association_scope
           scope = target_klass.unscoped
           scope = scope.create_with(creation_attributes)
-          scope = scope.apply_finder_options(@reflection.options.slice(:conditions, :readonly, :include))
+          scope = scope.apply_finder_options(@reflection.options.slice(:readonly, :include))
+          scope = scope.where(interpolate(@reflection.options[:conditions]))
           if select = select_value
             scope = scope.select(select)
           end
@@ -240,8 +241,12 @@ module ActiveRecord
           !loaded? && (!@owner.new_record? || foreign_key_present?) && target_klass
         end
 
-        def interpolate_sql(sql, record = nil)
-          @owner.send(:interpolate_sql, sql, record)
+        def interpolate(sql, record = nil)
+          if sql.respond_to?(:to_proc)
+            @owner.send(:instance_exec, record, &sql)
+          else
+            sql
+          end
         end
 
         def select_value
diff --git a/activerecord/lib/active_record/associations/class_methods/join_dependency/join_association.rb b/activerecord/lib/active_record/associations/class_methods/join_dependency/join_association.rb
index 856d826d67..aaa475109e 100644
--- a/activerecord/lib/active_record/associations/class_methods/join_dependency/join_association.rb
+++ b/activerecord/lib/active_record/associations/class_methods/join_dependency/join_association.rb
@@ -108,6 +108,10 @@ module ActiveRecord
           end
 
           def process_conditions(conditions, table_name)
+            if conditions.respond_to?(:to_proc)
+              conditions = instance_eval(&conditions)
+            end
+
             Arel.sql(sanitize_sql(conditions, table_name))
           end
 
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 b0ccab2de6..b9c9919e7a 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
@@ -11,13 +11,11 @@ module ActiveRecord
 
       protected
 
-        def insert_record(record, force = true, validate = true)
-          if record.new_record?
-            return false unless save_record(record, force, validate)
-          end
+        def insert_record(record, validate = true)
+          return if record.new_record? && !record.save(:validate => validate)
 
           if @reflection.options[:insert_sql]
-            @owner.connection.insert(interpolate_sql(@reflection.options[:insert_sql], record))
+            @owner.connection.insert(interpolate(@reflection.options[:insert_sql], record))
           else
             stmt = join_table.compile_insert(
               join_table[@reflection.foreign_key]             => @owner.id,
@@ -27,7 +25,7 @@ module ActiveRecord
             @owner.connection.insert stmt.to_sql
           end
 
-          true
+          record
         end
 
         def association_scope
@@ -42,7 +40,7 @@ module ActiveRecord
 
         def delete_records(records, method)
           if sql = @reflection.options[:delete_sql]
-            records.each { |record| @owner.connection.delete(interpolate_sql(sql, record)) }
+            records.each { |record| @owner.connection.delete(interpolate(sql, record)) }
           else
             relation = join_table
             stmt = relation.where(relation[@reflection.foreign_key].eq(@owner.id).
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index 0b246587c0..543b073393 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -8,9 +8,9 @@ module ActiveRecord
     class HasManyAssociation < AssociationCollection #:nodoc:
       protected
 
-        def insert_record(record, force = false, validate = true)
+        def insert_record(record, validate = true)
           set_owner_attributes(record)
-          save_record(record, force, validate)
+          record.save(:validate => validate)
         end
 
       private
diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb
index f299f51466..9f74d57c4d 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -22,12 +22,21 @@ module ActiveRecord
         end
       end
 
+      def <<(*records)
+        unless @owner.new_record?
+          records.flatten.each do |record|
+            raise_on_type_mismatch(record)
+            record.save! if record.new_record?
+          end
+        end
+
+        super
+      end
+
       protected
 
-        def insert_record(record, force = true, validate = true)
-          if record.new_record?
-            return unless save_record(record, force, validate)
-          end
+        def insert_record(record, validate = true)
+          return if record.new_record? && !record.save(:validate => validate)
 
           through_association = @owner.send(@reflection.through_reflection.name)
           through_association.create!(construct_join_attributes(record))
diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb
index ab593d09b9..0550bae408 100644
--- a/activerecord/lib/active_record/associations/through_association.rb
+++ b/activerecord/lib/active_record/associations/through_association.rb
@@ -119,14 +119,14 @@ module ActiveRecord
             scope = scope.where(@reflection.through_reflection.klass.send(:type_condition))
           end
 
-          scope = scope.where(@reflection.source_reflection.options[:conditions])
+          scope = scope.where(interpolate(@reflection.source_reflection.options[:conditions]))
           scope.where(through_conditions)
         end
 
         # If there is a hash of conditions then we make sure the keys are scoped to the
         # through table name if left ambiguous.
         def through_conditions
-          conditions = @reflection.through_reflection.options[:conditions]
+          conditions = interpolate(@reflection.through_reflection.options[:conditions])
 
           if conditions.is_a?(Hash)
             Hash[conditions.map { |key, value|
diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index c52af23fbd..95357ee5dc 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -312,7 +312,7 @@ module ActiveRecord
               association.destroy(record)
             elsif autosave != false && (@new_record_before_save || record.new_record?)
               if autosave
-                saved = association.send(:insert_record, record, false, false)
+                saved = association.send(:insert_record, record, false)
               else
                 association.send(:insert_record, record)
               end
diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index 16bdd7bb58..3d48ab89ac 100644
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -1790,12 +1790,6 @@ MSG
         self.class.connection.quote(value, column)
       end
 
-      # Interpolate custom SQL string in instance context.
-      # Optional record argument is meant for custom insert_sql.
-      def interpolate_sql(sql, record = nil)
-        instance_eval("%@#{sql.gsub('@', '\@')}@", __FILE__, __LINE__)
-      end
-
       # Instantiates objects for all attribute classes that needs more than one constructor parameter. This is done
       # by calling new on the column type or aggregation type (through composed_of) object with these parameters.
       # So having the pairs written_on(1) = "2004", written_on(2) = "6", written_on(3) = "24", will instantiate
diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb
index 16023defe3..fc68d7254a 100644
--- a/activerecord/lib/active_record/nested_attributes.rb
+++ b/activerecord/lib/active_record/nested_attributes.rb
@@ -413,7 +413,7 @@ module ActiveRecord
             if target_record
               existing_record = target_record
             else
-              association.send(:add_record_to_target_with_callbacks, existing_record)
+              association.send(:add_to_target, existing_record)
             end
           end