Skip to content

Commit

Permalink
Fixes #1083: Region-to-region multipart copying
Browse files Browse the repository at this point in the history
There was a subtle bug when doing to multipart copying between regions
due to ObjectMultipartCopier always atempting to fetch the size of the
source object from the target region (via the target client), instead of
querying the source region (via the source client.)

The solution is to ensure that, when doing region-to-region copying,
the source client is explicitly supplied to the ObjectMultipartCopier
so that the the correct region is queried for the content_length
of the source object.
  • Loading branch information
Zachery Moneypenny authored and trevorrowe committed Mar 3, 2016
1 parent fc4bbce commit 05a59f9
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ def initialize(object, options = {})
end

def copy_from(source, options = {})
options[:copy_source_client] = source.client if source.is_a?(S3::Object)
copy_object(source, @object, merge_options(source, options))
end

Expand All @@ -24,9 +25,11 @@ def copy_to(target, options = {})

def copy_object(source, target, options)
target_bucket, target_key = copy_target(target)

options[:bucket] = target_bucket
options[:key] = target_key
options[:copy_source] = copy_source(source)

if options.delete(:multipart_copy)
ObjectMultipartCopier.new(@options).copy(options)
else
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,12 +116,12 @@ def byte_range(offset, default_part_size, size)
end

def source_size(options)
if options[:content_length]
options.delete(:content_length)
else
bucket, key = options[:copy_source].match(/([^\/]+?)\/(.+)/)[1,2]
@client.head_object(bucket:bucket, key:key).content_length
end
return options.delete(:content_length) if options[:content_length]

client = options[:copy_source_client] || @client

bucket, key = options[:copy_source].match(/([^\/]+?)\/(.+)/)[1,2]
client.head_object(bucket: bucket, key: key).content_length
end

def default_part_size(source_size)
Expand Down
38 changes: 34 additions & 4 deletions aws-sdk-resources/spec/services/s3/object/multipart_copy_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,14 @@ module S3
end

it 'accepts an S3::Object source' do
src = S3::Object.new('source-bucket', 'unescaped/source/key path', stub_responses:true)
expect(client).to receive(:copy_object).with({
bucket: 'bucket',
key: 'unescaped/key path',
copy_source: 'source-bucket/unescaped/source/key%20path',
copy_source_client: src.client,
})
src = S3::Object.new('source-bucket', 'unescaped/source/key path', stub_responses:true)

object.copy_from(src)
end

Expand Down Expand Up @@ -235,9 +237,37 @@ module S3
it 'accepts file size option to avoid HEAD request' do
expect(client).not_to receive(:head_object)
object.copy_from('source-bucket/source/key',
multipart_copy: true,
content_length: 10 * 1024 * 1024
)
multipart_copy: true,
content_length: 10 * 1024 * 1024
)
end

context 'when the target and source objects are in different regions' do
let(:content_length) { 10 * 1024 * 1024 }

let(:source_bucket) { 'source-bucket' }
let(:target_bucket) { 'target-bucket' }

let(:key) { 'my/source-key' }

let(:source_client) { S3::Client.new(stub_responses: true) }
let(:target_client) { S3::Client.new(stub_responses: true) }

let(:source_object) { S3::Object.new(bucket_name: source_bucket, key: key, client: source_client) }
let(:target_object) { S3::Object.new(bucket_name: target_bucket, key: key, client: target_client) }

let(:head_response) { double Types::HeadObjectOutput, content_length: content_length }

before do
allow(source_client).to receive(:head_object).and_return(head_response)
end

it 'queries the content length of the source object from the source region' do
expect(source_client).to receive(:head_object).with({bucket: source_bucket, key: key})
expect(target_client).not_to receive(:head_object)

target_object.copy_from(source_object, multipart_copy: true)
end
end

it 'does not modify given options' do
Expand Down

0 comments on commit 05a59f9

Please sign in to comment.