Only set :endpoint from legacy option if present (AwsSdkAdapter) (#390)

* only set endpoint from legacy option if present

In the `aws-sdk-core` gem, the `endpoint` option is meant for
overriding the default endpoint value resolved from the region
option [1]. Passing a `nil` value for the endpoint option is
not the same as not passing it at all, as nil is still considered
to be an override [2][3].

A recent refactor to the `AwsSdkAdapter` looks to have tweaked the
options logic to always include the `endpoint` key in the object
being passed to `Aws::S3::Resource`. By default, this will raise
a "missing required option :endpoint" exception as the default
legacy option of `nil` will be set for the endpoint.

This change ensures that the `endpoint` value isn't set if
the legacy option is not specified.

[1] https://github.com/aws/aws-sdk-ruby/blob/c97f6932b6d5d6bc3e45aaf1068be52afd6781be/gems/aws-sdk-core/lib/seahorse/client/plugins/endpoint.rb#L11-L15
[2] https://github.com/aws/aws-sdk-ruby/blob/c97f6932b6d5d6bc3e45aaf1068be52afd6781be/gems/aws-sdk-core/lib/seahorse/client/plugins/endpoint.rb#L29-L32
[3] aws/aws-sdk-ruby#2066

* Improve the options setting so that we only set the option when we have a value, and only if it is not already set

* Upgrade to 6.2.1

Co-authored-by: Zoran Pesic <zoran1991@gmail.com>

CHANGES.md

@@ -1,3 +1,7 @@
+### 6.2.1
+
+* Bugfix: Improve handling of deprecated options in `AwsSdkAdapter`.  Fixes bug where `:region` was being set to `nil`. [#390](https://github.com/kjvarga/sitemap_generator/pull/390).
+
 ### 6.2.0
 
 * Raise `LoadError` when an adapter's dependency is missing to better support Sorbet [#387](https://github.com/kjvarga/sitemap_generator/pull/387).

VERSION

@@ -1 +1 @@
-6.2.0
+6.2.1

lib/sitemap_generator/adapters/aws_sdk_adapter.rb

@@ -28,12 +28,13 @@ class AwsSdkAdapter
     def initialize(bucket, aws_access_key_id: nil, aws_secret_access_key: nil, aws_region: nil, aws_endpoint: nil, **options)
       @bucket = bucket
       @options = options
-      @options[:access_key_id] ||= aws_access_key_id
-      @options[:secret_access_key] ||= aws_secret_access_key
-      @options[:region] ||= aws_region
-      @options[:endpoint] ||= aws_endpoint
+      set_option_unless_set(:access_key_id, aws_access_key_id)
+      set_option_unless_set(:secret_access_key, aws_secret_access_key)
+      set_option_unless_set(:region, aws_region)
+      set_option_unless_set(:endpoint, aws_endpoint)
     end
 
+
     # Call with a SitemapLocation and string data
     def write(location, raw_data)
       SitemapGenerator::FileAdapter.new.write(location, raw_data)
@@ -47,6 +48,10 @@ def write(location, raw_data)
 
     private
 
+    def set_option_unless_set(key, value)
+      @options[key] = value if @options[key].nil? && !value.nil?
+    end
+
     def s3_resource
       @s3_resource ||= Aws::S3::Resource.new(@options)
     end

spec/sitemap_generator/adapters/aws_sdk_adapter_spec.rb

@@ -29,6 +29,52 @@
     end
   end
 
+  shared_examples "deprecated option" do |deprecated_key, new_key|
+    context 'when a deprecated option set' do
+      context 'when it is not nil' do
+        let(:options) do
+          { deprecated_key => 'value' }
+        end
+
+        it 'sets the option' do
+          expect(adapterOptions[new_key]).to eq('value')
+        end
+
+        context 'when the new option key is set' do
+          context 'when it is not nil' do
+            let(:options) do
+              { deprecated_key => 'value', new_key => 'new_endpoint' }
+            end
+
+            it 'does not override it' do
+              expect(adapterOptions[new_key]).to eq('new_endpoint')
+            end
+          end
+
+          context 'when it is nil' do
+            let(:options) do
+              { deprecated_key => 'value', new_key => nil }
+            end
+
+            it 'overrides it' do
+              expect(adapterOptions[new_key]).to eq('value')
+            end
+          end
+        end
+      end
+
+      context 'when it is nil' do
+        let(:options) do
+          { deprecated_key => nil }
+        end
+
+        it 'does not set the option' do
+          expect(adapterOptions).not_to have_key(new_key)
+        end
+      end
+    end
+  end
+
   context 'when Aws::S3::Resource is not defined' do
     it 'raises a LoadError' do
       hide_const('Aws::S3::Resource')
@@ -63,80 +109,11 @@
   end
 
   describe '#initialize' do
-    context 'with region option' do
-      let(:options) { { region: 'region' } }
-
-      it 'sets region in options' do
-        expect(adapter.instance_variable_get(:@options)[:region]).to eql('region')
-      end
-    end
-
-    context 'with deprecated aws_region option' do
-      let(:options) { { aws_region: 'region' } }
+    subject(:adapterOptions) { adapter.instance_variable_get(:@options) }
 
-      it 'sets region in options' do
-        expect(adapter.instance_variable_get(:@options)[:region]).to eql('region')
-      end
-    end
-
-    context 'with access_key_id option' do
-      let(:options) do
-        { access_key_id: 'access_key_id' }
-      end
-
-      it 'sets access_key_id in options' do
-        expect(adapter.instance_variable_get(:@options)[:access_key_id]).to eq('access_key_id')
-      end
-    end
-
-    context 'with deprecated aws_access_key_id option' do
-      let(:options) do
-        { aws_access_key_id: 'access_key_id' }
-      end
-
-      it 'sets access_key_id in options' do
-        expect(adapter.instance_variable_get(:@options)[:access_key_id]).to eq('access_key_id')
-      end
-    end
-
-    context 'with secret_access_key option' do
-      let(:options) do
-        { secret_access_key: 'secret_access_key' }
-      end
-
-      it 'sets secret_access_key in options' do
-        expect(adapter.instance_variable_get(:@options)[:secret_access_key]).to eq('secret_access_key')
-      end
-    end
-
-    context 'with deprecated aws_secret_access_key option' do
-      let(:options) do
-        { aws_secret_access_key: 'secret_access_key' }
-      end
-
-      it 'sets secret_access_key in options' do
-        expect(adapter.instance_variable_get(:@options)[:secret_access_key]).to eq('secret_access_key')
-      end
-    end
-
-    context 'with endpoint option' do
-      let(:options) do
-        { endpoint: 'endpoint' }
-      end
-
-      it 'sets endpoint in options' do
-        expect(adapter.instance_variable_get(:@options)[:endpoint]).to eq('endpoint')
-      end
-    end
-
-    context 'with deprecated aws_endpoint option' do
-      let(:options) do
-        { aws_endpoint: 'endpoint' }
-      end
-
-      it 'sets endpoint in options' do
-        expect(adapter.instance_variable_get(:@options)[:endpoint]).to eq('endpoint')
-      end
-    end
+    it_behaves_like "deprecated option", :aws_endpoint, :endpoint
+    it_behaves_like "deprecated option", :aws_access_key_id, :access_key_id
+    it_behaves_like "deprecated option", :aws_secret_access_key, :secret_access_key
+    it_behaves_like "deprecated option", :aws_region, :region
   end
 end