search

thephpleague / flysystem

Abstraction for local and remote filesystems
https://flysystem.thephpleague.com
MIT License
13.22k stars 825 forks source link

Fix: AWS Copy SSE-C Options + Send all options to copy instead of just params #1779

Closed amenophis closed 1 month ago

amenophis commented 1 month ago

Hi @frankdejonge, This is the right MR with all changes.

I tested in with the official S3 SDK, not with AsyncAWS. Not sure we have to do changes for params with AsyncAWS.

Thanks in advance for review :pray:

frankdejonge commented 1 month ago

The change to send all options broke the test suite, it's reverted.

amenophis commented 1 month ago

@frankdejonge Forwarding params only make the copy didn't work from S3 to S3 with SSE-C enabled (the head issued by AWS SDK ObjectCopier ends with a 400 from AWS API) Could you show me what tests are failling with the change ?

Thanks :pray:

frankdejonge commented 1 month ago

@amenophis I found the error, it had to do with how MetadataDirective was handled (which is required for specifying new metadata during copy. I'm not sure how the options you corrected are forwarded, as metadata of otherwise, but you might need to use the config 'MetadataDirective' => 'REPLACE' for what you're trying to achieve. Hope this helps, 3.27.0 is just released.

amenophis commented 1 month ago

I will try this tomorrow. Thank you for your research!

amenophis commented 1 month ago

Hi @frankdejonge I checked the fix you have merged, it seems to fix our issue, but there is something that i didn't understand about the MetadataDirective. I've added it in the adapter configuration with REPLACE value, and if i watch at the options sent to the client->copy() method, The MetadataDirective option in defined twice, at root with COPY value and in 'params' with REPLACE value).

I don't think this is the expected behaviour.

WDYT ?

amenophis commented 1 month ago

Hi @frankdejonge WDYT about my last message ?

Thanks :pray: