From 0625a2ba80476bf0139f2ecb9019dc2c82e4a7de Mon Sep 17 00:00:00 2001 From: Jason Lee Date: Thu, 8 Feb 2018 10:15:55 +0800 Subject: Fix `blob.service_url` for supports string or nil `:filename` option. - Make sure `blob.service_url` present a `ActiveStorage::Filename` type to `serivce.url`. - Add `ActiveStorage::Filename.wrap` method. before: ```rb blob.service_url(filename: ActiveStorage::Filename.new("new.txt")) blob.service_url(filename: "new.txt") => NoMethodError: undefined method `parameters' for "new.txt":String params = {} blob.service_url(filename: params[:filename]) => NoMethodError: undefined method `parameters' for nil:NilClass ``` after: ```rb blob.service_url(filename: "new.txt") blob.service_url(filename: nil) ``` --- activestorage/app/models/active_storage/blob.rb | 4 +++- activestorage/app/models/active_storage/filename.rb | 8 ++++++++ activestorage/test/models/blob_test.rb | 2 ++ 3 files changed, 13 insertions(+), 1 deletion(-) (limited to 'activestorage') 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 -- cgit v1.2.3