From dd59d8c2429adee48a9bdf09a708988bd4c53e11 Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Fri, 24 Apr 2020 10:17:51 +0100 Subject: [PATCH] Merge pull request #2770 from overleaf/spd-filestore-deletes Delete project files in filestore when project is hard-deleted GitOrigin-RevId: c836f94b2dc1967d84424ff1b65b1025b3499309 --- .../Features/FileStore/FileStoreHandler.js | 26 ++++++++++++++- .../src/Features/Project/ProjectDeleter.js | 2 ++ .../web/test/acceptance/src/DeletionTests.js | 27 +++++++++++++++ .../src/helpers/MockFileStoreApi.js | 6 ++++ .../src/FileStore/FileStoreHandlerTests.js | 33 ++++++++++++++++--- .../unit/src/Project/ProjectDeleterTests.js | 13 +++++++- 6 files changed, 100 insertions(+), 7 deletions(-) diff --git a/services/web/app/src/Features/FileStore/FileStoreHandler.js b/services/web/app/src/Features/FileStore/FileStoreHandler.js index 7b999f91d9..dfaec07608 100644 --- a/services/web/app/src/Features/FileStore/FileStoreHandler.js +++ b/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}` : '') + ) } } diff --git a/services/web/app/src/Features/Project/ProjectDeleter.js b/services/web/app/src/Features/Project/ProjectDeleter.js index 67d94f485b..bffa9c57e8 100644 --- a/services/web/app/src/Features/Project/ProjectDeleter.js +++ b/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( { diff --git a/services/web/test/acceptance/src/DeletionTests.js b/services/web/test/acceptance/src/DeletionTests.js index b94afb6be0..635f728789 100644 --- a/services/web/test/acceptance/src/DeletionTests.js +++ b/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) }, diff --git a/services/web/test/acceptance/src/helpers/MockFileStoreApi.js b/services/web/test/acceptance/src/helpers/MockFileStoreApi.js index 1227310efc..eb187209f2 100644 --- a/services/web/test/acceptance/src/helpers/MockFileStoreApi.js +++ b/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) { diff --git a/services/web/test/unit/src/FileStore/FileStoreHandlerTests.js b/services/web/test/unit/src/FileStore/FileStoreHandlerTests.js index cb0a4efa4f..52261337c4 100644 --- a/services/web/test/unit/src/FileStore/FileStoreHandlerTests.js +++ b/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() }) }) diff --git a/services/web/test/unit/src/Project/ProjectDeleterTests.js b/services/web/test/unit/src/Project/ProjectDeleterTests.js index a658ffce5b..3c9c6ce47e 100644 --- a/services/web/test/unit/src/Project/ProjectDeleterTests.js +++ b/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() {