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 object options #6

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions lib/refile/s3.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@ class S3

attr_reader :access_key_id, :max_size

S3_AVAILABLE_OPTIONS = {
client: %i(access_key_id region secret_access_key),
copy_from: %i(copy_source server_side_encryption storage_class),
presigned_post: %i(key server_side_encryption storage_class),
put: %i(body content_length server_side_encryption storage_class)
}

# Sets up an S3 backend
#
# @param [String] region The AWS region to connect to
Expand All @@ -42,7 +49,7 @@ class S3
# @see http://docs.aws.amazon.com/AWSRubySDK/latest/AWS/S3.html
def initialize(region:, bucket:, max_size: nil, prefix: nil, hasher: Refile::RandomHasher.new, **s3_options)
@s3_options = { region: region }.merge s3_options
@s3 = Aws::S3::Resource.new @s3_options
@s3 = Aws::S3::Resource.new s3_options_for(:client)
credentials = @s3.client.config.credentials
raise S3CredentialsError unless credentials
@access_key_id = credentials.access_key_id
Expand All @@ -60,12 +67,14 @@ def initialize(region:, bucket:, max_size: nil, prefix: nil, hasher: Refile::Ran
verify_uploadable def upload(uploadable)
id = @hasher.hash(uploadable)

if uploadable.is_a?(Refile::File) and uploadable.backend.is_a?(S3) and uploadable.backend.access_key_id == access_key_id
object(id).copy_from(copy_source: [@bucket_name, uploadable.backend.object(uploadable.id).key].join("/"))
operation, options = if upload_via_copy_operation?(uploadable)
[:copy_from, { copy_source: [@bucket_name, uploadable.backend.object(uploadable.id).key].join("/") }]
else
object(id).put(body: uploadable, content_length: uploadable.size)
[:put, { body: uploadable, content_length: uploadable.size }]
end

object(id).send(operation, s3_options_for(operation, options))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a bit too clever, and I'm not really sure why it's needed. Can't we just add the options to how it was before?

Copy link
Author

Choose a reason for hiding this comment

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

In the previous version with AWS SDK V 1.x it was possible to pass these options as defaults to the client object whereas now they are only available as options for the "operation" methods (copy_from, put, presigned_post).

presigned_post and copy_from/put have slightly different APIs regarding their options (as described in #3). The two options implemented here happen to share same option keys across the three operations, but for example options to provide custom encryption keys seem to have different hash keys in presigned_post than in other two.

So the indirection is there so that it'll be simple to add the other keys from the respective operation APIs when someone needs them. If supporting those options is over-architecting at this point then a simpler implementation is indeed possible.


Refile::File.new(self, id)
end

Expand Down Expand Up @@ -145,11 +154,20 @@ def clear!(confirm = nil)
# @return [Refile::Signature]
def presign
id = RandomHasher.new.hash
signature = @bucket.presigned_post(key: [*@prefix, id].join("/"))
signature = @bucket.presigned_post(s3_options_for(:presigned_post, key: [*@prefix, id].join("/")))
signature.content_length_range(0..@max_size) if @max_size
Signature.new(as: "file", id: id, url: signature.url.to_s, fields: signature.fields)
end

def s3_options_for(operation, options = {})
keys = S3_AVAILABLE_OPTIONS.fetch(operation)
@s3_options.merge(options).select { |key, _| keys.include?(key) }
end

def upload_via_copy_operation?(uploadable)
uploadable.is_a?(Refile::File) and uploadable.backend.is_a?(S3) and uploadable.backend.access_key_id == access_key_id
end

verify_id def object(id)
@bucket.object([*@prefix, id].join("/"))
end
Expand Down
98 changes: 97 additions & 1 deletion spec/refile/s3_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,106 @@

WebMock.allow_net_connect!

config = YAML.load_file("s3.yml").map { |k, v| [k.to_sym, v] }.to_h
s3_config = YAML.load_file("s3.yml").map { |k, v| [k.to_sym, v] }.to_h

RSpec.describe Refile::S3 do
let(:config) { s3_config }
let(:backend) { Refile::S3.new(max_size: 100, **config) }

it_behaves_like :backend

describe '@s3_options' do
let(:value) { backend.instance_variable_get(:@s3_options) }

it 'is initialized' do
%i(access_key_id secret_access_key region).each do |attribute|
expect(value[attribute]).to eq(config[attribute])
end
end

context 'given additional configuration options' do
let(:config) { { server_side_encryption: 'aes256' }.merge s3_config }

it 'is initialized' do
expect(value[:server_side_encryption]).to eq('aes256')
end
end
end

describe '.s3_options_for' do
let(:config) do
{
access_key_id: 'xyz',
secret_access_key: 'abcd1234',
region: 'sa-east-1',
bucket: 'my-bucket',
server_side_encryption: 'aes256',
storage_class: 'STANDARD'
}
end
let(:options) { {} }
let(:value) { backend.s3_options_for key, options }

describe 'given operation' do
context 'client' do
let(:key) { :client }

it 'returns valid options' do
expect(value).to eq(
access_key_id: 'xyz',
secret_access_key: 'abcd1234',
region: 'sa-east-1'
)
end
end

context 'copy_from' do
let(:key) { :copy_from }
let(:options) { { copy_source: 'xyz' } }

it 'returns valid options' do
expect(value).to eq(
copy_source: 'xyz',
server_side_encryption: 'aes256',
storage_class: 'STANDARD'
)
end
end

context 'presigned_post' do
let(:key) { :presigned_post }
let(:options) { { key: 'xyz' } }

it 'returns valid options' do
expect(value).to eq(
key: 'xyz',
server_side_encryption: 'aes256',
storage_class: 'STANDARD'
)
end
end

context 'put' do
let(:key) { :put }
let(:options) { { body: 'xyz', content_length: 3 } }

it 'returns valid options' do
expect(value).to eq(
body: 'xyz',
content_length: 3,
server_side_encryption: 'aes256',
storage_class: 'STANDARD'
)
end
end
end

context 'given an invalid operation' do
let(:key) { :invalid }

it 'raises an error' do
expect { value }.to raise_error(KeyError)
end
end
end
end