Skip to content

Commit

Permalink
Stop #cache! loading whole content of file into memory. Fixes #2136
Browse files Browse the repository at this point in the history
This change deprecates #sanitized_file, which is seemingly unnecessary
  • Loading branch information
mshibuya committed Jun 21, 2019
1 parent d12a289 commit 28190e9
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 25 deletions.
12 changes: 3 additions & 9 deletions lib/carrierwave/uploader/cache.rb
Expand Up @@ -90,14 +90,8 @@ def cache_stored_file!
end

def sanitized_file
_content = file.read
if _content.is_a?(File) # could be if storage is Fog
sanitized = CarrierWave::Storage::Fog.new(self).retrieve!(File.basename(_content.path))
else
sanitized = SanitizedFile.new :tempfile => StringIO.new(_content),
:filename => File.basename(path), :content_type => file.content_type
end
sanitized
ActiveSupport::Deprecation.warn('#sanitized_file is deprecated, use #file instead.')
file
end

##
Expand Down Expand Up @@ -127,7 +121,7 @@ def cache_name
#
# [CarrierWave::FormNotMultipart] if the assigned parameter is a string
#
def cache!(new_file = sanitized_file)
def cache!(new_file = file)
new_file = CarrierWave::SanitizedFile.new(new_file)
return if new_file.empty?

Expand Down
2 changes: 0 additions & 2 deletions lib/carrierwave/uploader/versions.rb
Expand Up @@ -229,9 +229,7 @@ def recreate_versions!(*names)
# process this version and all sub-versions

if names.any?
file = sanitized_file
set_versions_to_cache_and_store(names)
cache!(file)
store!(file)
reset_versions_to_cache_and_store
else
Expand Down
12 changes: 12 additions & 0 deletions spec/storage/fog_helper.rb
Expand Up @@ -44,6 +44,18 @@ class FogSpec#{fog_credentials[:provider]}Uploader < CarrierWave::Uploader::Base
uploader.store!(file)
expect { uploader.cache_stored_file! }.not_to raise_error
end

it "should create local file for processing" do
@uploader.class_eval do
def check_file
raise unless File.exists?(file.path)
end
process :check_file
end
uploader = @uploader.new
uploader.store!(file)
uploader.cache_stored_file!
end
end

context '#acl_header' do
Expand Down
18 changes: 4 additions & 14 deletions spec/uploader/cache_spec.rb
Expand Up @@ -19,20 +19,6 @@
end
end

describe '#sanitized_file' do
before { uploader.store! CarrierWave::SanitizedFile.new(test_file) }

it "returns a sanitized file" do
expect(uploader.sanitized_file).to be_an_instance_of(CarrierWave::SanitizedFile)
end

it "only read the file once" do
expect(uploader.file).to receive(:read).once.and_return('this is stuff')

uploader.sanitized_file
end
end

context "permissions" do
it "sets permissions if options are given" do
uploader_class.permissions = permission
Expand Down Expand Up @@ -113,6 +99,10 @@
uploader.cache!(nil)
end

it "does not read whole content of file into memory" do
expect(uploader.file).not_to receive(:read)
uploader.cache!
end

context 'negative cache id' do
let(:cache_id) { '-1369894322-345-1234-2255' }
Expand Down

0 comments on commit 28190e9

Please sign in to comment.