Merge pull request #2770 from overleaf/spd-filestore-deletes

Delete project files in filestore when project is hard-deleted

GitOrigin-RevId: c836f94b2dc1967d84424ff1b65b1025b3499309

services/web/app/src/Features/FileStore/FileStoreHandler.js

@@ -183,6 +183,27 @@ const FileStoreHandler = {
     })
   },
 
+  deleteProject(projectId, callback) {
+    request(
+      {
+        method: 'delete',
+        uri: this._buildUrl(projectId),
+        timeout: FIVE_MINS_IN_MS
+      },
+      err => {
+        if (err) {
+          return callback(
+            new Errors.OError({
+              message: 'something went wrong deleting a project in filestore',
+              info: { projectId }
+            }).withCause(err)
+          )
+        }
+        callback()
+      }
+    )
+  },
+
   copyFile(oldProjectId, oldFileId, newProjectId, newFileId, callback) {
     logger.log(
       { oldProjectId, oldFileId, newProjectId, newFileId },
@@ -223,7 +244,10 @@ const FileStoreHandler = {
   },
 
   _buildUrl(projectId, fileId) {
-    return `${settings.apis.filestore.url}/project/${projectId}/file/${fileId}`
+    return (
+      `${settings.apis.filestore.url}/project/${projectId}` +
+      (fileId ? `/file/${fileId}` : '')
+    )
   }
 }
 

services/web/app/src/Features/Project/ProjectDeleter.js

@@ -13,6 +13,7 @@ const CollaboratorsGetter = require('../Collaborators/CollaboratorsGetter')
 const DocstoreManager = require('../Docstore/DocstoreManager')
 const EditorRealTimeController = require('../Editor/EditorRealTimeController')
 const HistoryManager = require('../History/HistoryManager')
+const FilestoreHandler = require('../FileStore/FileStoreHandler')
 const moment = require('moment')
 const { promiseMapWithLimit } = require('../../util/promises')
 
@@ -308,6 +309,7 @@ async function expireDeletedProject(projectId) {
 
     await DocstoreManager.promises.destroyProject(deletedProject.project._id)
     await HistoryManager.promises.deleteProject(deletedProject.project._id)
+    await FilestoreHandler.promises.deleteProject(deletedProject.project._id)
 
     await DeletedProject.update(
       {

services/web/test/acceptance/src/DeletionTests.js

@@ -7,6 +7,7 @@ const { db, ObjectId } = require('../../../app/src/infrastructure/mongojs')
 const { Subscription } = require('../../../app/src/models/Subscription')
 const SubscriptionViewModelBuilder = require('../../../app/src/Features/Subscription/SubscriptionViewModelBuilder')
 const MockDocstoreApi = require('./helpers/MockDocstoreApi')
+const MockFileStoreApi = require('./helpers/MockFileStoreApi')
 require('./helpers/MockV1Api')
 require('./helpers/MockProjectHistoryApi')
 
@@ -252,6 +253,9 @@ describe('Deleting a project', function() {
             done()
           }
         )
+        MockFileStoreApi.files[this.projectId.toString()] = {
+          dummyFile: 'wombat'
+        }
       })
     })
 
@@ -289,6 +293,29 @@ describe('Deleting a project', function() {
         )
       })
 
