@uppy/companion: remove redundant HEAD request for file size #5648
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
because i don't really think we need a separate request for it
Remove `size` method from providers that don't have a backup method for getting size (that is better than GET download response header) assumption: GET response content-length is as good as (or better than) HEAD response content-length, meaning it is not necessary to call getURLMeta when we already tried the original GET response content-length this reduces the chance of hitting the race condition where the size of a URL changes between the GET and HEAD requests are sent
Diff output filesdiff --git a/packages/@uppy/companion/lib/server/controllers/googlePicker.js b/packages/@uppy/companion/lib/server/controllers/googlePicker.js
index 60e11b7..3004df1 100644
--- a/packages/@uppy/companion/lib/server/controllers/googlePicker.js
+++ b/packages/@uppy/companion/lib/server/controllers/googlePicker.js
@@ -4,10 +4,9 @@ const express = require("express");
const assert = require("node:assert");
const { startDownUpload } = require("../helpers/upload");
const { validateURL } = require("../helpers/request");
-const { getURLMeta } = require("../helpers/request");
const logger = require("../logger");
const { downloadURL } = require("../download");
-const { getGoogleFileSize, streamGoogleFile } = require("../provider/google/drive");
+const { streamGoogleFile } = require("../provider/google/drive");
const { respondWithError } = require("../provider/error");
const getAuthHeader = (token) => ({ authorization: `Bearer ${token}` });
/**
@@ -20,13 +19,6 @@ const get = async (req, res) => {
const allowLocalUrls = false;
const { accessToken, platform, fileId } = req.body;
assert(platform === "drive" || platform === "photos");
- const getSize = async () => {
- if (platform === "drive") {
- return getGoogleFileSize({ id: fileId, token: accessToken });
- }
- const { size } = await getURLMeta(req.body.url, allowLocalUrls, { headers: getAuthHeader(accessToken) });
- return size;
- };
if (platform === "photos" && !validateURL(req.body.url, allowLocalUrls)) {
res.status(400).json({ error: "Invalid URL" });
return;
@@ -37,7 +29,7 @@ const get = async (req, res) => {
}
return downloadURL(req.body.url, allowLocalUrls, req.id, { headers: getAuthHeader(accessToken) });
};
- await startDownUpload({ req, res, getSize, download });
+ await startDownUpload({ req, res, download, getSize: undefined });
} catch (err) {
logger.error(err, "controller.googlePicker.error", req.id);
if (respondWithError(err, res)) {
diff --git a/packages/@uppy/companion/lib/server/controllers/url.js b/packages/@uppy/companion/lib/server/controllers/url.js
index d83a456..b29b0f4 100644
--- a/packages/@uppy/companion/lib/server/controllers/url.js
+++ b/packages/@uppy/companion/lib/server/controllers/url.js
@@ -52,13 +52,9 @@ const get = async (req, res) => {
res.status(400).json({ error: "Invalid request body" });
return;
}
- async function getSize() {
- const { size } = await getURLMeta(req.body.url, allowLocalUrls);
- return size;
- }
const download = () => downloadURL(req.body.url, allowLocalUrls, req.id);
try {
- await startDownUpload({ req, res, getSize, download });
+ await startDownUpload({ req, res, download, getSize: undefined });
} catch (err) {
logger.error(err, "controller.url.error", req.id);
if (respondWithError(err, res)) {
diff --git a/packages/@uppy/companion/lib/server/helpers/upload.js b/packages/@uppy/companion/lib/server/helpers/upload.js
index e461c57..dfb416b 100644
--- a/packages/@uppy/companion/lib/server/helpers/upload.js
+++ b/packages/@uppy/companion/lib/server/helpers/upload.js
@@ -6,12 +6,14 @@ async function startDownUpload({ req, res, getSize, download }) {
logger.debug("Starting download stream.", null, req.id);
const { stream, size: maybeSize } = await download();
let size;
- // if the provider already knows the size, we can use that
+ // if we already know the size from the GET response content-length header, we can use that
if (typeof maybeSize === "number" && !Number.isNaN(maybeSize) && maybeSize > 0) {
size = maybeSize;
}
- // if not we need to get the size
- if (size == null) {
+ // if not, we may need to explicitly get the size
+ // note that getSize might also return undefined/null, which is usually fine, it just means that
+ // the size is unknown and we cannot send the size to the Uploader
+ if (size == null && getSize != null) {
size = await getSize();
}
const { clientSocketConnectTimeout } = req.companion.options;
diff --git a/packages/@uppy/companion/lib/server/provider/Provider.d.ts b/packages/@uppy/companion/lib/server/provider/Provider.d.ts
index f58209f..c11325d 100644
--- a/packages/@uppy/companion/lib/server/provider/Provider.d.ts
+++ b/packages/@uppy/companion/lib/server/provider/Provider.d.ts
@@ -54,7 +54,9 @@ declare class Provider {
*/
thumbnail(options: object): Promise<any>;
/**
- * get the size of a certain file in the provider account
+ * first Companion will try to get the size from the content-length response header,
+ * if that fails, it will call this method to get the size.
+ * So if your provider has a different method for getting the size, you can return the size here
*
* @param {object} options
* @returns {Promise}
diff --git a/packages/@uppy/companion/lib/server/provider/Provider.js b/packages/@uppy/companion/lib/server/provider/Provider.js
index a428b02..90fc4cc 100644
--- a/packages/@uppy/companion/lib/server/provider/Provider.js
+++ b/packages/@uppy/companion/lib/server/provider/Provider.js
@@ -53,14 +53,16 @@ class Provider {
throw new Error("method not implemented");
}
/**
- * get the size of a certain file in the provider account
+ * first Companion will try to get the size from the content-length response header,
+ * if that fails, it will call this method to get the size.
+ * So if your provider has a different method for getting the size, you can return the size here
*
* @param {object} options
* @returns {Promise}
*/
// eslint-disable-next-line class-methods-use-this,no-unused-vars
async size(options) {
- throw new Error("method not implemented");
+ return undefined;
}
/**
* handle deauthorization notification from oauth providers
diff --git a/packages/@uppy/companion/lib/server/provider/box/index.js b/packages/@uppy/companion/lib/server/provider/box/index.js
index d80f7cd..ecf2a97 100644
--- a/packages/@uppy/companion/lib/server/provider/box/index.js
+++ b/packages/@uppy/companion/lib/server/provider/box/index.js
@@ -58,8 +58,8 @@ class Box extends Provider {
async download({ id, token }) {
return this.#withErrorHandling("provider.box.download.error", async () => {
const stream = (await getClient({ token })).stream.get(`files/${id}/content`, { responseType: "json" });
- await prepareStream(stream);
- return { stream };
+ const { size } = await prepareStream(stream);
+ return { stream, size };
});
}
async thumbnail({ id, token }) {
diff --git a/packages/@uppy/companion/lib/server/provider/dropbox/index.js b/packages/@uppy/companion/lib/server/provider/dropbox/index.js
index 8c7eb6a..09b9de0 100644
--- a/packages/@uppy/companion/lib/server/provider/dropbox/index.js
+++ b/packages/@uppy/companion/lib/server/provider/dropbox/index.js
@@ -86,8 +86,8 @@ class DropBox extends Provider {
body: Buffer.alloc(0), // if not, it will hang waiting for the writable stream
responseType: "json",
});
- await prepareStream(stream);
- return { stream };
+ const { size } = await prepareStream(stream);
+ return { stream, size };
});
}
async thumbnail({ id, token }) {
diff --git a/packages/@uppy/companion/lib/server/provider/facebook/index.d.ts b/packages/@uppy/companion/lib/server/provider/facebook/index.d.ts
index 8821671..ac1abfd 100644
--- a/packages/@uppy/companion/lib/server/provider/facebook/index.d.ts
+++ b/packages/@uppy/companion/lib/server/provider/facebook/index.d.ts
@@ -15,10 +15,6 @@ declare class Facebook extends Provider {
token: any;
}): Promise<any>;
thumbnail(): Promise<void>;
- size({ id, token }: {
- id: any;
- token: any;
- }): Promise<any>;
logout({ token }: {
token: any;
}): Promise<any>;
diff --git a/packages/@uppy/companion/lib/server/provider/facebook/index.js b/packages/@uppy/companion/lib/server/provider/facebook/index.js
index c394c8e..6ef11c0 100644
--- a/packages/@uppy/companion/lib/server/provider/facebook/index.js
+++ b/packages/@uppy/companion/lib/server/provider/facebook/index.js
@@ -3,7 +3,6 @@ var _a;
Object.defineProperty(exports, "__esModule", { value: true });
const crypto = require("node:crypto");
const Provider = require("../Provider");
-const { getURLMeta } = require("../../helpers/request");
const logger = require("../../logger");
const { adaptData, sortImages } = require("./adapter");
const { withProviderErrorHandling } = require("../providerErrors");
@@ -78,8 +77,8 @@ class Facebook extends Provider {
return this.#withErrorHandling("provider.facebook.download.error", async () => {
const url = await getMediaUrl({ secret: this.secret, token, id });
const stream = (await got).stream.get(url, { responseType: "json" });
- await prepareStream(stream);
- return { stream };
+ const { size } = await prepareStream(stream);
+ return { stream, size };
});
}
// eslint-disable-next-line class-methods-use-this
@@ -88,13 +87,6 @@ class Facebook extends Provider {
logger.error("call to thumbnail is not implemented", "provider.facebook.thumbnail.error");
throw new Error("call to thumbnail is not implemented");
}
- async size({ id, token }) {
- return this.#withErrorHandling("provider.facebook.size.error", async () => {
- const url = await getMediaUrl({ secret: this.secret, token, id });
- const { size } = await getURLMeta(url);
- return size;
- });
- }
async logout({ token }) {
return this.#withErrorHandling("provider.facebook.logout.error", async () => {
await runRequestBatch({
diff --git a/packages/@uppy/companion/lib/server/provider/google/drive/index.d.ts b/packages/@uppy/companion/lib/server/provider/google/drive/index.d.ts
index 8ac298f..0e08c0d 100644
--- a/packages/@uppy/companion/lib/server/provider/google/drive/index.d.ts
+++ b/packages/@uppy/companion/lib/server/provider/google/drive/index.d.ts
@@ -7,10 +7,6 @@ export class Drive extends Provider {
id: any;
token: any;
}): Promise<any>;
- size({ id, token }: {
- id: any;
- token: any;
- }): Promise<any>;
logout: typeof logout;
refreshToken: typeof refreshToken;
}
@@ -19,11 +15,8 @@ export function streamGoogleFile({ token, id: idIn }: {
id: any;
}): Promise<{
stream: import("got", { with: { "resolution-mode": "import" } }).Request;
+ size: any;
}>;
-export function getGoogleFileSize({ id, token }: {
- id: any;
- token: any;
-}): Promise<number>;
import Provider = require("../../Provider");
import { logout } from "../index";
import { refreshToken } from "../index";
diff --git a/packages/@uppy/companion/lib/server/provider/google/drive/index.js b/packages/@uppy/companion/lib/server/provider/google/drive/index.js
index d48a5cd..263c7a5 100644
--- a/packages/@uppy/companion/lib/server/provider/google/drive/index.js
+++ b/packages/@uppy/companion/lib/server/provider/google/drive/index.js
@@ -74,17 +74,8 @@ async function streamGoogleFile({ token, id: idIn }) {
responseType: "json",
});
}
- await prepareStream(stream);
- return { stream };
-}
-async function getGoogleFileSize({ id, token }) {
- const { mimeType, size } = await getStats({ id, token });
- if (isGsuiteFile(mimeType)) {
- // GSuite file sizes cannot be predetermined (but are max 10MB)
- // e.g. Transfer-Encoding: chunked
- return undefined;
- }
- return parseInt(size, 10);
+ const { size } = await prepareStream(stream);
+ return { stream, size };
}
/**
* Adapter for API https://developers.google.com/drive/api/v3/
@@ -170,19 +161,10 @@ class Drive extends Provider {
return streamGoogleFile({ token, id });
});
}
- // eslint-disable-next-line class-methods-use-this
- async size({ id, token }) {
- return withGoogleErrorHandling(
- Drive.oauthProvider,
- "provider.drive.size.error",
- async () => (getGoogleFileSize({ id, token })),
- );
- }
}
Drive.prototype.logout = logout;
Drive.prototype.refreshToken = refreshToken;
module.exports = {
Drive,
streamGoogleFile,
- getGoogleFileSize,
};
diff --git a/packages/@uppy/companion/lib/server/provider/instagram/graph/index.d.ts b/packages/@uppy/companion/lib/server/provider/instagram/graph/index.d.ts
index 96532dc..c678d62 100644
--- a/packages/@uppy/companion/lib/server/provider/instagram/graph/index.d.ts
+++ b/packages/@uppy/companion/lib/server/provider/instagram/graph/index.d.ts
@@ -19,10 +19,6 @@ declare class Instagram extends Provider {
token: any;
}): Promise<any>;
thumbnail(): Promise<void>;
- size({ id, token }: {
- id: any;
- token: any;
- }): Promise<any>;
logout(): Promise<{
revoked: boolean;
manual_revoke_url: string;
diff --git a/packages/@uppy/companion/lib/server/provider/instagram/graph/index.js b/packages/@uppy/companion/lib/server/provider/instagram/graph/index.js
index c674bbe..3f30f73 100644
--- a/packages/@uppy/companion/lib/server/provider/instagram/graph/index.js
+++ b/packages/@uppy/companion/lib/server/provider/instagram/graph/index.js
@@ -2,7 +2,6 @@
var _a;
Object.defineProperty(exports, "__esModule", { value: true });
const Provider = require("../../Provider");
-const { getURLMeta } = require("../../../helpers/request");
const logger = require("../../../logger");
const adaptData = require("./adapter");
const { withProviderErrorHandling } = require("../../providerErrors");
@@ -57,8 +56,8 @@ class Instagram extends Provider {
return this.#withErrorHandling("provider.instagram.download.error", async () => {
const url = await getMediaUrl({ token, id });
const stream = (await got).stream.get(url, { responseType: "json" });
- await prepareStream(stream);
- return { stream };
+ const { size } = await prepareStream(stream);
+ return { stream, size };
});
}
// eslint-disable-next-line class-methods-use-this
@@ -67,13 +66,6 @@ class Instagram extends Provider {
logger.error("call to thumbnail is not implemented", "provider.instagram.thumbnail.error");
throw new Error("call to thumbnail is not implemented");
}
- async size({ id, token }) {
- return this.#withErrorHandling("provider.instagram.size.error", async () => {
- const url = await getMediaUrl({ token, id });
- const { size } = await getURLMeta(url);
- return size;
- });
- }
// eslint-disable-next-line class-methods-use-this
async logout() {
// access revoke is not supported by Instagram's API
diff --git a/packages/@uppy/companion/lib/server/provider/onedrive/index.js b/packages/@uppy/companion/lib/server/provider/onedrive/index.js
index 3f8fb0c..586fa9e 100644
--- a/packages/@uppy/companion/lib/server/provider/onedrive/index.js
+++ b/packages/@uppy/companion/lib/server/provider/onedrive/index.js
@@ -58,8 +58,8 @@ class OneDrive extends Provider {
const stream = (await getClient({ token })).stream.get(`${getRootPath(query)}/items/${id}/content`, {
responseType: "json",
});
- await prepareStream(stream);
- return { stream };
+ const { size } = await prepareStream(stream);
+ return { stream, size };
});
}
// eslint-disable-next-line class-methods-use-this
diff --git a/packages/@uppy/companion/lib/server/provider/unsplash/index.d.ts b/packages/@uppy/companion/lib/server/provider/unsplash/index.d.ts
index 70c0e53..3ece655 100644
--- a/packages/@uppy/companion/lib/server/provider/unsplash/index.d.ts
+++ b/packages/@uppy/companion/lib/server/provider/unsplash/index.d.ts
@@ -14,10 +14,6 @@ declare class Unsplash extends Provider {
id: any;
token: any;
}): Promise<any>;
- size({ id, token }: {
- id: any;
- token: any;
- }): Promise<any>;
#private;
}
import Provider = require("../Provider");
diff --git a/packages/@uppy/companion/lib/server/provider/unsplash/index.js b/packages/@uppy/companion/lib/server/provider/unsplash/index.js
index cf69ba7..fc30e4a 100644
--- a/packages/@uppy/companion/lib/server/provider/unsplash/index.js
+++ b/packages/@uppy/companion/lib/server/provider/unsplash/index.js
@@ -1,7 +1,6 @@
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
const Provider = require("../Provider");
-const { getURLMeta } = require("../../helpers/request");
const adaptData = require("./adapter");
const { withProviderErrorHandling } = require("../providerErrors");
const { prepareStream } = require("../../helpers/utils");
@@ -41,20 +40,13 @@ class Unsplash extends Provider {
const client = await getClient({ token });
const { links: { download: url, download_location: attributionUrl } } = await getPhotoMeta(client, id);
const stream = (await got).stream.get(url, { responseType: "json" });
- await prepareStream(stream);
+ const { size } = await prepareStream(stream);
// To attribute the author of the image, we call the `download_location`
// endpoint to increment the download count on Unsplash.
// https://help.unsplash.com/en/articles/2511258-guideline-triggering-a-download
await client.get(attributionUrl, { prefixUrl: "", responseType: "json" });
// finally, stream on!
- return { stream };
- });
- }
- async size({ id, token }) {
- return this.#withErrorHandling("provider.unsplash.size.error", async () => {
- const { links: { download: url } } = await getPhotoMeta(await getClient({ token }), id);
- const { size } = await getURLMeta(url);
- return size;
+ return { stream, size };
});
}
// eslint-disable-next-line class-methods-use-this
diff --git a/packages/@uppy/companion/lib/server/provider/webdav/index.d.ts b/packages/@uppy/companion/lib/server/provider/webdav/index.d.ts
index 220d9a3..a1f4b10 100644
--- a/packages/@uppy/companion/lib/server/provider/webdav/index.d.ts
+++ b/packages/@uppy/companion/lib/server/provider/webdav/index.d.ts
@@ -33,11 +33,6 @@ declare class WebdavProvider extends Provider {
id: any;
providerUserSession: any;
}): Promise<void>;
- size({ id, token, providerUserSession }: {
- id: any;
- token: any;
- providerUserSession: any;
- }): Promise<any>;
withErrorHandling(tag: any, fn: any): Promise<any>;
}
import Provider = require("../Provider");
diff --git a/packages/@uppy/companion/lib/server/provider/webdav/index.js b/packages/@uppy/companion/lib/server/provider/webdav/index.js
index 4412ab7..59c1c3f 100644
--- a/packages/@uppy/companion/lib/server/provider/webdav/index.js
+++ b/packages/@uppy/companion/lib/server/provider/webdav/index.js
@@ -118,8 +118,10 @@ class WebdavProvider extends Provider {
async download({ id, providerUserSession }) {
return this.withErrorHandling("provider.webdav.download.error", async () => {
const client = await this.getClient({ providerUserSession });
+ /** @type {any} */
+ const stat = await client.stat(id);
const stream = client.createReadStream(`/${id}`);
- return { stream };
+ return { stream, size: stat.size };
});
}
// eslint-disable-next-line
@@ -128,15 +130,6 @@ class WebdavProvider extends Provider {
logger.error("call to thumbnail is not implemented", "provider.webdav.thumbnail.error");
throw new Error("call to thumbnail is not implemented");
}
- // eslint-disable-next-line
- async size({ id, token, providerUserSession }) {
- return this.withErrorHandling("provider.webdav.size.error", async () => {
- const client = await this.getClient({ providerUserSession });
- /** @type {any} */
- const stat = await client.stat(id);
- return stat.size;
- });
- }
// eslint-disable-next-line class-methods-use-this
async withErrorHandling(tag, fn) {
try {
diff --git a/packages/@uppy/companion/lib/server/provider/zoom/index.js b/packages/@uppy/companion/lib/server/provider/zoom/index.js
index b674764..3fa2d2f 100644
--- a/packages/@uppy/companion/lib/server/provider/zoom/index.js
+++ b/packages/@uppy/companion/lib/server/provider/zoom/index.js
@@ -86,8 +86,8 @@ class Zoom extends Provider {
throw new Error("Download URL not found");
}
const stream = client.stream.get(`${url}?access_token=${token}`, { prefixUrl: "", responseType: "json" });
- await prepareStream(stream);
- return { stream };
+ const { size } = await prepareStream(stream);
+ return { stream, size };
});
}
async size({ id: meetingId, token, query }) { |
That seems sensible to me. In theory a HEAD response includes the same header that a GET response would (without the response body). If a GET response doesn't include Content-Length, a HEAD response will likely also not include it and therefore does not provide much value. At least, that what the spec says. I don't know if there are cases in reality where a HEAD helps, but I doubt they're frequent. So I'm 👍 for removing the HEAD request. |
Murderlon
approved these changes
Feb 17, 2025
Murderlon
changed the title
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 PR does 2 things:
Always get size from stream response headers because i don't really think we need a separate HEAD request for it.
and:
Remove
sizemethod from providers that don't have a backup method for getting size (that is better than GET download response header)assumption: GET response content-length is as good as (or better than) HEAD response content-length,
meaning it is not necessary to call getURLMeta when we already tried the original GET response content-length
this reduces the chance of hitting the race condition where the size of a URL changes between the GET and HEAD requests are sent.