Skip to content

Commit

Permalink
@uppy/companion: remove redundant HEAD request for file size (#5648)
Browse files Browse the repository at this point in the history
* Always get size from stream response headers

because i don't really think we need a separate request for it

* Remove explicit size request

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
  • Loading branch information
mifi authored Feb 17, 2025
1 parent 4f04568 commit 834b013
Show file tree
Hide file tree
Showing 14 changed files with 35 additions and 113 deletions.
20 changes: 4 additions & 16 deletions examples/custom-provider/server/CustomProvider.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -61,26 +61,14 @@ class MyCustomProvider {
},
})

if (!resp.ok) {
throw new Error(`Errornous HTTP response (${resp.status} ${resp.statusText})`)
}
return { stream: Readable.fromWeb(resp.body) }
}

// eslint-disable-next-line class-methods-use-this
async size ({ id, token }) {
const resp = await fetch(`${BASE_URL}/photos/${id}`, {
headers: {
Authorization: `Bearer ${token}`,
},
})
const contentLengthStr = resp.headers['content-length']
const contentLength = parseInt(contentLengthStr, 10);
const size = !Number.isNaN(contentLength) && contentLength >= 0 ? contentLength : undefined;

if (!resp.ok) {
throw new Error(`Errornous HTTP response (${resp.status} ${resp.statusText})`)
}

const { size } = await resp.json()
return size
return { stream: Readable.fromWeb(resp.body), size }
}
}

Expand Down
13 changes: 2 additions & 11 deletions packages/@uppy/companion/src/server/controllers/googlePicker.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,9 @@ 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')


Expand All @@ -26,14 +25,6 @@ const get = async (req, res) => {
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' })
Expand All @@ -47,7 +38,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)) return
Expand Down
7 changes: 1 addition & 6 deletions packages/@uppy/companion/src/server/controllers/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,10 @@ const get = async (req, res) => {
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)) return
Expand Down
8 changes: 5 additions & 3 deletions packages/@uppy/companion/src/server/helpers/upload.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,14 @@ async function startDownUpload({ req, res, getSize, download }) {
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
Expand Down
6 changes: 4 additions & 2 deletions packages/@uppy/companion/src/server/provider/Provider.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,16 @@ class Provider {
}

/**
* 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
}

/**
Expand Down
4 changes: 2 additions & 2 deletions packages/@uppy/companion/src/server/provider/box/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ class Box extends Provider {
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 }
})
}

Expand Down
4 changes: 2 additions & 2 deletions packages/@uppy/companion/src/server/provider/dropbox/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,8 @@ class DropBox extends Provider {
responseType: 'json',
})

await prepareStream(stream)
return { stream }
const { size } = await prepareStream(stream)
return { stream, size }
})
}

Expand Down
13 changes: 2 additions & 11 deletions packages/@uppy/companion/src/server/provider/facebook/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
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')
Expand Down Expand Up @@ -92,8 +91,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 }
})
}

Expand All @@ -104,14 +103,6 @@ class Facebook extends Provider {
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({
Expand Down
24 changes: 2 additions & 22 deletions packages/@uppy/companion/src/server/provider/google/drive/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,20 +74,8 @@ async function streamGoogleFile({ token, id: idIn }) {
stream = client.stream.get(`files/${encodeURIComponent(id)}`, { searchParams: { alt: 'media', supportsAllDrives: true }, 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 }
}

/**
Expand Down Expand Up @@ -185,13 +173,6 @@ 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
Expand All @@ -200,5 +181,4 @@ Drive.prototype.refreshToken = refreshToken
module.exports = {
Drive,
streamGoogleFile,
getGoogleFileSize,
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
const Provider = require('../../Provider')
const { getURLMeta } = require('../../../helpers/request')
const logger = require('../../../logger')
const adaptData = require('./adapter')
const { withProviderErrorHandling } = require('../../providerErrors')
Expand Down Expand Up @@ -55,8 +54,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 }
})
}

Expand All @@ -67,14 +66,6 @@ class Instagram extends Provider {
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ class OneDrive extends Provider {
async download ({ id, token, query }) {
return this.#withErrorHandling('provider.onedrive.download.error', async () => {
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 }
})
}

Expand Down
13 changes: 2 additions & 11 deletions packages/@uppy/companion/src/server/provider/unsplash/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
const Provider = require('../Provider')
const { getURLMeta } = require('../../helpers/request')
const adaptData = require('./adapter')
const { withProviderErrorHandling } = require('../providerErrors')
const { prepareStream } = require('../../helpers/utils')
Expand Down Expand Up @@ -43,23 +42,15 @@ class Unsplash extends Provider {
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 }
})
}

Expand Down
15 changes: 3 additions & 12 deletions packages/@uppy/companion/src/server/provider/webdav/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,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 }
})
}

Expand All @@ -152,17 +154,6 @@ class WebdavProvider extends Provider {
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 {
Expand Down
4 changes: 2 additions & 2 deletions packages/@uppy/companion/src/server/provider/zoom/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,8 @@ class Zoom extends Provider {
if (!url) 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 }
})
}

Expand Down

0 comments on commit 834b013

Please sign in to comment.