Always create ActiveStorage::Blob before uploading to service

actiontext/test/test_helper.rb

@@ -28,7 +28,7 @@ end
 class ActiveSupport::TestCase
   private
     def create_file_blob(filename:, content_type:, metadata: nil)
-      ActiveStorage::Blob.create_after_upload! io: file_fixture(filename).open, filename: filename, content_type: content_type, metadata: metadata
+      ActiveStorage::Blob.create_and_upload! io: file_fixture(filename).open, filename: filename, content_type: content_type, metadata: metadata
     end
 end
 

activestorage/CHANGELOG.md

@@ -1,3 +1,15 @@
+*   Replace `Blob.create_after_upload!` with `Blob.create_and_upload!` and deprecate the former.
+
+    `create_after_upload!` has been removed since it could lead to data
+    curruption by uploading to a key on the storage service which happened to
+    be already taken. Creating the record would then correctly raise a
+    database uniqueness exception but the stored object would already have
+    overwritten another. `create_and_upload!` swaps the order of operations
+    so that the key gets reserved up-front or the uniqueness error gets raised,
+    before the upload to a key takes place.
+
+    *Julik Tarkhanov*
+
 *   Set content disposition in direct upload using `filename` and `disposition` parameters to `ActiveStorage::Service#headers_for_direct_upload`.
 
     *Peter Zhu*

activestorage/app/models/active_storage/blob.rb

@@ -5,8 +5,9 @@ require "active_storage/downloader"
 # A blob is a record that contains the metadata about a file and a key for where that file resides on the service.
 # Blobs can be created in two ways:
 #
-# 1. Subsequent to the file being uploaded server-side to the service via <tt>create_after_upload!</tt>.
-# 2. Ahead of the file being directly uploaded client-side to the service via <tt>create_before_direct_upload!</tt>.
+# 1. Ahead of the file being uploaded server-side to the service, via <tt>create_and_upload!</tt>. A rewindable
+#    <tt>io</tt> with the file contents must be available at the server for this operation.
+# 2. Ahead of the file being directly uploaded client-side to the service, via <tt>create_before_direct_upload!</tt>.
 #
 # The first option doesn't require any client-side JavaScript integration, and can be used by any other back-end
 # service that deals with files. The second option is faster, since you're not using your own server as a staging
@@ -49,9 +50,8 @@ class ActiveStorage::Blob < ActiveRecord::Base
       find ActiveStorage.verifier.verify(id, purpose: :blob_id)
     end
 
-    # Returns a new, unsaved blob instance after the +io+ has been uploaded to the service.
-    # When providing a content type, pass <tt>identify: false</tt> to bypass automatic content type inference.
-    def build_after_upload(io:, filename:, content_type: nil, metadata: nil, identify: true, record: nil)
+    def build_after_upload(io:, filename:, content_type: nil, metadata: nil, identify: true, record: nil) #:nodoc:
+      ActiveSupport::Deprecation.warn("ActiveStorage::Blob.build_after_upload is deprecated and will be removed in Rails 6.2")
       new(filename: filename, content_type: content_type, metadata: metadata).tap do |blob|
         blob.upload(io, identify: identify)
       end
@@ -63,12 +63,24 @@ class ActiveStorage::Blob < ActiveRecord::Base
       end
     end
 
-    # Returns a saved blob instance after the +io+ has been uploaded to the service. Note, the blob is first built,
-    # then the +io+ is uploaded, then the blob is saved. This is done this way to avoid uploading (which may take
-    # time), while having an open database transaction.
-    # When providing a content type, pass <tt>identify: false</tt> to bypass automatic content type inference.
-    def create_after_upload!(io:, filename:, content_type: nil, metadata: nil, identify: true, record: nil)
-      build_after_upload(io: io, filename: filename, content_type: content_type, metadata: metadata, identify: identify).tap(&:save!)
+    def create_after_unfurling!(io:, filename:, content_type: nil, metadata: nil, identify: true, record: nil) #:nodoc:
+      build_after_unfurling(io: io, filename: filename, content_type: content_type, metadata: metadata, identify: identify).tap(&:save!)
+    end
+
+    # Creates a new blob instance and then uploads the contents of
+    # the given <tt>io</tt> to the service. The blob instance is going to
+    # be saved before the upload begins to prevent the upload clobbering another due to key collisions.
+    # When providing a content type, pass <tt>identify: false</tt> to bypass
+    # automatic content type inference.
+    def create_and_upload!(io:, filename:, content_type: nil, metadata: nil, identify: true, record: nil)
+      create_after_unfurling!(io: io, filename: filename, content_type: content_type, metadata: metadata, identify: identify).tap do |blob|
+        blob.upload_without_unfurling(io)
+      end
+    end
+
+    def create_after_upload!(**arguments_for_create_and_upload) #:nodoc:
+      ActiveSupport::Deprecation.warn("ActiveStorage::Blob.create_after_upload! has been renamed to create_and_upload!. The old name will be removed in Rails 6.2")
+      create_and_upload!(**arguments_for_create_and_upload)
     end
 
     # Returns a saved blob _without_ uploading a file to the service. This blob will point to a key where there is
