[misc] promisify FileHandler and remove dependency on async

Signed-off-by: Jakob Ackermann <das7pad@outlook.com>

services/filestore/app/js/FileHandler.js

@@ -1,176 +1,153 @@
-const { promisify } = require('util')
+const { callbackify } = require('util')
 const fs = require('fs')
 const PersistorManager = require('./PersistorManager')
 const LocalFileWriter = require('./LocalFileWriter')
 const FileConverter = require('./FileConverter')
 const KeyBuilder = require('./KeyBuilder')
-const async = require('async')
 const ImageOptimiser = require('./ImageOptimiser')
 const { ConversionError } = require('./Errors')
 
 module.exports = {
-  insertFile,
-  deleteFile,
-  getFile,
-  getFileSize,
-  getDirectorySize,
+  insertFile: callbackify(insertFile),
+  deleteFile: callbackify(deleteFile),
+  getFile: callbackify(getFile),
+  getFileSize: callbackify(getFileSize),
+  getDirectorySize: callbackify(getDirectorySize),
   promises: {
-    getFile: promisify(getFile),
-    insertFile: promisify(insertFile),
-    deleteFile: promisify(deleteFile),
-    getFileSize: promisify(getFileSize),
-    getDirectorySize: promisify(getDirectorySize)
+    getFile,
+    insertFile,
+    deleteFile,
+    getFileSize,
+    getDirectorySize
   }
 }
 
