diff --git a/services/web/app.mjs b/services/web/app.mjs index 528f2c079b..5ece02cf32 100644 --- a/services/web/app.mjs +++ b/services/web/app.mjs @@ -18,6 +18,7 @@ import mongoose from './app/src/infrastructure/Mongoose.js' import { triggerGracefulShutdown } from './app/src/infrastructure/GracefulShutdown.js' import FileWriter from './app/src/infrastructure/FileWriter.js' import { fileURLToPath } from 'node:url' +import Features from './app/src/infrastructure/Features.js' logger.initialize(process.env.METRICS_APP_NAME || 'web') logger.logger.serializers.user = Serializers.user @@ -48,6 +49,15 @@ if (Settings.catchErrors) { // Create ./data/dumpFolder if needed FileWriter.ensureDumpFolderExists() +if ( + !Features.hasFeature('project-history-blobs') && + !Features.hasFeature('filestore') +) { + throw new Error( + 'invalid config: must enable either project-history-blobs (Settings.enableProjectHistoryBlobs=true) or enable filestore (Settings.disableFilestore=false)' + ) +} + const port = Settings.port || Settings.internal.web.port || 3000 const host = Settings.internal.web.host || '127.0.0.1' if (process.argv[1] === fileURLToPath(import.meta.url)) { diff --git a/services/web/app/src/Features/FileStore/FileStoreHandler.js b/services/web/app/src/Features/FileStore/FileStoreHandler.js index 7ca8e496d4..f787dc216d 100644 --- a/services/web/app/src/Features/FileStore/FileStoreHandler.js +++ b/services/web/app/src/Features/FileStore/FileStoreHandler.js @@ -130,6 +130,12 @@ const FileStoreHandler = { const fileRef = new File(fileArgs) const fileId = fileRef._id + const url = FileStoreHandler._buildUrl(projectId, fileId) + + if (!Features.hasFeature('filestore')) { + return callbackOnce(null, { url, fileRef }) + } + const readStream = fs.createReadStream(fsPath) readStream.on('error', function (err) { logger.warn( @@ -139,7 +145,6 @@ const FileStoreHandler = { callbackOnce(err) }) readStream.on('open', function () { - const url = FileStoreHandler._buildUrl(projectId, fileId) const opts = { method: 'post', uri: url, diff --git a/services/web/app/src/Features/Project/ProjectDuplicator.js b/services/web/app/src/Features/Project/ProjectDuplicator.js index 4c5c7b9a0a..da18c7e9b8 100644 --- a/services/web/app/src/Features/Project/ProjectDuplicator.js +++ b/services/web/app/src/Features/Project/ProjectDuplicator.js @@ -213,8 +213,8 @@ async function _copyFiles(sourceEntries, sourceProject, targetProject) { file.hash = sourceFile.hash } let createdBlob = false + const usingFilestore = Features.hasFeature('filestore') if (file.hash != null && Features.hasFeature('project-history-blobs')) { - const usingFilestore = Features.hasFeature('filestore') try { await HistoryManager.promises.copyBlob( sourceHistoryId, @@ -250,6 +250,12 @@ async function _copyFiles(sourceEntries, sourceProject, targetProject) { if (createdBlob && Features.hasFeature('project-history-blobs')) { return { createdBlob, file, path, url: null } } + if (!usingFilestore) { + // Note: This is also checked in app.mjs + throw new OError( + 'bad config: need to enable either filestore or project-history-blobs' + ) + } const url = await FileStoreHandler.promises.copyFile( sourceProject._id, sourceFile._id, 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 e4b9a6063f..d7a0a9364c 100644 --- a/services/web/modules/history-v1/test/acceptance/src/ProjectStructureTests.mjs +++ b/services/web/modules/history-v1/test/acceptance/src/ProjectStructureTests.mjs @@ -187,22 +187,33 @@ describe('ProjectStructureChanges', function () { const cases = [ { - label: 'with filestore disabled', + label: 'with filestore disabled and project-history-blobs enabled', disableFilestore: true, + enableProjectHistoryBlobs: true, }, { - label: 'with filestore enabled', + label: 'with filestore enabled and project-history-blobs enabled', disableFilestore: false, + enableProjectHistoryBlobs: true, + }, + { + label: 'with filestore enabled and project-history-blobs disabled', + disableFilestore: false, + enableProjectHistoryBlobs: false, }, ] - for (const { label, disableFilestore } of cases) { + for (const { label, disableFilestore, enableProjectHistoryBlobs } of cases) { describe(label, function () { - const previousSetting = Settings.disableFilestore + const previousDisableFilestore = Settings.disableFilestore + const previousEnableProjectHistoryBlobs = + Settings.enableProjectHistoryBlobs beforeEach(function () { Settings.disableFilestore = disableFilestore + Settings.enableProjectHistoryBlobs = enableProjectHistoryBlobs }) afterEach(function () { - Settings.disableFilestore = previousSetting + Settings.disableFilestore = previousDisableFilestore + Settings.enableProjectHistoryBlobs = previousEnableProjectHistoryBlobs }) describe('creating a project from the example template', function () { diff --git a/services/web/modules/history-v1/test/acceptance/src/RestoringFilesTest.mjs b/services/web/modules/history-v1/test/acceptance/src/RestoringFilesTest.mjs index fc02d5aed1..33ce4814a3 100644 --- a/services/web/modules/history-v1/test/acceptance/src/RestoringFilesTest.mjs +++ b/services/web/modules/history-v1/test/acceptance/src/RestoringFilesTest.mjs @@ -8,8 +8,10 @@ import User from '../../../../../test/acceptance/src/helpers/User.mjs' import MockProjectHistoryApiClass from '../../../../../test/acceptance/src/mocks/MockProjectHistoryApi.mjs' import MockDocstoreApiClass from '../../../../../test/acceptance/src/mocks/MockDocstoreApi.mjs' import MockFilestoreApiClass from '../../../../../test/acceptance/src/mocks/MockFilestoreApi.mjs' +import MockV1HistoryApiClass from '../../../../../test/acceptance/src/mocks/MockV1HistoryApi.mjs' +import Features from '../../../../../app/src/infrastructure/Features.js' -let MockProjectHistoryApi, MockDocstoreApi, MockFilestoreApi +let MockProjectHistoryApi, MockDocstoreApi, MockFilestoreApi, MockV1HistoryApi const __dirname = fileURLToPath(new URL('.', import.meta.url)) @@ -17,6 +19,7 @@ before(function () { MockProjectHistoryApi = MockProjectHistoryApiClass.instance() MockDocstoreApi = MockDocstoreApiClass.instance() MockFilestoreApi = MockFilestoreApiClass.instance() + MockV1HistoryApi = MockV1HistoryApiClass.instance() }) describe('RestoringFiles', function () { @@ -118,20 +121,41 @@ describe('RestoringFiles', function () { ) }) - it('should have created a file', function (done) { - this.owner.getProject(this.project_id, (error, project) => { - if (error) { - throw error - } - let file = _.find( - project.rootFolder[0].fileRefs, - file => file.name === 'image.png' - ) - file = MockFilestoreApi.getFile(this.project_id, file._id) - expect(file).to.deep.equal(this.pngData) - done() + if (Features.hasFeature('project-history-blobs')) { + it('should have created a file in history-v1', function (done) { + this.owner.getProject(this.project_id, (error, project) => { + if (error) { + throw error + } + let file = _.find( + project.rootFolder[0].fileRefs, + file => file.name === 'image.png' + ) + file = + MockV1HistoryApi.blobs[project.overleaf.history.id.toString()][ + file.hash + ] + expect(file).to.deep.equal(Buffer.from(this.pngData)) + done() + }) }) - }) + } + if (Features.hasFeature('filestore')) { + it('should have created a file in filestore', function (done) { + this.owner.getProject(this.project_id, (error, project) => { + if (error) { + throw error + } + let file = _.find( + project.rootFolder[0].fileRefs, + file => file.name === 'image.png' + ) + file = MockFilestoreApi.getFile(this.project_id, file._id) + expect(file).to.deep.equal(this.pngData) + done() + }) + }) + } }) describe('restoring to a directory that exists', function () { diff --git a/services/web/test/acceptance/src/HistoryTests.mjs b/services/web/test/acceptance/src/HistoryTests.mjs index 9ac8adfe3a..9e45f73354 100644 --- a/services/web/test/acceptance/src/HistoryTests.mjs +++ b/services/web/test/acceptance/src/HistoryTests.mjs @@ -84,13 +84,15 @@ describe('HistoryTests', function () { 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() - }) + if (Features.hasFeature('filestore')) { + 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() @@ -122,14 +124,16 @@ describe('HistoryTests', function () { expect(response.statusCode).to.equal(404) await expectNoIncrement() }) - it('should fetch the file size from filestore when missing in history-v1', async function () { - MockV1HistoryApi.reset() - const { response } = await user.doRequest('HEAD', blobURLWithFallback) - expect(response.statusCode).to.equal(200) - expect(response.headers['x-served-by']).to.include('filestore') - expect(response.headers['content-length']).to.equal('3694') - await expectFilestoreHit() - }) + if (Features.hasFeature('filestore')) { + it('should fetch the file size from filestore when missing in history-v1', async function () { + MockV1HistoryApi.reset() + const { response } = await user.doRequest('HEAD', blobURLWithFallback) + expect(response.statusCode).to.equal(200) + expect(response.headers['x-served-by']).to.include('filestore') + expect(response.headers['content-length']).to.equal('3694') + await expectFilestoreHit() + }) + } it('should return 404 with both files missing', async function () { MockFilestoreApi.reset() MockV1HistoryApi.reset() @@ -177,17 +181,19 @@ describe('HistoryTests', function () { expect(response.headers).not.to.have.property('cache-control') expect(response.headers).not.to.have.property('etag') }) - it('should fetch the file size from filestore when missing in history-v1', async function () { - MockV1HistoryApi.reset() - const { response, body } = await user.doRequest( - 'GET', - blobURLWithFallback - ) - expect(response.statusCode).to.equal(200) - expect(response.headers['x-served-by']).to.include('filestore') - expect(body).to.equal(fileContent) - await expectFilestoreHit() - }) + if (Features.hasFeature('filestore')) { + it('should fetch the file size from filestore when missing in history-v1', async function () { + MockV1HistoryApi.reset() + const { response, body } = await user.doRequest( + 'GET', + blobURLWithFallback + ) + expect(response.statusCode).to.equal(200) + expect(response.headers['x-served-by']).to.include('filestore') + expect(body).to.equal(fileContent) + await expectFilestoreHit() + }) + } it('should return 404 with both files missing', async function () { MockFilestoreApi.reset() MockV1HistoryApi.reset() @@ -210,13 +216,15 @@ describe('HistoryTests', function () { await expectHistoryV1Hit() }) } - it('should fetch the file size from filestore when missing in history-v1', async function () { - MockV1HistoryApi.reset() - const { response } = await user.doRequest('HEAD', blobURLWithFallback) - expect(response.statusCode).to.equal(200) - expect(response.headers['x-served-by']).to.include('filestore') - expect(response.headers['content-length']).to.equal('3694') - }) + if (Features.hasFeature('filestore')) { + it('should fetch the file size from filestore when missing in history-v1', async function () { + MockV1HistoryApi.reset() + const { response } = await user.doRequest('HEAD', blobURLWithFallback) + expect(response.statusCode).to.equal(200) + expect(response.headers['x-served-by']).to.include('filestore') + expect(response.headers['content-length']).to.equal('3694') + }) + } it('should return 404 with both files missing', async function () { MockFilestoreApi.reset() MockV1HistoryApi.reset() @@ -251,13 +259,15 @@ describe('HistoryTests', function () { expect(response.headers).not.to.have.property('cache-control') expect(response.headers).not.to.have.property('etag') }) - it('should fetch the file size from filestore when missing in history-v1', async function () { - MockV1HistoryApi.reset() - const { response, body } = await user.doRequest('GET', fileURL) - expect(response.statusCode).to.equal(200) - expect(response.headers['x-served-by']).to.include('filestore') - expect(body).to.equal(fileContent) - }) + if (Features.hasFeature('filestore')) { + it('should fetch the file size from filestore when missing in history-v1', async function () { + MockV1HistoryApi.reset() + const { response, body } = await user.doRequest('GET', fileURL) + expect(response.statusCode).to.equal(200) + expect(response.headers['x-served-by']).to.include('filestore') + expect(body).to.equal(fileContent) + }) + } it('should return 404 with both files missing', async function () { MockFilestoreApi.reset() MockV1HistoryApi.reset() diff --git a/services/web/test/acceptance/src/ProjectDuplicateNameTests.mjs b/services/web/test/acceptance/src/ProjectDuplicateNameTests.mjs index ed6d8efe22..b9a0e72865 100644 --- a/services/web/test/acceptance/src/ProjectDuplicateNameTests.mjs +++ b/services/web/test/acceptance/src/ProjectDuplicateNameTests.mjs @@ -7,15 +7,18 @@ import User from './helpers/User.mjs' import UserHelper from './helpers/UserHelper.mjs' import MockDocstoreApiClass from './mocks/MockDocstoreApi.mjs' import MockFilestoreApiClass from './mocks/MockFilestoreApi.mjs' +import MockV1HistoryApiClass from './mocks/MockV1HistoryApi.mjs' import { fileURLToPath } from 'node:url' +import Features from '../../../app/src/infrastructure/Features.js' -let MockDocstoreApi, MockFilestoreApi +let MockDocstoreApi, MockFilestoreApi, MockV1HistoryApi const __dirname = fileURLToPath(new URL('.', import.meta.url)) before(function () { MockDocstoreApi = MockDocstoreApiClass.instance() MockFilestoreApi = MockFilestoreApiClass.instance() + MockV1HistoryApi = MockV1HistoryApiClass.instance() }) describe('ProjectDuplicateNames', function () { @@ -80,10 +83,19 @@ describe('ProjectDuplicateNames', function () { expect(Object.keys(docs).length).to.equal(2) }) - it('should create one file in the filestore', function () { - const files = MockFilestoreApi.files[this.example_project_id] - expect(Object.keys(files).length).to.equal(1) - }) + if (Features.hasFeature('project-history-blobs')) { + it('should create one file in the history-v1', function () { + const files = + MockV1HistoryApi.blobs[this.project.overleaf.history.id.toString()] + expect(Object.keys(files).length).to.equal(1) + }) + } + if (Features.hasFeature('filestore')) { + it('should create one file in the filestore', function () { + const files = MockFilestoreApi.files[this.example_project_id] + expect(Object.keys(files).length).to.equal(1) + }) + } describe('for an existing doc', function () { describe('trying to add a doc with the same name', function () { diff --git a/services/web/test/unit/src/FileStore/FileStoreHandlerTests.js b/services/web/test/unit/src/FileStore/FileStoreHandlerTests.js index 26e4da3162..cbc63d045d 100644 --- a/services/web/test/unit/src/FileStore/FileStoreHandlerTests.js +++ b/services/web/test/unit/src/FileStore/FileStoreHandlerTests.js @@ -190,91 +190,174 @@ describe('FileStoreHandler', function () { }) }) - it('should create read stream', function (done) { - this.fs.createReadStream.returns({ - pipe() {}, - on(type, cb) { - if (type === 'open') { - cb() - } - }, + describe('when filestore feature is enabled', function () { + beforeEach(function () { + this.Features.hasFeature.withArgs('filestore').returns(true) + }) + it('should create read stream', function (done) { + this.fs.createReadStream.returns({ + pipe() {}, + on(type, cb) { + if (type === 'open') { + cb() + } + }, + }) + this.handler.uploadFileFromDisk( + this.projectId, + this.fileArgs, + this.fsPath, + () => { + this.fs.createReadStream.calledWith(this.fsPath).should.equal(true) + done() + } + ) }) - this.handler.uploadFileFromDisk( - this.projectId, - this.fileArgs, - this.fsPath, - () => { - this.fs.createReadStream.calledWith(this.fsPath).should.equal(true) - done() - } - ) - }) - it('should pipe the read stream to request', function (done) { - this.request.returns(this.writeStream) - this.fs.createReadStream.returns({ - on(type, cb) { - if (type === 'open') { - cb() - } - }, - pipe: o => { - this.writeStream.should.equal(o) - done() - }, + it('should pipe the read stream to request', function (done) { + this.request.returns(this.writeStream) + this.fs.createReadStream.returns({ + on(type, cb) { + if (type === 'open') { + cb() + } + }, + pipe: o => { + this.writeStream.should.equal(o) + done() + }, + }) + this.handler.uploadFileFromDisk( + this.projectId, + this.fileArgs, + this.fsPath, + () => {} + ) }) - this.handler.uploadFileFromDisk( - this.projectId, - this.fileArgs, - this.fsPath, - () => {} - ) - }) - it('should pass the correct options to request', function (done) { - const fileUrl = this.getFileUrl(this.projectId, this.fileId) - this.fs.createReadStream.returns({ - pipe() {}, - on(type, cb) { - if (type === 'open') { - cb() + it('should pass the correct options to request', function (done) { + const fileUrl = this.getFileUrl(this.projectId, this.fileId) + this.fs.createReadStream.returns({ + pipe() {}, + on(type, cb) { + if (type === 'open') { + cb() + } + }, + }) + this.handler.uploadFileFromDisk( + this.projectId, + this.fileArgs, + this.fsPath, + () => { + this.request.args[0][0].method.should.equal('post') + this.request.args[0][0].uri.should.equal(fileUrl) + done() } - }, + ) }) - this.handler.uploadFileFromDisk( - this.projectId, - this.fileArgs, - this.fsPath, - () => { - this.request.args[0][0].method.should.equal('post') - this.request.args[0][0].uri.should.equal(fileUrl) - done() - } - ) - }) - it('should callback with the url and fileRef', function (done) { - const fileUrl = this.getFileUrl(this.projectId, this.fileId) - this.fs.createReadStream.returns({ - pipe() {}, - on(type, cb) { - if (type === 'open') { - cb() + it('should callback with the url and fileRef', function (done) { + const fileUrl = this.getFileUrl(this.projectId, this.fileId) + this.fs.createReadStream.returns({ + pipe() {}, + on(type, cb) { + if (type === 'open') { + cb() + } + }, + }) + this.handler.uploadFileFromDisk( + this.projectId, + this.fileArgs, + this.fsPath, + (err, url, fileRef) => { + expect(err).to.not.exist + expect(url).to.equal(fileUrl) + expect(fileRef._id).to.equal(this.fileId) + expect(fileRef.hash).to.equal(this.hashValue) + done() } - }, + ) + }) + describe('when upload to filestore fails', function () { + beforeEach(function () { + this.writeStream.on = function (type, fn) { + if (type === 'response') { + fn({ statusCode: 500 }) + } + } + }) + + it('should callback with an error', function (done) { + this.fs.createReadStream.callCount = 0 + this.fs.createReadStream.returns({ + pipe() {}, + on(type, cb) { + if (type === 'open') { + cb() + } + }, + }) + this.handler.uploadFileFromDisk( + this.projectId, + this.fileArgs, + this.fsPath, + err => { + expect(err).to.exist + expect(err).to.be.instanceof(Error) + expect(this.fs.createReadStream.callCount).to.equal( + this.handler.RETRY_ATTEMPTS + ) + done() + } + ) + }) + }) + }) + describe('when filestore feature is disabled', function () { + beforeEach(function () { + this.Features.hasFeature.withArgs('filestore').returns(false) + }) + it('should not open file handle', function (done) { + this.handler.uploadFileFromDisk( + this.projectId, + this.fileArgs, + this.fsPath, + () => { + expect(this.fs.createReadStream).to.not.have.been.called + done() + } + ) + }) + + it('should not talk to filestore', function (done) { + this.handler.uploadFileFromDisk( + this.projectId, + this.fileArgs, + this.fsPath, + () => { + expect(this.request).to.not.have.been.called + done() + } + ) + }) + + it('should callback with the url and fileRef', function (done) { + const fileUrl = this.getFileUrl(this.projectId, this.fileId) + this.handler.uploadFileFromDisk( + this.projectId, + this.fileArgs, + this.fsPath, + (err, url, fileRef) => { + expect(err).to.not.exist + expect(url).to.equal(fileUrl) + expect(fileRef._id).to.equal(this.fileId) + expect(fileRef.hash).to.equal(this.hashValue) + done() + } + ) }) - this.handler.uploadFileFromDisk( - this.projectId, - this.fileArgs, - this.fsPath, - (err, url, fileRef) => { - expect(err).to.not.exist - expect(url).to.equal(fileUrl) - expect(fileRef._id).to.equal(this.fileId) - expect(fileRef.hash).to.equal(this.hashValue) - done() - } - ) }) describe('symlink', function () { @@ -312,41 +395,6 @@ describe('FileStoreHandler', function () { ) }) }) - - describe('when upload fails', function () { - beforeEach(function () { - this.writeStream.on = function (type, fn) { - if (type === 'response') { - fn({ statusCode: 500 }) - } - } - }) - - it('should callback with an error', function (done) { - this.fs.createReadStream.callCount = 0 - this.fs.createReadStream.returns({ - pipe() {}, - on(type, cb) { - if (type === 'open') { - cb() - } - }, - }) - this.handler.uploadFileFromDisk( - this.projectId, - this.fileArgs, - this.fsPath, - err => { - expect(err).to.exist - expect(err).to.be.instanceof(Error) - expect(this.fs.createReadStream.callCount).to.equal( - this.handler.RETRY_ATTEMPTS - ) - done() - } - ) - }) - }) }) describe('deleteFile', function () {