[web] disable filestore writes (#23470)

GitOrigin-RevId: 96ccd9205f9bf21420e67aaa68f8bc035eeb87d8

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)) {

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,

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,

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 () {

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 () {

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()

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 () {

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()
+      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,
+          () => {}
+        )
+      })
+
+      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()
           }
-        },
-        pipe: o => {
-          this.writeStream.should.equal(o)
-          done()
-        },
+        )
       })
-      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 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()
+            }
+          )
+        })
       })
-      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()
-        }
-      )
     })
+    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 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 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 () {