From 44896704972d9b0d1bec1331999607dc986e2a6e Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Tue, 31 Mar 2020 15:38:42 +0100 Subject: [PATCH 1/2] Only delete the converted-cache folder if conversions are enabled --- services/filestore/app/js/FileHandler.js | 14 +++-- .../test/unit/js/FileHandlerTests.js | 57 +++++++++++++------ 2 files changed, 50 insertions(+), 21 deletions(-) diff --git a/services/filestore/app/js/FileHandler.js b/services/filestore/app/js/FileHandler.js index a6032350b1..6f49e05d7b 100644 --- a/services/filestore/app/js/FileHandler.js +++ b/services/filestore/app/js/FileHandler.js @@ -1,3 +1,4 @@ +const Settings = require('settings-sharelatex') const { callbackify } = require('util') const fs = require('fs') const PersistorManager = require('./PersistorManager') @@ -32,7 +33,9 @@ async function insertFile(bucket, key, stream) { info: { bucket, key, convertedKey } }) } - await PersistorManager.promises.deleteDirectory(bucket, convertedKey) + if (Settings.enableConversions) { + await PersistorManager.promises.deleteDirectory(bucket, convertedKey) + } await PersistorManager.promises.sendStream(bucket, key, stream) } @@ -44,10 +47,11 @@ async function deleteFile(bucket, key) { info: { bucket, key, convertedKey } }) } - await Promise.all([ - PersistorManager.promises.deleteFile(bucket, key), - PersistorManager.promises.deleteDirectory(bucket, convertedKey) - ]) + const jobs = [PersistorManager.promises.deleteFile(bucket, key)] + if (Settings.enableConversions) { + jobs.push(PersistorManager.promises.deleteDirectory(bucket, convertedKey)) + } + await Promise.all([jobs]) } async function deleteProject(bucket, key) { diff --git a/services/filestore/test/unit/js/FileHandlerTests.js b/services/filestore/test/unit/js/FileHandlerTests.js index 7823c9454f..60ee2553c3 100644 --- a/services/filestore/test/unit/js/FileHandlerTests.js +++ b/services/filestore/test/unit/js/FileHandlerTests.js @@ -15,14 +15,8 @@ describe('FileHandler', function() { KeyBuilder, ImageOptimiser, FileHandler, + Settings, fs - const settings = { - s3: { - buckets: { - user_files: 'user_files' - } - } - } const bucket = 'my_bucket' const key = `${ObjectId()}/${ObjectId()}` @@ -72,18 +66,19 @@ describe('FileHandler', function() { compressPng: sinon.stub().resolves() } } + Settings = {} fs = { createReadStream: sinon.stub().returns(readStream) } FileHandler = SandboxedModule.require(modulePath, { requires: { - 'settings-sharelatex': settings, './PersistorManager': PersistorManager, './LocalFileWriter': LocalFileWriter, './FileConverter': FileConverter, './KeyBuilder': KeyBuilder, './ImageOptimiser': ImageOptimiser, + 'settings-sharelatex': Settings, fs: fs }, globals: { console } @@ -105,12 +100,11 @@ describe('FileHandler', function() { }) }) - it('should delete the convertedKey folder', function(done) { + it('should not make a delete request for the convertedKey folder', function(done) { FileHandler.insertFile(bucket, key, stream, err => { expect(err).not.to.exist - expect( - PersistorManager.promises.deleteDirectory - ).to.have.been.calledWith(bucket, convertedFolderKey) + expect(PersistorManager.promises.deleteDirectory).not.to.have.been + .called done() }) }) @@ -122,6 +116,22 @@ describe('FileHandler', function() { done() }) }) + + describe('when conversions are enabled', function() { + beforeEach(function() { + Settings.enableConversions = true + }) + + it('should delete the convertedKey folder', function(done) { + FileHandler.insertFile(bucket, key, stream, err => { + expect(err).not.to.exist + expect( + PersistorManager.promises.deleteDirectory + ).to.have.been.calledWith(bucket, convertedFolderKey) + done() + }) + }) + }) }) describe('deleteFile', function() { @@ -136,12 +146,11 @@ describe('FileHandler', function() { }) }) - it('should tell the filestore manager to delete the cached folder', function(done) { + it('should not tell the filestore manager to delete the cached folder', function(done) { FileHandler.deleteFile(bucket, key, err => { expect(err).not.to.exist - expect( - PersistorManager.promises.deleteDirectory - ).to.have.been.calledWith(bucket, convertedFolderKey) + expect(PersistorManager.promises.deleteDirectory).not.to.have.been + .called done() }) }) @@ -153,6 +162,22 @@ describe('FileHandler', function() { done() }) }) + + describe('when conversions are enabled', function() { + beforeEach(function() { + Settings.enableConversions = true + }) + + it('should delete the convertedKey folder', function(done) { + FileHandler.deleteFile(bucket, key, err => { + expect(err).not.to.exist + expect( + PersistorManager.promises.deleteDirectory + ).to.have.been.calledWith(bucket, convertedFolderKey) + done() + }) + }) + }) }) describe('deleteProject', function() { From dceef85ccb20197ccca4368f53455fa4cd829b2a Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Tue, 31 Mar 2020 16:03:38 +0100 Subject: [PATCH 2/2] Update app/js/FileHandler.js Co-Authored-By: Jakob Ackermann --- services/filestore/app/js/FileHandler.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/filestore/app/js/FileHandler.js b/services/filestore/app/js/FileHandler.js index 6f49e05d7b..056206b0fe 100644 --- a/services/filestore/app/js/FileHandler.js +++ b/services/filestore/app/js/FileHandler.js @@ -51,7 +51,7 @@ async function deleteFile(bucket, key) { if (Settings.enableConversions) { jobs.push(PersistorManager.promises.deleteDirectory(bucket, convertedKey)) } - await Promise.all([jobs]) + await Promise.all(jobs) } async function deleteProject(bucket, key) {