Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
This fixes an issue with file upload streaming. Previously passing the file was added in PR carrierwaveuploader#468, and was subsequently broken by PR carrierwaveuploader#1517.

This corrects the approach implemented in PR carrierwaveuploader#468, without introducing the problem in PR carrierwaveuploader#1517.

New behavior:

  In the case of a fog based retrieval request, the file will be fetched as it was previously. The file read method will then see that file.body is set to a non-nil value, and return the file content from the file object.

  In the case of a fog based storage request, the file will be uploaded either as full text, or in batches as it was in PR carrierwaveuploader#468. However, to address the issue observed in PR carrierwaveuploader#1517 we set the body to nil, and then store a reference to the source file (the CarrierWave::SanitizedFile/read compatible object passed). This in turn results in the read method seeing no body on the file object, and consulting the source file for the file contents.

The approach to storage requests is taken over two other approaches:

  Approach carrierwaveuploader#1:

    We could read the file into memory as carrierwave currently does, however, this results in large files often exhausting the heap space of the process, thus making carrierwave a poor choice for large file uploads.

  Approach carrierwaveuploader#2:

    We could not store a reference to the original source file, however, this would result in an additional retrieval requests carrierwave does not currently make, potentially costing in bandwidth for current users.
  • Loading branch information
DarkArc committed Apr 12, 2018
1 parent f64f499 commit eefd86f
Showing 1 changed file with 36 additions and 9 deletions.
45 changes: 36 additions & 9 deletions lib/carrierwave/storage/fog.rb
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,9 @@ def initialize(uploader, base, path)
#
# [String] contents of file
def read
file.body
# Return the file body if there is one
return file.body if file.body
@source_file&.read
end

##
Expand Down Expand Up @@ -310,15 +312,31 @@ def store(new_file)
if new_file.is_a?(self.class)
new_file.copy_to(path)
else
fog_file = new_file.to_file
@content_type ||= new_file.content_type
@file = directory.files.create({
:body => (fog_file ? fog_file : new_file).read,
:content_type => @content_type,
:key => path,
:public => @uploader.fog_public
}.merge(@uploader.fog_attributes))
fog_file.close if fog_file && !fog_file.closed?

begin
streamable_fog_file = new_file.to_file

streaming_unsupported = streamable_fog_file.nil?
input_source = if streaming_unsupported
new_file.read
else
streamable_fog_file
end

# Set the file to pull in fog attributes, however, do not keep the body
# if this was a streamed file upload the body will be set to a closed
# file at the end of this call. Thus, it will be unreadable.
@file = store_at_path(input_source, path)
@file.body = nil

# Maintain a link to the file used to perform this call,
# this can be used to read the file content without making additional
# calls, even in the case of a streamed upload.
@source_file = new_file
ensure
streamable_fog_file.close if streamable_fog_file && !streamable_fog_file.closed?
end
end
true
end
Expand Down Expand Up @@ -458,6 +476,15 @@ def file
def acl_header
{'x-amz-acl' => @uploader.fog_public ? 'public-read' : 'private'}
end

def store_at_path(input_source, path)
directory.files.create({
:body => input_source,
:content_type => @content_type,
:key => path,
:public => @uploader.fog_public
}.merge(@uploader.fog_attributes))
end
end

end # Fog
Expand Down

0 comments on commit eefd86f

Please sign in to comment.