diff --git a/packages/@uppy/companion/__mocks__/tus-js-client.ts b/packages/@uppy/companion/__mocks__/tus-js-client.ts index dfe41dbbd..b5af738e2 100644 --- a/packages/@uppy/companion/__mocks__/tus-js-client.ts +++ b/packages/@uppy/companion/__mocks__/tus-js-client.ts @@ -4,12 +4,23 @@ type UploadOptions = { onSuccess: () => void } & Record +let lastUploadFile: unknown + +export function __getLastUploadFile(): unknown { + return lastUploadFile +} + +export function __resetTusMockState(): void { + lastUploadFile = undefined +} + export class Upload { url: string options: UploadOptions - constructor(_file: unknown, options: UploadOptions) { + constructor(file: unknown, options: UploadOptions) { + lastUploadFile = file this.url = 'https://tus.endpoint/files/foo-bar' this.options = options } diff --git a/packages/@uppy/companion/src/server/Uploader.ts b/packages/@uppy/companion/src/server/Uploader.ts index d81acb886..6cdd1957f 100644 --- a/packages/@uppy/companion/src/server/Uploader.ts +++ b/packages/@uppy/companion/src/server/Uploader.ts @@ -3,7 +3,7 @@ import { createReadStream, createWriteStream, ReadStream } from 'node:fs' import { stat, unlink } from 'node:fs/promises' import { join } from 'node:path' import type { Readable as NodeReadableStream } from 'node:stream' -import { Readable as NodeReadable, Transform } from 'node:stream' +import { Transform } from 'node:stream' import { pipeline } from 'node:stream/promises' import type { PutObjectCommandInput, S3Client } from '@aws-sdk/client-s3' import { Upload } from '@aws-sdk/lib-storage' @@ -803,19 +803,7 @@ export default class Uploader { tusOptions.uploadSize = this.size } - const nodeReader = NodeReadable.toWeb(stream).getReader() - const reader: Pick, 'read'> = { - async read() { - const { done, value } = await nodeReader.read() - // Ensure we always provide `value`, since some ReadableStream lib variants - // type it differently when `done: true`. - return done - ? { done: true, value: undefined } - : { done: false, value } - }, - } - - this.tus = new tus.Upload(reader, tusOptions) + this.tus = new tus.Upload(stream, tusOptions) this.tus.start() }) diff --git a/packages/@uppy/companion/src/server/s3-client.ts b/packages/@uppy/companion/src/server/s3-client.ts index bece0570f..3472b0d45 100644 --- a/packages/@uppy/companion/src/server/s3-client.ts +++ b/packages/@uppy/companion/src/server/s3-client.ts @@ -32,6 +32,10 @@ export default function s3Client( const v = obj[key] return typeof v === 'boolean' ? v : undefined } + const isRegion = ( + value: unknown, + ): value is NonNullable => + typeof value === 'string' || typeof value === 'function' if (s3['accessKeyId'] || s3['secretAccessKey']) { throw new Error( @@ -51,25 +55,23 @@ export default function s3Client( ) } - const region = - getString(s3, 'region') ?? - (rawClientOptions && typeof rawClientOptions['region'] === 'string' - ? rawClientOptions['region'] - : undefined) - if (!region) { - // No region means this Companion instance isn't configured for S3 uploads. - return null - } + const explicitRegion = getString(s3, 'region') + const rawClientRegion = rawClientOptions?.['region'] + const region = explicitRegion ?? (isRegion(rawClientRegion) ? rawClientRegion : undefined) - let endpoint: unknown = s3.endpoint - if (typeof endpoint === 'string') { + let endpoint: string | undefined = + typeof s3.endpoint === 'string' ? s3.endpoint : undefined + if (endpoint) { // TODO: deprecate those replacements in favor of what AWS SDK supports out of the box. - endpoint = endpoint.replace(/{service}/, 's3').replace(/{region}/, region) + endpoint = endpoint.replace(/{service}/, 's3') + if (explicitRegion) { + endpoint = endpoint.replace(/{region}/, explicitRegion) + } } const forcePathStyle = getBool(s3, 'forcePathStyle') let s3ClientOptions: S3ClientConfig = { - region, + ...(region === undefined ? {} : { region }), ...(forcePathStyle === undefined ? {} : { forcePathStyle }), } @@ -96,7 +98,7 @@ export default function s3Client( } } else { // no accelearate, we allow custom s3 endpoint - if (typeof endpoint === 'string') { + if (endpoint !== undefined) { s3ClientOptions = { ...s3ClientOptions, endpoint, @@ -124,7 +126,15 @@ export default function s3Client( ...(sessionToken ? { sessionToken } : {}), } } - s3Client = new S3Client(s3ClientOptions) + try { + s3Client = new S3Client(s3ClientOptions) + } catch (err) { + // Keep S3 optional when no region can be resolved at all. + if (err instanceof Error && err.message === 'Region is missing') { + return null + } + throw err + } } return s3Client diff --git a/packages/@uppy/companion/test/s3-client.test.ts b/packages/@uppy/companion/test/s3-client.test.ts new file mode 100644 index 000000000..c56ee595e --- /dev/null +++ b/packages/@uppy/companion/test/s3-client.test.ts @@ -0,0 +1,51 @@ +import { afterEach, describe, expect, test } from 'vitest' +import { defaultOptions } from '../src/config/companion.ts' +import s3Client from '../src/server/s3-client.ts' +import type { CompanionRuntimeOptions } from '../src/types/companion-options.ts' + +const originalAwsRegion = process.env['AWS_REGION'] + +afterEach(() => { + if (originalAwsRegion === undefined) { + delete process.env['AWS_REGION'] + } else { + process.env['AWS_REGION'] = originalAwsRegion + } +}) + +describe('s3-client', () => { + test('creates an S3 client when region comes from AWS SDK environment resolution', () => { + process.env['AWS_REGION'] = 'us-east-1' + + const options = { + ...defaultOptions, + s3: { + ...defaultOptions.s3, + endpoint: 'https://s3.amazonaws.com', + bucket: 'test-bucket', + key: 'test-key', + secret: 'test-secret', + }, + } satisfies CompanionRuntimeOptions + + expect(s3Client(options)).not.toBeNull() + }) + + test('creates an S3 client when region is provided as awsClientOptions provider', () => { + const options = { + ...defaultOptions, + s3: { + ...defaultOptions.s3, + endpoint: 'https://s3.amazonaws.com', + bucket: 'test-bucket', + key: 'test-key', + secret: 'test-secret', + awsClientOptions: { + region: async () => 'us-east-1', + }, + }, + } satisfies CompanionRuntimeOptions + + expect(s3Client(options)).not.toBeNull() + }) +}) diff --git a/packages/@uppy/companion/test/uploader.test.ts b/packages/@uppy/companion/test/uploader.test.ts index 8b5beda4e..aa88ca762 100644 --- a/packages/@uppy/companion/test/uploader.test.ts +++ b/packages/@uppy/companion/test/uploader.test.ts @@ -4,6 +4,7 @@ import type { Request } from 'express' import express from 'express' import nock from 'nock' import request from 'supertest' +import * as tusClient from 'tus-js-client' import { afterAll, afterEach, @@ -24,8 +25,25 @@ import * as socketClient from './mocksocket.ts' vi.mock('tus-js-client') vi.mock('express-prom-bundle') +function getTusMockLastUploadFile(): unknown { + const getter = Reflect.get(tusClient, '__getLastUploadFile') + if (typeof getter !== 'function') { + throw new Error('Expected tus mock to expose __getLastUploadFile()') + } + return getter() +} + +function resetTusMockState(): void { + const reset = Reflect.get(tusClient, '__resetTusMockState') + if (typeof reset !== 'function') { + throw new Error('Expected tus mock to expose __resetTusMockState()') + } + reset() +} + afterEach(() => { nock.cleanAll() + resetTusMockState() }) afterAll(() => { nock.restore() @@ -162,6 +180,7 @@ describe('uploader', () => { socketClient.onUploadSuccess(uploadToken, onUploadSuccess) await promise await uploader.tryUploadStream(stream, requireMockReq()) + expect(getTusMockLastUploadFile()).toBeInstanceOf(Readable) expect(onUploadError).not.toHaveBeenCalled()