-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
coalesce options bucket
and getKey
#5169
base: 4.x
Are you sure you want to change the base?
Conversation
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
this is a breaking change!
also:
req
because it's an internal, unstable api that might change any time and should not be used directlybucket
whenever we are aware of it#4763 (comment)
closes #4898