Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

S3 should use tagging_directive REPLACE on copy operation #569

Merged
merged 1 commit into from Feb 8, 2022

Conversation

jrochkind
Copy link
Contributor

It was always using metadata_directive: REPLACE; this does not apply to AWS tags applied to an S3 object. It should also use tagging_directive to apply to AWS tags as well.

This makes it so tags aren't copied when copying from (say) cache to storage. More significantly, it allows the caller to pass S3 tagging param in upload_options, and have this take effect, whether on a promotion or any other kind of upload, regardless of whether the S3 adapter decides to use a copy_from operation under the hood.

Without this change, whether tagging passed as upload_options actually works (as well as whether the destination winds up with copied tags from source) depends on whether the S3 adapter decides to use copy_from, which should be an implementation detail that does not effect outcomes.

More at https://discourse.shrinerb.com/t/gotcha-on-s3-upload-options-and-tags/559

It was always using metadata_directive: REPLACE; this does not apply to AWS tags applied to an S3 object. It should also use tagging_directive to apply to AWS tags as well.

This makes it so tags aren't copied when copying from (say) cache to storage. More significantly, it allows the caller to pass S3 `tagging` param in `upload_options`, and have this take effect, whether on a promotion or any other kind of `upload`, regardless of whether the S3 adapter decides to use a `copy_from` operation under the hood.

Without this change, whether `tagging` passed as `upload_options` actually works (as well as whether the destination winds up with copied tags from source) depends on whether the S3 adapter decides to use `copy_from`, which should be an implementation detail that does not effect outcomes.

More at https://discourse.shrinerb.com/t/gotcha-on-s3-upload-options-and-tags/559
@jrochkind
Copy link
Contributor Author

I don't understand the test failure. jruby-only. Is it unrelated/pre-existing?

  1) Failure:
Shrine::Plugins::Derivatives::Attacher::versions compatibility::#load_data#test_0004_loads versions data without original (symbol) [/home/runner/work/shrine/shrine/test/plugin/derivatives_test.rb:1352]:
--- expected
+++ actual
@@ -1 +1 @@
-{:version=>#<#<Class:0xXXXXXX>::UploadedFile storage=:store id="b534db7cf2ceda002c643dd2b93337ae" metadata={"filename"=>nil, "size"=>4, "mime_type"=>nil}>}
+{}

@janko
Copy link
Member

janko commented Feb 8, 2022

Thanks, looks good 👍🏻 The JRuby failure was there before, I think it's a regression from JRuby 9.2 to JRuby 9.3, but I haven't investigated it yet.

@janko janko merged commit 98c427b into master Feb 8, 2022
@janko janko deleted the s3_copy_tagging_directive branch February 8, 2022 21:36
@jrochkind
Copy link
Contributor Author

Awesome, thank you!

I need this func to move forward with something I'm trying to do... do you have any rough estimate of when a shrine release will happen?

(Otherwise I can point at github and/or monkey-patch, sure).

@janko
Copy link
Member

janko commented Feb 8, 2022

I don't have any estimate at the moment, since I've been pretty deep into other open-source work for the past several months. I will try to make a release sometime next month.

jrochkind added a commit to sciencehistory/scihist_digicoll that referenced this pull request Feb 9, 2022
Necessary for our configuration for automated content-type tagging.

PR'd and accepted to shrine at shrinerb/shrine#569

But hasn't been released in shrine yet. So we need to patch in a fixed version of the #copy method (the smallest unit we can patch).

Wrapped in sane_patch to raise and remind us to remove when we get an upgraded shrine that has this fix in it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants