Merge branch 'master' of github.com:rails/rails

12 files changed, 90 insertions, 49 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 5cefb5dfd7..d101b81e28 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,7 @@
+*   Bump minimum SQLite version to 3.8
+
+    *Yasuo Honda*
+
 *   Fix parent record should not get saved with duplicate children records.
 
     Fixes #32940.
diff --git a/activerecord/lib/active_record/associations/association.rb b/activerecord/lib/active_record/associations/association.rb
index d2934c830a..44596f4424 100644
--- a/activerecord/lib/active_record/associations/association.rb
+++ b/activerecord/lib/active_record/associations/association.rb
@@ -275,6 +275,7 @@ module ActiveRecord
         def build_record(attributes)
           reflection.build_association(attributes) do |record|
             initialize_attributes(record, attributes)
+            yield(record) if block_given?
           end
         end
 
diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb
index d61d105544..7f1df9a21d 100644
--- a/activerecord/lib/active_record/associations/collection_association.rb
+++ b/activerecord/lib/active_record/associations/collection_association.rb
@@ -105,9 +105,7 @@ module ActiveRecord
         if attributes.is_a?(Array)
           attributes.collect { |attr| build(attr, &block) }
         else
-          add_to_target(build_record(attributes)) do |record|
-            yield(record) if block_given?
-          end
+          add_to_target(build_record(attributes, &block))
         end
       end
 
@@ -361,8 +359,7 @@ module ActiveRecord
             attributes.collect { |attr| _create_record(attr, raise, &block) }
           else
             transaction do
