Merge pull request #31918 from huacnlee/fix/blob-service-url-for-string-filename

Fix `blob.service_url` for supports string type `:filename` option

3 files changed, 13 insertions, 1 deletions

diff --git a/activestorage/app/models/active_storage/blob.rb b/activestorage/app/models/active_storage/blob.rb
index 892a833fae..a1e69e2264 100644
--- a/activestorage/app/models/active_storage/blob.rb
+++ b/activestorage/app/models/active_storage/blob.rb
@@ -109,7 +109,9 @@ class ActiveStorage::Blob < ActiveRecord::Base
   # with users. Instead, the +service_url+ should only be exposed as a redirect from a stable, possibly authenticated URL.
   # Hiding the +service_url+ behind a redirect also gives you the power to change services without updating all URLs. And
   # it allows permanent URLs that redirect to the +service_url+ to be cached in the view.
-  def service_url(expires_in: service.url_expires_in, disposition: :inline, filename: self.filename, **options)
+  def service_url(expires_in: service.url_expires_in, disposition: :inline, filename: nil, **options)
+    filename = ActiveStorage::Filename.wrap(filename || self.filename)
+
     service.url key, expires_in: expires_in, filename: filename, content_type: content_type,
       disposition: forcibly_serve_as_binary? ? :attachment : disposition, **options
   end
diff --git a/activestorage/app/models/active_storage/filename.rb b/activestorage/app/models/active_storage/filename.rb
index b9413dec95..2b8880716e 100644
--- a/activestorage/app/models/active_storage/filename.rb
+++ b/activestorage/app/models/active_storage/filename.rb
@@ -5,6 +5,14 @@
 class ActiveStorage::Filename
   include Comparable
 
+  class << self
+    # Returns a Filename instance based on the given filename. If the filename is a Filename, it is
+    # returned unmodified. If it is a String, it is passed to ActiveStorage::Filename.new.
+    def wrap(filename)
+      filename.kind_of?(self) ? filename : new(filename)
+    end
+  end
+
   def initialize(filename)
     @filename = filename
   end
diff --git a/activestorage/test/models/blob_test.rb b/activestorage/test/models/blob_test.rb
index 5cd2a94326..9b555f9a1d 100644
--- a/activestorage/test/models/blob_test.rb
+++ b/activestorage/test/models/blob_test.rb
@@ -82,6 +82,8 @@ class ActiveStorage::BlobTest < ActiveSupport::TestCase
     freeze_time do
       assert_equal expected_url_for(blob), blob.service_url
       assert_equal expected_url_for(blob, filename: new_filename), blob.service_url(filename: new_filename)
+      assert_equal expected_url_for(blob, filename: new_filename), blob.service_url(filename: "new.txt")
+      assert_equal expected_url_for(blob, filename: blob.filename), blob.service_url(filename: nil)
     end
   end