PR feedback #1

Author: Matt Muller

gems/aws-sdk-core/lib/aws-sdk-core/lru_cache.rb

@@ -4,6 +4,9 @@ module Aws
   # @api private
   # A simple thread safe LRU cache
   class LRUCache
+    # @param [Hash] options
+    # @option options [Integer] :max_entries (100) Maximum number of entries
+    # @option options [Integer] :expiration (nil) Expiration time in seconds
     def initialize(options = {})
       @max_entries = options[:max_entries] || 100
       @expiration = options[:expiration]

gems/aws-sdk-s3/CHANGELOG.md

@@ -1,7 +1,7 @@
 Unreleased Changes
 ------------------
 
-* Feature - Support S3 Access Grants authentication. Access Grants can be enabled with the `s3_access_grants` option, and custom options can be passed into the `access_grants_credentials_provider` option. This feature is enabled if `aws-sdk-s3control` is installed.
+* Feature - Support S3 Access Grants authentication. Access Grants can be enabled with the `access_grants` option, and custom options can be passed into the `access_grants_credentials_provider` option. This feature requires `aws-sdk-s3control` to be installed.
 
 1.148.0 (2024-04-25)
 ------------------

gems/aws-sdk-s3/lib/aws-sdk-s3/access_grants_credentials_provider.rb

@@ -3,9 +3,12 @@
 module Aws
   module S3
     # @api private
-    ACCESS_GRANTS_CREDENTIALS_CACHE = LRUCache.new
+    ACCESS_GRANTS_CREDENTIALS_CACHE = LRUCache.new(max_entries: 100)
     # @api private
-    ACCESS_GRANTS_ACCOUNT_ID_CACHE = LRUCache.new(expiration: 60 * 10)
+    ACCESS_GRANTS_ACCOUNT_ID_CACHE = LRUCache.new(
+      max_entries: 100,
+      expiration: 60 * 10
+    )
 
     # Returns Credentials class for S3 Access Grants. Accepts GetDataAccess
     # params and other configuration as options. See

gems/aws-sdk-s3/lib/aws-sdk-s3/express_credentials_provider.rb

@@ -3,7 +3,7 @@
 module Aws
   module S3
     # @api private
-    EXPRESS_CREDENTIALS_CACHE = LRUCache.new
+    EXPRESS_CREDENTIALS_CACHE = LRUCache.new(max_entries: 100)
 
     # Returns Credentials class for S3 Express. Accepts CreateSession
     # params as options. See {Client#create_session} for details.
@@ -18,21 +18,33 @@ class ExpressCredentialsProvider
       #   credentials are refreshed.
       def initialize(options = {})
         @client = options.delete(:client)
-        @caching = options.delete(:caching) || true
+        @caching = options.delete(:caching) != false
         @options = options
         @cache = EXPRESS_CREDENTIALS_CACHE
       end
 
       def express_credentials_for(bucket)
-        @cache[bucket] || new_credentials_for(bucket)
+        if @caching
+          cached_credentials_for(bucket)
+        else
+          new_credentials_for(bucket)
+        end
       end
 
       attr_accessor :client
 
       private
 
+      def cached_credentials_for(bucket)
+        if @cache.key?(bucket)
+          @cache[bucket]
+        else
+          @cache[bucket] = new_credentials_for(bucket)
+        end
+      end
+
       def new_credentials_for(bucket)
-        @cache[bucket] = ExpressCredentials.new(
+        ExpressCredentials.new(
           bucket: bucket,
           client: @client,
           **@options

gems/aws-sdk-s3/spec/express_credentials_provider_spec.rb

@@ -16,23 +16,48 @@ module S3
       end
 
       describe '#express_credentials_for' do
-        before(:each) do
+        after do
           EXPRESS_CREDENTIALS_CACHE.clear
+        end
+
+        it 'returns a new set of credentials for the bucket' do
           expect(ExpressCredentials).to receive(:new)
             .with(bucket: 'bucket', client: client, session_mode: 'ReadWrite')
             .and_return(express_credentials)
-        end
 
-        it 'returns a new set of credentials for the bucket' do
           expect(subject.express_credentials_for('bucket'))
             .to eq(express_credentials)
         end
 
         it 'returns the same set of credentials for the bucket' do
+          expect(ExpressCredentials).to receive(:new)
+            .with(bucket: 'bucket', client: client, session_mode: 'ReadWrite')
+            .and_return(express_credentials)
+
           credentials = subject.express_credentials_for('bucket')
           expect(subject.express_credentials_for('bucket'))
             .to be(credentials)
         end
+
+        context 'caching disabled' do
+          subject do
+            ExpressCredentialsProvider.new(
+              client: client,
+              session_mode: 'ReadWrite',
+              caching: false
+            )
+          end
+
+          it 'returns a different set of credentials for the bucket' do
+            expect(ExpressCredentials).to receive(:new)
+              .with(bucket: 'bucket', client: client, session_mode: 'ReadWrite')
+              .and_return(express_credentials).twice
+
+            credentials = subject.express_credentials_for('bucket')
+            expect(subject.express_credentials_for('bucket'))
+              .to be(credentials)
+          end
+        end
       end
 
       describe '#client' do