-              add_to_target(build_record(attributes)) do |record|
-                yield(record) if block_given?
+              add_to_target(build_record(attributes, &block)) do |record|
                 insert_record(record, true, raise) {
                   @_was_loaded = loaded?
                   @association_ids = nil
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 59929b8c4e..617956c768 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -90,7 +90,7 @@ module ActiveRecord
         def build_record(attributes)
           ensure_not_nested
 
-          record = super(attributes)
+          record = super
 
           inverse = source_reflection.inverse_of
           if inverse
diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb
index d211884135..390bfd8b08 100644
--- a/activerecord/lib/active_record/associations/has_one_association.rb
+++ b/activerecord/lib/active_record/associations/has_one_association.rb
@@ -106,7 +106,7 @@ module ActiveRecord
           end
         end
 
-        def _create_record(attributes, raise_error = false)
+        def _create_record(attributes, raise_error = false, &block)
           unless owner.persisted?
             raise ActiveRecord::RecordNotSaved, "You cannot call create unless the parent is saved"
           end
diff --git a/activerecord/lib/active_record/associations/singular_association.rb b/activerecord/lib/active_record/associations/singular_association.rb
index ead89bfe6c..cfab16a745 100644
--- a/activerecord/lib/active_record/associations/singular_association.rb
+++ b/activerecord/lib/active_record/associations/singular_association.rb
@@ -17,9 +17,8 @@ module ActiveRecord
         replace(record)
       end
 
-      def build(attributes = {})
-        record = build_record(attributes)
-        yield(record) if block_given?
+      def build(attributes = {}, &block)
+        record = build_record(attributes, &block)
         set_new_record(record)
         record
       end
@@ -62,9 +61,8 @@ module ActiveRecord
           replace(record)
         end
 
-        def _create_record(attributes, raise_error = false)
-          record = build_record(attributes)
-          yield(record) if block_given?
+        def _create_record(attributes, raise_error = false, &block)
+          record = build_record(attributes, &block)
           saved = record.save
           set_new_record(record)
           raise RecordInvalid.new(record) if !saved && raise_error
diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb
index 5afb0bc068..15e6565e69 100644
--- a/activerecord/lib/active_record/associations/through_association.rb
+++ b/activerecord/lib/active_record/associations/through_association.rb
@@ -114,7 +114,7 @@ module ActiveRecord
             attributes[inverse.foreign_key] = target.id
           end
 
-          super(attributes)
+          super
         end
     end
   end
diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb
index 22d195c9a4..6d2f75a3ae 100644
--- a/activerecord/lib/active_record/reflection.rb
+++ b/activerecord/lib/active_record/reflection.rb
@@ -13,33 +13,37 @@ module ActiveRecord
       class_attribute :aggregate_reflections, instance_writer: false, default: {}
     end
 
-    def self.create(macro, name, scope, options, ar)
-      klass = \
-        case macro
-        when :composed_of
-          AggregateReflection
-        when :has_many
-          HasManyReflection
-        when :has_one
-          HasOneReflection
-        when :belongs_to
-          BelongsToReflection
-        else
-          raise "Unsupported Macro: #{macro}"
-        end
+    class << self
+      def create(macro, name, scope, options, ar)
+        reflection = reflection_class_for(macro).new(name, scope, options, ar)
+        options[:through] ? ThroughReflection.new(reflection) : reflection
+      end
 
-      reflection = klass.new(name, scope, options, ar)
-      options[:through] ? ThroughReflection.new(reflection) : reflection
-    end
+      def add_reflection(ar, name, reflection)
+        ar.clear_reflections_cache
+        name = name.to_s
+        ar._reflections = ar._reflections.except(name).merge!(name => reflection)
+      end
 
-    def self.add_reflection(ar, name, reflection)
-      ar.clear_reflections_cache
-      name = name.to_s
-      ar._reflections = ar._reflections.except(name).merge!(name => reflection)
-    end
+      def add_aggregate_reflection(ar, name, reflection)
+        ar.aggregate_reflections = ar.aggregate_reflections.merge(name.to_s => reflection)
+      end
 
-    def self.add_aggregate_reflection(ar, name, reflection)
-      ar.aggregate_reflections = ar.aggregate_reflections.merge(name.to_s => reflection)
+      private
+        def reflection_class_for(macro)
+          case macro
+          when :composed_of
+            AggregateReflection
+          when :has_many
+            HasManyReflection
+          when :has_one
+            HasOneReflection
+          when :belongs_to
+            BelongsToReflection
+          else
+            raise "Unsupported Macro: #{macro}"
+          end
+        end
     end
 
     # \Reflection enables the ability to examine the associations and aggregations of
diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb
index 82adb19f5b..c5d5fca672 100644
--- a/activerecord/lib/active_record/transactions.rb
+++ b/activerecord/lib/active_record/transactions.rb
@@ -328,10 +328,12 @@ module ActiveRecord
     # but call it after the commit of a destroyed object.
     def committed!(should_run_callbacks: true) #:nodoc:
       if should_run_callbacks && (destroyed? || persisted?)
+        @_committed_already_called = true
         _run_commit_without_transaction_enrollment_callbacks
         _run_commit_callbacks
       end
     ensure
+      @_committed_already_called = false
       force_clear_transaction_record_state
     end
 
@@ -380,6 +382,7 @@ module ActiveRecord
     end
 
     private
+      attr_reader :_committed_already_called, :_trigger_update_callback, :_trigger_destroy_callback
 
       # Save the new record state and id of a record so it can be restored later if a transaction fails.
       def remember_transaction_record_state
@@ -390,6 +393,15 @@ module ActiveRecord
           frozen?: frozen?,
         )
         @_start_transaction_state[:level] = (@_start_transaction_state[:level] || 0) + 1
+        remember_new_record_before_last_commit
+      end
+
+      def remember_new_record_before_last_commit
+        if _committed_already_called
+          @_new_record_before_last_commit = false
+        else
+          @_new_record_before_last_commit = @_start_transaction_state[:new_record]
+        end
       end
 
       # Clear the new record state and id of a record.
@@ -421,22 +433,16 @@ module ActiveRecord
         end
       end
 
-      # Determine if a record was created or destroyed in a transaction. State should be one of :new_record or :destroyed.
-      def transaction_record_state(state)
-        @_start_transaction_state[state]
-      end
-
       # Determine if a transaction included an action for :create, :update, or :destroy. Used in filtering callbacks.
       def transaction_include_any_action?(actions)
         actions.any? do |action|
           case action
           when :create
-            transaction_record_state(:new_record)
-          when :destroy
-            defined?(@_trigger_destroy_callback) && @_trigger_destroy_callback
+            persisted? && @_new_record_before_last_commit
           when :update
