Identify directly-uploaded blobs before saving the associated record

An Active Storage `Blob` must be identified before it can be reliably
validated.  For direct uploads, a `Blob` is identified when it is
attached, rather than when it is created.

Before this commit, the sequence of events when attaching a `Blob` was:

1. Find the `Blob`.
2. Assign the `Blob` to an `Attachment`.
3. Save the owner record.
4. Save the `Attachment`.
5. Identify the `Blob`'s true `content_type` from its file.
6. Save the `Blob`.

This meant that the owner record's validations might not see the
`Blob`'s true `content_type`.

After this commit, the sequence of events will be:

1. Find the `Blob`.
2. Identify the `Blob`'s true `content_type` from its file.
3. Assign the `Blob` to an `Attachment`.
4. Save the owner record.
5. Save the `Attachment`.
6. Save the `Blob`.

Thus the `Blob`'s true `content_type` will be available when running the
owner record's validations.

activestorage/app/models/active_storage/attachment.rb

@@ -11,12 +11,12 @@ class ActiveStorage::Attachment < ActiveRecord::Base
   self.table_name = "active_storage_attachments"
 
   belongs_to :record, polymorphic: true, touch: true
-  belongs_to :blob, class_name: "ActiveStorage::Blob"
+  belongs_to :blob, class_name: "ActiveStorage::Blob", autosave: true
 
   delegate_missing_to :blob
   delegate :signed_id, to: :blob
 
-  after_create_commit :mirror_blob_later, :analyze_blob_later, :identify_blob
+  after_create_commit :mirror_blob_later, :analyze_blob_later
   after_destroy_commit :purge_dependent_blob_later
 
   # Synchronously deletes the attachment and {purges the blob}[rdoc-ref:ActiveStorage::Blob#purge].
@@ -38,10 +38,6 @@ def purge_later
   end
 
   private
-    def identify_blob
-      blob.identify
-    end
-
     def analyze_blob_later
       blob.analyze_later unless blob.analyzed?
     end

activestorage/app/models/active_storage/blob.rb

@@ -53,6 +53,8 @@ class ActiveStorage::Blob < ActiveRecord::Base
     self.service_name ||= self.class.service.name
   end
 
+  after_commit :update_service_metadata, if: :content_type_previously_changed?
+
   before_destroy(prepend: true) do
     raise ActiveRecord::InvalidForeignKey if attachments.exists?
   end
@@ -326,6 +328,10 @@ def service_metadata
         { content_type: content_type }
       end
     end
+
+    def update_service_metadata
+      service.update_metadata key, **service_metadata if service_metadata.any?
+    end
 end
 
 ActiveSupport.run_load_hooks :active_storage_blob, ActiveStorage::Blob

activestorage/app/models/active_storage/blob/identifiable.rb

@@ -2,9 +2,14 @@
 
 module ActiveStorage::Blob::Identifiable
   def identify
+    identify_without_saving
+    save!
+  end
+
+  def identify_without_saving
     unless identified?
-      update! content_type: identify_content_type, identified: true
-      update_service_metadata
+      self.content_type = identify_content_type
+      self.identified = true
     end
   end
 
@@ -24,8 +29,4 @@ def download_identifiable_chunk
         ""
       end
     end
-
-    def update_service_metadata
-      service.update_metadata key, **service_metadata if service_metadata.any?
-    end
 end

activestorage/lib/active_storage/attached/changes/create_many.rb

@@ -6,6 +6,7 @@ class Attached::Changes::CreateMany #:nodoc:
 
     def initialize(name, record, attachables)
       @name, @record, @attachables = name, record, Array(attachables)
+      blobs.each(&:identify_without_saving)
     end
 
     def attachments

activestorage/lib/active_storage/attached/changes/create_one.rb

@@ -9,6 +9,7 @@ class Attached::Changes::CreateOne #:nodoc:
 
     def initialize(name, record, attachable)
       @name, @record, @attachable = name, record, attachable
+      blob.identify_without_saving
     end
 
     def attachment

activestorage/lib/active_storage/attached/many.rb

@@ -29,7 +29,8 @@ def blobs
     #   document.images.attach([ first_blob, second_blob ])
     def attach(*attachables)
       if record.persisted? && !record.changed?
