Add ability to set acl and object_options per upload

[dimroc]

This builds on Shrine’s upload_options plugin as further described here:
https://github.com/janko-m/shrine/blob/864528eac0c3182f6c553b227535901d69b682c6/lib/shrine/plugins/upload_options.rb

Allowing the following:

class ImageUploader < Shrine
  plugin :validation_helpers
  plugin :upload_options, store: {
    acl: "publicRead",
    object_options: { cache_control: 'public, max-age: 7200' }
  }
  ...

ACL can now be set per uploader or even per upload with a block:

class PredictionUploader < Shrine
  plugin :validation_helpers
  plugin :upload_options, store: -> (io,context) do
    # do something
  end

[renchap]

Thanks for this PR!
I am not sure I like the object_options key, as it makes upload_options structure different from the uploader ones.

Do you think we could do something like this:

class ImageUploader < Shrine
  plugin :upload_options, store: {
    acl: "publicRead",
    cache_control: 'public, max-age: 7200',
  }
  ...

This means we treat the acl option differently than the others, but I think it looks better. We could also change the default_acl uploader option to be acl, so it is even closer:

# Current implementation
storage = Shrine::Storage::GoogleCloudStorage.new(
  bucket: 'my-bucket',
  cache_control: 'public, max-age: 7200',
  default_acl: 'publicRead',
)

# Proposal
storage = Shrine::Storage::GoogleCloudStorage.new(
  bucket: 'my-bucket',
  cache_control: 'public, max-age: 7200',
  acl: 'publicRead',
)

@janko-m any opinion here? (sorry to tag you on those issues, but I prefer to be consistent with the other plugins and Shrine conventions)

[janko]

@renchap Np, I always like to help with conventions if I can. The way that Shrine::Storage::S3 handles upload options is the following:

First, all options passed to Shrine::Storage::S3.new are used for configuration, exception :upload_options which is a hash of default parameters forwarded to aws-sdk-s3 on uploading, copying and generating presigns. This makes sense because all three of these actions accept many same-named options. Note that I only added :upload_options option because it's also forwarded to #presign, whereas upload_options plugin only forwards to #upload.

s3 = Shrine::Storage::S3.new(upload_options: {acl: "public-read", ...})

The Shrine::Storage::S3#upload method forwards any additional options directly to aws-sdk-s3's upload or copy methods. Aws-sdk-s3 accepts options like :acl, :content_type, :cache_control and similar. Note that these options are the same one you can pass to :upload_options on initialization.

s3.upload(io, id, acl: "private")
# or
Shrine.plugin :upload_options, store: {acl: "private"}

I prefer @renchap's suggestion of keeping the upload options flat instead of introducing the :object_options nesting.

[dimroc]

Hi guys, great work with shrine and GCS, I'm a fan. @renchap I agree with your proposal so I've gone ahead and updated the PR. Just so we're on the same page, I originally reused the existing object_options to be consistent with the current implementation:

# Actual current implementation
storage = Shrine::Storage::GoogleCloudStorage.new(
  bucket: 'my-bucket',
  object_options: { cache_control: 'public, max-age: 7200' },
  default_acl: 'publicRead',
)

As you've recommended, I've changed it to

# In PR
storage = Shrine::Storage::GoogleCloudStorage.new(
  bucket: 'my-bucket',
  acl: 'publicRead',
  cache_control: 'public, max-age: 7200'
)

This should will still support other http headers like content_disposition. I've also gone ahead and updated the README.md.

[dimroc]

Also note that this will be a breaking change for existing users of default_acl, since it's been changed to acl.

[janko]

@dimroc Actually, I only agree with the first part of @renchap's suggestion about keeping upload options in GoogleCloudStorage#upload flat. In S3 storage I separated upload options from configuration options on S3#initialize via the :upload_options parameter (and :acl and :cache_control will be passed only on upload). I found that cleaner because it emphasizes which are the configuration options passed to Aws::S3::Client the S3 case.

In GoogleCloudStorage we have :object_options acting as kind of :upload_options, the only one that's outside is :default_acl. I think it would make sense to keep GoogleCloudStorage#initialize as-is for now, and only update #upload. That way this PR would be focused to only the change described in the PR description, and that is to be able to pass ACL per-upload.

[dimroc]

Regarding changes to #upload, I think we're all good @janko-m. I'll make those changes ASAP. Regarding initialization, @renchap wasn't a fan of object_options. What would you all say to renaming object_options to upload_options for consistency across storages? In this PR (easier for me) or another PR?

Shrine::Storage::GoogleCloudStorage.new(
  bucket: "store",
  upload_options: {
    acl: 'publicRead',
    cache_control: 'public, max-age: 7200'
  },
)

as opposed to the existing approach

Shrine::Storage::GoogleCloudStorage.new(
  bucket: "store",
  default_acl: 'publicRead',
  object_options: {
    cache_control: 'public, max-age: 7200'
  },
)

[renchap]

I just merged #16 which switches to google-cloud-storage.
It changed the way #upload works and transforms any additional arguments into calls to arg_name=arg_value on the google-cloud-storage file object.
I might have missed somthing but I think the master now behaves correctly regarding upload_options and this PR is no longer needed.
@dimroc can you check I did not miss anything?

[dimroc]

Looks good to me. I updated to the new version and it all checks out.