From eb93ae4b10fb89b0d06eec196f1f132a67209aeb Mon Sep 17 00:00:00 2001
From: Simon Detheridge <s@sd.ai>
Date: Thu, 5 Mar 2020 14:12:15 +0000
Subject: [PATCH] Use Bucket.deleteFiles to delete directory contents, instead
 of iterating

---
 services/filestore/app/js/GcsPersistor.js     | 32 ++++------
 .../test/unit/js/GcsPersistorTests.js         | 60 +++----------------
 2 files changed, 17 insertions(+), 75 deletions(-)

diff --git a/services/filestore/app/js/GcsPersistor.js b/services/filestore/app/js/GcsPersistor.js
index f4702e06a4..a2525361c2 100644
--- a/services/filestore/app/js/GcsPersistor.js
+++ b/services/filestore/app/js/GcsPersistor.js
@@ -191,41 +191,29 @@ async function deleteFile(bucketName, key) {
 }
 
 async function deleteDirectory(bucketName, key) {
-  let files
+  if (!key.match(/^[a-z0-9_-]+/i)) {
+    throw new NotFoundError({
+      message: 'deleteDirectoryKey is invalid or missing',
+      info: { bucketName, key }
+    })
+  }
 
   try {
-    const [response] = await storage
+    await storage
       .bucket(bucketName)
-      .getFiles({ directory: key })
-    files = response
+      .deleteFiles({ directory: key, force: true })
   } catch (err) {
     const error = PersistorHelper.wrapError(
       err,
-      'failed to list objects in GCS',
+      'failed to delete directory in GCS',
       { bucketName, key },
-      ReadError
+      WriteError
     )
     if (error instanceof NotFoundError) {
       return
     }
     throw error
   }
-
-  for (const index in files) {
-    try {
-      await files[index].delete()
-    } catch (err) {
-      const error = PersistorHelper.wrapError(
-        err,
-        'failed to delete object in GCS',
-        { bucketName, key },
-        WriteError
-      )
-      if (!(error instanceof NotFoundError)) {
-        throw error
-      }
-    }
-  }
 }
 
 async function directorySize(bucketName, key) {
diff --git a/services/filestore/test/unit/js/GcsPersistorTests.js b/services/filestore/test/unit/js/GcsPersistorTests.js
index ec071192e2..a63296a18f 100644
--- a/services/filestore/test/unit/js/GcsPersistorTests.js
+++ b/services/filestore/test/unit/js/GcsPersistorTests.js
@@ -84,7 +84,8 @@ describe('GcsPersistorTests', function() {
 
     GcsBucket = {
       file: sinon.stub().returns(GcsFile),
-      getFiles: sinon.stub().resolves([files])
+      getFiles: sinon.stub().resolves([files]),
+      deleteFiles: sinon.stub().resolves()
     }
 
     Storage = class {
@@ -516,67 +517,20 @@ describe('GcsPersistorTests', function() {
         return GcsPersistor.promises.deleteDirectory(bucket, key)
       })
 
-      it('should list the objects in the directory', function() {
+      it('should delete the objects in the directory', function() {
         expect(Storage.prototype.bucket).to.have.been.calledWith(bucket)
-        expect(GcsBucket.getFiles).to.have.been.calledWith({
-          directory: key
+        expect(GcsBucket.deleteFiles).to.have.been.calledWith({
+          directory: key,
+          force: true
         })
       })
-
-      it('should delete the objects', function() {
-        expect(files[0].delete).to.have.been.called
-        expect(files[1].delete).to.have.been.called
-      })
-    })
-
-    describe('when there are no files', function() {
-      beforeEach(async function() {
-        GcsBucket.getFiles.resolves([[]])
-        return GcsPersistor.promises.deleteDirectory(bucket, key)
-      })
-
-      it('should list the objects in the directory', function() {
-        expect(GcsBucket.getFiles).to.have.been.calledWith({
-          directory: key
-        })
-      })
-
-      it('should not try to delete any objects', function() {
-        expect(files[0].delete).not.to.have.been.called
-        expect(files[1].delete).not.to.have.been.called
-      })
-    })
-
-    describe('when there is an error listing the objects', function() {
-      let error
-
-      beforeEach(async function() {
-        GcsBucket.getFiles = sinon.stub().rejects(genericError)
-        try {
-          await GcsPersistor.promises.deleteDirectory(bucket, key)
-        } catch (err) {
-          error = err
-        }
-      })
-
-      it('should generate a ReadError', function() {
-        expect(error).to.be.an.instanceOf(Errors.ReadError)
-      })
-
-      it('should wrap the error', function() {
-        expect(error.cause).to.equal(genericError)
-      })
-
-      it('should not try to delete any objects', function() {
-        expect(files[0].delete).not.to.have.been.called
-      })
     })
 
     describe('when there is an error deleting the objects', function() {
       let error
 
       beforeEach(async function() {
-        files[0].delete = sinon.stub().rejects(genericError)
+        GcsBucket.deleteFiles = sinon.stub().rejects(genericError)
         try {
           await GcsPersistor.promises.deleteDirectory(bucket, key)
         } catch (err) {