diff --git a/actiontext/test/test_helper.rb b/actiontext/test/test_helper.rb index 51a207f76a..556ace73e5 100644 --- a/actiontext/test/test_helper.rb +++ b/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 diff --git a/activestorage/CHANGELOG.md b/activestorage/CHANGELOG.md index 5ad078d2a4..8d096c2d0e 100644 --- a/activestorage/CHANGELOG.md +++ b/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* diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb index dc53ac5852..71e25a57f7 100644 --- a/activestorage/app/models/active_storage/blob.rb +++ b/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 create_after_upload!. -# 2. Ahead of the file being directly uploaded client-side to the service via create_before_direct_upload!. +# 1. Ahead of the file being uploaded server-side to the service, via create_and_upload!. A rewindable +# io 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 create_before_direct_upload!. # # 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 identify: false 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 identify: false 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 io 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 identify: false 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 diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb index 958aed735d..02d4b10fc0 100644 --- a/activestorage/test/models/blob_test.rb +++ b/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 diff --git a/activestorage/test/test_helper.rb b/activestorage/test/test_helper.rb index c3c84bb723..1face2204c 100644 --- a/activestorage/test/test_helper.rb +++ b/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)