review: remove unnecessary S3 client caching in AssetProxy

Author: pjhartzell

src/lib/asset-proxy.js

@@ -7,6 +7,8 @@ import { NotFoundError, ValidationError, ForbiddenError } from './errors.js'
 const VIRTUAL_HOST_PATTERN = /^([^.]+)\.s3(?:\.([^.]+))?\.amazonaws\.com$/
 const PATH_STYLE_PATTERN = /^s3(?:[.-]([^.]+))?\.amazonaws\.com$/
 
+const s3Client = s3()
+
 export const ALTERNATE_ASSETS_EXTENSION = 'https://stac-extensions.github.io/alternate-assets/v1.2.0/schema.json'
 
 export const BucketOption = Object.freeze({
@@ -112,7 +114,6 @@ const determineS3Region = (asset, itemOrCollection) => {
 export class AssetProxy {
   constructor() {
     this.bucketsCache = null
-    this.s3ClientCache = new Map()
     this.bucketOption = process.env['ASSET_PROXY_BUCKET_OPTION'] || 'NONE'
     this.bucketList = process.env['ASSET_PROXY_BUCKET_LIST']
     this.urlExpiry = parseInt(process.env['ASSET_PROXY_URL_EXPIRY'] || '300', 10)
@@ -139,10 +140,8 @@ export class AssetProxy {
 
     case BucketOption.ALL_BUCKETS_IN_ACCOUNT:
       try {
-        const region = process.env['AWS_REGION'] || 'us-west-2'
-        const client = this.getS3Client(region)
         const command = new ListBucketsCommand({})
-        const response = await client.send(command)
+        const response = await s3Client.send(command)
         const bucketNames = response.Buckets?.map((b) => b.Name)
           ?.filter((name) => typeof name === 'string') || []
         this.bucketsCache = new Set(bucketNames)
@@ -158,20 +157,6 @@ export class AssetProxy {
     }
   }
 
-  /**
-   * @param {string} region - AWS region
-   * @returns {Object} S3 client instance
-   */
-  getS3Client(region) {
-    if (this.s3ClientCache.has(region)) {
-      return this.s3ClientCache.get(region)
-    }
-
-    const client = s3({ region })
-    this.s3ClientCache.set(region, client)
-    return client
-  }
-
   /**
    * @returns {boolean}
    */
@@ -280,20 +265,18 @@ export class AssetProxy {
   /**
    * @param {string} bucket - S3 bucket name
    * @param {string} key - S3 object key
-   * @param {string} region - AWS region
+   * @param {string} region - AWS region of the S3 bucket
    * @returns {Promise<string>} Pre-signed URL
    */
   async createPresignedUrl(bucket, key, region) {
-    const client = this.getS3Client(region)
-
     const command = new GetObjectCommand({
       Bucket: bucket,
       Key: key,
       RequestPayer: 'requester'
     })
 
-    const presignedUrl = await getSignedUrl(client, command, {
-      expiresIn: this.urlExpiry
+    const presignedUrl = await getSignedUrl(s3Client, command, {
+      expiresIn: this.urlExpiry, signingRegion: region
     })
 
     logger.debug('Generated pre-signed URL for asset', {

tests/unit/test-asset-proxy.js

@@ -1,8 +1,16 @@
 // @ts-nocheck
 
 import test from 'ava'
+import { mockClient } from 'aws-sdk-client-mock'
+import { S3Client, ListBucketsCommand } from '@aws-sdk/client-s3'
 import { AssetProxy, BucketOption, ALTERNATE_ASSETS_EXTENSION } from '../../src/lib/asset-proxy.js'
 
+const s3Mock = mockClient(S3Client)
+
+test.beforeEach(() => {
+  s3Mock.reset()
+})
+
 test('BucketOption - exports expected constants', (t) => {
   t.is(BucketOption.NONE, 'NONE')
   t.is(BucketOption.ALL, 'ALL')
@@ -80,19 +88,14 @@ test('AssetProxy - initialize() with ALL_BUCKETS_IN_ACCOUNT mode fetches buckets
     process.env['ASSET_PROXY_BUCKET_OPTION'] = 'ALL_BUCKETS_IN_ACCOUNT'
     process.env['AWS_REGION'] = 'us-west-2'
 
-    const proxy = new AssetProxy()
-
-    const mockS3Client = {
-      send: async () => ({
-        Buckets: [
-          { Name: 'bucket-1' },
-          { Name: 'bucket-2' },
-        ]
-      })
-    }
-
-    proxy.getS3Client = () => mockS3Client
+    s3Mock.on(ListBucketsCommand).resolves({
+      Buckets: [
+        { Name: 'bucket-1' },
+        { Name: 'bucket-2' },
+      ]
+    })
 
+    const proxy = new AssetProxy()
     await proxy.initialize()
 
     t.truthy(proxy.bucketsCache)
@@ -110,15 +113,10 @@ test('AssetProxy - initialize() with ALL_BUCKETS_IN_ACCOUNT mode throws on error
   try {
     process.env['ASSET_PROXY_BUCKET_OPTION'] = 'ALL_BUCKETS_IN_ACCOUNT'
 
-    const proxy = new AssetProxy()
+    // Set up the mock to reject with an error
+    s3Mock.on(ListBucketsCommand).rejects(new Error('Access denied'))
 
-    const mockS3Client = {
-      send: async () => {
-        throw new Error('Access denied')
-      }
-    }
-
-    proxy.getS3Client = () => mockS3Client
+    const proxy = new AssetProxy()
 
     await t.throwsAsync(
       async () => proxy.initialize(),
@@ -172,13 +170,11 @@ test('AssetProxy - isEnabled() returns true for ALL_BUCKETS_IN_ACCOUNT', async (
   try {
     process.env['ASSET_PROXY_BUCKET_OPTION'] = 'ALL_BUCKETS_IN_ACCOUNT'
 
-    const proxy = new AssetProxy()
-
-    const mockS3Client = {
-      send: async () => ({ Buckets: [{ Name: 'bucket-1' }] })
-    }
+    s3Mock.on(ListBucketsCommand).resolves({
+      Buckets: [{ Name: 'bucket-1' }]
+    })
 
-    proxy.getS3Client = () => mockS3Client
+    const proxy = new AssetProxy()
     await proxy.initialize()
 
     t.true(proxy.isEnabled())
@@ -234,18 +230,14 @@ test('AssetProxy - shouldProxyBucket() with ALL_BUCKETS_IN_ACCOUNT mode only pro
   try {
     process.env['ASSET_PROXY_BUCKET_OPTION'] = 'ALL_BUCKETS_IN_ACCOUNT'
 
-    const proxy = new AssetProxy()
+    s3Mock.on(ListBucketsCommand).resolves({
+      Buckets: [
+        { Name: 'fetched-bucket-1' },
+        { Name: 'fetched-bucket-2' }
+      ]
+    })
 
-    const mockS3Client = {
-      send: async () => ({
-        Buckets: [
-          { Name: 'fetched-bucket-1' },
-          { Name: 'fetched-bucket-2' }
-        ]
-      })
-    }
-
-    proxy.getS3Client = () => mockS3Client
+    const proxy = new AssetProxy()
     await proxy.initialize()
 
     t.true(proxy.shouldProxyBucket('fetched-bucket-1'))