actions: add CopyObject action #47
Conversation
Codecov Report
@@ Coverage Diff @@
## main #47 +/- ##
==========================================
- Coverage 91.53% 91.12% -0.42%
==========================================
Files 26 27 +1
Lines 1300 1352 +52
==========================================
+ Hits 1190 1232 +42
- Misses 110 120 +10
Continue to review full report at Codecov.
|
src/actions/copy_object.rs
Outdated
| self.headers.iter(), | ||
| ), | ||
| None => { | ||
| url.query_pairs_mut().append_pair( |
There was a problem hiding this comment.
This should instead call crate::signing::util::add_query_params(url, self.query.iter()) so that any custom query parameters are also included.
Example from another action:
rusty-s3/src/actions/list_objects_v2.rs
Line 149 in 9e10f5f
src/actions/copy_object.rs
Outdated
| /// Create a copy of an object that is already stored in S3, using a `PUT` request. | ||
| /// | ||
| /// Find out more about CopyObject from the [AWS API Reference][api] | ||
| /// | ||
| /// [api]: https://docs.aws.amazon.com/AmazonS3/latest/API/API_CopyObject.html |
There was a problem hiding this comment.
Some actions also mention a few important pieces of information I found from the AWS docs, for this one I think it would be worth to briefly mention some of the footguns with this method:
- only objects up to 5 GB can be copied using this method
- even if the server returns a 200 response the copy might have failed
|
The time crate bumped MSRV again @guerinoni 🙄 |
|
@Sporif also rebase on |
src/actions/copy_object.rs
Outdated
| query.insert( | ||
| "x-amz-copy-source", | ||
| format!("{}/{}", bucket.name(), src_object), | ||
| ); |
There was a problem hiding this comment.
Should this use SortingIterator while signing to save a Map allocation, like here?
There was a problem hiding this comment.
By prefixing the current bucket name, this action does not allow copying from a different bucket.
There was a problem hiding this comment.
Should this use
SortingIteratorwhile signing to save a Map allocation, like here?
Sure, sounds like a good use for it.
By prefixing the current bucket name, this action does not allow copying from a different bucket.
We could take both a src_bucket and a dst_bucket
There was a problem hiding this comment.
Taking a Bucket is unnecessarily involved, as you need to pass connection details for bucket construction which will go completely unused. How about taking the raw source string, and having two separate functions on Bucket, e.g.:
Bucket::copy_objectwould prepend the current bucket name,Bucket::copy_object_from_bucketwould pass the source string as-is, and require the caller to prepend a (potentially different) bucket name
|
Implemented the changes as best I could, but there's one big problem I hadn't noticed before: the copy doesn't actually work (at least on minio) as the copied object is empty. No idea why. |
Sporif
changed the title
|
Ok so I'm really dumb, none of this is needed, |
I think it'd be still worth it to have it as a separate method. I'll try looking into why the test fails |
No description provided.