Merge pull request #21321 from overleaf/jpa-reconfigure-filestore

[filestore] refactor acceptance tests

GitOrigin-RevId: a6bb1527220b1c062d980c79d2ccb62973b99d2c

package-lock.json

@@ -17185,12 +17185,6 @@
         "nan": "^2.14.0"
       }
     },
-    "node_modules/disrequire": {
-      "version": "1.1.2",
-      "resolved": "https://registry.npmjs.org/disrequire/-/disrequire-1.1.2.tgz",
-      "integrity": "sha512-UaachK0eL7neLyL2emXptVGyggGJKowJd24rqCZi9N2CDxuCQTk7wdePTgS7py4HMh+qxUI6zzTVinVwCDTbIA==",
-      "dev": true
-    },
     "node_modules/dnd-core": {
       "version": "16.0.1",
       "resolved": "https://registry.npmjs.org/dnd-core/-/dnd-core-16.0.1.tgz",
@@ -38451,14 +38445,12 @@
         "aws-sdk": "^2.718.0",
         "chai": "^4.3.6",
         "chai-as-promised": "^7.1.1",
-        "disrequire": "^1.1.0",
         "mocha": "^10.2.0",
         "mongodb": "^6.1.0",
         "sandboxed-module": "2.0.4",
         "sinon": "9.0.2",
         "sinon-chai": "^3.7.0",
         "streamifier": "^0.1.1",
-        "timekeeper": "^2.2.0",
         "typescript": "^5.0.4"
       }
     },
@@ -49235,7 +49227,6 @@
         "bunyan": "^1.8.15",
         "chai": "^4.3.6",
         "chai-as-promised": "^7.1.1",
-        "disrequire": "^1.1.0",
         "express": "^4.21.0",
         "glob": "^7.1.6",
         "lodash.once": "^4.1.1",
@@ -49247,7 +49238,6 @@
         "sinon": "9.0.2",
         "sinon-chai": "^3.7.0",
         "streamifier": "^0.1.1",
-        "timekeeper": "^2.2.0",
         "tiny-async-pool": "^1.1.0",
         "typescript": "^5.0.4"
       },
@@ -59089,12 +59079,6 @@
         "nan": "^2.14.0"
       }
     },
-    "disrequire": {
-      "version": "1.1.2",
-      "resolved": "https://registry.npmjs.org/disrequire/-/disrequire-1.1.2.tgz",
-      "integrity": "sha512-UaachK0eL7neLyL2emXptVGyggGJKowJd24rqCZi9N2CDxuCQTk7wdePTgS7py4HMh+qxUI6zzTVinVwCDTbIA==",
-      "dev": true
-    },
     "dnd-core": {
       "version": "16.0.1",
       "resolved": "https://registry.npmjs.org/dnd-core/-/dnd-core-16.0.1.tgz",

services/filestore/app/js/FileHandler.js

@@ -1,7 +1,7 @@
 const Settings = require('@overleaf/settings')
 const { callbackify } = require('util')
 const fs = require('fs')
-const PersistorManager = require('./PersistorManager')
+let PersistorManager = require('./PersistorManager')
 const LocalFileWriter = require('./LocalFileWriter')
 const FileConverter = require('./FileConverter')
 const KeyBuilder = require('./KeyBuilder')
@@ -30,6 +30,12 @@ module.exports = {
   },
 }
 
+if (process.env.NODE_ENV === 'test') {
+  module.exports._TESTONLYSwapPersistorManager = _PersistorManager => {
+    PersistorManager = _PersistorManager
+  }
+}
+
 async function copyObject(bucket, sourceKey, destinationKey) {
   await PersistorManager.copyObject(bucket, sourceKey, destinationKey)
 }

services/filestore/package.json

@@ -39,14 +39,12 @@
     "aws-sdk": "^2.718.0",
     "chai": "^4.3.6",
     "chai-as-promised": "^7.1.1",
-    "disrequire": "^1.1.0",
     "mocha": "^10.2.0",
     "mongodb": "^6.1.0",
     "sandboxed-module": "2.0.4",
     "sinon": "9.0.2",
     "sinon-chai": "^3.7.0",
     "streamifier": "^0.1.1",
-    "timekeeper": "^2.2.0",
     "typescript": "^5.0.4"
   }
 }

services/filestore/test/acceptance/js/FilestoreApp.js

@@ -1,36 +1,20 @@
 const logger = require('@overleaf/logger')
+const ObjectPersistor = require('@overleaf/object-persistor')
 const Settings = require('@overleaf/settings')
-const fs = require('fs')
-const Path = require('path')
 const { promisify } = require('util')
-const disrequire = require('disrequire')
 const AWS = require('aws-sdk')
+const App = require('../../../app')
+const FileHandler = require('../../../app/js/FileHandler')
 
 logger.logger.level('info')
 