@@ -165,8 +177,10 @@ class ActiveStorage::Blob < ActiveRecord::Base
   # and store that in +byte_size+ on the blob record. The content type is automatically extracted from the +io+ unless
   # you specify a +content_type+ and pass +identify+ as false.
   #
-  # Normally, you do not have to call this method directly at all. Use the factory class methods of +build_after_upload+
-  # and +create_after_upload!+.
+  # Normally, you do not have to call this method directly at all.
+  # Use the +create_and_upload!+ class method instead. If you do use
+  # this method directly, make sure you are using it on a persisted Blob as otherwise
+  # another blob's data might get overwrittein on the service.
   def upload(io, identify: true)
     unfurl io, identify: identify
     upload_without_unfurling io

activestorage/test/models/blob_test.rb

@@ -18,7 +18,36 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     end
   end
 
-  test "create after upload sets byte size and checksum" do
+  test "create_and_upload does not permit a conflicting blob key to overwrite an existing object" do
+    data = "First file"
+    first_blob = create_blob data: data
+    assert_raise ActiveRecord::RecordNotUnique do
+      ActiveStorage::Blob.stub :generate_unique_secure_token, first_blob.key do
+        data = "This would overwrite"
+        create_blob data: data
+      end
+    end
+    readback = first_blob.download
+    assert_equal "First file", readback
+  end
+
+  test "create_after_upload! has the same effect as create_and_upload!" do
+    data = "Some other, even more funky file"
+    blob = ActiveStorage::Blob.create_after_upload!(io: StringIO.new(data), filename: "funky.bin")
+    assert blob.persisted?
+    readback = blob.download
+    assert_equal "Some other, even more funky file", readback
+  end
+
+  test "build_after_upload uploads to service but does not save the Blob" do
+    data = "A potentially overwriting file"
+    blob = ActiveStorage::Blob.build_after_upload(io: StringIO.new(data), filename: "funky.bin")
+    assert_not blob.persisted?
+    readback = blob.download
+    assert_equal "A potentially overwriting file", readback
+  end
+
+  test "create_and_upload sets byte size and checksum" do
     data = "Hello world!"
     blob = create_blob data: data
 
@@ -27,31 +56,31 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     assert_equal Digest::MD5.base64digest(data), blob.checksum
   end
 
-  test "create after upload extracts content type from data" do
+  test "create_and_upload extracts content type from data" do
     blob = create_file_blob content_type: "application/octet-stream"
     assert_equal "image/jpeg", blob.content_type
   end
 
-  test "create after upload extracts content type from filename" do
+  test "create_and_upload extracts content type from filename" do
     blob = create_blob content_type: "application/octet-stream"
     assert_equal "text/plain", blob.content_type
   end
 
-  test "create after upload extracts content_type from io when no content_type given and identify: false" do
+  test "create_and_upload extracts content_type from io when no content_type given and identify: false" do
     blob = create_blob content_type: nil, identify: false
     assert_equal "text/plain", blob.content_type
   end
 
-  test "create after upload uses content_type when identify: false" do
+  test "create_and_upload uses content_type when identify: false" do
     blob = create_blob data: "Article,dates,analysis\n1, 2, 3", filename: "table.csv", content_type: "text/csv", identify: false
     assert_equal "text/csv", blob.content_type
   end
 
-  test "create after upload generates a 28-character base36 key" do
+  test "create_and_upload generates a 28-character base36 key" do
     assert_match(/^[a-z0-9]{28}$/, create_blob.key)
   end
 
-  test "create after upload accepts a record for overrides" do
+  test "create_and_upload accepts a record for overrides" do
     assert_nothing_raised do
       create_blob(record: User.new)
     end

activestorage/test/test_helper.rb

@@ -52,11 +52,11 @@ class ActiveSupport::TestCase
 
   private
     def create_blob(data: "Hello world!", filename: "hello.txt", content_type: "text/plain", identify: true, record: nil)
-      ActiveStorage::Blob.create_after_upload! io: StringIO.new(data), filename: filename, content_type: content_type, identify: identify, record: record
+      ActiveStorage::Blob.create_and_upload! io: StringIO.new(data), filename: filename, content_type: content_type, identify: identify, record: record
     end
 
     def create_file_blob(filename: "racecar.jpg", content_type: "image/jpeg", metadata: nil, record: nil)
-      ActiveStorage::Blob.create_after_upload! io: file_fixture(filename).open, filename: filename, content_type: content_type, metadata: metadata, record: record
+      ActiveStorage::Blob.create_and_upload! io: file_fixture(filename).open, filename: filename, content_type: content_type, metadata: metadata, record: record
     end
 
     def create_blob_before_direct_upload(filename: "hello.txt", byte_size:, checksum:, content_type: "text/plain", record: nil)