-        record.update(name => blobs + attachables.flatten)
+        record.public_send("#{name}=", blobs + attachables.flatten)
+        record.save
       else
         record.public_send("#{name}=", (change&.attachables || blobs) + attachables.flatten)
       end

activestorage/lib/active_storage/attached/one.rb

@@ -29,7 +29,8 @@ def blank?
     #   person.avatar.attach(avatar_blob) # ActiveStorage::Blob object
     def attach(attachable)
       if record.persisted? && !record.changed?
-        record.update(name => attachable)
+        record.public_send("#{name}=", attachable)
+        record.save
       else
         record.public_send("#{name}=", attachable)
       end

activestorage/test/models/attachment_test.rb

@@ -2,6 +2,7 @@
 
 require "test_helper"
 require "database/setup"
+require "active_support/testing/method_call_assertions"
 
 class ActiveStorage::AttachmentTest < ActiveSupport::TestCase
   include ActiveJob::TestHelper
@@ -50,6 +51,38 @@ class ActiveStorage::AttachmentTest < ActiveSupport::TestCase
     end
   end
 
+  test "directly-uploaded blob identification for one attached occurs before validation" do
+    blob = directly_upload_file_blob(filename: "racecar.jpg", content_type: "application/octet-stream")
+
+    assert_blob_identified_before_owner_validated(@user, blob, "image/jpeg") do
+      @user.avatar.attach(blob)
+    end
+  end
+
+  test "directly-uploaded blob identification for many attached occurs before validation" do
+    blob = directly_upload_file_blob(filename: "racecar.jpg", content_type: "application/octet-stream")
+
+    assert_blob_identified_before_owner_validated(@user, blob, "image/jpeg") do
+      @user.highlights.attach(blob)
+    end
+  end
+
+  test "directly-uploaded blob identification for one attached occurs outside transaction" do
+    blob = directly_upload_file_blob(filename: "racecar.jpg")
+
+    assert_blob_identified_outside_transaction(blob) do
+      @user.avatar.attach(blob)
+    end
+  end
+
+  test "directly-uploaded blob identification for many attached occurs outside transaction" do
+    blob = directly_upload_file_blob(filename: "racecar.jpg")
+
+    assert_blob_identified_outside_transaction(blob) do
+      @user.highlights.attach(blob)
+    end
+  end
+
   test "getting a signed blob ID from an attachment" do
     blob = create_blob
     @user.avatar.attach(blob)
@@ -65,4 +98,33 @@ class ActiveStorage::AttachmentTest < ActiveSupport::TestCase
     signed_id_generated_old_way = ActiveStorage.verifier.generate(@user.avatar.id, purpose: :blob_id)
     assert_equal blob, ActiveStorage::Blob.find_signed!(signed_id_generated_old_way)
   end
+
+  private
+    def assert_blob_identified_before_owner_validated(owner, blob, content_type)
+      validated_content_type = nil
+
+      owner.class.validate do
+        validated_content_type ||= blob.content_type
+      end
+
+      yield
+
+      assert_equal content_type, validated_content_type
+      assert_equal content_type, blob.reload.content_type
+    end
+
+    def assert_blob_identified_outside_transaction(blob)
+      baseline_transaction_depth = ActiveRecord::Base.connection.open_transactions
+      max_transaction_depth = -1
+
+      track_transaction_depth = ->(*) do
+        max_transaction_depth = [ActiveRecord::Base.connection.open_transactions, max_transaction_depth].max
+      end
+
+      blob.stub(:identify_without_saving, track_transaction_depth) do
+        yield
+      end
+
+      assert_equal 0, (max_transaction_depth - baseline_transaction_depth)
+    end
 end

activestorage/test/models/blob_test.rb

@@ -260,6 +260,16 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     assert_equal ["is invalid"], blob.errors[:service_name]
   end
 
+  test "updating the content_type updates service metadata" do
+    blob = directly_upload_file_blob(filename: "racecar.jpg", content_type: "application/octet-stream")
+
+    expected_arguments = [blob.key, content_type: "image/jpeg"]
+
+    assert_called_with(blob.service, :update_metadata, expected_arguments) do
+      blob.update!(content_type: "image/jpeg")
+    end
+  end
+
   private
     def expected_url_for(blob, disposition: :attachment, filename: nil, content_type: nil, service_name: :local)
       filename ||= blob.filename