From 7913ffe840821c3802946c864641d6ebdc3d6eea Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Mon, 20 Jan 2025 09:02:27 -0500 Subject: [PATCH] Merge pull request #22960 from overleaf/em-promisify-filestore-controller Promisify FileStoreController GitOrigin-RevId: b5f5861a7601a1bf4af3024394f910a0f5a14206 --- .../FileStore/FileStoreController.mjs | 165 ++++++++++------- .../FileStore/FileStoreControllerTests.mjs | 171 ++++++++---------- 2 files changed, 168 insertions(+), 168 deletions(-) diff --git a/services/web/app/src/Features/FileStore/FileStoreController.mjs b/services/web/app/src/Features/FileStore/FileStoreController.mjs index 52f320f13d..0d4b3af2ad 100644 --- a/services/web/app/src/Features/FileStore/FileStoreController.mjs +++ b/services/web/app/src/Features/FileStore/FileStoreController.mjs @@ -1,79 +1,103 @@ +// @ts-check + +import { pipeline } from 'node:stream/promises' import logger from '@overleaf/logger' +import { expressify } from '@overleaf/promise-utils' import FileStoreHandler from './FileStoreHandler.js' import ProjectLocator from '../Project/ProjectLocator.js' import Errors from '../Errors/Errors.js' import { preparePlainTextResponse } from '../../infrastructure/Response.js' -export default { - getFile(req, res) { - const projectId = req.params.Project_id - const fileId = req.params.File_id - const queryString = req.query - const userAgent = req.get('User-Agent') - ProjectLocator.findElement( - { project_id: projectId, element_id: fileId, type: 'file' }, - function (err, file) { - if (err) { - if (err instanceof Errors.NotFoundError) { - logger.warn( - { err, projectId, fileId, queryString }, - 'entity not found when downloading file' - ) - // res.sendStatus() sends a description of the status as body. - // Using res.status().end() avoids sending that fake body. - return res.status(404).end() - } else { - logger.err( - { err, projectId, fileId, queryString }, - 'error finding element for downloading file' - ) - return res.status(500).end() - } - } - FileStoreHandler.getFileStream( - projectId, - fileId, - queryString, - function (err, stream) { - if (err) { - logger.err( - { err, projectId, fileId, queryString }, - 'error getting file stream for downloading file' - ) - return res.sendStatus(500) - } - // mobile safari will try to render html files, prevent this - if (isMobileSafari(userAgent) && isHtml(file)) { - preparePlainTextResponse(res) - } - res.setContentDisposition('attachment', { filename: file.name }) - // allow the browser to cache these immutable files - // note: both "private" and "max-age" appear to be required for caching - res.setHeader('Cache-Control', 'private, max-age=3600') - stream.pipe(res) - } - ) - } - ) - }, +async function getFile(req, res) { + const projectId = req.params.Project_id + const fileId = req.params.File_id + const queryString = req.query + const userAgent = req.get('User-Agent') + req.logger.addFields({ projectId, fileId, queryString }) - getFileHead(req, res) { - const projectId = req.params.Project_id - const fileId = req.params.File_id - FileStoreHandler.getFileSize(projectId, fileId, (err, fileSize) => { - if (err) { - if (err instanceof Errors.NotFoundError) { - res.status(404).end() - } else { - logger.err({ err, projectId, fileId }, 'error getting file size') - res.status(500).end() - } - return - } - res.setHeader('Content-Length', fileSize) - res.status(200).end() + let file + try { + file = await ProjectLocator.promises.findElement({ + project_id: projectId, + element_id: fileId, + type: 'file', }) - }, + } catch (err) { + if (err instanceof Errors.NotFoundError) { + logger.warn( + { err, projectId, fileId, queryString }, + 'entity not found when downloading file' + ) + // res.sendStatus() sends a description of the status as body. + // Using res.status().end() avoids sending that fake body. + return res.status(404).end() + } else { + // Instead of using the global error handler, we send an empty response in + // case the client forgets to check the response status. This is arguably + // not our responsibility, and it won't work if something else breaks in + // this endpoint, so it could be revisited in the future. + logger.err( + { err, projectId, fileId, queryString }, + 'error finding element for downloading file' + ) + return res.status(500).end() + } + } + + const stream = await FileStoreHandler.promises.getFileStream( + projectId, + fileId, + queryString + ) + + // mobile safari will try to render html files, prevent this + if (isMobileSafari(userAgent) && isHtml(file)) { + preparePlainTextResponse(res) + } + res.setContentDisposition('attachment', { filename: file.name }) + // allow the browser to cache these immutable files + // note: both "private" and "max-age" appear to be required for caching + res.setHeader('Cache-Control', 'private, max-age=3600') + try { + await pipeline(stream, res) + } catch (err) { + if ( + err instanceof Error && + 'code' in err && + err.code === 'ERR_STREAM_PREMATURE_CLOSE' + ) { + // Ignore clients closing the connection prematurely + return + } + throw err + } +} + +async function getFileHead(req, res) { + const projectId = req.params.Project_id + const fileId = req.params.File_id + + let fileSize + try { + fileSize = await FileStoreHandler.promises.getFileSize(projectId, fileId) + } catch (err) { + if (err instanceof Errors.NotFoundError) { + return res.status(404).end() + } else { + // Instead of using the global error handler, we send an empty response in + // case the client forgets to check the response status. This is arguably + // not our responsibility, and it won't work if something else breaks in + // this endpoint, so it could be revisited in the future. + logger.err( + { err, projectId, fileId }, + 'error finding element for downloading file' + ) + return res.status(500).end() + } + } + + res.setHeader('Content-Length', fileSize) + res.status(200).end() } function isHtml(file) { @@ -98,3 +122,8 @@ function isMobileSafari(userAgent) { (userAgent.indexOf('iPhone') >= 0 || userAgent.indexOf('iPad') >= 0) ) } + +export default { + getFile: expressify(getFile), + getFileHead: expressify(getFileHead), +} diff --git a/services/web/test/unit/src/FileStore/FileStoreControllerTests.mjs b/services/web/test/unit/src/FileStore/FileStoreControllerTests.mjs index a7810df02c..5781745c24 100644 --- a/services/web/test/unit/src/FileStore/FileStoreControllerTests.mjs +++ b/services/web/test/unit/src/FileStore/FileStoreControllerTests.mjs @@ -14,11 +14,15 @@ const expectedFileHeaders = { describe('FileStoreController', function () { beforeEach(async function () { this.FileStoreHandler = { - getFileStream: sinon.stub(), - getFileSize: sinon.stub(), + promises: { + getFileStream: sinon.stub(), + getFileSize: sinon.stub(), + }, } - this.ProjectLocator = { findElement: sinon.stub() } + this.ProjectLocator = { promises: { findElement: sinon.stub() } } + this.Stream = { pipeline: sinon.stub().resolves() } this.controller = await esmock.strict(MODULE_PATH, { + 'node:stream/promises': this.Stream, '@overleaf/settings': this.settings, '../../../../app/src/Features/Project/ProjectLocator': this.ProjectLocator, @@ -37,69 +41,57 @@ describe('FileStoreController', function () { get(key) { return undefined }, + logger: { + addFields: sinon.stub(), + }, } this.res = new MockResponse() + this.next = sinon.stub() this.file = { name: 'myfile.png' } }) describe('getFile', function () { beforeEach(function () { - this.FileStoreHandler.getFileStream.callsArgWith(3, null, this.stream) - this.ProjectLocator.findElement.callsArgWith(1, null, this.file) + this.FileStoreHandler.promises.getFileStream.resolves(this.stream) + this.ProjectLocator.promises.findElement.resolves(this.file) }) - it('should call the file store handler with the project_id file_id and any query string', function (done) { - this.stream.pipe = des => { - this.FileStoreHandler.getFileStream - .calledWith( - this.req.params.Project_id, - this.req.params.File_id, - this.req.query - ) - .should.equal(true) - done() - } - this.controller.getFile(this.req, this.res) + it('should call the file store handler with the project_id file_id and any query string', async function () { + await this.controller.getFile(this.req, this.res) + this.FileStoreHandler.promises.getFileStream.should.have.been.calledWith( + this.req.params.Project_id, + this.req.params.File_id, + this.req.query + ) }) - it('should pipe to res', function (done) { - this.stream.pipe = des => { - des.should.equal(this.res) - done() - } - this.controller.getFile(this.req, this.res) + it('should pipe to res', async function () { + await this.controller.getFile(this.req, this.res) + this.Stream.pipeline.should.have.been.calledWith(this.stream, this.res) }) - it('should get the file from the db', function (done) { - this.stream.pipe = des => { - const opts = { - project_id: this.projectId, - element_id: this.fileId, - type: 'file', - } - this.ProjectLocator.findElement.calledWith(opts).should.equal(true) - done() - } - this.controller.getFile(this.req, this.res) + it('should get the file from the db', async function () { + await this.controller.getFile(this.req, this.res) + this.ProjectLocator.promises.findElement.should.have.been.calledWith({ + project_id: this.projectId, + element_id: this.fileId, + type: 'file', + }) }) - it('should set the Content-Disposition header', function (done) { - this.stream.pipe = des => { - this.res.setContentDisposition.should.be.calledWith('attachment', { - filename: this.file.name, - }) - done() - } - this.controller.getFile(this.req, this.res) + it('should set the Content-Disposition header', async function () { + await this.controller.getFile(this.req, this.res) + this.res.setContentDisposition.should.be.calledWith('attachment', { + filename: this.file.name, + }) }) - it('should return a 404 when not found', function (done) { - this.res.callback = () => { - expect(this.res.statusCode).to.equal(404) - done() - } - this.ProjectLocator.findElement.yields(new Errors.NotFoundError()) - this.controller.getFile(this.req, this.res) + it('should return a 404 when not found', async function () { + this.ProjectLocator.promises.findElement.rejects( + new Errors.NotFoundError() + ) + await this.controller.getFile(this.req, this.res) + expect(this.res.statusCode).to.equal(404) }) // Test behaviour around handling html files @@ -115,14 +107,11 @@ describe('FileStoreController', function () { }) describe('from a non-ios browser', function () { - it('should not set Content-Type', function (done) { - this.stream.pipe = des => { - this.res.headers.should.deep.equal({ - ...expectedFileHeaders, - }) - done() - } - this.controller.getFile(this.req, this.res) + it('should not set Content-Type', async function () { + await this.controller.getFile(this.req, this.res) + this.res.headers.should.deep.equal({ + ...expectedFileHeaders, + }) }) }) @@ -135,16 +124,13 @@ describe('FileStoreController', function () { } }) - it("should set Content-Type to 'text/plain'", function (done) { - this.stream.pipe = des => { - this.res.headers.should.deep.equal({ - ...expectedFileHeaders, - 'Content-Type': 'text/plain; charset=utf-8', - 'X-Content-Type-Options': 'nosniff', - }) - done() - } - this.controller.getFile(this.req, this.res) + it("should set Content-Type to 'text/plain'", async function () { + await this.controller.getFile(this.req, this.res) + this.res.headers.should.deep.equal({ + ...expectedFileHeaders, + 'Content-Type': 'text/plain; charset=utf-8', + 'X-Content-Type-Options': 'nosniff', + }) }) }) @@ -157,16 +143,13 @@ describe('FileStoreController', function () { } }) - it("should set Content-Type to 'text/plain'", function (done) { - this.stream.pipe = des => { - this.res.headers.should.deep.equal({ - ...expectedFileHeaders, - 'Content-Type': 'text/plain; charset=utf-8', - 'X-Content-Type-Options': 'nosniff', - }) - done() - } - this.controller.getFile(this.req, this.res) + it("should set Content-Type to 'text/plain'", async function () { + await this.controller.getFile(this.req, this.res) + this.res.headers.should.deep.equal({ + ...expectedFileHeaders, + 'Content-Type': 'text/plain; charset=utf-8', + 'X-Content-Type-Options': 'nosniff', + }) }) }) }) @@ -194,14 +177,11 @@ describe('FileStoreController', function () { this.user_agent = `Some ${browser} thing` }) - it('Should not set the Content-type', function (done) { - this.stream.pipe = des => { - this.res.headers.should.deep.equal({ - ...expectedFileHeaders, - }) - done() - } - this.controller.getFile(this.req, this.res) + it('Should not set the Content-type', async function () { + await this.controller.getFile(this.req, this.res) + this.res.headers.should.deep.equal({ + ...expectedFileHeaders, + }) }) }) }) @@ -212,12 +192,12 @@ describe('FileStoreController', function () { describe('getFileHead', function () { it('reports the file size', function (done) { const expectedFileSize = 99393 - this.FileStoreHandler.getFileSize.yields( + this.FileStoreHandler.promises.getFileSize.rejects( new Error('getFileSize: unexpected arguments') ) - this.FileStoreHandler.getFileSize + this.FileStoreHandler.promises.getFileSize .withArgs(this.projectId, this.fileId) - .yields(null, expectedFileSize) + .resolves(expectedFileSize) this.res.end = () => { expect(this.res.status.lastCall.args).to.deep.equal([200]) @@ -232,7 +212,9 @@ describe('FileStoreController', function () { }) it('returns 404 on NotFoundError', function (done) { - this.FileStoreHandler.getFileSize.yields(new Errors.NotFoundError()) + this.FileStoreHandler.promises.getFileSize.rejects( + new Errors.NotFoundError() + ) this.res.end = () => { expect(this.res.status.lastCall.args).to.deep.equal([404]) @@ -241,16 +223,5 @@ describe('FileStoreController', function () { this.controller.getFileHead(this.req, this.res) }) - - it('returns 500 on error', function (done) { - this.FileStoreHandler.getFileSize.yields(new Error('boom!')) - - this.res.end = () => { - expect(this.res.status.lastCall.args).to.deep.equal([500]) - done() - } - - this.controller.getFileHead(this.req, this.res) - }) }) })