Skip to content

Commit

Permalink
Don't include extra options in the request
Browse files Browse the repository at this point in the history
  • Loading branch information
@packagethief
packagethief committed Oct 16, 2009
1 parent ba17303 commit 55c64b6
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion lib/aws/s3/object.rb
View file
Expand Up @@ -181,7 +181,7 @@ def copy(key, copy_key, bucket = nil, options = {})
source_key = path!(bucket, key)
default_options = {'x-amz-copy-source' => source_key}
target_key = path!(bucket, copy_key)
returning put(target_key, default_options.merge(options)) do
returning put(target_key, default_options) do
acl(copy_key, bucket, acl(key, bucket)) if options[:copy_acl]
end
end
Expand Down

4 comments on commit 55c64b6

@watson
Copy link

@watson watson commented on 55c64b6 Dec 9, 2010

Choose a reason for hiding this comment

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

Any good reason not to merge the user-provided options into the hash that is parsed to put? Why even ask for the options if they are not used?

@packagethief
Copy link
Member Author

Choose a reason for hiding this comment

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

The options are used on line 185 to check whether we should also copy the acl.

@watson
Copy link

@watson watson commented on 55c64b6 Dec 10, 2010

Choose a reason for hiding this comment

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

True - missed that :)

And I can see a reason for not wanting to manipulate the headers during a copy since a copy is expected to copy without making any changes, but in my case I would like to set a Content-Disposition header on the copied object. I could of cause do this by performing an update just afterwards, but if allowing such a thing during a copy I would to save a HTTP request, which is always nice :)

Maybe it could be changed to this:
returning put(target_key, default_options.merge(options.reject {|k,v| k == :copy_acl})) do

or:
def copy(key, copy_key, bucket = nil, options = {}, aws_options = {})
...
returning put(target_key, default_options.merge(aws_options)) do

what do you think?

@watson
Copy link

@watson watson commented on 55c64b6 Dec 12, 2010

Choose a reason for hiding this comment

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

Hmm... Forked the repo and added this change, but it seems that Amazon ignores new headers like :content_disposition when given the :x-amz-copy-source header. So I guess there is no other way than first doing the copy/rename and then following it up with an update afterwards :(

Please sign in to comment.