From 2ce82fababa28c3e4a5fe85ae65d7c3fde9da3d6 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Thu, 9 Jan 2025 14:02:04 +0000 Subject: [PATCH] Merge pull request #22533 from overleaf/ar-only-use-history-for-blobs-when-enabled [web] only use history for blobs when enabled GitOrigin-RevId: 010983e9b29657d4c594e03945dca5700577bf0a --- .../app/src/Features/Compile/ClsiManager.js | 3 +- .../Downloads/ProjectZipStreamManager.mjs | 29 +++- .../Features/FileStore/FileStoreHandler.js | 57 +++++--- .../src/Features/History/HistoryController.js | 3 + .../src/Features/History/HistoryURLHelper.js | 3 +- .../src/Features/Project/ProjectController.js | 3 + .../src/Features/Project/ProjectDuplicator.js | 29 ++-- .../Features/Project/ProjectEditorHandler.js | 9 +- .../web/app/src/infrastructure/Features.js | 6 + .../web/app/views/project/editor/_meta.pug | 1 + .../contexts/file-tree-actionable.tsx | 3 +- .../web/frontend/js/features/utils/fileUrl.js | 6 +- services/web/frontend/js/utils/meta.ts | 1 + .../acceptance/src/ProjectStructureTests.mjs | 2 +- .../web/test/acceptance/src/HistoryTests.mjs | 83 ++++++------ .../web/test/frontend/helpers/reset-meta.ts | 1 + .../test/unit/src/Compile/ClsiManagerTests.js | 4 + .../ProjectZipStreamManagerTests.mjs | 127 +++++++++++------- .../src/FileStore/FileStoreHandlerTests.js | 77 +++++++---- .../src/Project/ProjectDuplicatorTests.js | 4 + .../src/References/ReferencesHandlerTests.mjs | 1 + .../TpdsUpdateSenderTests.js | 1 + 22 files changed, 303 insertions(+), 150 deletions(-) diff --git a/services/web/app/src/Features/Compile/ClsiManager.js b/services/web/app/src/Features/Compile/ClsiManager.js index 91aceb4f4c..a211f82408 100644 --- a/services/web/app/src/Features/Compile/ClsiManager.js +++ b/services/web/app/src/Features/Compile/ClsiManager.js @@ -15,6 +15,7 @@ const { Cookie } = require('tough-cookie') const ClsiCookieManager = require('./ClsiCookieManager')( Settings.apis.clsi?.backendGroupName ) +const Features = require('../../infrastructure/Features') const NewBackendCloudClsiCookieManager = require('./ClsiCookieManager')( Settings.apis.clsi_new?.backendGroupName ) @@ -744,7 +745,7 @@ function _finaliseRequest(projectId, options, project, docs, files) { const filestoreURL = `${Settings.apis.filestore.url}/project/${project._id}/file/${file._id}` let url = filestoreURL let fallbackURL - if (file.hash) { + if (file.hash && Features.hasFeature('project-history-blobs')) { const { bucket, key } = getBlobLocation(historyId, file.hash) url = `${Settings.apis.filestore.url}/bucket/${bucket}/key/${key}` fallbackURL = filestoreURL diff --git a/services/web/app/src/Features/Downloads/ProjectZipStreamManager.mjs b/services/web/app/src/Features/Downloads/ProjectZipStreamManager.mjs index 9fbacb2b5c..2c072516aa 100644 --- a/services/web/app/src/Features/Downloads/ProjectZipStreamManager.mjs +++ b/services/web/app/src/Features/Downloads/ProjectZipStreamManager.mjs @@ -4,6 +4,8 @@ import logger from '@overleaf/logger' import ProjectEntityHandler from '../Project/ProjectEntityHandler.js' import ProjectGetter from '../Project/ProjectGetter.js' import HistoryManager from '../History/HistoryManager.js' +import FileStoreHandler from '../FileStore/FileStoreHandler.js' +import Features from '../../infrastructure/Features.js' let ProjectZipStreamManager export default ProjectZipStreamManager = { @@ -108,17 +110,35 @@ export default ProjectZipStreamManager = { }) }, + getFileStream: (projectId, file, callback) => { + if (Features.hasFeature('project-history-blobs')) { + HistoryManager.requestBlobWithFallback( + projectId, + file.hash, + file._id, + (error, result) => { + if (error) { + return callback(error) + } + const { stream } = result + callback(null, stream) + } + ) + } else { + FileStoreHandler.getFileStream(projectId, file._id, {}, callback) + } + }, + addAllFilesToArchive(projectId, archive, callback) { ProjectEntityHandler.getAllFiles(projectId, (error, files) => { if (error) { return callback(error) } const jobs = Object.entries(files).map(([path, file]) => cb => { - HistoryManager.requestBlobWithFallback( + ProjectZipStreamManager.getFileStream( projectId, - file.hash, - file._id, - (error, result) => { + file, + (error, stream) => { if (error) { logger.warn( { err: error, projectId, fileId: file._id }, @@ -129,7 +149,6 @@ 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/app/src/Features/FileStore/FileStoreHandler.js b/services/web/app/src/Features/FileStore/FileStoreHandler.js index cb5be9fb06..0c1d2cb391 100644 --- a/services/web/app/src/Features/FileStore/FileStoreHandler.js +++ b/services/web/app/src/Features/FileStore/FileStoreHandler.js @@ -11,6 +11,7 @@ const { File } = require('../../models/File') const Errors = require('../Errors/Errors') const OError = require('@overleaf/o-error') const { promisifyAll } = require('@overleaf/promise-utils') +const Features = require('../../infrastructure/Features') const ONE_MIN_IN_MS = 60 * 1000 const FIVE_MINS_IN_MS = ONE_MIN_IN_MS * 5 @@ -38,6 +39,28 @@ const FileStoreHandler = { }) }, + _uploadToHistory(historyId, hash, size, fsPath, callback) { + if (Features.hasFeature('project-history-blobs')) { + Async.retry( + FileStoreHandler.RETRY_ATTEMPTS, + cb => + HistoryManager.uploadBlobFromDisk(historyId, hash, size, fsPath, cb), + error => callback(error, true) + ) + } else { + callback(null, false) + } + }, + + _uploadToFileStore(projectId, fileArgs, fsPath, callback) { + Async.retry( + FileStoreHandler.RETRY_ATTEMPTS, + (cb, results) => + FileStoreHandler._doUploadFileFromDisk(projectId, fileArgs, fsPath, cb), + callback + ) + }, + uploadFileFromDiskWithHistoryId( projectId, historyId, @@ -68,30 +91,20 @@ const FileStoreHandler = { if (err) { return callback(err) } - Async.retry( - FileStoreHandler.RETRY_ATTEMPTS, - cb => - HistoryManager.uploadBlobFromDisk( - historyId, - hash, - stat.size, - fsPath, - cb - ), - function (err) { + FileStoreHandler._uploadToHistory( + historyId, + hash, + stat.size, + fsPath, + function (err, createdBlob) { if (err) { return callback(err) } fileArgs = { ...fileArgs, hash } - Async.retry( - FileStoreHandler.RETRY_ATTEMPTS, - (cb, results) => - FileStoreHandler._doUploadFileFromDisk( - projectId, - fileArgs, - fsPath, - cb - ), + FileStoreHandler._uploadToFileStore( + projectId, + fileArgs, + fsPath, function (err, result) { if (err) { OError.tag(err, 'Error uploading file, retries failed', { @@ -100,7 +113,7 @@ const FileStoreHandler = { }) return callback(err) } - callback(err, result.url, result.fileRef, true) + callback(err, result.url, result.fileRef, createdBlob) } ) } @@ -159,7 +172,7 @@ const FileStoreHandler = { const historyId = project.overleaf?.history?.id const fileId = file._id const hash = file.hash - if (historyId && hash) { + if (historyId && hash && Features.hasFeature('project-history-blobs')) { // new behaviour - request from history const range = _extractRange(query?.range) HistoryManager.requestBlobWithFallback( diff --git a/services/web/app/src/Features/History/HistoryController.js b/services/web/app/src/Features/History/HistoryController.js index 0380f51899..d6618ffa76 100644 --- a/services/web/app/src/Features/History/HistoryController.js +++ b/services/web/app/src/Features/History/HistoryController.js @@ -77,6 +77,9 @@ module.exports = HistoryController = { * has a hash. * */ fileToBlobRedirectMiddleware(req, res, next) { + if (!Features.hasFeature('project-history-blobs')) { + return next() + } const projectId = req.params.Project_id const fileId = req.params.File_id ProjectLocator.findElement( diff --git a/services/web/app/src/Features/History/HistoryURLHelper.js b/services/web/app/src/Features/History/HistoryURLHelper.js index 91fb8a7115..8b8d8cbdd7 100644 --- a/services/web/app/src/Features/History/HistoryURLHelper.js +++ b/services/web/app/src/Features/History/HistoryURLHelper.js @@ -7,7 +7,8 @@ function projectHistoryURLWithFilestoreFallback( origin ) { const filestoreURL = `${Settings.apis.filestore.url}/project/${projectId}/file/${fileRef._id}?from=${origin}` - if (fileRef.hash) { + // TODO: When this file is converted to ES modules we will be able to use Features.hasFeature('project-history-blobs'). Currently we can't stub the feature return value in tests. + if (fileRef.hash && Settings.enableProjectHistoryBlobs) { return { url: `${Settings.apis.project_history.url}/project/${historyId}/blob/${fileRef.hash}`, fallbackURL: filestoreURL, diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index a568781449..31bc4997c1 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/services/web/app/src/Features/Project/ProjectController.js @@ -800,6 +800,9 @@ const _ProjectController = { isInvitedMember ), chatEnabled: Features.hasFeature('chat'), + projectHistoryBlobsEnabled: Features.hasFeature( + 'project-history-blobs' + ), roMirrorOnClientNoLocalStorage: Settings.adminOnlyLogin || project.name.startsWith('Debug: '), languages: Settings.languages, diff --git a/services/web/app/src/Features/Project/ProjectDuplicator.js b/services/web/app/src/Features/Project/ProjectDuplicator.js index bb38b2cda8..4c5c7b9a0a 100644 --- a/services/web/app/src/Features/Project/ProjectDuplicator.js +++ b/services/web/app/src/Features/Project/ProjectDuplicator.js @@ -213,7 +213,8 @@ async function _copyFiles(sourceEntries, sourceProject, targetProject) { file.hash = sourceFile.hash } let createdBlob = false - if (file.hash != null) { + if (file.hash != null && Features.hasFeature('project-history-blobs')) { + const usingFilestore = Features.hasFeature('filestore') try { await HistoryManager.promises.copyBlob( sourceHistoryId, @@ -221,20 +222,32 @@ async function _copyFiles(sourceEntries, sourceProject, targetProject) { file.hash ) createdBlob = true + if (!usingFilestore) { + return { createdBlob, file, path, url: null } + } } catch (err) { - logger.error( - { - err, + if (!usingFilestore) { + throw OError.tag(err, 'unexpected error copying blob', { sourceProjectId: sourceProject._id, targetProjectId: targetProject._id, sourceFile, sourceHistoryId, - }, - 'unexpected error copying blob' - ) + }) + } else { + logger.error( + { + err, + sourceProjectId: sourceProject._id, + targetProjectId: targetProject._id, + sourceFile, + sourceHistoryId, + }, + 'unexpected error copying blob' + ) + } } } - if (createdBlob && Features.hasFeature('saas')) { + if (createdBlob && Features.hasFeature('project-history-blobs')) { return { createdBlob, file, path, url: null } } const url = await FileStoreHandler.promises.copyFile( diff --git a/services/web/app/src/Features/Project/ProjectEditorHandler.js b/services/web/app/src/Features/Project/ProjectEditorHandler.js index a0d82a3bc9..84c0a5831a 100644 --- a/services/web/app/src/Features/Project/ProjectEditorHandler.js +++ b/services/web/app/src/Features/Project/ProjectEditorHandler.js @@ -1,6 +1,7 @@ let ProjectEditorHandler const _ = require('lodash') const Path = require('path') +const Features = require('../../infrastructure/Features') function mergeDeletedDocs(a, b) { const docIdsInA = new Set(a.map(doc => doc._id.toString())) @@ -123,12 +124,18 @@ module.exports = ProjectEditorHandler = { }, buildFileModelView(file) { + const additionalFileProperties = {} + + if (Features.hasFeature('project-history-blobs')) { + additionalFileProperties.hash = file.hash + } + return { _id: file._id, name: file.name, linkedFileData: file.linkedFileData, created: file.created, - hash: file.hash, + ...additionalFileProperties, } }, diff --git a/services/web/app/src/infrastructure/Features.js b/services/web/app/src/infrastructure/Features.js index 066d2e4305..aaf51103b9 100644 --- a/services/web/app/src/infrastructure/Features.js +++ b/services/web/app/src/infrastructure/Features.js @@ -19,6 +19,8 @@ const trackChangesModuleAvailable = * @property {boolean | undefined} enableGithubSync * @property {boolean | undefined} enableGitBridge * @property {boolean | undefined} enableHomepage + * @property {boolean | undefined} enableProjectHistoryBlobs + * @property {boolean | undefined} disableFilestore * @property {boolean | undefined} enableSaml * @property {boolean | undefined} ldap * @property {boolean | undefined} oauth @@ -86,6 +88,10 @@ const Features = { _.get(Settings, ['apis', 'linkedUrlProxy', 'url']) && Settings.enabledLinkedFileTypes.includes('url') ) + case 'project-history-blobs': + return Boolean(Settings.enableProjectHistoryBlobs) + case 'filestore': + return Boolean(Settings.disableFilestore) === false case 'support': return supportModuleAvailable case 'symbol-palette': diff --git a/services/web/app/views/project/editor/_meta.pug b/services/web/app/views/project/editor/_meta.pug index 51457615de..d2c9a7054f 100644 --- a/services/web/app/views/project/editor/_meta.pug +++ b/services/web/app/views/project/editor/_meta.pug @@ -10,6 +10,7 @@ meta(name="ol-isRestrictedTokenMember" data-type="boolean" content=isRestrictedT meta(name="ol-maxDocLength" data-type="json" content=maxDocLength) meta(name="ol-wikiEnabled" data-type="boolean" content=settings.proxyLearn) meta(name="ol-chatEnabled" data-type="boolean" content=chatEnabled) +meta(name="ol-projectHistoryBlobsEnabled" data-type="boolean" content=projectHistoryBlobsEnabled) meta(name="ol-gitBridgePublicBaseUrl" content=gitBridgePublicBaseUrl) meta(name="ol-gitBridgeEnabled" data-type="boolean" content=gitBridgeEnabled) meta(name="ol-compilesUserContentDomain" content=settings.compilesUserContentDomain) diff --git a/services/web/frontend/js/features/file-tree/contexts/file-tree-actionable.tsx b/services/web/frontend/js/features/file-tree/contexts/file-tree-actionable.tsx index 01bd35359d..aaba66661d 100644 --- a/services/web/frontend/js/features/file-tree/contexts/file-tree-actionable.tsx +++ b/services/web/frontend/js/features/file-tree/contexts/file-tree-actionable.tsx @@ -34,6 +34,7 @@ import { import { Folder } from '../../../../../types/folder' import { useReferencesContext } from '@/features/ide-react/context/references-context' import { usePermissionsContext } from '@/features/ide-react/context/permissions-context' +import { fileUrl } from '@/features/utils/fileUrl' type DroppedFile = File & { relativePath?: string @@ -494,7 +495,7 @@ export const FileTreeActionableProvider: FC = ({ children }) => { const selectedEntity = findInTree(fileTreeData, selectedEntityId) if (selectedEntity?.type === 'fileRef') { - return `/project/${projectId}/blob/${selectedEntity.entity.hash}?fallback=${selectedEntityId}` + return fileUrl(projectId, selectedEntityId, selectedEntity.entity.hash) } if (selectedEntity?.type === 'doc') { diff --git a/services/web/frontend/js/features/utils/fileUrl.js b/services/web/frontend/js/features/utils/fileUrl.js index 343b3b42c8..1922144b4b 100644 --- a/services/web/frontend/js/features/utils/fileUrl.js +++ b/services/web/frontend/js/features/utils/fileUrl.js @@ -1,8 +1,12 @@ // Helper function to compute the url for a file in history-v1 or filestore. // This will be obsolete when the migration to history-v1 is complete. +import getMeta from '@/utils/meta' + +const projectHistoryBlobsEnabled = getMeta('ol-projectHistoryBlobsEnabled') + export function fileUrl(projectId, id, hash) { - if (hash) { + if (projectHistoryBlobsEnabled && hash) { return `/project/${projectId}/blob/${hash}?fallback=${id}` } else { return `/project/${projectId}/file/${id}` diff --git a/services/web/frontend/js/utils/meta.ts b/services/web/frontend/js/utils/meta.ts index 09df91cdef..762cea1e63 100644 --- a/services/web/frontend/js/utils/meta.ts +++ b/services/web/frontend/js/utils/meta.ts @@ -165,6 +165,7 @@ export interface Meta { 'ol-prefetchedProjectsBlob': GetProjectsResponseBody | undefined 'ol-primaryEmail': { email: string; confirmed: boolean } 'ol-project': any // TODO + 'ol-projectHistoryBlobsEnabled': boolean 'ol-projectSyncSuccessMessage': string 'ol-projectTags': Tag[] 'ol-project_id': string diff --git a/services/web/modules/history-v1/test/acceptance/src/ProjectStructureTests.mjs b/services/web/modules/history-v1/test/acceptance/src/ProjectStructureTests.mjs index 3ac572ee1d..2978f99df9 100644 --- a/services/web/modules/history-v1/test/acceptance/src/ProjectStructureTests.mjs +++ b/services/web/modules/history-v1/test/acceptance/src/ProjectStructureTests.mjs @@ -257,7 +257,7 @@ describe('ProjectStructureChanges', function () { expect(updates[2].type).to.equal('add-file') expect(updates[2].userId).to.equal(owner._id) expect(updates[2].pathname).to.equal('/frog.jpg') - if (Features.hasFeature('saas')) { + if (Features.hasFeature('project-history-blobs')) { expect(updates[2].url).to.be.null } else { expect(updates[2].url).to.be.a('string') diff --git a/services/web/test/acceptance/src/HistoryTests.mjs b/services/web/test/acceptance/src/HistoryTests.mjs index f0c86b43fe..a87b98d158 100644 --- a/services/web/test/acceptance/src/HistoryTests.mjs +++ b/services/web/test/acceptance/src/HistoryTests.mjs @@ -9,6 +9,7 @@ import { fileURLToPath } from 'node:url' import sinon from 'sinon' import logger from '@overleaf/logger' import Metrics from './helpers/metrics.js' +import Features from '../../../app/src/infrastructure/Features.js' const User = UserHelper.promises let MockV1HistoryApi, MockFilestoreApi @@ -75,41 +76,45 @@ describe('HistoryTests', function () { 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() - }) + if (Features.hasFeature('project-history-blobs')) { + 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 () { - const { response } = await user.doRequest('HEAD', fileURL) - expect(response.statusCode).to.equal(200) - expect(response.headers['x-served-by']).to.include('history-v1') - expect(response.headers['content-length']).to.equal('3694') - await expectHistoryV1Hit() - }) + if (Features.hasFeature('project-history-blobs')) { + it('should fetch the file size from history-v1', async function () { + const { response } = await user.doRequest('HEAD', fileURL) + expect(response.statusCode).to.equal(200) + expect(response.headers['x-served-by']).to.include('history-v1') + expect(response.headers['content-length']).to.equal('3694') + await expectHistoryV1Hit() + }) + } it('should return 404 without fallback', async function () { MockV1HistoryApi.reset() const { response } = await user.doRequest('HEAD', fileURL) @@ -133,13 +138,15 @@ describe('HistoryTests', function () { }) }) describe('GET', function () { - it('should fetch the file from history-v1', async function () { - const { response, body } = await user.doRequest('GET', fileURL) - expect(response.statusCode).to.equal(200) - expect(response.headers['x-served-by']).to.include('history-v1') - expect(body).to.equal(fileContent) - await expectHistoryV1Hit() - }) + if (Features.hasFeature('project-history-blobs')) { + it('should fetch the file from history-v1', async function () { + const { response, body } = await user.doRequest('GET', fileURL) + expect(response.statusCode).to.equal(200) + expect(response.headers['x-served-by']).to.include('history-v1') + expect(body).to.equal(fileContent) + await expectHistoryV1Hit() + }) + } it('should return 404 without fallback', async function () { MockV1HistoryApi.reset() const { response } = await user.doRequest('GET', fileURL) diff --git a/services/web/test/frontend/helpers/reset-meta.ts b/services/web/test/frontend/helpers/reset-meta.ts index 9e998c1cd1..f5a979828a 100644 --- a/services/web/test/frontend/helpers/reset-meta.ts +++ b/services/web/test/frontend/helpers/reset-meta.ts @@ -1,5 +1,6 @@ export function resetMeta() { window.metaAttributesCache = new Map() + window.metaAttributesCache.set('ol-projectHistoryBlobsEnabled', true) window.metaAttributesCache.set('ol-i18n', { currentLangCode: 'en' }) window.metaAttributesCache.set('ol-ExposedSettings', { appName: 'Overleaf', diff --git a/services/web/test/unit/src/Compile/ClsiManagerTests.js b/services/web/test/unit/src/Compile/ClsiManagerTests.js index c2ff18a7ba..ff50ac16bb 100644 --- a/services/web/test/unit/src/Compile/ClsiManagerTests.js +++ b/services/web/test/unit/src/Compile/ClsiManagerTests.js @@ -144,6 +144,9 @@ describe('ClsiManager', function () { enablePdfCaching: true, clsiCookie: { key: 'clsiserver' }, } + this.Features = { + hasFeature: sinon.stub().withArgs('project-history-blobs').returns(true), + } this.HistoryManager = { getBlobLocation: sinon.stub().callsFake((historyId, hash) => { if (hash === GLOBAL_BLOB_HASH) { @@ -162,6 +165,7 @@ describe('ClsiManager', function () { '../../models/Project': { Project: this.Project, }, + '../../infrastructure/Features': this.Features, '../Project/ProjectEntityHandler': this.ProjectEntityHandler, '../Project/ProjectGetter': this.ProjectGetter, '../DocumentUpdater/DocumentUpdaterHandler': diff --git a/services/web/test/unit/src/Downloads/ProjectZipStreamManagerTests.mjs b/services/web/test/unit/src/Downloads/ProjectZipStreamManagerTests.mjs index 12712f07bd..f86b99bd96 100644 --- a/services/web/test/unit/src/Downloads/ProjectZipStreamManagerTests.mjs +++ b/services/web/test/unit/src/Downloads/ProjectZipStreamManagerTests.mjs @@ -27,6 +27,7 @@ describe('ProjectZipStreamManager', function () { info: sinon.stub(), debug: sinon.stub(), } + return (this.ProjectZipStreamManager = await esmock.strict(modulePath, { archiver: (this.archiver = sinon.stub().returns(this.archive)), '@overleaf/logger': this.logger, @@ -36,6 +37,14 @@ describe('ProjectZipStreamManager', function () { (this.HistoryManager = {}), '../../../../app/src/Features/Project/ProjectGetter': (this.ProjectGetter = {}), + '../../../../app/src/Features/FileStore/FileStoreHandler': + (this.FileStoreHandler = {}), + '../../../../app/src/infrastructure/Features': (this.Features = { + hasFeature: sinon + .stub() + .withArgs('project-history-blobs') + .returns(true), + }), })) }) @@ -380,65 +389,89 @@ describe('ProjectZipStreamManager', function () { this.ProjectEntityHandler.getAllFiles = sinon .stub() .callsArgWith(1, null, this.files) - this.HistoryManager.requestBlobWithFallback = ( - projectId, - hash, - fileId, - callback - ) => { - return callback(null, { stream: this.streams[fileId] }) - } - sinon.spy(this.HistoryManager, 'requestBlobWithFallback') - this.ProjectZipStreamManager.addAllFilesToArchive( - this.project_id, - this.archive, - this.callback - ) - return (() => { - const result = [] + }) + describe('with project-history-blobs feature enabled', function () { + beforeEach(function () { + this.HistoryManager.requestBlobWithFallback = ( + projectId, + hash, + fileId, + callback + ) => { + return callback(null, { stream: this.streams[fileId] }) + } + sinon.spy(this.HistoryManager, 'requestBlobWithFallback') + this.ProjectZipStreamManager.addAllFilesToArchive( + this.project_id, + this.archive, + this.callback + ) for (const path in this.streams) { const stream = this.streams[path] - result.push(stream.emit('end')) + stream.emit('end') } - return result - })() - }) + }) - it('should get the files for the project', function () { - return this.ProjectEntityHandler.getAllFiles - .calledWith(this.project_id) - .should.equal(true) - }) + it('should get the files for the project', function () { + return this.ProjectEntityHandler.getAllFiles + .calledWith(this.project_id) + .should.equal(true) + }) - it('should get a stream for each file', function () { - return (() => { - const result = [] + it('should get a stream for each file', function () { for (const path in this.files) { const file = this.files[path] - result.push( - this.HistoryManager.requestBlobWithFallback - .calledWith(this.project_id, file.hash, file._id) - .should.equal(true) - ) - } - return result - })() - }) - it('should add each file to the archive', function () { - return (() => { - const result = [] + this.HistoryManager.requestBlobWithFallback + .calledWith(this.project_id, file.hash, file._id) + .should.equal(true) + } + }) + + it('should add each file to the archive', function () { for (let path in this.files) { const file = this.files[path] path = path.slice(1) // remove "/" - result.push( - this.archive.append - .calledWith(this.streams[file._id], { name: path }) - .should.equal(true) - ) + this.archive.append + .calledWith(this.streams[file._id], { name: path }) + .should.equal(true) } - return result - })() + }) + }) + + describe('with project-history-blobs feature disabled', function () { + beforeEach(function () { + this.FileStoreHandler.getFileStream = ( + projectId, + fileId, + query, + callback + ) => callback(null, this.streams[fileId]) + + sinon.spy(this.FileStoreHandler, 'getFileStream') + this.Features.hasFeature + .withArgs('project-history-blobs') + .returns(false) + this.ProjectZipStreamManager.addAllFilesToArchive( + this.project_id, + this.archive, + this.callback + ) + for (const path in this.streams) { + const stream = this.streams[path] + stream.emit('end') + } + }) + + it('should get a stream for each file', function () { + for (const path in this.files) { + const file = this.files[path] + + this.FileStoreHandler.getFileStream + .calledWith(this.project_id, file._id) + .should.equal(true) + } + }) }) }) }) diff --git a/services/web/test/unit/src/FileStore/FileStoreHandlerTests.js b/services/web/test/unit/src/FileStore/FileStoreHandlerTests.js index 817f090ac3..26e4da3162 100644 --- a/services/web/test/unit/src/FileStore/FileStoreHandlerTests.js +++ b/services/web/test/unit/src/FileStore/FileStoreHandlerTests.js @@ -8,6 +8,7 @@ const MODULE_PATH = '../../../../app/src/Features/FileStore/FileStoreHandler.js' describe('FileStoreHandler', function () { beforeEach(function () { + this.fileSize = 999 this.fs = { createReadStream: sinon.stub(), lstat: sinon.stub().callsArgWith(1, null, { @@ -40,7 +41,6 @@ describe('FileStoreHandler', function () { this.fileId = 'file_id_here' this.projectId = '1312312312' this.historyId = 123 - this.fileSize = 999 this.hashValue = '2aae6c35c94fcfb415dbe95f408b9ce91ee846ed' this.fsPath = 'uploads/myfile.eps' this.getFileUrl = (projectId, fileId) => @@ -69,6 +69,10 @@ describe('FileStoreHandler', function () { }), } + this.Features = { + hasFeature: sinon.stub(), + } + this.handler = SandboxedModule.require(MODULE_PATH, { requires: { '@overleaf/settings': this.settings, @@ -76,6 +80,7 @@ describe('FileStoreHandler', function () { '../History/HistoryManager': this.HistoryManager, '../Project/ProjectDetailsHandler': this.ProjectDetailsHandler, './FileHashManager': this.FileHashManager, + '../../infrastructure/Features': this.Features, // FIXME: need to stub File object here '../../models/File': { File: this.FileModel, @@ -134,31 +139,55 @@ describe('FileStoreHandler', function () { ) }) - it('should upload the file to the history store as a blob', function (done) { - this.fs.createReadStream.returns({ - pipe() {}, - on(type, cb) { - if (type === 'open') { - cb() + describe('when project-history-blobs feature is enabled', function () { + it('should upload the file to the history store as a blob', function (done) { + this.fs.createReadStream.returns({ + pipe() {}, + on(type, cb) { + if (type === 'open') { + cb() + } + }, + }) + this.Features.hasFeature.withArgs('project-history-blobs').returns(true) + this.handler.uploadFileFromDisk( + this.projectId, + this.fileArgs, + this.fsPath, + () => { + this.HistoryManager.uploadBlobFromDisk + .calledWith( + this.historyId, + this.hashValue, + this.fileSize, + this.fsPath + ) + .should.equal(true) + done() } - }, + ) + }) + }) + describe('when project-history-blobs feature is disabled', function () { + it('should not upload the file to the history store as a blob', function (done) { + this.fs.createReadStream.returns({ + pipe() {}, + on(type, cb) { + if (type === 'open') { + cb() + } + }, + }) + this.handler.uploadFileFromDisk( + this.projectId, + this.fileArgs, + this.fsPath, + () => { + this.HistoryManager.uploadBlobFromDisk.called.should.equal(false) + done() + } + ) }) - this.handler.uploadFileFromDisk( - this.projectId, - this.fileArgs, - this.fsPath, - () => { - this.HistoryManager.uploadBlobFromDisk - .calledWith( - this.historyId, - this.hashValue, - this.fileSize, - this.fsPath - ) - .should.equal(true) - done() - } - ) }) it('should create read stream', function (done) { diff --git a/services/web/test/unit/src/Project/ProjectDuplicatorTests.js b/services/web/test/unit/src/Project/ProjectDuplicatorTests.js index b272f4bdd9..1a49171163 100644 --- a/services/web/test/unit/src/Project/ProjectDuplicatorTests.js +++ b/services/web/test/unit/src/Project/ProjectDuplicatorTests.js @@ -221,6 +221,9 @@ describe('ProjectDuplicator', function () { flushProjectToTpds: sinon.stub().resolves(), }, } + this.Features = { + hasFeature: sinon.stub().withArgs('project-history-blobs').returns(true), + } this.ProjectDuplicator = SandboxedModule.require(MODULE_PATH, { requires: { @@ -241,6 +244,7 @@ describe('ProjectDuplicator', function () { '../ThirdPartyDataStore/TpdsProjectFlusher': this.TpdsProjectFlusher, '../Tags/TagsHandler': this.TagsHandler, '../History/HistoryManager': this.HistoryManager, + '../../infrastructure/Features': this.Features, }, }) }) diff --git a/services/web/test/unit/src/References/ReferencesHandlerTests.mjs b/services/web/test/unit/src/References/ReferencesHandlerTests.mjs index eb140b350d..57570dcf12 100644 --- a/services/web/test/unit/src/References/ReferencesHandlerTests.mjs +++ b/services/web/test/unit/src/References/ReferencesHandlerTests.mjs @@ -54,6 +54,7 @@ describe('ReferencesHandler', function () { filestore: { url: 'http://some.url/filestore' }, project_history: { url: 'http://project-history.local' }, }, + enableProjectHistoryBlobs: true, }), request: (this.request = { get: sinon.stub(), diff --git a/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateSenderTests.js b/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateSenderTests.js index 8552cfa99e..f7f733388e 100644 --- a/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateSenderTests.js +++ b/services/web/test/unit/src/ThirdPartyDataStore/TpdsUpdateSenderTests.js @@ -57,6 +57,7 @@ describe('TpdsUpdateSender', function () { url: projectHistoryUrl, }, }, + enableProjectHistoryBlobs: true, } const getUsers = sinon.stub() getUsers