-const fsReaddir = promisify(fs.readdir)
 const sleep = promisify(setTimeout)
 
 class FilestoreApp {
-  constructor() {
-    this.running = false
-    this.initing = false
-  }
-
   async runServer() {
-    if (this.running) {
-      return
-    }
-
-    if (this.initing) {
-      return await this.waitForInit()
-    }
-    this.initing = true
-
-    this.app = await FilestoreApp.requireApp()
-
+    if (!this.server) {
       await new Promise((resolve, reject) => {
-      this.server = this.app.listen(
+        this.server = App.listen(
           Settings.internal.filestore.port,
           '127.0.0.1',
           err => {
@@ -41,6 +25,7 @@ class FilestoreApp {
           }
         )
       })
+    }
 
     if (Settings.filestore.backend === 's3') {
       try {
@@ -51,14 +36,11 @@ class FilestoreApp {
       }
     }
 
-    this.initing = false
-    this.persistor = require('../../../app/js/PersistorManager')
-  }
-
-  async waitForInit() {
-    while (this.initing) {
-      await sleep(1000)
-    }
+    this.persistor = ObjectPersistor({
+      ...Settings.filestore,
+      paths: Settings.path,
+    })
+    FileHandler._TESTONLYSwapPersistorManager(this.persistor)
   }
 
   async stop() {
@@ -105,19 +87,6 @@ class FilestoreApp {
       }
     }
   }
-
-  static async requireApp() {
-    // unload the app, as we may be doing this on multiple runs with
-    // different settings, which affect startup in some cases
-    const files = await fsReaddir(Path.resolve(__dirname, '../../../app/js'))
-    files.forEach(file => {
-      disrequire(Path.resolve(__dirname, '../../../app/js', file))
-    })
-    disrequire(Path.resolve(__dirname, '../../../app'))
-    disrequire('@overleaf/object-persistor')
-
-    return require('../../../app')
-  }
 }
 
 module.exports = FilestoreApp

services/filestore/test/acceptance/js/FilestoreTests.js

@@ -12,7 +12,6 @@ const { Storage } = require('@google-cloud/storage')
 const streamifier = require('streamifier')
 chai.use(require('chai-as-promised'))
 const { ObjectId } = require('mongodb')
-const tk = require('timekeeper')
 const ChildProcess = require('child_process')
 
 const fsWriteFile = promisify(fs.writeFile)
@@ -506,11 +505,9 @@ describe('Filestore', function () {
 
       if (backendSettings.backend === 'gcs') {
         describe('when deleting a file in GCS', function () {
-          let fileId, fileUrl, content, error, date
+          let fileId, fileUrl, content, error, dateBefore, dateAfter
 
           beforeEach(async function () {
-            date = new Date()
-            tk.freeze(date)
             fileId = new ObjectId()
             fileUrl = `${filestoreUrl}/project/${projectId}/file/${fileId}`
 
@@ -519,23 +516,27 @@ describe('Filestore', function () {
             const readStream = streamifier.createReadStream(content)
             let res = await fetch(fileUrl, { method: 'POST', body: readStream })
             if (!res.ok) throw new Error(res.statusText)
+            dateBefore = new Date()
             res = await fetch(fileUrl, { method: 'DELETE' })
+            dateAfter = new Date()
             if (!res.ok) throw new Error(res.statusText)
           })
 
-          afterEach(function () {
-            tk.reset()
-          })
-
           it('should not throw an error', function () {
             expect(error).not.to.exist
           })
 
           it('should copy the file to the deleted-files bucket', async function () {
-            await TestHelper.expectPersistorToHaveFile(
+            let date = dateBefore
+            const keys = []
+            while (date <= dateAfter) {
+              keys.push(`${projectId}/${fileId}-${date.toISOString()}`)
+              date = new Date(date.getTime() + 1)
+            }
+            await TestHelper.expectPersistorToHaveSomeFile(
               app.persistor,
               `${Settings.filestore.stores.user_files}-deleted`,
-              `${projectId}/${fileId}-${date.toISOString()}`,
+              keys,
               content
             )
           })

services/filestore/test/acceptance/js/TestHelper.js

@@ -1,5 +1,6 @@
 const streamifier = require('streamifier')
 const fetch = require('node-fetch')
+const ObjectPersistor = require('@overleaf/object-persistor')
 
 const { expect } = require('chai')
 
@@ -7,6 +8,7 @@ module.exports = {
   uploadStringToPersistor,
   getStringFromPersistor,
   expectPersistorToHaveFile,
+  expectPersistorToHaveSomeFile,
   expectPersistorNotToHaveFile,
   streamToString,
   getMetric,
@@ -50,6 +52,25 @@ async function expectPersistorToHaveFile(persistor, bucket, key, content) {
   expect(foundContent).to.equal(content)
 }
 
+async function expectPersistorToHaveSomeFile(persistor, bucket, keys, content) {
+  let foundContent
+  for (const key of keys) {
+    try {
+      foundContent = await getStringFromPersistor(persistor, bucket, key)
+      break
+    } catch (err) {
+      if (err instanceof ObjectPersistor.Errors.NotFoundError) {
+        continue
+      }
+      throw err
+    }
+  }
+  if (foundContent === undefined) {
+    expect.fail(`Could not find any of the specified keys: ${keys}`)
+  }
+  expect(foundContent).to.equal(content)
+}
+
 async function expectPersistorNotToHaveFile(persistor, bucket, key) {
   await expect(
     getStringFromPersistor(persistor, bucket, key)

services/filestore/test/unit/js/FileHandlerTests.js

@@ -89,7 +89,7 @@ describe('FileHandler', function () {
         },
         fs,
       },
-      globals: { console },
+      globals: { console, process },
     })
   })