-            !(transaction_record_state(:new_record) || destroyed?) &&
-              (defined?(@_trigger_update_callback) && @_trigger_update_callback)
+            !(@_new_record_before_last_commit || destroyed?) && _trigger_update_callback
+          when :destroy
+            _trigger_destroy_callback
           end
         end
       end
diff --git a/activerecord/test/cases/adapters/mysql2/virtual_column_test.rb b/activerecord/test/cases/adapters/mysql2/virtual_column_test.rb
index ffde8ed4d8..8494acee3b 100644
--- a/activerecord/test/cases/adapters/mysql2/virtual_column_test.rb
+++ b/activerecord/test/cases/adapters/mysql2/virtual_column_test.rb
@@ -18,6 +18,7 @@ if ActiveRecord::Base.connection.supports_virtual_columns?
         t.string  :name
         t.virtual :upper_name,  type: :string,  as: "UPPER(`name`)"
         t.virtual :name_length, type: :integer, as: "LENGTH(`name`)", stored: true
+        t.virtual :name_octet_length, type: :integer, as: "OCTET_LENGTH(`name`)", stored: true
       end
       VirtualColumn.create(name: "Rails")
     end
@@ -55,7 +56,8 @@ if ActiveRecord::Base.connection.supports_virtual_columns?
     def test_schema_dumping
       output = dump_table_schema("virtual_columns")
       assert_match(/t\.virtual\s+"upper_name",\s+type: :string,\s+as: "(?:UPPER|UCASE)\(`name`\)"$/i, output)
-      assert_match(/t\.virtual\s+"name_length",\s+type: :integer,\s+as: "LENGTH\(`name`\)",\s+stored: true$/i, output)
+      assert_match(/t\.virtual\s+"name_length",\s+type: :integer,\s+as: "(?:octet_length|length)\(`name`\)",\s+stored: true$/i, output)
+      assert_match(/t\.virtual\s+"name_octet_length",\s+type: :integer,\s+as: "(?:octet_length|length)\(`name`\)",\s+stored: true$/i, output)
     end
   end
 end
diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb
index 0facc286da..46fa36d7dd 100644
--- a/activerecord/test/cases/associations/has_many_through_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -737,6 +737,18 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
       [:added, :before, "Roger"],
       [:added, :after, "Roger"]
     ], log.last(4)
+
+    post.people_with_callbacks.build { |person| person.first_name = "Ted" }
+    assert_equal [
+      [:added, :before, "Ted"],
+      [:added, :after, "Ted"]
+    ], log.last(2)
+
+    post.people_with_callbacks.create { |person| person.first_name = "Sam" }
+    assert_equal [
+      [:added, :before, "Sam"],
+      [:added, :after, "Sam"]
+    ], log.last(2)
   end
 
   def test_dynamic_find_should_respect_association_include
diff --git a/activerecord/test/cases/transaction_callbacks_test.rb b/activerecord/test/cases/transaction_callbacks_test.rb
index 05941c75ac..c0be45eee7 100644
--- a/activerecord/test/cases/transaction_callbacks_test.rb
+++ b/activerecord/test/cases/transaction_callbacks_test.rb
@@ -139,6 +139,23 @@ class TransactionCallbacksTest < ActiveRecord::TestCase
     assert_equal [], reply.history
   end
 
+  def test_only_call_after_commit_on_destroy_after_transaction_commits_for_destroyed_new_record
+    new_record = TopicWithCallbacks.new(title: "New topic", written_on: Date.today)
+    add_transaction_execution_blocks new_record
+
+    new_record.destroy
+    assert_equal [:commit_on_destroy], new_record.history
+  end
+
+  def test_save_in_after_create_commit_wont_invoke_extra_after_create_commit
+    new_record = TopicWithCallbacks.new(title: "New topic", written_on: Date.today)
+    add_transaction_execution_blocks new_record
+    new_record.after_commit_block(:create) { |r| r.save! }
+
+    new_record.save!
+    assert_equal [:commit_on_create, :commit_on_update], new_record.history
+  end
+
   def test_only_call_after_commit_on_create_and_doesnt_leaky
     r = ReplyWithCallbacks.new(content: "foo")
     r.save_on_after_create = true