Merge pull request #111 from overleaf/spd-no-extra-deletes

Only delete the converted-cache folder if conversions are enabled

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([
+  const jobs = [PersistorManager.promises.deleteFile(bucket, key)]
-    PersistorManager.promises.deleteFile(bucket, key),
+  if (Settings.enableConversions) {
-    PersistorManager.promises.deleteDirectory(bucket, convertedKey)
+    jobs.push(PersistorManager.promises.deleteDirectory(bucket, convertedKey))
-  ])
+  }
+  await Promise.all(jobs)
 }
 
 async function deleteProject(bucket, key) {

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(
+        expect(PersistorManager.promises.deleteDirectory).not.to.have.been
-          PersistorManager.promises.deleteDirectory
+          .called
-        ).to.have.been.calledWith(bucket, convertedFolderKey)
         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(
+        expect(PersistorManager.promises.deleteDirectory).not.to.have.been
-          PersistorManager.promises.deleteDirectory
+          .called
-        ).to.have.been.calledWith(bucket, convertedFolderKey)
         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() {