From 151da8bdfe2ac88fdeb7fa06f626c9b1abb395d0 Mon Sep 17 00:00:00 2001 From: Mikael Finstad Date: Thu, 13 Jun 2024 23:32:38 +0200 Subject: [PATCH] coalesce options `bucket` and `getKey` (#5169) * coalesce options `bucket` and `getKey` this is a breaking change! also: - remove `req` because it's an internal, unstable api that might change any time and should not be used directly - use an params object for getKey/bucket functions instead of positional arguments, to make it more clean, and easier to add more data in the future - add metadata to `bucket` whenever we are aware of it * fix lint * Apply suggestions from code review Co-authored-by: Antoine du Hamel * add back `Request` object also add filename to `bucket` for consistency * add migration steps * fix lint * Update docs/guides/migration-guides.md Co-authored-by: Antoine du Hamel --------- Co-authored-by: Antoine du Hamel --- docs/companion.md | 29 +++++++++---------- docs/guides/migration-guides.md | 4 +++ examples/aws-companion/server.cjs | 2 +- examples/digitalocean-spaces/server.cjs | 2 +- .../@uppy/companion/src/server/Uploader.js | 25 +++++++++------- .../companion/src/server/controllers/s3.js | 26 ++++++++++------- .../companion/src/server/helpers/upload.js | 2 +- .../companion/src/server/helpers/utils.js | 20 +++++++++++-- .../companion/test/__tests__/uploader.js | 8 +++-- 9 files changed, 71 insertions(+), 47 deletions(-) diff --git a/docs/companion.md b/docs/companion.md index 742c25458..6a8fffbf1 100644 --- a/docs/companion.md +++ b/docs/companion.md @@ -283,7 +283,7 @@ const options = { endpoint: 'https://{service}.{region}.amazonaws.com', conditions: [], useAccelerateEndpoint: false, - getKey: (req, filename) => `${crypto.randomUUID()}-${filename}`, + getKey: ({ filename }) => `${crypto.randomUUID()}-${filename}`, expires: 800, // seconds }, allowLocalUrls: false, @@ -473,13 +473,14 @@ from the AWS SDK. The name of the bucket to store uploaded files in. -It can be function that returns the name of the bucket as a `string` and takes -the following arguments: +A `string` or function that returns the name of the bucket as a `string` and +takes one argument which is an object with the following properties: -- [`http.IncomingMessage`][], the HTTP request (will be `null` for remote - uploads) -- metadata provided by the user for the file (will be `undefined` for local - uploads) +- `filename`, the original name of the uploaded file; +- `metadata` provided by the user for the file (will only be provided during the + initial calls for each uploaded files, otherwise it will be `undefined`). +- `req`, Express.js `Request` object. Do not use any Companion internals from + the req object, as these might change in any minor version of Companion. ##### `s3.region` `COMPANION_AWS_REGION` @@ -508,18 +509,16 @@ expected, please [open an issue on the Uppy repository](https://github.com/transloadit/uppy/issues/new) so we can document it here. -##### `s3.getKey(req, filename, metadata)` +##### `s3.getKey({ filename, metadata, req })` Get the key name for a file. The key is the file path to which the file will be uploaded in your bucket. This option should be a function receiving three arguments: -- `req` [`http.IncomingMessage`][], the HTTP request, for _regular_ S3 uploads - using the `@uppy/aws-s3` plugin. This parameter is _not_ available for - multipart uploads using the `@uppy/aws-s3` or `@uppy/aws-s3-multipart` - plugins. This parameter is `null` for remote uploads. - `filename`, the original name of the uploaded file; - `metadata`, user-provided metadata for the file. +- `req`, Express.js `Request` object. Do not use any Companion internals from + the req object, as these might change in any minor version of Companion. This function should return a string `key`. The `req` parameter can be used to upload to a user-specific folder in your bucket, for example: @@ -530,7 +529,7 @@ app.use( uppy.app({ providerOptions: { s3: { - getKey: (req, filename, metadata) => `${req.user.id}/${filename}`, + getKey: ({ req, filename, metadata }) => `${req.user.id}/${filename}`, /* auth options */ }, }, @@ -546,7 +545,7 @@ app.use( uppy.app({ providerOptions: { s3: { - getKey: (req, filename, metadata) => filename, + getKey: ({ filename, metadata }) => filename, }, }, }), @@ -909,8 +908,6 @@ This would get the Companion instance running on `http://localhost:3020`. It uses [`node --watch`](https://nodejs.org/api/cli.html#--watch) so it will automatically restart when files are changed. -[`http.incomingmessage`]: - https://nodejs.org/api/http.html#class-httpincomingmessage [box]: /docs/box [dropbox]: /docs/dropbox [facebook]: /docs/facebook diff --git a/docs/guides/migration-guides.md b/docs/guides/migration-guides.md index ba4dcfabc..03694fa5b 100644 --- a/docs/guides/migration-guides.md +++ b/docs/guides/migration-guides.md @@ -11,6 +11,10 @@ These cover all the major Uppy versions and how to migrate to them. - The URL endpoint (used by the `Url`/`Link` plugin) is now turned off by default and must be explicitly enabled with `COMPANION_ENABLE_URL_ENDPOINT=true` or `enableUrlEndpoint: true`. +- The option `getKey(req, filename, metadata)` has changed signature to + `getKey({ filename, metadata, req })`. +- The option `bucket(req, metadata)` has changed signature to + `bucketOrFn({ req, metadata, filename })`. - Custom provider breaking changes. If you have not implemented a custom provider, you should not be affected. - The static `getExtraConfig` property has been renamed to diff --git a/examples/aws-companion/server.cjs b/examples/aws-companion/server.cjs index 98dc31231..c64f9830f 100644 --- a/examples/aws-companion/server.cjs +++ b/examples/aws-companion/server.cjs @@ -29,7 +29,7 @@ const options = { }, }, s3: { - getKey: (req, filename) => `${crypto.randomUUID()}-${filename}`, + getKey: ({ filename }) => `${crypto.randomUUID()}-${filename}`, key: process.env.COMPANION_AWS_KEY, secret: process.env.COMPANION_AWS_SECRET, bucket: process.env.COMPANION_AWS_BUCKET, diff --git a/examples/digitalocean-spaces/server.cjs b/examples/digitalocean-spaces/server.cjs index dc369fa8b..f98219648 100644 --- a/examples/digitalocean-spaces/server.cjs +++ b/examples/digitalocean-spaces/server.cjs @@ -37,7 +37,7 @@ const { app: companionApp } = companion.app({ s3: { // This is the crucial part; set an endpoint template for the service you want to use. endpoint: 'https://{region}.digitaloceanspaces.com', - getKey: (req, filename) => `${crypto.randomUUID()}-${filename}`, + getKey: ({ filename }) => `${crypto.randomUUID()}-${filename}`, key: process.env.COMPANION_AWS_KEY, secret: process.env.COMPANION_AWS_SECRET, diff --git a/packages/@uppy/companion/src/server/Uploader.js b/packages/@uppy/companion/src/server/Uploader.js index e3cfb3adb..25febdb30 100644 --- a/packages/@uppy/companion/src/server/Uploader.js +++ b/packages/@uppy/companion/src/server/Uploader.js @@ -220,7 +220,7 @@ class Uploader { if (this.readStream) this.readStream.destroy(err) } - async _uploadByProtocol() { + async _uploadByProtocol(req) { // todo a default protocol should not be set. We should ensure that the user specifies their protocol. // after we drop old versions of uppy client we can remove this const protocol = this.options.protocol || PROTOCOLS.multipart @@ -229,7 +229,7 @@ class Uploader { case PROTOCOLS.multipart: return this.#uploadMultipart(this.readStream) case PROTOCOLS.s3Multipart: - return this.#uploadS3Multipart(this.readStream) + return this.#uploadS3Multipart(this.readStream, req) case PROTOCOLS.tus: return this.#uploadTus(this.readStream) default: @@ -271,8 +271,9 @@ class Uploader { /** * * @param {import('stream').Readable} stream + * @param {import('express').Request} req */ - async uploadStream(stream) { + async uploadStream(stream, req) { try { if (this.#uploadState !== states.idle) throw new Error('Can only start an upload in the idle state') if (this.readStream) throw new Error('Already uploading') @@ -290,7 +291,7 @@ class Uploader { if (this.#uploadState !== states.uploading) return undefined const { url, extraData } = await Promise.race([ - this._uploadByProtocol(), + this._uploadByProtocol(req), // If we don't handle stream errors, we get unhandled error in node. new Promise((resolve, reject) => this.readStream.on('error', reject)), ]) @@ -310,12 +311,13 @@ class Uploader { /** * * @param {import('stream').Readable} stream + * @param {import('express').Request} req */ - async tryUploadStream(stream) { + async tryUploadStream(stream, req) { try { emitter().emit('upload-start', { token: this.token }) - const ret = await this.uploadStream(stream) + const ret = await this.uploadStream(stream, req) if (!ret) return const { url, extraData } = ret this.#emitSuccess(url, extraData) @@ -637,7 +639,7 @@ class Uploader { /** * Upload the file to S3 using a Multipart upload. */ - async #uploadS3Multipart(stream) { + async #uploadS3Multipart(stream, req) { if (!this.options.s3) { throw new Error('The S3 client is not configured on this companion instance.') } @@ -647,13 +649,14 @@ class Uploader { * @type {{client: import('@aws-sdk/client-s3').S3Client, options: Record}} */ const s3Options = this.options.s3 + const { metadata } = this.options const { client, options } = s3Options const params = { - Bucket: getBucket(options.bucket, null, this.options.metadata), - Key: options.getKey(null, filename, this.options.metadata), - ContentType: this.options.metadata.type, - Metadata: rfc2047EncodeMetadata(this.options.metadata), + Bucket: getBucket({ bucketOrFn: options.bucket, req, metadata }), + Key: options.getKey({ req, filename, metadata }), + ContentType: metadata.type, + Metadata: rfc2047EncodeMetadata(metadata), Body: stream, } diff --git a/packages/@uppy/companion/src/server/controllers/s3.js b/packages/@uppy/companion/src/server/controllers/s3.js index fc189d3b2..a084de2cb 100644 --- a/packages/@uppy/companion/src/server/controllers/s3.js +++ b/packages/@uppy/companion/src/server/controllers/s3.js @@ -52,10 +52,11 @@ module.exports = function s3 (config) { const client = getS3Client(req, res) if (!client) return - const bucket = getBucket(config.bucket, req) + const { metadata = {}, filename } = req.query - const metadata = req.query.metadata || {} - const key = config.getKey(req, req.query.filename, metadata) + const bucket = getBucket({ bucketOrFn: config.bucket, req, filename, metadata }) + + const key = config.getKey({ req, filename, metadata }) if (typeof key !== 'string') { res.status(500).json({ error: 'S3 uploads are misconfigured: filename returned from `getKey` must be a string' }) return @@ -106,8 +107,12 @@ module.exports = function s3 (config) { const client = getS3Client(req, res) if (!client) return - const key = config.getKey(req, req.body.filename, req.body.metadata || {}) - const { type, metadata } = req.body + const { type, metadata = {}, filename } = req.body + + const key = config.getKey({ req, filename, metadata }) + + const bucket = getBucket({ bucketOrFn: config.bucket, req, filename, metadata }) + if (typeof key !== 'string') { res.status(500).json({ error: 's3: filename returned from `getKey` must be a string' }) return @@ -116,7 +121,6 @@ module.exports = function s3 (config) { res.status(400).json({ error: 's3: content type must be a string' }) return } - const bucket = getBucket(config.bucket, req) const params = { Bucket: bucket, @@ -160,7 +164,7 @@ module.exports = function s3 (config) { return } - const bucket = getBucket(config.bucket, req) + const bucket = getBucket({ bucketOrFn: config.bucket, req }) const parts = [] @@ -211,7 +215,7 @@ module.exports = function s3 (config) { return } - const bucket = getBucket(config.bucket, req) + const bucket = getBucket({ bucketOrFn: config.bucket, req }) getSignedUrl(client, new UploadPartCommand({ Bucket: bucket, @@ -260,7 +264,7 @@ module.exports = function s3 (config) { return } - const bucket = getBucket(config.bucket, req) + const bucket = getBucket({ bucketOrFn: config.bucket, req }) Promise.all( partNumbersArray.map((partNumber) => { @@ -303,7 +307,7 @@ module.exports = function s3 (config) { return } - const bucket = getBucket(config.bucket, req) + const bucket = getBucket({ bucketOrFn: config.bucket, req }) client.send(new AbortMultipartUploadCommand({ Bucket: bucket, @@ -344,7 +348,7 @@ module.exports = function s3 (config) { return } - const bucket = getBucket(config.bucket, req) + const bucket = getBucket({ bucketOrFn: config.bucket, req }) client.send(new CompleteMultipartUploadCommand({ Bucket: bucket, diff --git a/packages/@uppy/companion/src/server/helpers/upload.js b/packages/@uppy/companion/src/server/helpers/upload.js index 5f637c43b..24de62705 100644 --- a/packages/@uppy/companion/src/server/helpers/upload.js +++ b/packages/@uppy/companion/src/server/helpers/upload.js @@ -21,7 +21,7 @@ async function startDownUpload({ req, res, getSize, download }) { await uploader.awaitReady(clientSocketConnectTimeout) logger.debug('Socket connection received. Starting remote download/upload.', null, req.id) - await uploader.tryUploadStream(stream) + await uploader.tryUploadStream(stream, req) })().catch((err) => logger.error(err)) // Respond the request diff --git a/packages/@uppy/companion/src/server/helpers/utils.js b/packages/@uppy/companion/src/server/helpers/utils.js index 627d87fcb..0f8ededaa 100644 --- a/packages/@uppy/companion/src/server/helpers/utils.js +++ b/packages/@uppy/companion/src/server/helpers/utils.js @@ -146,7 +146,7 @@ module.exports.decrypt = (encrypted, secret) => { return decrypted } -module.exports.defaultGetKey = (req, filename) => `${crypto.randomUUID()}-${filename}` +module.exports.defaultGetKey = ({ filename }) => `${crypto.randomUUID()}-${filename}` class StreamHttpJsonError extends Error { statusCode @@ -207,8 +207,22 @@ module.exports.rfc2047EncodeMetadata = (metadata) => ( Object.fromEntries(Object.entries(metadata).map((entry) => entry.map(rfc2047Encode))) ) -module.exports.getBucket = (bucketOrFn, req, metadata) => { - const bucket = typeof bucketOrFn === 'function' ? bucketOrFn(req, metadata) : bucketOrFn +/** + * + * @param {{ + * bucketOrFn: string | ((a: { + * req: import('express').Request, + * metadata: Record, + * filename: string | undefined, + * }) => string), + * req: import('express').Request, + * metadata?: Record, + * filename?: string, + * }} param0 + * @returns + */ +module.exports.getBucket = ({ bucketOrFn, req, metadata, filename }) => { + const bucket = typeof bucketOrFn === 'function' ? bucketOrFn({ req, metadata, filename }) : bucketOrFn if (typeof bucket !== 'string' || bucket === '') { // This means a misconfiguration or bug diff --git a/packages/@uppy/companion/test/__tests__/uploader.js b/packages/@uppy/companion/test/__tests__/uploader.js index ac258c867..3e6d1398a 100644 --- a/packages/@uppy/companion/test/__tests__/uploader.js +++ b/packages/@uppy/companion/test/__tests__/uploader.js @@ -22,6 +22,8 @@ process.env.COMPANION_DATADIR = './test/output' process.env.COMPANION_DOMAIN = 'localhost:3020' const { companionOptions } = standalone() +const mockReq = {} + describe('uploader with tus protocol', () => { test('uploader respects uploadUrls', async () => { const opts = { @@ -87,7 +89,7 @@ describe('uploader with tus protocol', () => { }) socketClient.onUploadSuccess(uploadToken, onUploadSuccess) await promise - await uploader.tryUploadStream(stream) + await uploader.tryUploadStream(stream, mockReq) expect(firstReceivedProgress).toBe(8) @@ -136,7 +138,7 @@ describe('uploader with tus protocol', () => { return new Promise((resolve, reject) => { // validate that the test is resolved on socket connection uploader.awaitReady(60000).then(() => { - uploader.tryUploadStream(stream).then(() => { + uploader.tryUploadStream(stream, mockReq).then(() => { try { expect(fs.existsSync(uploader.path)).toBe(false) resolve() @@ -286,7 +288,7 @@ describe('uploader with tus protocol', () => { const uploadToken = uploader.token // validate that the test is resolved on socket connection - uploader.awaitReady(60000).then(() => uploader.tryUploadStream(stream)) + uploader.awaitReady(60000).then(() => uploader.tryUploadStream(stream, mockReq)) socketClient.connect(uploadToken) return new Promise((resolve, reject) => {