From 652b5aaab59f994263e66cedc5afd07c75f3d959 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Thu, 28 Nov 2024 09:43:38 +0100 Subject: [PATCH] Merge pull request #22183 from overleaf/jpa-zip-from-hash [web] build project zip using history-v1 blobs with filestore fallback GitOrigin-RevId: 2e197ae83e7ac8bcfe44091c327b721825b13a05 --- .../Downloads/ProjectZipStreamManager.mjs | 9 +++-- .../web/test/acceptance/src/HistoryTests.mjs | 37 +++++++++++++++++++ .../ProjectZipStreamManagerTests.mjs | 22 +++++++---- 3 files changed, 56 insertions(+), 12 deletions(-) diff --git a/services/web/app/src/Features/Downloads/ProjectZipStreamManager.mjs b/services/web/app/src/Features/Downloads/ProjectZipStreamManager.mjs index fa65c9fa30..9fbacb2b5c 100644 --- a/services/web/app/src/Features/Downloads/ProjectZipStreamManager.mjs +++ b/services/web/app/src/Features/Downloads/ProjectZipStreamManager.mjs @@ -3,7 +3,7 @@ import async from 'async' import logger from '@overleaf/logger' import ProjectEntityHandler from '../Project/ProjectEntityHandler.js' import ProjectGetter from '../Project/ProjectGetter.js' -import FileStoreHandler from '../FileStore/FileStoreHandler.js' +import HistoryManager from '../History/HistoryManager.js' let ProjectZipStreamManager export default ProjectZipStreamManager = { @@ -114,11 +114,11 @@ export default ProjectZipStreamManager = { return callback(error) } const jobs = Object.entries(files).map(([path, file]) => cb => { - FileStoreHandler.getFileStream( + HistoryManager.requestBlobWithFallback( projectId, + file.hash, file._id, - {}, - (error, stream) => { + (error, result) => { if (error) { logger.warn( { err: error, projectId, fileId: file._id }, @@ -129,6 +129,7 @@ export default ProjectZipStreamManager = { if (path[0] === '/') { path = path.slice(1) } + const { stream } = result archive.append(stream, { name: path }) stream.on('end', () => cb()) } diff --git a/services/web/test/acceptance/src/HistoryTests.mjs b/services/web/test/acceptance/src/HistoryTests.mjs index 17479f3e71..f0c86b43fe 100644 --- a/services/web/test/acceptance/src/HistoryTests.mjs +++ b/services/web/test/acceptance/src/HistoryTests.mjs @@ -6,6 +6,8 @@ import MockV1HistoryApiClass from './mocks/MockV1HistoryApi.js' import ProjectGetter from '../../../app/src/Features/Project/ProjectGetter.js' import MockFilestoreApiClass from './mocks/MockFilestoreApi.js' import { fileURLToPath } from 'node:url' +import sinon from 'sinon' +import logger from '@overleaf/logger' import Metrics from './helpers/metrics.js' const User = UserHelper.promises @@ -64,6 +66,41 @@ describe('HistoryTests', function () { expect(await getSourceMetric('filestore')).to.equal(filestoreSource) } + describe('/project/:projectId/download/zip', function () { + let spy, downloadZIPURL + beforeEach(async function () { + spy = sinon.spy(logger, 'error') + downloadZIPURL = `/project/${projectId}/download/zip` + }) + afterEach(function () { + spy.restore() + }) + it('should work from history-v1', async function () { + const { response, body } = await user.doRequest('GET', downloadZIPURL) + expect(response.statusCode).to.equal(200) + expect(body).to.include('2pixel.png') + await expectHistoryV1Hit() + }) + it('should work from filestore', async function () { + MockV1HistoryApi.reset() + const { response, body } = await user.doRequest('GET', downloadZIPURL) + expect(response.statusCode).to.equal(200) + expect(body).to.include('2pixel.png') + await expectFilestoreHit() + }) + it('should not include when missing in both places', async function () { + MockFilestoreApi.reset() + MockV1HistoryApi.reset() + const { response, body } = await user.doRequest('GET', downloadZIPURL) + expect(response.statusCode).to.equal(200) + expect( + spy.args.find(([, msg]) => msg === 'error adding files to zip stream') + ).to.exist + expect(body).to.not.include('2pixel.png') + await expectNoIncrement() + }) + }) + describe('/project/:projectId/blob/:hash', function () { describe('HEAD', function () { it('should fetch the file size from history-v1', async function () { diff --git a/services/web/test/unit/src/Downloads/ProjectZipStreamManagerTests.mjs b/services/web/test/unit/src/Downloads/ProjectZipStreamManagerTests.mjs index b55d47153d..12712f07bd 100644 --- a/services/web/test/unit/src/Downloads/ProjectZipStreamManagerTests.mjs +++ b/services/web/test/unit/src/Downloads/ProjectZipStreamManagerTests.mjs @@ -32,8 +32,8 @@ describe('ProjectZipStreamManager', function () { '@overleaf/logger': this.logger, '../../../../app/src/Features/Project/ProjectEntityHandler': (this.ProjectEntityHandler = {}), - '../../../../app/src/Features/FileStore/FileStoreHandler': - (this.FileStoreHandler = {}), + '../../../../app/src/Features/History/HistoryManager.js': + (this.HistoryManager = {}), '../../../../app/src/Features/Project/ProjectGetter': (this.ProjectGetter = {}), })) @@ -366,9 +366,11 @@ describe('ProjectZipStreamManager', function () { this.files = { '/image.png': { _id: 'file-id-1', + hash: 'abc', }, '/folder/picture.png': { _id: 'file-id-2', + hash: 'def', }, } this.streams = { @@ -378,11 +380,15 @@ describe('ProjectZipStreamManager', function () { this.ProjectEntityHandler.getAllFiles = sinon .stub() .callsArgWith(1, null, this.files) - this.FileStoreHandler.getFileStream = (projectId, fileId, ...rest) => { - const [, callback] = rest - return callback(null, this.streams[fileId]) + this.HistoryManager.requestBlobWithFallback = ( + projectId, + hash, + fileId, + callback + ) => { + return callback(null, { stream: this.streams[fileId] }) } - sinon.spy(this.FileStoreHandler, 'getFileStream') + sinon.spy(this.HistoryManager, 'requestBlobWithFallback') this.ProjectZipStreamManager.addAllFilesToArchive( this.project_id, this.archive, @@ -410,8 +416,8 @@ describe('ProjectZipStreamManager', function () { for (const path in this.files) { const file = this.files[path] result.push( - this.FileStoreHandler.getFileStream - .calledWith(this.project_id, file._id) + this.HistoryManager.requestBlobWithFallback + .calledWith(this.project_id, file.hash, file._id) .should.equal(true) ) }