coalesce options bucket and getKey

[mifi]

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

#4763 (comment)

closes #4898

[github-actions]

Diff output files

diff --git a/packages/@uppy/companion/lib/server/Uploader.d.ts b/packages/@uppy/companion/lib/server/Uploader.d.ts
index 41e1124..945e941 100644
--- a/packages/@uppy/companion/lib/server/Uploader.d.ts
+++ b/packages/@uppy/companion/lib/server/Uploader.d.ts
@@ -94,15 +94,16 @@ declare class Uploader {
     downloadedBytes: number;
     readStream: import("stream").Readable | fs.ReadStream;
     abortReadStream(err: any): void;
-    _uploadByProtocol(): Promise<any>;
+    _uploadByProtocol(req: any): Promise<any>;
     _downloadStreamAsFile(stream: any): Promise<void>;
     tmpPath: string;
     _needDownloadFirst(): boolean;
     /**
      *
      * @param {import('stream').Readable} stream
+     * @param {import('express').Request} req
      */
-    uploadStream(stream: import('stream').Readable): Promise<{
+    uploadStream(stream: import('stream').Readable, req: import('express').Request): Promise<{
         url: any;
         extraData: any;
     }>;
@@ -110,8 +111,9 @@ declare class Uploader {
     /**
      *
      * @param {import('stream').Readable} stream
+     * @param {import('express').Request} req
      */
-    tryUploadStream(stream: import('stream').Readable): Promise<void>;
+    tryUploadStream(stream: import('stream').Readable, req: import('express').Request): Promise<void>;
     /**
      * returns a substring of the token. Used as traceId for logging
      * we avoid using the entire token because this is meant to be a short term
diff --git a/packages/@uppy/companion/lib/server/Uploader.js b/packages/@uppy/companion/lib/server/Uploader.js
index a391f1f..8368897 100644
--- a/packages/@uppy/companion/lib/server/Uploader.js
+++ b/packages/@uppy/companion/lib/server/Uploader.js
@@ -192,7 +192,7 @@ class Uploader {
       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;
@@ -200,7 +200,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:
@@ -231,8 +231,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");
@@ -253,7 +254,7 @@ class Uploader {
         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)),
       ]);
@@ -274,11 +275,12 @@ 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;
       }
@@ -564,7 +566,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.");
     }
@@ -573,12 +575,13 @@ class Uploader {
      * @type {{client: import('@aws-sdk/client-s3').S3Client, options: Record<string, any>}}
      */
     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,
     };
     if (options.acl != null) {
diff --git a/packages/@uppy/companion/lib/server/controllers/s3.js b/packages/@uppy/companion/lib/server/controllers/s3.js
index 2c49841..7526f30 100644
--- a/packages/@uppy/companion/lib/server/controllers/s3.js
+++ b/packages/@uppy/companion/lib/server/controllers/s3.js
@@ -49,9 +49,9 @@ module.exports = function s3(config) {
     if (!client) {
       return;
     }
-    const bucket = getBucket(config.bucket, req);
-    const metadata = req.query.metadata || {};
-    const key = config.getKey(req, req.query.filename, metadata);
+    const { metadata = {}, filename } = req.query;
+    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;
@@ -100,8 +100,9 @@ module.exports = function s3(config) {
     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;
@@ -110,7 +111,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,
       Key: key,
@@ -153,7 +153,7 @@ module.exports = function s3(config) {
       });
       return;
     }
-    const bucket = getBucket(config.bucket, req);
+    const bucket = getBucket({ bucketOrFn: config.bucket, req });
     const parts = [];
     function listPartsPage(startAt) {
       client.send(
@@ -205,7 +205,7 @@ module.exports = function s3(config) {
       res.status(400).json({ error: "s3: the part number must be a number between 1 and 10000." });
       return;
     }
-    const bucket = getBucket(config.bucket, req);
+    const bucket = getBucket({ bucketOrFn: config.bucket, req });
     getSignedUrl(
       client,
       new UploadPartCommand({
@@ -258,7 +258,7 @@ module.exports = function s3(config) {
       res.status(400).json({ error: "s3: the part numbers must be a number between 1 and 10000." });
       return;
     }
-    const bucket = getBucket(config.bucket, req);
+    const bucket = getBucket({ bucketOrFn: config.bucket, req });
     Promise.all(partNumbersArray.map((partNumber) => {
       return getSignedUrl(
         client,
@@ -302,7 +302,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,
@@ -346,7 +346,7 @@ module.exports = function s3(config) {
       res.status(400).json({ error: "s3: `parts` must be an array of {ETag, PartNumber} objects." });
       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/lib/server/helpers/upload.js b/packages/@uppy/companion/lib/server/helpers/upload.js
index 923af2a..92991a8 100644
--- a/packages/@uppy/companion/lib/server/helpers/upload.js
+++ b/packages/@uppy/companion/lib/server/helpers/upload.js
@@ -17,7 +17,7 @@ async function startDownUpload({ req, res, getSize, download }) {
       logger.debug("Waiting for socket connection before beginning remote download/upload.", null, req.id);
       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
     // NOTE: the Uploader will continue running after the http request is responded
diff --git a/packages/@uppy/companion/lib/server/helpers/utils.d.ts b/packages/@uppy/companion/lib/server/helpers/utils.d.ts
index 106da9c..e8462ea 100644
--- a/packages/@uppy/companion/lib/server/helpers/utils.d.ts
+++ b/packages/@uppy/companion/lib/server/helpers/utils.d.ts
@@ -3,11 +3,22 @@ export function jsonStringify(data: object): string;
 export function getURLBuilder(options: object): (subPath: string, isExternal: boolean, excludeHost?: boolean) => string;
 export function encrypt(input: string, secret: string | Buffer): string;
 export function decrypt(encrypted: string, secret: string | Buffer): string;
-export function defaultGetKey(req: any, filename: any): string;
+export function defaultGetKey({ filename }: {
+    filename: any;
+}): string;
 export function prepareStream(stream: any): Promise<any>;
 export function getBasicAuthHeader(key: any, secret: any): string;
 export function rfc2047EncodeMetadata(metadata: any): any;
-export function getBucket(bucketOrFn: any, req: any, metadata: any): string;
+export function getBucket({ bucketOrFn, req, metadata, filename }: {
+    bucketOrFn: string | ((a: {
+        req: import('express').Request;
+        metadata: Record<string, string>;
+        filename: string | undefined;
+    }) => string);
+    req: import('express').Request;
+    metadata?: Record<string, string>;
+    filename?: string;
+}): string;
 export class StreamHttpJsonError extends Error {
     constructor({ statusCode, responseJson }: {
         statusCode: any;
diff --git a/packages/@uppy/companion/lib/server/helpers/utils.js b/packages/@uppy/companion/lib/server/helpers/utils.js
index dd33ba5..b998ab3 100644
--- a/packages/@uppy/companion/lib/server/helpers/utils.js
+++ b/packages/@uppy/companion/lib/server/helpers/utils.js
@@ -127,7 +127,7 @@ module.exports.decrypt = (encrypted, secret) => {
   decrypted += decipher.final("utf8");
   return decrypted;
 };
-module.exports.defaultGetKey = (req, filename) => `${crypto.randomUUID()}-${filename}`;
+module.exports.defaultGetKey = ({ filename }) => `${crypto.randomUUID()}-${filename}`;
 class StreamHttpJsonError extends Error {
   statusCode;
   responseJson;
@@ -180,8 +180,21 @@ const rfc2047Encode = (dataIn) => {
 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<string, string>,
+ * filename: string | undefined,
+ * }) => string),
+ * req: import('express').Request,
+ * metadata?: Record<string, string>,
+ * 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
     throw new TypeError("s3: bucket key must be a string or a function resolving the bucket string");

[aduh95]

I don't think removing req is a good idea, it was requested and implemented by the community (#4488), IMO we should not remove it without a good reason (and ideally an alternative), and we should first deprecate it.

[Murderlon]

Does this PR close this? #4898

[mifi]

Does this PR close this? #4898

yes I believe so too!

[mifi]

I don't think removing req is a good idea, it was requested and implemented by the community (#4488), IMO we should not remove it without a good reason (and ideally an alternative), and we should first deprecate it.

I was afraid that storing the req object (in the callstack in Uploader.js) after res.end() has been called might cause trouble. but I guess we can try and see if it works well. When I think more about it I agree it's nice to be able to access stuff on the req object. I've added a note in the docs that the user shouldn't try to access internal companion data on the Request object.

[aduh95]

Can we add a note in the migration guide?

[mifi]

Can we add a note in the migration guide?

done

[aduh95]

I'm not a fan of the name bucketOrFn, but I don't really have an alternative, I guess it's fine