Alternative implementation of passing s3 options to put, copy_from an…

…d presigned_post (issue refile#3)

lib/refile/s3.rb

@@ -21,6 +21,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),
+      presigned_post: %i(key server_side_encryption),
+      put: %i(body content_length server_side_encryption)
+    }
+
     # Sets up an S3 backend with the given credentials.
     #
     # @param [String] access_key_id
@@ -36,10 +43,8 @@ class S3
     def initialize(access_key_id:, secret_access_key:, region:, bucket:, max_size: nil, prefix: nil, hasher: Refile::RandomHasher.new, **s3_options)
       @access_key_id = access_key_id
       @secret_access_key = secret_access_key
-      @s3_presigned_post_options = s3_options.delete(:s3_presigned_post_options) { {} }
-      @s3_object_operation_options = s3_options.delete(:s3_object_operation_options) { {} }
       @s3_options = { access_key_id: access_key_id, secret_access_key: secret_access_key, region: region }.merge s3_options
-      @s3 = Aws::S3::Resource.new @s3_options
+      @s3 = Aws::S3::Resource.new s3_options_for(:client)
       @bucket_name = bucket
       @bucket = @s3.bucket @bucket_name
       @hasher = hasher
@@ -60,7 +65,7 @@ def initialize(access_key_id:, secret_access_key:, region:, bucket:, max_size: n
         [:put, { body: uploadable, content_length: uploadable.size }]
       end
 
-      object(id).send(operation, s3_object_operation_options(options))
+      object(id).send(operation, s3_options_for(operation, options))
 
       Refile::File.new(self, id)
     end
@@ -141,17 +146,14 @@ def clear!(confirm = nil)
     # @return [Refile::Signature]
     def presign
       id = RandomHasher.new.hash
-      signature = @bucket.presigned_post(s3_presigned_post_options(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_presigned_post_options(options)
-      @s3_presigned_post_options.merge(options)
-    end
-
-    def s3_object_operation_options(options)
-      @s3_object_operation_options.merge(options)
+    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)

spec/refile/s3_spec.rb

@@ -19,32 +19,85 @@
         expect(value[attribute]).to eq(config[attribute])
       end
     end
-  end
-
-  %i(s3_object_operation_options s3_presigned_post_options).each do |option|
-    describe "@#{option}" do
-      let(:additional_config) do
-        { "#{option}".to_sym => { server_side_encryption: 'aes256' } }
-      end
 
-      let(:config) { additional_config.merge s3_config }
-      let(:value) { backend.send(:instance_variable_get, "@#{option}") }
+    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')
-        expect(backend.instance_variable_get(:@s3_options)).not_to have_key(option)
       end
     end
+  end
 
-    describe ".#{option}" do
-      let(:additional_config) do
-        { "#{option}".to_sym => { server_side_encryption: 'aes256' } }
+  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'
+      }
+    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
 
-      let(:config) { additional_config.merge s3_config }
+      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'
+          )
+        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'
+          )
+        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'
+          )
+        end
+      end
+    end
+
+    context 'given an invalid operation' do
+      let(:key) { :invalid }
 
-      it 'is merges provided options' do
-        expect(backend.send(option, { server_side_encryption: 'aws:kms' })[:server_side_encryption]).to eq('aws:kms')
+      it 'raises an error' do
+        expect { value }.to raise_error(KeyError)
       end
     end
   end