6 files changed, 194 insertions, 111 deletions

diff --git a/activestorage/test/models/attached/many_test.rb b/activestorage/test/models/attached/many_test.rb
index 334254d099..e826109874 100644
--- a/activestorage/test/models/attached/many_test.rb
+++ b/activestorage/test/models/attached/many_test.rb
@@ -16,6 +16,9 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
     @user.highlights.attach create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg")
     assert_equal "funky.jpg", @user.highlights.first.filename.to_s
     assert_equal "town.jpg", @user.highlights.second.filename.to_s
+
+    assert_not_empty @user.highlights_attachments
+    assert_equal @user.highlights_blobs.count, 2
   end
 
   test "attaching existing blobs from signed IDs to an existing record" do
@@ -468,6 +471,33 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
     end
   end
 
+  test "purging attachment with shared blobs" do
+    [
+      create_blob(filename: "funky.jpg"),
+      create_blob(filename: "town.jpg"),
+      create_blob(filename: "worm.jpg")
+    ].tap do |blobs|
+      @user.highlights.attach blobs
+      assert @user.highlights.attached?
+
+      another_user = User.create!(name: "John")
+      shared_blobs = [blobs.second, blobs.third]
+      another_user.highlights.attach shared_blobs
+      assert another_user.highlights.attached?
+
+      @user.highlights.purge
+      assert_not @user.highlights.attached?
+
+      assert_not ActiveStorage::Blob.exists?(blobs.first.id)
+      assert ActiveStorage::Blob.exists?(blobs.second.id)
+      assert ActiveStorage::Blob.exists?(blobs.third.id)
+
+      assert_not ActiveStorage::Blob.service.exist?(blobs.first.key)
+      assert ActiveStorage::Blob.service.exist?(blobs.second.key)
+      assert ActiveStorage::Blob.service.exist?(blobs.third.key)
+    end
+  end
+
   test "purging later" do
     [ create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") ].tap do |blobs|
       @user.highlights.attach blobs
@@ -485,6 +515,35 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
     end
   end
 
+  test "purging attachment later with shared blobs" do
+    [
+      create_blob(filename: "funky.jpg"),
+      create_blob(filename: "town.jpg"),
+      create_blob(filename: "worm.jpg")
+    ].tap do |blobs|
+      @user.highlights.attach blobs
+      assert @user.highlights.attached?
+
+      another_user = User.create!(name: "John")
+      shared_blobs = [blobs.second, blobs.third]
+      another_user.highlights.attach shared_blobs
+      assert another_user.highlights.attached?
+
+      perform_enqueued_jobs do
+        @user.highlights.purge_later
+      end
+
+      assert_not @user.highlights.attached?
+      assert_not ActiveStorage::Blob.exists?(blobs.first.id)
+      assert ActiveStorage::Blob.exists?(blobs.second.id)
+      assert ActiveStorage::Blob.exists?(blobs.third.id)
+
+      assert_not ActiveStorage::Blob.service.exist?(blobs.first.key)
+      assert ActiveStorage::Blob.service.exist?(blobs.second.key)
+      assert ActiveStorage::Blob.service.exist?(blobs.third.key)
+    end
+  end
+
   test "purging dependent attachment later on destroy" do
     [ create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") ].tap do |blobs|
       @user.highlights.attach blobs
@@ -534,7 +593,7 @@ class ActiveStorage::ManyAttachedTest < ActiveSupport::TestCase
       assert_equal "town.jpg", @user.highlights.first.filename.to_s
       assert_equal "funky.jpg", @user.highlights.second.filename.to_s
     ensure
-      User.send(:remove_method, :highlights)
+      User.remove_method :highlights
     end
   end
 end
diff --git a/activestorage/test/models/attached/one_test.rb b/activestorage/test/models/attached/one_test.rb
index 3333fd9323..ac08d324bb 100644
--- a/activestorage/test/models/attached/one_test.rb
+++ b/activestorage/test/models/attached/one_test.rb
@@ -15,6 +15,9 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase
   test "attaching an existing blob to an existing record" do
     @user.avatar.attach create_blob(filename: "funky.jpg")
     assert_equal "funky.jpg", @user.avatar.filename.to_s
+
+    assert_not_nil @user.avatar_attachment
+    assert_not_nil @user.avatar_blob
   end
 
   test "attaching an existing blob from a signed ID to an existing record" do
@@ -412,6 +415,22 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase
     end
   end
 
+  test "purging an attachment with a shared blob" do
+    create_blob(filename: "funky.jpg").tap do |blob|
+      @user.avatar.attach blob
+      assert @user.avatar.attached?
+
+      another_user = User.create!(name: "John")
+      another_user.avatar.attach blob
+      assert another_user.avatar.attached?
+
+      @user.avatar.purge
+      assert_not @user.avatar.attached?
+      assert ActiveStorage::Blob.exists?(blob.id)
+      assert ActiveStorage::Blob.service.exist?(blob.key)
+    end
+  end
+
   test "purging later" do
     create_blob(filename: "funky.jpg").tap do |blob|
       @user.avatar.attach blob
@@ -427,6 +446,25 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase
     end
   end
 
+  test "purging an attachment later with shared blob" do
+    create_blob(filename: "funky.jpg").tap do |blob|
+      @user.avatar.attach blob
+      assert @user.avatar.attached?
+
+      another_user = User.create!(name: "John")
+      another_user.avatar.attach blob
+      assert another_user.avatar.attached?
+
+      perform_enqueued_jobs do
+        @user.avatar.purge_later
+      end
+
+      assert_not @user.avatar.attached?
+      assert ActiveStorage::Blob.exists?(blob.id)
+      assert ActiveStorage::Blob.service.exist?(blob.key)
+    end
+  end
+
   test "purging dependent attachment later on destroy" do
     create_blob(filename: "funky.jpg").tap do |blob|
       @user.avatar.attach blob
@@ -472,7 +510,7 @@ class ActiveStorage::OneAttachedTest < ActiveSupport::TestCase
 
       assert_equal "gpj.yknuf", @user.avatar
     ensure
-      User.send(:remove_method, :avatar)
+      User.remove_method :avatar
     end
   end
 end
diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb
index c2e7aae13a..9fd75a1b4a 100644
--- a/activestorage/test/models/blob_test.rb
+++ b/activestorage/test/models/blob_test.rb
@@ -47,6 +47,10 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     assert_equal "text/csv", blob.content_type
   end
 
+  test "create after upload generates a 28-character base36 key" do
+    assert_match(/^[a-z0-9]{28}$/, create_blob.key)
+  end
+
   test "image?" do
     blob = create_file_blob filename: "racecar.jpg"
     assert_predicate blob, :image?
@@ -100,19 +104,17 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     end
   end
 
-  test "open in a custom tempdir" do
-    tempdir = Dir.mktmpdir
-
-    create_file_blob(filename: "racecar.jpg").open(tempdir: tempdir) do |file|
+  test "open in a custom tmpdir" do
+    create_file_blob(filename: "racecar.jpg").open(tmpdir: tmpdir = Dir.mktmpdir) do |file|
       assert file.binmode?
       assert_equal 0, file.pos
       assert_match(/\.jpg\z/, file.path)
-      assert file.path.starts_with?(tempdir)
+      assert file.path.starts_with?(tmpdir)
       assert_equal file_fixture("racecar.jpg").binread, file.read, "Expected downloaded file to match fixture file"
     end
   end
 
-  test "urls expiring in 5 minutes" do
+  test "URLs expiring in 5 minutes" do
     blob = create_blob
 
     freeze_time do
@@ -121,16 +123,25 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     end
   end
 
-  test "urls force attachment as content disposition for content types served as binary" do
+  test "URLs force content_type to binary and attachment as content disposition for content types served as binary" do
     blob = create_blob(content_type: "text/html")
 
     freeze_time do
-      assert_equal expected_url_for(blob, disposition: :attachment), blob.service_url
-      assert_equal expected_url_for(blob, disposition: :attachment), blob.service_url(disposition: :inline)
+      assert_equal expected_url_for(blob, disposition: :attachment, content_type: "application/octet-stream"), blob.service_url
+      assert_equal expected_url_for(blob, disposition: :attachment, content_type: "application/octet-stream"), blob.service_url(disposition: :inline)
+    end
+  end
+
+  test "URLs force attachment as content disposition when the content type is not allowed inline" do
+    blob = create_blob(content_type: "application/zip")
+
+    freeze_time do
+      assert_equal expected_url_for(blob, disposition: :attachment, content_type: "application/zip"), blob.service_url
+      assert_equal expected_url_for(blob, disposition: :attachment, content_type: "application/zip"), blob.service_url(disposition: :inline)
     end
   end
 
-  test "urls allow for custom filename" do
+  test "URLs allow for custom filename" do
     blob = create_blob(filename: "original.txt")
     new_filename = ActiveStorage::Filename.new("new.txt")
 
@@ -142,13 +153,13 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     end
   end
 
-  test "urls allow for custom options" do
+  test "URLs allow for custom options" do
     blob = create_blob(filename: "original.txt")
 
     arguments = [
       blob.key,
       expires_in: ActiveStorage.service_urls_expire_in,
-      disposition: :inline,
+      disposition: :attachment,
       content_type: blob.content_type,
       filename: blob.filename,
       thumb_size: "300x300",
@@ -174,18 +185,22 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     assert_not ActiveStorage::Blob.service.exist?(variant.key)
   end
 
-  test "purge fails when attachments exist" do
+  test "purge does nothing when attachments exist" do
     create_blob.tap do |blob|
       User.create! name: "DHH", avatar: blob
-      assert_raises(ActiveRecord::InvalidForeignKey) { blob.purge }
+      assert_no_difference(-> { ActiveStorage::Blob.count }) { blob.purge }
       assert ActiveStorage::Blob.service.exist?(blob.key)
     end
   end
 
   private
-    def expected_url_for(blob, disposition: :inline, filename: nil)
+    def expected_url_for(blob, disposition: :attachment, filename: nil, content_type: nil)
       filename ||= blob.filename
-      query_string = { content_type: blob.content_type, disposition: "#{disposition}; #{filename.parameters}" }.to_param
-      "https://example.com/rails/active_storage/disk/#{ActiveStorage.verifier.generate(blob.key, expires_in: 5.minutes, purpose: :blob_key)}/#{filename}?#{query_string}"
+      content_type ||= blob.content_type
+
+      query = { disposition: ActionDispatch::Http::ContentDisposition.format(disposition: disposition, filename: filename.sanitized), content_type: content_type }
+      key_params = { key: blob.key }.merge(query)
+
+      "https://example.com/rails/active_storage/disk/#{ActiveStorage.verifier.generate(key_params, expires_in: 5.minutes, purpose: :blob_key)}/#{filename}?#{query.to_param}"
     end
 end
diff --git a/activestorage/test/models/filename/parameters_test.rb b/activestorage/test/models/filename/parameters_test.rb
deleted file mode 100644
index 431be00639..0000000000
--- a/activestorage/test/models/filename/parameters_test.rb
+++ /dev/null
@@ -1,32 +0,0 @@
-# frozen_string_literal: true
-
-require "test_helper"
-
-class ActiveStorage::Filename::ParametersTest < ActiveSupport::TestCase
-  test "parameterizing a Latin filename" do
-    filename = ActiveStorage::Filename.new("racecar.jpg")
-
-    assert_equal %(filename="racecar.jpg"), filename.parameters.ascii
-    assert_equal "filename*=UTF-8''racecar.jpg", filename.parameters.utf8
-    assert_equal "#{filename.parameters.ascii}; #{filename.parameters.utf8}", filename.parameters.combined
-    assert_equal filename.parameters.combined, filename.parameters.to_s
-  end
-
-  test "parameterizing a Latin filename with accented characters" do
-    filename = ActiveStorage::Filename.new("råcëçâr.jpg")
-
-    assert_equal %(filename="racecar.jpg"), filename.parameters.ascii
-    assert_equal "filename*=UTF-8''r%C3%A5c%C3%AB%C3%A7%C3%A2r.jpg", filename.parameters.utf8
-    assert_equal "#{filename.parameters.ascii}; #{filename.parameters.utf8}", filename.parameters.combined
-    assert_equal filename.parameters.combined, filename.parameters.to_s
-  end
-
-  test "parameterizing a non-Latin filename" do
-    filename = ActiveStorage::Filename.new("автомобиль.jpg")
-
-    assert_equal %(filename="%3F%3F%3F%3F%3F%3F%3F%3F%3F%3F.jpg"), filename.parameters.ascii
-    assert_equal "filename*=UTF-8''%D0%B0%D0%B2%D1%82%D0%BE%D0%BC%D0%BE%D0%B1%D0%B8%D0%BB%D1%8C.jpg", filename.parameters.utf8
-    assert_equal "#{filename.parameters.ascii}; #{filename.parameters.utf8}", filename.parameters.combined
-    assert_equal filename.parameters.combined, filename.parameters.to_s
-  end
-end
diff --git a/activestorage/test/models/filename_test.rb b/activestorage/test/models/filename_test.rb
index 88405e41c0..715116309f 100644
--- a/activestorage/test/models/filename_test.rb
+++ b/activestorage/test/models/filename_test.rb
@@ -30,8 +30,8 @@ class ActiveStorage::FilenameTest < ActiveSupport::TestCase
   end
 
   test "sanitize transcodes to valid UTF-8" do
-    { "\xF6".dup.force_encoding(Encoding::ISO8859_1) => "ö",
-      "\xC3".dup.force_encoding(Encoding::ISO8859_1) => "Ã",
+    { (+"\xF6").force_encoding(Encoding::ISO8859_1) => "ö",
+      (+"\xC3").force_encoding(Encoding::ISO8859_1) => "Ã",
       "\xAD" => "�",
       "\xCF" => "�",
       "\x00" => "",
diff --git a/activestorage/test/models/variant_test.rb b/activestorage/test/models/variant_test.rb
index 6577f1cd9f..d98935eb9f 100644
--- a/activestorage/test/models/variant_test.rb
+++ b/activestorage/test/models/variant_test.rb
@@ -26,16 +26,14 @@ class ActiveStorage::VariantTest < ActiveSupport::TestCase
   end
 
   test "monochrome with default variant_processor" do
-    begin
-      ActiveStorage.variant_processor = nil
-
-      blob = create_file_blob(filename: "racecar.jpg")
-      variant = blob.variant(monochrome: true).processed
-      image = read_image(variant)
-      assert_match(/Gray/, image.colorspace)
-    ensure
-      ActiveStorage.variant_processor = :mini_magick
-    end
+    ActiveStorage.variant_processor = nil
+
+    blob = create_file_blob(filename: "racecar.jpg")
+    variant = blob.variant(monochrome: true).processed
+    image = read_image(variant)
+    assert_match(/Gray/, image.colorspace)
+  ensure
+    ActiveStorage.variant_processor = :mini_magick
   end
 
   test "disabled variation of JPEG blob" do
@@ -66,45 +64,41 @@ class ActiveStorage::VariantTest < ActiveSupport::TestCase
   end
 
   test "disabled variation using :combine_options" do
-    begin
-      ActiveStorage.variant_processor = nil
-      blob = create_file_blob(filename: "racecar.jpg")
-      variant = ActiveSupport::Deprecation.silence do
-        blob.variant(combine_options: {
-          crop: "100x100+0+0",
-          monochrome: false
-        }).processed
-      end
-      assert_match(/racecar\.jpg/, variant.service_url)
-
-      image = read_image(variant)
-      assert_equal 100, image.width
-      assert_equal 100, image.height
-      assert_match(/RGB/, image.colorspace)
-    ensure
-      ActiveStorage.variant_processor = :mini_magick
+    ActiveStorage.variant_processor = nil
+    blob = create_file_blob(filename: "racecar.jpg")
+    variant = ActiveSupport::Deprecation.silence do
+      blob.variant(combine_options: {
+        crop: "100x100+0+0",
+        monochrome: false
+      }).processed
     end
+    assert_match(/racecar\.jpg/, variant.service_url)
+
+    image = read_image(variant)
+    assert_equal 100, image.width
+    assert_equal 100, image.height
+    assert_match(/RGB/, image.colorspace)
+  ensure
+    ActiveStorage.variant_processor = :mini_magick
   end
 
   test "center-weighted crop of JPEG blob using :combine_options" do
-    begin
-      ActiveStorage.variant_processor = nil
-      blob = create_file_blob(filename: "racecar.jpg")
-      variant = ActiveSupport::Deprecation.silence do
-        blob.variant(combine_options: {
-          gravity: "center",
-          resize: "100x100^",
-          crop: "100x100+0+0",
-        }).processed
-      end
-      assert_match(/racecar\.jpg/, variant.service_url)
-
-      image = read_image(variant)
-      assert_equal 100, image.width
-      assert_equal 100, image.height
-    ensure
-      ActiveStorage.variant_processor = :mini_magick
+    ActiveStorage.variant_processor = nil
+    blob = create_file_blob(filename: "racecar.jpg")
+    variant = ActiveSupport::Deprecation.silence do
+      blob.variant(combine_options: {
+        gravity: "center",
+        resize: "100x100^",
+        crop: "100x100+0+0",
+      }).processed
     end
+    assert_match(/racecar\.jpg/, variant.service_url)
+
+    image = read_image(variant)
+    assert_equal 100, image.width
+    assert_equal 100, image.height
+  ensure
+    ActiveStorage.variant_processor = :mini_magick
   end
 
   test "center-weighted crop of JPEG blob using :resize_to_fill" do
@@ -139,6 +133,17 @@ class ActiveStorage::VariantTest < ActiveSupport::TestCase
     assert_equal 20, image.height
   end
 
+  test "resized variation of TIFF blob" do
+    blob = create_file_blob(filename: "racecar.tif")
+    variant = blob.variant(resize: "50x50").processed
+    assert_match(/racecar\.png/, variant.service_url)
+
+    image = read_image(variant)
+    assert_equal "PNG", image.type
+    assert_equal 50, image.width
+    assert_equal 33, image.height
+  end
+
   test "optimized variation of GIF blob" do
     blob = create_file_blob(filename: "image.gif", content_type: "image/gif")
 
@@ -156,22 +161,20 @@ class ActiveStorage::VariantTest < ActiveSupport::TestCase
   test "service_url doesn't grow in length despite long variant options" do
     blob = create_file_blob(filename: "racecar.jpg")
     variant = blob.variant(font: "a" * 10_000).processed
-    assert_operator variant.service_url.length, :<, 525
+    assert_operator variant.service_url.length, :<, 730
   end
 
   test "works for vips processor" do
-    begin
-      ActiveStorage.variant_processor = :vips
-      blob = create_file_blob(filename: "racecar.jpg")
-      variant = blob.variant(thumbnail_image: 100).processed
-
-      image = read_image(variant)
-      assert_equal 100, image.width
-      assert_equal 67, image.height
-    rescue LoadError
-      # libvips not installed
-    ensure
-      ActiveStorage.variant_processor = :mini_magick
-    end
+    ActiveStorage.variant_processor = :vips
+    blob = create_file_blob(filename: "racecar.jpg")
+    variant = blob.variant(thumbnail_image: 100).processed
+
+    image = read_image(variant)
+    assert_equal 100, image.width
+    assert_equal 67, image.height
+  rescue LoadError
+    # libvips not installed
+  ensure
+    ActiveStorage.variant_processor = :mini_magick
   end
 end