Skip to content

Commit 3dec688

Browse files
pjhartzellpjhartzell
committed
review: pull asset proxy bucket management into its own class
1 parent 3e96a02 commit 3dec688

File tree

3 files changed

+107
-99
lines changed

3 files changed

+107
-99
lines changed

‎src/lib/asset-proxy.js‎

Lines changed: 80 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
/* eslint-disable max-classes-per-file */
12
import {
23
GetObjectCommand,
34
ListBucketsCommand,
@@ -32,39 +33,41 @@ const parseS3Url = (url) => {
3233
return { bucket, key }
3334
}
3435

35-
export class AssetProxy {
36-
constructor() {
37-
this.bucketOption = process.env['ASSET_PROXY_BUCKET_OPTION'] || 'NONE'
38-
this.bucketList = process.env['ASSET_PROXY_BUCKET_LIST']
39-
this.urlExpiry = parseInt(process.env['ASSET_PROXY_URL_EXPIRY'] || '300', 10)
40-
this.isEnabled = this.bucketOption !== BucketOption.NONE
36+
class AssetBuckets {
37+
/**
38+
* @param {string} bucketOption - Bucket option (NONE, ALL, ALL_BUCKETS_IN_ACCOUNT, LIST)
39+
* @param {string[]|null} bucketNames - Array of bucket names (required for LIST option)
40+
*/
41+
constructor(bucketOption, bucketNames) {
42+
this.bucketOption = bucketOption
43+
this.bucketNames = bucketNames
4144
this.buckets = {}
4245
}
4346

4447
/**
45-
* @returns {Promise<AssetProxy>} Initialized AssetProxy instance
48+
* @param {string} bucketOption - Bucket option (NONE, ALL, ALL_BUCKETS_IN_ACCOUNT, LIST)
49+
* @param {string[]|null} bucketNames - Array of bucket names (required for LIST option)
50+
* @returns {Promise<AssetBuckets>} Initialized AssetBuckets instance
4651
*/
47-
static async create() {
48-
const dbInstance = new AssetProxy()
49-
await dbInstance._initBuckets()
50-
return dbInstance
52+
static async create(bucketOption, bucketNames) {
53+
const instance = new AssetBuckets(bucketOption, bucketNames)
54+
await instance._initBuckets()
55+
return instance
5156
}
5257

5358
/**
5459
* @returns {Promise<void>}
5560
*/
5661
async _initBuckets() {
5762
switch (this.bucketOption) {
58-
case BucketOption.LIST:
59-
if (this.bucketList) {
60-
const bucketNames = this.bucketList.split(',').map((b) => b.trim()).filter((b) => b)
63+
case BucketOption.LIST: {
64+
if (this.bucketNames && this.bucketNames.length > 0) {
6165
await Promise.all(
62-
bucketNames.map(async (name) => { await this.getBucket(name) })
66+
this.bucketNames.map(async (name) => { await this.getBucket(name) })
6367
)
6468

65-
const invalidBuckets = Object.values(this.buckets)
66-
.filter((b) => b.region === null)
67-
.map((b) => b.name)
69+
const invalidBuckets = Object.keys(this.buckets)
70+
.filter((bucketName) => this.buckets[bucketName].region === null)
6871
if (invalidBuckets.length > 0) {
6972
throw new Error(
7073
`Could not access or determine region for the following buckets: ${
@@ -78,33 +81,30 @@ export class AssetProxy {
7881
)
7982
} else {
8083
throw new Error(
81-
'ASSET_PROXY_BUCKET_LIST must be set when ASSET_PROXY_BUCKET_OPTION is LIST'
84+
'ASSET_PROXY_BUCKET_LIST must not be empty when ASSET_PROXY_BUCKET_OPTION is LIST'
8285
)
8386
}
8487
break
88+
}
8589

86-
case BucketOption.ALL_BUCKETS_IN_ACCOUNT:
87-
try {
88-
const command = new ListBucketsCommand({})
89-
const response = await s3Client.send(command)
90-
const buckets = response.Buckets || []
90+
case BucketOption.ALL_BUCKETS_IN_ACCOUNT: {
91+
const command = new ListBucketsCommand({})
92+
const response = await s3Client.send(command)
93+
const buckets = response.Buckets || []
9194

92-
await Promise.all(
93-
buckets
94-
.map((bucket) => bucket.Name)
95-
.filter((name) => typeof name === 'string')
96-
.map(async (name) => { await this.getBucket(name) })
97-
)
95+
await Promise.all(
96+
buckets
97+
.map((bucket) => bucket.Name)
98+
.filter((name) => typeof name === 'string')
99+
.map(async (name) => { await this.getBucket(name) })
100+
)
98101

99-
const count = Object.keys(this.buckets).length
100-
logger.info(
101-
`Fetched ${count} buckets from AWS account for asset proxy`
102-
)
103-
} catch (error) {
104-
const message = error instanceof Error ? error.message : String(error)
105-
throw new Error(`Failed to fetch buckets for asset proxy: ${message}`)
106-
}
102+
const count = Object.keys(this.buckets).length
103+
logger.info(
104+
`Fetched ${count} buckets from AWS account for asset proxy`
105+
)
107106
break
107+
}
108108

109109
default:
110110
break
@@ -120,10 +120,12 @@ export class AssetProxy {
120120
const command = new HeadBucketCommand({ Bucket: bucketName })
121121
const response = await s3Client.send(command)
122122
const statusCode = response.$metadata.httpStatusCode
123+
let name = null
123124
let region = null
124125

125126
switch (statusCode) {
126127
case 200:
128+
name = bucketName
127129
region = response.BucketRegion === 'EU'
128130
? 'eu-west-1'
129131
: response.BucketRegion || 'us-east-1'
@@ -141,7 +143,7 @@ export class AssetProxy {
141143
logger.warn(`Unexpected status code ${statusCode} for bucket ${bucketName}`)
142144
}
143145

144-
this.buckets[bucketName] = { name: bucketName, region }
146+
this.buckets[bucketName] = { name, region }
145147
}
146148
return this.buckets[bucketName]
147149
}
@@ -157,6 +159,42 @@ export class AssetProxy {
157159
}
158160
return false
159161
}
162+
}
163+
164+
export class AssetProxy {
165+
/**
166+
* @param {AssetBuckets} buckets - AssetBuckets instance
167+
* @param {number} urlExpiry - Pre-signed URL expiry time in seconds
168+
* @param {string} bucketOption - Bucket option (NONE, ALL, ALL_BUCKETS_IN_ACCOUNT, LIST)
169+
*/
170+
constructor(buckets, urlExpiry, bucketOption) {
171+
this.buckets = buckets
172+
this.urlExpiry = urlExpiry
173+
this.isEnabled = bucketOption !== BucketOption.NONE
174+
}
175+
176+
/**
177+
* @returns {Promise<AssetProxy>} Initialized AssetProxy instance
178+
*/
179+
static async create() {
180+
const bucketOption = process.env['ASSET_PROXY_BUCKET_OPTION'] || 'NONE'
181+
const urlExpiry = parseInt(process.env['ASSET_PROXY_URL_EXPIRY'] || '300', 10)
182+
const bucketList = process.env['ASSET_PROXY_BUCKET_LIST']
183+
184+
let bucketNames = null
185+
if (bucketOption === BucketOption.LIST) {
186+
if (!bucketList) {
187+
throw new Error(
188+
'ASSET_PROXY_BUCKET_LIST must be set when ASSET_PROXY_BUCKET_OPTION is LIST'
189+
)
190+
}
191+
bucketNames = bucketList.split(',').map((b) => b.trim()).filter((b) => b)
192+
}
193+
194+
const buckets = await AssetBuckets.create(bucketOption, bucketNames)
195+
196+
return new AssetProxy(buckets, urlExpiry, bucketOption)
197+
}
160198

161199
/**
162200
* @param {Object} assets - Assets object
@@ -185,7 +223,7 @@ export class AssetProxy {
185223
continue
186224
}
187225

188-
if (!this.shouldProxyBucket(bucket)) {
226+
if (!this.buckets.shouldProxyBucket(bucket)) {
189227
proxiedAssets[assetKey] = asset
190228
logger.warn(`Asset ${assetKey} bucket ${bucket} is not configured for proxying`)
191229
// eslint-disable-next-line no-continue
@@ -267,11 +305,11 @@ export class AssetProxy {
267305
}
268306

269307
const { bucket, key } = parseS3Url(asset.href)
270-
if (!bucket || !key || !this.shouldProxyBucket(bucket)) {
308+
if (!bucket || !key || !this.buckets.shouldProxyBucket(bucket)) {
271309
return null
272310
}
273311

274-
const region = await this.getBucket(bucket).then((b) => b.region)
312+
const region = await this.buckets.getBucket(bucket).then((b) => b.region)
275313
if (!region) {
276314
// Should not get here if bucketOption is LIST or ALL_BUCKETS_IN_ACCOUNT
277315
// If bucketOption is ALL, the bucket either does not exist or access is denied

‎tests/system/test-api-asset-proxy.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,8 +65,8 @@ test('AssetProxy initialized with ALL_BUCKETS_IN_ACCOUNT mode fetches buckets',
6565

6666
t.truthy(assetProxy.buckets)
6767
t.true(assetProxy.isEnabled)
68-
t.true(assetProxy.shouldProxyBucket('landsat-pds'))
69-
t.true(!assetProxy.shouldProxyBucket('some-other-bucket'))
68+
t.true(assetProxy.buckets.shouldProxyBucket('landsat-pds'))
69+
t.true(!assetProxy.buckets.shouldProxyBucket('some-other-bucket'))
7070
})
7171

7272
test('GET /collections/:collectionId/items/:itemId/assets/:assetKey - 302 redirect to presigned URL', async (t) => {

‎tests/unit/test-asset-proxy.js‎

Lines changed: 25 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -18,31 +18,17 @@ test('BucketOption - exports expected constants', (t) => {
1818
t.is(BucketOption.LIST, 'LIST')
1919
})
2020

21-
test('AssetProxy - constructor initializes with expected defaults', async (t) => {
21+
test.only('AssetProxy - constructor initializes with expected defaults', async (t) => {
2222
const before = { ...process.env }
2323
try {
2424
delete process.env['ASSET_PROXY_BUCKET_OPTION']
2525

2626
const proxy = await AssetProxy.create()
27-
t.is(proxy.bucketOption, 'NONE')
2827
t.is(proxy.urlExpiry, 300)
2928
t.is(proxy.isEnabled, false)
30-
} finally {
31-
process.env = before
32-
}
33-
})
34-
35-
test('AssetProxy - constructor reads env vars correctly', async (t) => {
36-
const before = { ...process.env }
37-
try {
38-
process.env['ASSET_PROXY_BUCKET_OPTION'] = 'ALL'
39-
process.env['ASSET_PROXY_URL_EXPIRY'] = '600'
40-
process.env['ASSET_PROXY_BUCKET_LIST'] = 'bucket1,bucket2'
41-
42-
const proxy = await AssetProxy.create()
43-
t.is(proxy.bucketOption, 'ALL')
44-
t.is(proxy.urlExpiry, 600)
45-
t.is(proxy.bucketList, 'bucket1,bucket2')
29+
t.is(proxy.buckets.bucketOption, 'NONE')
30+
t.is(proxy.buckets.bucketNames, null)
31+
t.deepEqual(proxy.buckets.buckets, {})
4632
} finally {
4733
process.env = before
4834
}
@@ -62,10 +48,10 @@ test('AssetProxy - LIST mode parses bucket list', async (t) => {
6248
const proxy = await AssetProxy.create()
6349

6450
t.truthy(proxy.buckets)
65-
t.truthy(proxy.buckets['bucket1'])
66-
t.truthy(proxy.buckets['bucket2'])
67-
t.truthy(proxy.buckets['bucket3'])
68-
t.is(Object.keys(proxy.buckets).length, 3)
51+
t.truthy(proxy.buckets.buckets['bucket1'])
52+
t.truthy(proxy.buckets.buckets['bucket2'])
53+
t.truthy(proxy.buckets.buckets['bucket3'])
54+
t.is(Object.keys(proxy.buckets.buckets).length, 3)
6955
} finally {
7056
process.env = before
7157
}
@@ -107,26 +93,10 @@ test('AssetProxy - ALL_BUCKETS_IN_ACCOUNT mode fetches buckets', async (t) => {
10793
const proxy = await AssetProxy.create()
10894

10995
t.truthy(proxy.buckets)
110-
t.truthy(proxy.buckets['bucket-1'])
111-
t.truthy(proxy.buckets['bucket-2'])
112-
t.is(proxy.buckets['some-other-bucket'], undefined)
113-
t.is(Object.keys(proxy.buckets).length, 2)
114-
} finally {
115-
process.env = before
116-
}
117-
})
118-
119-
test('AssetProxy - ALL_BUCKETS_IN_ACCOUNT mode throws on error', async (t) => {
120-
const before = { ...process.env }
121-
try {
122-
process.env['ASSET_PROXY_BUCKET_OPTION'] = 'ALL_BUCKETS_IN_ACCOUNT'
123-
124-
s3Mock.on(ListBucketsCommand).rejects(new Error('Access denied'))
125-
126-
await t.throwsAsync(
127-
async () => AssetProxy.create(),
128-
{ message: /Failed to fetch buckets for asset proxy: Access denied/ }
129-
)
96+
t.truthy(proxy.buckets.buckets['bucket-1'])
97+
t.truthy(proxy.buckets.buckets['bucket-2'])
98+
t.is(proxy.buckets.buckets['some-other-bucket'], undefined)
99+
t.is(Object.keys(proxy.buckets.buckets).length, 2)
130100
} finally {
131101
process.env = before
132102
}
@@ -196,32 +166,32 @@ test('AssetProxy - isEnabled returns true for ALL_BUCKETS_IN_ACCOUNT', async (t)
196166
}
197167
})
198168

199-
test('AssetProxy - shouldProxyBucket() with NONE mode returns false', async (t) => {
169+
test('AssetProxy - bucket filtering with NONE mode returns false', async (t) => {
200170
const before = { ...process.env }
201171
try {
202172
process.env['ASSET_PROXY_BUCKET_OPTION'] = 'NONE'
203173

204174
const proxy = await AssetProxy.create()
205-
t.false(proxy.shouldProxyBucket('any-bucket'))
175+
t.false(proxy.buckets.shouldProxyBucket('any-bucket'))
206176
} finally {
207177
process.env = before
208178
}
209179
})
210180

211-
test('AssetProxy - shouldProxyBucket() with ALL mode returns true', async (t) => {
181+
test('AssetProxy - bucket filtering with ALL mode returns true', async (t) => {
212182
const before = { ...process.env }
213183
try {
214184
process.env['ASSET_PROXY_BUCKET_OPTION'] = 'ALL'
215185

216186
const proxy = await AssetProxy.create()
217-
t.true(proxy.shouldProxyBucket('any-bucket'))
218-
t.true(proxy.shouldProxyBucket('another-bucket'))
187+
t.true(proxy.buckets.shouldProxyBucket('any-bucket'))
188+
t.true(proxy.buckets.shouldProxyBucket('another-bucket'))
219189
} finally {
220190
process.env = before
221191
}
222192
})
223193

224-
test('AssetProxy - shouldProxyBucket() with LIST mode only proxies buckets in list', async (t) => {
194+
test('AssetProxy - bucket filtering with LIST mode only proxies buckets in list', async (t) => {
225195
const before = { ...process.env }
226196
try {
227197
process.env['ASSET_PROXY_BUCKET_OPTION'] = 'LIST'
@@ -234,15 +204,15 @@ test('AssetProxy - shouldProxyBucket() with LIST mode only proxies buckets in li
234204

235205
const proxy = await AssetProxy.create()
236206

237-
t.true(proxy.shouldProxyBucket('allowed-bucket'))
238-
t.true(proxy.shouldProxyBucket('another-allowed'))
239-
t.false(proxy.shouldProxyBucket('not-in-list'))
207+
t.true(proxy.buckets.shouldProxyBucket('allowed-bucket'))
208+
t.true(proxy.buckets.shouldProxyBucket('another-allowed'))
209+
t.false(proxy.buckets.shouldProxyBucket('not-in-list'))
240210
} finally {
241211
process.env = before
242212
}
243213
})
244214

245-
test('AssetProxy - shouldProxyBucket() with ALL_BUCKETS_IN_ACCOUNT mode only proxies fetched buckets', async (t) => {
215+
test('AssetProxy - bucket filtering with ALL_BUCKETS_IN_ACCOUNT mode only proxies fetched buckets', async (t) => {
246216
const before = { ...process.env }
247217
try {
248218
process.env['ASSET_PROXY_BUCKET_OPTION'] = 'ALL_BUCKETS_IN_ACCOUNT'
@@ -261,9 +231,9 @@ test('AssetProxy - shouldProxyBucket() with ALL_BUCKETS_IN_ACCOUNT mode only pro
261231

262232
const proxy = await AssetProxy.create()
263233

264-
t.true(proxy.shouldProxyBucket('fetched-bucket-1'))
265-
t.true(proxy.shouldProxyBucket('fetched-bucket-2'))
266-
t.false(proxy.shouldProxyBucket('not-fetched-bucket'))
234+
t.true(proxy.buckets.shouldProxyBucket('fetched-bucket-1'))
235+
t.true(proxy.buckets.shouldProxyBucket('fetched-bucket-2'))
236+
t.false(proxy.buckets.shouldProxyBucket('not-fetched-bucket'))
267237
} finally {
268238
process.env = before
269239
}

0 commit comments

Comments
 (0)