+      it('Should destroy the files', function(done) {
+        expect(MockFileStoreApi.files[this.projectId.toString()]).to.exist
+
+        request.post(
+          `/internal/project/${this.projectId}/expire-deleted-project`,
+          {
+            auth: {
+              user: settings.apis.web.user,
+              pass: settings.apis.web.pass,
+              sendImmediately: true
+            }
+          },
+          (error, res) => {
+            expect(error).not.to.exist
+            expect(res.statusCode).to.equal(200)
+
+            expect(MockFileStoreApi.files[this.projectId.toString()]).not.to
+              .exist
+            done()
+          }
+        )
+      })
+
       it('Should remove the project data from mongo', function(done) {
         db.deletedProjects.findOne(
           { 'deleterData.deletedProjectId': ObjectId(this.projectId) },

services/web/test/acceptance/src/helpers/MockFileStoreApi.js

@@ -63,6 +63,12 @@ module.exports = MockFileStoreApi = {
       }
     )
 
+    app.delete('/project/:projectId', (req, res) => {
+      const { projectId } = req.params
+      delete this.files[projectId]
+      res.sendStatus(204)
+    })
+
     return app
       .listen(3009, error => {
         if (error != null) {

services/web/test/unit/src/FileStore/FileStoreHandlerTests.js

@@ -2,6 +2,7 @@ const { assert } = require('chai')
 const sinon = require('sinon')
 const { expect } = require('chai')
 const SandboxedModule = require('sandboxed-module')
+const Errors = require('../../../../app/src/Features/Errors/Errors')
 
 const MODULE_PATH = '../../../../app/src/Features/FileStore/FileStoreHandler.js'
 
@@ -41,6 +42,8 @@ describe('FileStoreHandler', function() {
     this.fsPath = 'uploads/myfile.eps'
     this.getFileUrl = (projectId, fileId) =>
       `${this.filestoreUrl}/project/${projectId}/file/${fileId}`
+    this.getProjectUrl = projectId =>
+      `${this.filestoreUrl}/project/${projectId}`
     this.FileModel = class File {
       constructor(options) {
         ;({ name: this.name, hash: this.hash } = options)
@@ -51,9 +54,6 @@ describe('FileStoreHandler', function() {
         }
       }
     }
-    this.Errors = {
-      NotFoundError: sinon.stub()
-    }
     this.logger = {
       log: sinon.stub(),
       warn: sinon.stub(),
@@ -75,7 +75,7 @@ describe('FileStoreHandler', function() {
         '../../models/File': {
           File: this.FileModel
         },
-        '../Errors/Errors': this.Errors,
+        '../Errors/Errors': Errors,
         fs: this.fs
       }
     })
@@ -269,6 +269,29 @@ describe('FileStoreHandler', function() {
     })
   })
 
+  describe('deleteProject', function() {
+    it('should send a delete request to filestore api', function(done) {
+      const projectUrl = this.getProjectUrl(this.projectId)
+      this.request.callsArgWith(1, null)
+
+      this.handler.deleteProject(this.projectId, err => {
+        assert.equal(err, undefined)
+        this.request.args[0][0].method.should.equal('delete')
+        this.request.args[0][0].uri.should.equal(projectUrl)
+        done()
+      })
+    })
+
+    it('should wrap the error if there is one', function(done) {
+      const error = 'my error'
+      this.request.callsArgWith(1, error)
+      this.handler.deleteProject(this.projectId, err => {
+        assert.equal(err.cause, error)
+        done()
+      })
+    })
+  })
+
   describe('getFileStream', function() {
     beforeEach(function() {
       this.query = {}
@@ -399,7 +422,7 @@ describe('FileStoreHandler', function() {
       this.request.head.yields(null, { statusCode: 404 })
 
       this.handler.getFileSize(this.projectId, this.fileId, err => {
-        expect(err).to.be.instanceof(this.Errors.NotFoundError)
+        expect(err).to.be.instanceof(Errors.NotFoundError)
         done()
       })
     })

services/web/test/unit/src/Project/ProjectDeleterTests.js

@@ -123,6 +123,11 @@ describe('ProjectDeleter', function() {
 
     this.ProjectMock = sinon.mock(Project)
     this.DeletedProjectMock = sinon.mock(DeletedProject)
+    this.FileStoreHandler = {
+      promises: {
+        deleteProject: sinon.stub().resolves()
+      }
+    }
 
     this.ProjectDeleter = SandboxedModule.require(modulePath, {
       requires: {
@@ -133,7 +138,7 @@ describe('ProjectDeleter', function() {
         '../DocumentUpdater/DocumentUpdaterHandler': this
           .DocumentUpdaterHandler,
         '../Tags/TagsHandler': this.TagsHandler,
-        '../FileStore/FileStoreHandler': (this.FileStoreHandler = {}),
+        '../FileStore/FileStoreHandler': this.FileStoreHandler,
         '../Collaborators/CollaboratorsHandler': this.CollaboratorsHandler,
         '../Collaborators/CollaboratorsGetter': this.CollaboratorsGetter,
         '../Docstore/DocstoreManager': this.DocstoreManager,
@@ -435,6 +440,12 @@ describe('ProjectDeleter', function() {
         this.HistoryManager.promises.deleteProject
       ).to.have.been.calledWith(this.deletedProjects[0].project._id)
     })
+
+    it('should destroy the files in filestore', function() {
+      expect(
+        this.FileStoreHandler.promises.deleteProject
+      ).to.have.been.calledWith(this.deletedProjects[0].project._id)
+    })
   })
 
   describe('archiveProject', function() {