-function insertFile(bucket, key, stream, callback) {
+async function insertFile(bucket, key, stream) {
   const convertedKey = KeyBuilder.getConvertedFolderKey(key)
-  PersistorManager.deleteDirectory(bucket, convertedKey, function(error) {
-    if (error) {
-      return callback(error)
-    }
-    PersistorManager.sendStream(bucket, key, stream, callback)
-  })
+  await PersistorManager.promises.deleteDirectory(bucket, convertedKey)
+  await PersistorManager.promises.sendStream(bucket, key, stream)
 }
 
-function deleteFile(bucket, key, callback) {
+async function deleteFile(bucket, key) {
   const convertedKey = KeyBuilder.getConvertedFolderKey(key)
-  async.parallel(
-    [
-      done => PersistorManager.deleteFile(bucket, key, done),
-      done => PersistorManager.deleteDirectory(bucket, convertedKey, done)
-    ],
-    callback
-  )
+  await Promise.all([
+    PersistorManager.promises.deleteFile(bucket, key),
+    PersistorManager.promises.deleteDirectory(bucket, convertedKey)
+  ])
 }
 
-function getFile(bucket, key, opts, callback) {
+async function getFile(bucket, key, opts) {
   opts = opts || {}
   if (!opts.format && !opts.style) {
-    PersistorManager.getFileStream(bucket, key, opts, callback)
+    return PersistorManager.promises.getFileStream(bucket, key, opts)
   } else {
-    _getConvertedFile(bucket, key, opts, callback)
+    return _getConvertedFile(bucket, key, opts)
   }
 }
 
-function getFileSize(bucket, key, callback) {
-  PersistorManager.getFileSize(bucket, key, callback)
+async function getFileSize(bucket, key) {
+  return PersistorManager.promises.getFileSize(bucket, key)
 }
 
-function getDirectorySize(bucket, projectId, callback) {
-  PersistorManager.directorySize(bucket, projectId, callback)
+async function getDirectorySize(bucket, projectId) {
+  return PersistorManager.promises.directorySize(bucket, projectId)
 }
 
-function _getConvertedFile(bucket, key, opts, callback) {
+async function _getConvertedFile(bucket, key, opts) {
   const convertedKey = KeyBuilder.addCachingToKey(key, opts)
-  PersistorManager.checkIfFileExists(bucket, convertedKey, (err, exists) => {
-    if (err) {
-      return callback(err)
-    }
-
-    if (exists) {
-      PersistorManager.getFileStream(bucket, convertedKey, opts, callback)
-    } else {
-      _getConvertedFileAndCache(bucket, key, convertedKey, opts, callback)
-    }
-  })
+  const exists = await PersistorManager.promises.checkIfFileExists(
+    bucket,
+    convertedKey
+  )
+  if (exists) {
+    return PersistorManager.promises.getFileStream(bucket, convertedKey, opts)
+  } else {
+    return _getConvertedFileAndCache(bucket, key, convertedKey, opts)
+  }
 }
 
-function _getConvertedFileAndCache(bucket, key, convertedKey, opts, callback) {
+async function _getConvertedFileAndCache(bucket, key, convertedKey, opts) {
   let convertedFsPath
-
-  async.series(
-    [
-      cb => {
-        _convertFile(bucket, key, opts, function(err, fileSystemPath) {
-          convertedFsPath = fileSystemPath
-          cb(err)
-        })
-      },
-      cb => ImageOptimiser.compressPng(convertedFsPath, cb),
-      cb => PersistorManager.sendFile(bucket, convertedKey, convertedFsPath, cb)
-    ],
-    function(err) {
-      if (err) {
-        LocalFileWriter.deleteFile(convertedFsPath, function() {})
-        return callback(
-          new ConversionError({
-            message: 'failed to convert file',
-            info: { opts, bucket, key, convertedKey }
-          }).withCause(err)
-        )
-      }
-      // Send back the converted file from the local copy to avoid problems
-      // with the file not being present in S3 yet.  As described in the
-      // documentation below, we have already made a 'HEAD' request in
-      // checkIfFileExists so we only have "eventual consistency" if we try
-      // to stream it from S3 here.  This was a cause of many 403 errors.
-      //
-      // "Amazon S3 provides read-after-write consistency for PUTS of new
-      // objects in your S3 bucket in all regions with one caveat. The
-      // caveat is that if you make a HEAD or GET request to the key name
-      // (to find if the object exists) before creating the object, Amazon
-      // S3 provides eventual consistency for read-after-write.""
-      // https://docs.aws.amazon.com/AmazonS3/latest/dev/Introduction.html#ConsistencyModel
-      const readStream = fs.createReadStream(convertedFsPath)
-      readStream.on('end', function() {
-        LocalFileWriter.deleteFile(convertedFsPath, function() {})
-      })
-      callback(null, readStream)
-    }
-  )
+  try {
+    convertedFsPath = await _convertFile(bucket, key, opts)
+    await ImageOptimiser.promises.compressPng(convertedFsPath)
+    await PersistorManager.promises.sendFile(
+      bucket,
+      convertedKey,
+      convertedFsPath
+    )
+  } catch (err) {
+    LocalFileWriter.deleteFile(convertedFsPath, () => {})
+    throw new ConversionError({
+      message: 'failed to convert file',
+      info: { opts, bucket, key, convertedKey }
+    }).withCause(err)
+  }
+  // Send back the converted file from the local copy to avoid problems
+  // with the file not being present in S3 yet.  As described in the
+  // documentation below, we have already made a 'HEAD' request in
+  // checkIfFileExists so we only have "eventual consistency" if we try
+  // to stream it from S3 here.  This was a cause of many 403 errors.
+  //
+  // "Amazon S3 provides read-after-write consistency for PUTS of new
+  // objects in your S3 bucket in all regions with one caveat. The
+  // caveat is that if you make a HEAD or GET request to the key name
+  // (to find if the object exists) before creating the object, Amazon
+  // S3 provides eventual consistency for read-after-write.""
+  // https://docs.aws.amazon.com/AmazonS3/latest/dev/Introduction.html#ConsistencyModel
+  const readStream = fs.createReadStream(convertedFsPath)
+  readStream.on('end', function() {
+    LocalFileWriter.deleteFile(convertedFsPath, function() {})
+  })
+  return readStream
 }
 
-function _convertFile(bucket, originalKey, opts, callback) {
-  _writeFileToDisk(bucket, originalKey, opts, function(err, originalFsPath) {
-    if (err) {
-      return callback(
-        new ConversionError({
-          message: 'unable to write file to disk',
-          info: { bucket, originalKey, opts }
-        }).withCause(err)
-      )
-    }
+async function _convertFile(bucket, originalKey, opts) {
+  let originalFsPath
+  try {
+    originalFsPath = await _writeFileToDisk(bucket, originalKey, opts)
+  } catch (err) {
+    throw new ConversionError({
+      message: 'unable to write file to disk',
+      info: { bucket, originalKey, opts }
+    }).withCause(err)
+  }
 
-    const done = function(err, destPath) {
-      if (err) {
-        return callback(
-          new ConversionError({
-            message: 'error converting file',
-            info: { bucket, originalKey, opts }
-          }).withCause(err)
-        )
+  let promise
+  if (opts.format) {
+    promise = FileConverter.promises.convert(originalFsPath, opts.format)
+  } else if (opts.style === 'thumbnail') {
+    promise = FileConverter.promises.thumbnail(originalFsPath)
+  } else if (opts.style === 'preview') {
+    promise = FileConverter.promises.preview(originalFsPath)
+  } else {
+    throw new ConversionError({
+      message: 'invalid file conversion options',
+      info: {
+        bucket,
+        originalKey,
+        opts
       }
-      LocalFileWriter.deleteFile(originalFsPath, function() {})
-      callback(err, destPath)
-    }
-
-    if (opts.format) {
-      FileConverter.convert(originalFsPath, opts.format, done)
-    } else if (opts.style === 'thumbnail') {
-      FileConverter.thumbnail(originalFsPath, done)
-    } else if (opts.style === 'preview') {
-      FileConverter.preview(originalFsPath, done)
-    } else {
-      callback(
-        new ConversionError({
-          message: 'invalid file conversion options',
-          info: {
-            bucket,
-            originalKey,
-            opts
-          }
-        })
-      )
-    }
-  })
+    })
+  }
+  let destPath
+  try {
+    destPath = await promise
+  } catch (err) {
+    throw new ConversionError({
+      message: 'error converting file',
+      info: { bucket, originalKey, opts }
+    }).withCause(err)
+  }
+  LocalFileWriter.deleteFile(originalFsPath, function() {})
+  return destPath
 }
 
-function _writeFileToDisk(bucket, key, opts, callback) {
-  PersistorManager.getFileStream(bucket, key, opts, function(err, fileStream) {
-    if (err) {
-      return callback(err)
-    }
-    LocalFileWriter.writeStream(fileStream, key, callback)
-  })
+async function _writeFileToDisk(bucket, key, opts) {
+  const fileStream = await PersistorManager.promises.getFileStream(
+    bucket,
+    key,
+    opts
+  )
+  return LocalFileWriter.promises.writeStream(fileStream, key)
 }

services/filestore/package-lock.json

@@ -1256,11 +1256,6 @@
       "integrity": "sha512-+Ryf6g3BKoRc7jfp7ad8tM4TtMiaWvbF/1/sQcZPkkS7ag3D5nMBCe2UfOTONtAkaG0tO0ij3C5Lwmf1EiyjHg==",
       "dev": true
     },
-    "async": {
-      "version": "0.2.10",
-      "resolved": "https://registry.npmjs.org/async/-/async-0.2.10.tgz",
-      "integrity": "sha512-eAkdoKxU6/LkKDBzLpT+t6Ff5EtfSF4wx1WfJiPEEV7WNLnDaRXk0oVysiEPm262roaachGexwUv94WhSgN5TQ=="
-    },
     "async-listener": {
       "version": "0.6.10",
       "resolved": "https://registry.npmjs.org/async-listener/-/async-listener-0.6.10.tgz",

services/filestore/package.json

@@ -21,7 +21,6 @@
   },
   "dependencies": {
     "@overleaf/o-error": "^2.1.0",
-    "async": "~0.2.10",
     "aws-sdk": "^2.628.0",
     "body-parser": "^1.2.0",
     "express": "^4.2.0",

services/filestore/test/unit/js/FileHandlerTests.js

@@ -4,6 +4,9 @@ const { expect } = chai
 const modulePath = '../../../app/js/FileHandler.js'
 const SandboxedModule = require('sandboxed-module')
 
+chai.use(require('sinon-chai'))
+chai.use(require('chai-as-promised'))
+
 describe('FileHandler', function() {
   let PersistorManager,
     LocalFileWriter,
@@ -32,29 +35,41 @@ describe('FileHandler', function() {
 
   beforeEach(function() {
     PersistorManager = {
-      getFileStream: sinon.stub().yields(null, sourceStream),
-      checkIfFileExists: sinon.stub().yields(),
-      deleteFile: sinon.stub().yields(),
-      deleteDirectory: sinon.stub().yields(),
-      sendStream: sinon.stub().yields(),
-      insertFile: sinon.stub().yields(),
-      sendFile: sinon.stub().yields(),
-      directorySize: sinon.stub().yields()
+      promises: {
+        getFileStream: sinon.stub().resolves(sourceStream),
+        checkIfFileExists: sinon.stub().resolves(),
+        deleteFile: sinon.stub().resolves(),
+        deleteDirectory: sinon.stub().resolves(),
+        sendStream: sinon.stub().resolves(),
+        insertFile: sinon.stub().resolves(),
+        sendFile: sinon.stub().resolves(),
+        directorySize: sinon.stub().resolves()
+      }
     }
     LocalFileWriter = {
-      writeStream: sinon.stub().yields(),
-      deleteFile: sinon.stub().yields()
+      // the callback style is used for detached cleanup calls
+      deleteFile: sinon.stub().yields(),
+      promises: {
+        writeStream: sinon.stub().resolves(),
+        deleteFile: sinon.stub().resolves()
+      }
     }
     FileConverter = {
-      convert: sinon.stub().yields(),
-      thumbnail: sinon.stub().yields(),
-      preview: sinon.stub().yields()
+      promises: {
+        convert: sinon.stub().resolves(),
+        thumbnail: sinon.stub().resolves(),
+        preview: sinon.stub().resolves()
+      }
     }
     KeyBuilder = {
       addCachingToKey: sinon.stub().returns(convertedKey),
       getConvertedFolderKey: sinon.stub().returns(convertedFolderKey)
     }
-    ImageOptimiser = { compressPng: sinon.stub().yields() }
+    ImageOptimiser = {
+      promises: {
+        compressPng: sinon.stub().resolves()
+      }
+    }
     fs = {
       createReadStream: sinon.stub().returns(readStream)
     }
@@ -79,7 +94,7 @@ describe('FileHandler', function() {
     it('should send file to the filestore', function(done) {
       FileHandler.insertFile(bucket, key, stream, err => {
         expect(err).not.to.exist
-        expect(PersistorManager.sendStream).to.have.been.calledWith(
+        expect(PersistorManager.promises.sendStream).to.have.been.calledWith(
           bucket,
           key,
           stream
@@ -91,10 +106,9 @@ describe('FileHandler', function() {
     it('should delete the convertedKey folder', function(done) {
       FileHandler.insertFile(bucket, key, stream, err => {
         expect(err).not.to.exist
-        expect(PersistorManager.deleteDirectory).to.have.been.calledWith(
-          bucket,
-          convertedFolderKey
-        )
+        expect(
+          PersistorManager.promises.deleteDirectory
+        ).to.have.been.calledWith(bucket, convertedFolderKey)
         done()
       })
     })
@@ -104,7 +118,10 @@ describe('FileHandler', function() {
     it('should tell the filestore manager to delete the file', function(done) {
       FileHandler.deleteFile(bucket, key, err => {
         expect(err).not.to.exist
-        expect(PersistorManager.deleteFile).to.have.been.calledWith(bucket, key)
+        expect(PersistorManager.promises.deleteFile).to.have.been.calledWith(
+          bucket,
+          key
+        )
         done()
       })
     })
@@ -112,10 +129,9 @@ describe('FileHandler', function() {
     it('should tell the filestore manager to delete the cached folder', function(done) {
       FileHandler.deleteFile(bucket, key, err => {
         expect(err).not.to.exist
-        expect(PersistorManager.deleteDirectory).to.have.been.calledWith(
-          bucket,
-          convertedFolderKey
-        )
+        expect(
+          PersistorManager.promises.deleteDirectory
+        ).to.have.been.calledWith(bucket, convertedFolderKey)
         done()
       })
     })
@@ -134,7 +150,7 @@ describe('FileHandler', function() {
       const options = { start: 0, end: 8 }
       FileHandler.getFile(bucket, key, options, err => {
         expect(err).not.to.exist
-        expect(PersistorManager.getFileStream).to.have.been.calledWith(
+        expect(PersistorManager.promises.getFileStream).to.have.been.calledWith(
           bucket,
           key,
           options
@@ -155,23 +171,27 @@ describe('FileHandler', function() {
         })
 
         it('should convert the file', function() {
-          expect(FileConverter.convert).to.have.been.called
-          expect(ImageOptimiser.compressPng).to.have.been.called
+          expect(FileConverter.promises.convert).to.have.been.called
+        })
+
+        it('should compress the converted file', function() {
+          expect(ImageOptimiser.promises.compressPng).to.have.been.called
         })
 
         it('should return the the converted stream', function() {
           expect(result.err).not.to.exist
           expect(result.stream).to.equal(readStream)
-          expect(PersistorManager.getFileStream).to.have.been.calledWith(
-            bucket,
-            key
-          )
+          expect(
+            PersistorManager.promises.getFileStream
+          ).to.have.been.calledWith(bucket, key)
         })
       })
 
       describe('when the file is cached', function() {
         beforeEach(function(done) {
-          PersistorManager.checkIfFileExists = sinon.stub().yields(null, true)
+          PersistorManager.promises.checkIfFileExists = sinon
+            .stub()
+            .resolves(true)
           FileHandler.getFile(bucket, key, { format: 'png' }, (err, stream) => {
             result = { err, stream }
             done()
@@ -179,17 +199,19 @@ describe('FileHandler', function() {
         })
 
         it('should not convert the file', function() {
-          expect(FileConverter.convert).not.to.have.been.called
-          expect(ImageOptimiser.compressPng).not.to.have.been.called
+          expect(FileConverter.promises.convert).not.to.have.been.called
+        })
+
+        it('should not compress the converted file again', function() {
+          expect(ImageOptimiser.promises.compressPng).not.to.have.been.called
         })
 
         it('should return the cached stream', function() {
           expect(result.err).not.to.exist
           expect(result.stream).to.equal(sourceStream)
-          expect(PersistorManager.getFileStream).to.have.been.calledWith(
-            bucket,
-            convertedKey
-          )
+          expect(
+            PersistorManager.promises.getFileStream
+          ).to.have.been.calledWith(bucket, convertedKey)
         })
       })
     })
@@ -198,8 +220,8 @@ describe('FileHandler', function() {
       it('generates a thumbnail when requested', function(done) {
         FileHandler.getFile(bucket, key, { style: 'thumbnail' }, err => {
           expect(err).not.to.exist
-          expect(FileConverter.thumbnail).to.have.been.called
-          expect(FileConverter.preview).not.to.have.been.called
+          expect(FileConverter.promises.thumbnail).to.have.been.called
+          expect(FileConverter.promises.preview).not.to.have.been.called
           done()
         })
       })
@@ -207,8 +229,8 @@ describe('FileHandler', function() {
       it('generates a preview when requested', function(done) {
         FileHandler.getFile(bucket, key, { style: 'preview' }, err => {
           expect(err).not.to.exist
-          expect(FileConverter.thumbnail).not.to.have.been.called
-          expect(FileConverter.preview).to.have.been.called
+          expect(FileConverter.promises.thumbnail).not.to.have.been.called
+          expect(FileConverter.promises.preview).to.have.been.called
           done()
         })
       })
@@ -219,7 +241,7 @@ describe('FileHandler', function() {
     it('should call the filestore manager to get directory size', function(done) {
       FileHandler.getDirectorySize(bucket, key, err => {
         expect(err).not.to.exist
-        expect(PersistorManager.directorySize).to.have.been.calledWith(
+        expect(PersistorManager.promises.directorySize).to.have.been.calledWith(
           bucket,
           key
         )