Skip to content

Commit

Permalink
Change #cache_storage to use #storage by default to avoid confusion
Browse files Browse the repository at this point in the history
Most users won't expect that they must be configured separately,
like said in #2353. But we preserve ability to set cache_storage explicitly,
because there can be a case which gets benefit from it.
(e.g. For an application which utilizes multiple servers but does not
need caches to be persisted across requests, using :fog for storage and
:file for cache_storage results in smaller overhead)
  • Loading branch information
mshibuya committed Jun 22, 2019
1 parent ca201ee commit 629afec
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 6 deletions.
4 changes: 2 additions & 2 deletions lib/carrierwave/uploader/cache.rb
Expand Up @@ -64,7 +64,7 @@ module ClassMethods
# It's recommended that you keep cache files in one place only.
#
def clean_cached_files!(seconds=60*60*24)
cache_storage.new(CarrierWave::Uploader::Base.new).clean_cache!(seconds)
(cache_storage || storage).new(CarrierWave::Uploader::Base.new).clean_cache!(seconds)
end
end

Expand Down Expand Up @@ -208,7 +208,7 @@ def original_filename=(filename)
end

def cache_storage
@cache_storage ||= self.class.cache_storage.new(self)
@cache_storage ||= (self.class.cache_storage || self.class.storage).new(self)
end
end # Cache
end # Uploader
Expand Down
6 changes: 3 additions & 3 deletions lib/carrierwave/uploader/configuration.rb
Expand Up @@ -110,8 +110,8 @@ def storage(storage = nil)
# cache_storage CarrierWave::Storage::File
# cache_storage MyCustomStorageEngine
#
def cache_storage(storage = nil)
if storage
def cache_storage(storage = false)
unless storage == false
self._cache_storage = storage.is_a?(Symbol) ? eval(storage_engines[storage]) : storage
end
_cache_storage
Expand Down Expand Up @@ -186,7 +186,7 @@ def reset_config
:fog => "CarrierWave::Storage::Fog"
}
config.storage = :file
config.cache_storage = :file
config.cache_storage = nil
config.fog_attributes = {}
config.fog_credentials = {}
config.fog_public = true
Expand Down
16 changes: 15 additions & 1 deletion spec/uploader/configuration_spec.rb
@@ -1,5 +1,4 @@
require 'spec_helper'
require 'carrierwave/storage/fog'

describe CarrierWave do
describe '.configure' do
Expand Down Expand Up @@ -81,6 +80,21 @@
end
end

describe ".cache_storage" do
it "returns the same storage as given by #storage" do
uploader_class.storage :file
expect(uploader_class.new.send(:cache_storage)).to be_a(CarrierWave::Storage::File)
uploader_class.storage :fog
expect(uploader_class.new.send(:cache_storage)).to be_a(CarrierWave::Storage::Fog)
end

it "can be explicitly set" do
uploader_class.storage :fog
uploader_class.cache_storage :file
expect(uploader_class.new.send(:cache_storage)).to be_a(CarrierWave::Storage::File)
end
end

describe '.add_config' do
before do
uploader_class.add_config :foo_bar
Expand Down
2 changes: 2 additions & 0 deletions spec/uploader/versions_spec.rb
Expand Up @@ -308,6 +308,7 @@ def move_to_cache

describe '#store!' do
before do
@uploader_class.cache_storage = :file
@uploader_class.storage = mock_storage('base')
@uploader_class.version(:thumb).storage = mock_storage('thumb')
@uploader_class.version(:preview).storage = mock_storage('preview')
Expand Down Expand Up @@ -490,6 +491,7 @@ def move_to_cache

describe '#remove!' do
before do
@uploader_class.cache_storage = :file
@uploader_class.storage = mock_storage('base')
@uploader_class.version(:thumb).storage = mock_storage('thumb')

Expand Down

2 comments on commit 629afec

@rogerhu
Copy link
Contributor

@rogerhu rogerhu commented on 629afec Oct 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change and https://github.com/carrierwaveuploader/carrierwave/pull/1879/files causes AWS S3 storage to be used as the temporary cache. We didn't grant DELETE permissions to avoid files being altered. Also, not sure if this change works well with recreate_versions! change (https://github.com/carrierwaveuploader/carrierwave/pull/1879/files)

@rogerhu
Copy link
Contributor

@rogerhu rogerhu commented on 629afec Oct 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also realized the reason we were having problems was that our Uploader class defined a cache_dir:

class BaseLogFileUploader < CarrierWave::Uploader::Base
def cache_dir
    Rails.root.join('tmp', 'uploads')
  end

Changing to an S3-backend also would require changing this path to reflect a temporary S3 bucket.

Please sign in to comment.