coalesce options bucket and getKey
#5169
Open
+67
−47
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
Diff output filesdiff --git a/packages/@uppy/companion/lib/server/Uploader.d.ts b/packages/@uppy/companion/lib/server/Uploader.d.ts
index ae38c05..9a696f5 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 d487105..e6c5899 100644
--- a/packages/@uppy/companion/lib/server/Uploader.js
+++ b/packages/@uppy/companion/lib/server/Uploader.js
@@ -208,7 +208,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;
@@ -216,7 +216,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:
@@ -247,8 +247,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");
@@ -269,7 +270,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)),
]);
@@ -290,11 +291,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;
}
@@ -601,7 +603,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.");
}
@@ -610,12 +612,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 0776a58..3d8a66d 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"); |
Murderlon
approved these changes
May 21, 2024
|
Does this PR close this? #4898 |
Co-authored-by: Antoine du Hamel <antoine@transloadit.com>
also add filename to `bucket` for consistency
yes I believe so too! |
I was afraid that storing the req object (in the callstack in Uploader.js) after |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this is a breaking change!
also:
reqbecause it's an internal, unstable api that might change any time and should not be used directlybucketwhenever we are aware of it#4763 (comment)
closes #4898