Decaf cleanup for FileHandler and LocalFileWriter

Simplified code and tests where possible

services/filestore/app/js/Errors.js

@@ -18,7 +18,10 @@ class BackwardCompatibleError extends OError {
 }
 
 class NotFoundError extends BackwardCompatibleError {}
+class WriteError extends BackwardCompatibleError {}
+class ReadError extends BackwardCompatibleError {}
 class ConversionsDisabledError extends BackwardCompatibleError {}
+class ConversionError extends BackwardCompatibleError {}
 
 class FailedCommandError extends OError {
   constructor(command, code, stdout, stderr) {
@@ -35,4 +38,11 @@ class FailedCommandError extends OError {
   }
 }
 
-module.exports = { NotFoundError, FailedCommandError, ConversionsDisabledError }
+module.exports = {
+  NotFoundError,
+  FailedCommandError,
+  ConversionsDisabledError,
+  WriteError,
+  ReadError,
+  ConversionError
+}

services/filestore/app/js/FileHandler.js

@@ -1,18 +1,5 @@
-/* eslint-disable
+const { promisify } = require('util')
-    camelcase,
+const fs = require('fs')
-    no-self-assign,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-let FileHandler
-const settings = require('settings-sharelatex')
 const PersistorManager = require('./PersistorManager')
 const LocalFileWriter = require('./LocalFileWriter')
 const logger = require('logger-sharelatex')
@@ -20,216 +7,196 @@ const FileConverter = require('./FileConverter')
 const KeyBuilder = require('./KeyBuilder')
 const async = require('async')
 const ImageOptimiser = require('./ImageOptimiser')
-const Errors = require('./Errors')
+const { WriteError, ReadError, ConversionError } = require('./Errors')
 
-module.exports = FileHandler = {
+module.exports = {
-  insertFile(bucket, key, stream, callback) {
+  insertFile,
-    const convertedKey = KeyBuilder.getConvertedFolderKey(key)
+  deleteFile,
-    return PersistorManager.deleteDirectory(bucket, convertedKey, function(
+  getFile,
-      error
+  getFileSize,
-    ) {
+  getDirectorySize,
-      if (error != null) {
+  promises: {
-        return callback(error)
+    getFile: promisify(getFile),
-      }
+    insertFile: promisify(insertFile),
-      return PersistorManager.sendStream(bucket, key, stream, callback)
+    deleteFile: promisify(deleteFile),
-    })
+    getFileSize: promisify(getFileSize),
-  },
+    getDirectorySize: promisify(getDirectorySize)
-
-  deleteFile(bucket, key, callback) {
-    const convertedKey = KeyBuilder.getConvertedFolderKey(key)
-    return async.parallel(
-      [
-        done => PersistorManager.deleteFile(bucket, key, done),
-        done => PersistorManager.deleteDirectory(bucket, convertedKey, done)
-      ],
-      callback
-    )
-  },
-
-  getFile(bucket, key, opts, callback) {
-    // In this call, opts can contain credentials
-    if (opts == null) {
-      opts = {}
-    }
-    logger.log({ bucket, key, opts: this._scrubSecrets(opts) }, 'getting file')
-    if (opts.format == null && opts.style == null) {
-      return this._getStandardFile(bucket, key, opts, callback)
-    } else {
-      return this._getConvertedFile(bucket, key, opts, callback)
-    }
-  },
-
-  getFileSize(bucket, key, callback) {
-    return PersistorManager.getFileSize(bucket, key, callback)
-  },
-
-  _getStandardFile(bucket, key, opts, callback) {
-    return PersistorManager.getFileStream(bucket, key, opts, function(
-      err,
-      fileStream
-    ) {
-      if (err != null && !(err instanceof Errors.NotFoundError)) {
-        logger.err(
-          { bucket, key, opts: FileHandler._scrubSecrets(opts) },
-          'error getting fileStream'
-        )
-      }
-      return callback(err, fileStream)
-    })
-  },
-
-  _getConvertedFile(bucket, key, opts, callback) {
-    const convertedKey = KeyBuilder.addCachingToKey(key, opts)
-    return PersistorManager.checkIfFileExists(
-      bucket,
-      convertedKey,
-      (err, exists) => {
-        if (err != null) {
-          return callback(err)
-        }
-        if (exists) {
-          return PersistorManager.getFileStream(
-            bucket,
-            convertedKey,
-            opts,
-            callback
-          )
-        } else {
-          return this._getConvertedFileAndCache(
-            bucket,
-            key,
-            convertedKey,
-            opts,
-            callback
-          )
-        }
-      }
-    )
-  },
-
-  _getConvertedFileAndCache(bucket, key, convertedKey, opts, callback) {
-    let convertedFsPath = ''
-    const originalFsPath = ''
-    return async.series(
-      [
-        cb => {
-          return this._convertFile(bucket, key, opts, function(
-            err,
-            fileSystemPath,
-            originalFsPath
-          ) {
-            convertedFsPath = fileSystemPath
-            originalFsPath = originalFsPath
-            return cb(err)
-          })
-        },
-        cb => ImageOptimiser.compressPng(convertedFsPath, cb),
-        cb =>
-          PersistorManager.sendFile(bucket, convertedKey, convertedFsPath, cb)
-      ],
-      function(err) {
-        if (err != null) {
-          LocalFileWriter.deleteFile(convertedFsPath, function() {})
-          LocalFileWriter.deleteFile(originalFsPath, function() {})
-          return callback(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
-        return LocalFileWriter.getStream(convertedFsPath, function(
-          err,
-          readStream
-        ) {
-          if (err != null) {
-            return callback(err)
-          }
-          readStream.on('end', function() {
-            logger.log({ convertedFsPath }, 'deleting temporary file')
-            return LocalFileWriter.deleteFile(convertedFsPath, function() {})
-          })
-          return callback(null, readStream)
-        })
-      }
-    )
-  },
-
-  _convertFile(bucket, originalKey, opts, callback) {
-    return this._writeS3FileToDisk(bucket, originalKey, opts, function(
-      err,
-      originalFsPath
-    ) {
-      if (err != null) {
-        return callback(err)
-      }
-      const done = function(err, destPath) {
-        if (err != null) {
-          logger.err(
-            { err, bucket, originalKey, opts: FileHandler._scrubSecrets(opts) },
-            'error converting file'
-          )
-          return callback(err)
-        }
-        LocalFileWriter.deleteFile(originalFsPath, function() {})
-        return callback(err, destPath, originalFsPath)
-      }
-
-      logger.log({ opts }, 'converting file depending on opts')
-
-      if (opts.format != null) {
-        return FileConverter.convert(originalFsPath, opts.format, done)
-      } else if (opts.style === 'thumbnail') {
-        return FileConverter.thumbnail(originalFsPath, done)
-      } else if (opts.style === 'preview') {
-        return FileConverter.preview(originalFsPath, done)
-      } else {
-        return callback(
-          new Error(
-            `should have specified opts to convert file with ${JSON.stringify(
-              opts
-            )}`
-          )
-        )
-      }
-    })
-  },
-
-  _writeS3FileToDisk(bucket, key, opts, callback) {
-    return PersistorManager.getFileStream(bucket, key, opts, function(
-      err,
-      fileStream
-    ) {
-      if (err != null) {
-        return callback(err)
-      }
-      return LocalFileWriter.writeStream(fileStream, key, callback)
-    })
-  },
-
-  getDirectorySize(bucket, project_id, callback) {
-    logger.log({ bucket, project_id }, 'getting project size')
-    return PersistorManager.directorySize(bucket, project_id, function(
-      err,
-      size
-    ) {
-      if (err != null) {
-        logger.err({ bucket, project_id }, 'error getting size')
-      }
-      return callback(err, size)
-    })
-  },
-
-  _scrubSecrets(opts) {
-    const safe = Object.assign({}, opts)
-    delete safe.credentials
-    return safe
   }
 }
+
+function insertFile(bucket, key, stream, callback) {
+  const convertedKey = KeyBuilder.getConvertedFolderKey(key)
+  PersistorManager.deleteDirectory(bucket, convertedKey, function(error) {
+    if (error) {
+      return callback(new WriteError('error inserting file').withCause(error))
+    }
+    PersistorManager.sendStream(bucket, key, stream, callback)
+  })
+}
+
+function deleteFile(bucket, key, callback) {
+  const convertedKey = KeyBuilder.getConvertedFolderKey(key)
+  async.parallel(
+    [
+      done => PersistorManager.deleteFile(bucket, key, done),
+      done => PersistorManager.deleteDirectory(bucket, convertedKey, done)
+    ],
+    callback
+  )
+}
+
+function getFile(bucket, key, opts, callback) {
+  // In this call, opts can contain credentials
+  if (!opts) {
+    opts = {}
+  }
+  logger.log({ bucket, key, opts: _scrubSecrets(opts) }, 'getting file')
+  if (!opts.format && !opts.style) {
+    _getStandardFile(bucket, key, opts, callback)
+  } else {
+    _getConvertedFile(bucket, key, opts, callback)
+  }
+}
+
+function getFileSize(bucket, key, callback) {
+  PersistorManager.getFileSize(bucket, key, callback)
+}
+
+function getDirectorySize(bucket, projectId, callback) {
+  logger.log({ bucket, project_id: projectId }, 'getting project size')
+  PersistorManager.directorySize(bucket, projectId, function(err, size) {
+    if (err) {
+      logger.err({ bucket, project_id: projectId }, 'error getting size')
+      err = new ReadError('error getting project size').withCause(err)
+    }
+    return callback(err, size)
+  })
+}
+
+function _getStandardFile(bucket, key, opts, callback) {
+  PersistorManager.getFileStream(bucket, key, opts, function(err, fileStream) {
+    if (err && err.name !== 'NotFoundError') {
+      logger.err(
+        { bucket, key, opts: _scrubSecrets(opts) },
+        'error getting fileStream'
+      )
+    }
+    callback(err, fileStream)
+  })
+}
+
+function _getConvertedFile(bucket, key, opts, callback) {
+  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)
+    }
+  })
+}
+
+function _getConvertedFileAndCache(bucket, key, convertedKey, opts, callback) {
+  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('failed to convert file').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)
+    }
+  )
+}
+
+function _convertFile(bucket, originalKey, opts, callback) {
+  _writeFileToDisk(bucket, originalKey, opts, function(err, originalFsPath) {
+    if (err) {
+      return callback(
+        new ConversionError('unable to write file to disk').withCause(err)
+      )
+    }
+
+    const done = function(err, destPath) {
+      if (err) {
+        logger.err(
+          { err, bucket, originalKey, opts: _scrubSecrets(opts) },
+          'error converting file'
+        )
+        return callback(
+          new ConversionError('error converting file').withCause(err)
+        )
+      }
+      LocalFileWriter.deleteFile(originalFsPath, function() {})
+      callback(err, destPath)
+    }
+
+    logger.log({ opts }, 'converting file depending on opts')
+
+    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(
+          `should have specified opts to convert file with ${JSON.stringify(
+            opts
+          )}`
+        )
+      )
+    }
+  })
+}
+
+function _writeFileToDisk(bucket, key, opts, callback) {
+  PersistorManager.getFileStream(bucket, key, opts, function(err, fileStream) {
+    if (err) {
+      return callback(
+        new ReadError('unable to get read stream for file').withCause(err)
+      )
+    }
+    LocalFileWriter.writeStream(fileStream, key, callback)
+  })
+}
+
+function _scrubSecrets(opts) {
+  const safe = Object.assign({}, opts)
+  delete safe.credentials
+  return safe
+}

services/filestore/app/js/LocalFileWriter.js

@@ -1,91 +1,57 @@
-/* eslint-disable
-    handle-callback-err,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 const fs = require('fs')
 const uuid = require('node-uuid')
 const path = require('path')
-const _ = require('underscore')
+const Stream = require('stream')
+const { callbackify, promisify } = require('util')
 const logger = require('logger-sharelatex')
 const metrics = require('metrics-sharelatex')
 const Settings = require('settings-sharelatex')
-const Errors = require('./Errors')
+const { WriteError } = require('./Errors')
 
 module.exports = {
-  writeStream(stream, key, callback) {
+  promises: {
-    const timer = new metrics.Timer('writingFile')
+    writeStream,
-    callback = _.once(callback)
+    deleteFile
-    const fsPath = this._getPath(key)
-    logger.log({ fsPath }, 'writing file locally')
-    const writeStream = fs.createWriteStream(fsPath)
-    writeStream.on('finish', function() {
-      timer.done()
-      logger.log({ fsPath }, 'finished writing file locally')
-      return callback(null, fsPath)
-    })
-    writeStream.on('error', function(err) {
-      logger.err(
-        { err, fsPath },
-        'problem writing file locally, with write stream'
-      )
-      return callback(err)
-    })
-    stream.on('error', function(err) {
-      logger.log(
-        { err, fsPath },
-        'problem writing file locally, with read stream'
-      )
-      return callback(err)
-    })
-    return stream.pipe(writeStream)
   },
+  writeStream: callbackify(writeStream),
+  deleteFile: callbackify(deleteFile)
+}
 
-  getStream(fsPath, _callback) {
+const pipeline = promisify(Stream.pipeline)
-    if (_callback == null) {
-      _callback = function(err, res) {}
-    }
-    const callback = _.once(_callback)
-    const timer = new metrics.Timer('readingFile')
-    logger.log({ fsPath }, 'reading file locally')
-    const readStream = fs.createReadStream(fsPath)
-    readStream.on('end', function() {
-      timer.done()
-      return logger.log({ fsPath }, 'finished reading file locally')
-    })
-    readStream.on('error', function(err) {
-      logger.err(
-        { err, fsPath },
-        'problem reading file locally, with read stream'
-      )
-      if (err.code === 'ENOENT') {
-        return callback(new Errors.NotFoundError(err.message), null)
-      } else {
-        return callback(err)
-      }
-    })
-    return callback(null, readStream)
-  },
 
-  deleteFile(fsPath, callback) {
+async function writeStream(stream, key) {
-    if (fsPath == null || fsPath === '') {
+  const timer = new metrics.Timer('writingFile')
-      return callback()
+  const fsPath = _getPath(key)
-    }
-    logger.log({ fsPath }, 'removing local temp file')
-    return fs.unlink(fsPath, callback)
-  },
 
-  _getPath(key) {
+  logger.log({ fsPath }, 'writing file locally')
-    if (key == null) {
+
-      key = uuid.v1()
+  const writeStream = fs.createWriteStream(fsPath)
-    }
+  try {
-    key = key.replace(/\//g, '-')
+    await pipeline(stream, writeStream)
-    return path.join(Settings.path.uploadFolder, key)
+    timer.done()
+    logger.log({ fsPath }, 'finished writing file locally')
+    return fsPath
+  } catch (err) {
+    logger.err({ err, fsPath }, 'problem writing file locally')
+    throw new WriteError({
+      message: 'problem writing file locally',
+      info: { err, fsPath }
+    }).withCause(err)
   }
 }
+
+async function deleteFile(fsPath) {
+  if (!fsPath) {
+    return
+  }
+  logger.log({ fsPath }, 'removing local temp file')
+  await promisify(fs.unlink)(fsPath)
+}
+
+function _getPath(key) {
+  if (key == null) {
+    key = uuid.v1()
+  }
+  key = key.replace(/\//g, '-')
+  return path.join(Settings.path.uploadFolder, key)
+}

services/filestore/npm-shrinkwrap.json

@@ -4763,6 +4763,12 @@
         "type-detect": "^4.0.8"
       }
     },
+    "sinon-chai": {
+      "version": "3.3.0",
+      "resolved": "https://registry.npmjs.org/sinon-chai/-/sinon-chai-3.3.0.tgz",
+      "integrity": "sha512-r2JhDY7gbbmh5z3Q62pNbrjxZdOAjpsqW/8yxAZRSqLZqowmfGZPGUZPFf3UX36NLis0cv8VEM5IJh9HgkSOAA==",
+      "dev": true
+    },
     "slice-ansi": {
       "version": "2.1.0",
       "resolved": "https://registry.npmjs.org/slice-ansi/-/slice-ansi-2.1.0.tgz",

services/filestore/package.json

@@ -62,6 +62,7 @@
     "prettier-eslint": "^9.0.1",
     "prettier-eslint-cli": "^5.0.0",
     "sandboxed-module": "2.0.3",
-    "sinon": "7.1.1"
+    "sinon": "7.1.1",
+    "sinon-chai": "^3.3.0"
   }
 }

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

@@ -1,367 +1,233 @@
-/* eslint-disable
-    handle-callback-err,
-    no-return-assign,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-const { assert } = require('chai')
 const sinon = require('sinon')
 const chai = require('chai')
-const should = chai.should()
 const { expect } = chai
 const modulePath = '../../../app/js/FileHandler.js'
 const SandboxedModule = require('sandboxed-module')
 
 describe('FileHandler', function() {
-  beforeEach(function() {
+  let PersistorManager,
-    this.settings = {
+    LocalFileWriter,
-      s3: {
+    FileConverter,
-        buckets: {
+    KeyBuilder,
-          user_files: 'user_files'
+    ImageOptimiser,
-        }
+    FileHandler,
+    fs
+  const settings = {
+    s3: {
+      buckets: {
+        user_files: 'user_files'
       }
     }
-    this.PersistorManager = {
+  }
-      getFileStream: sinon.stub(),
+
-      checkIfFileExists: sinon.stub(),
+  const bucket = 'my_bucket'
-      deleteFile: sinon.stub(),
+  const key = 'key/here'
-      deleteDirectory: sinon.stub(),
+  const convertedFolderKey = 'convertedFolder'
-      sendStream: sinon.stub(),
+  const sourceStream = 'sourceStream'
-      insertFile: sinon.stub(),
+  const convertedKey = 'convertedKey'
-      directorySize: sinon.stub()
+  const readStream = {
+    stream: 'readStream',
+    on: sinon.stub()
+  }
+
+  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()
     }
-    this.LocalFileWriter = {
+    LocalFileWriter = {
-      writeStream: sinon.stub(),
+      writeStream: sinon.stub().yields(),
-      getStream: sinon.stub(),
+      deleteFile: sinon.stub().yields()
-      deleteFile: sinon.stub()
     }
-    this.FileConverter = {
+    FileConverter = {
-      convert: sinon.stub(),
+      convert: sinon.stub().yields(),
-      thumbnail: sinon.stub(),
+      thumbnail: sinon.stub().yields(),
-      preview: sinon.stub()
+      preview: sinon.stub().yields()
     }
-    this.keyBuilder = {
+    KeyBuilder = {
-      addCachingToKey: sinon.stub(),
+      addCachingToKey: sinon.stub().returns(convertedKey),
-      getConvertedFolderKey: sinon.stub()
+      getConvertedFolderKey: sinon.stub().returns(convertedFolderKey)
     }
-    this.ImageOptimiser = { compressPng: sinon.stub() }
+    ImageOptimiser = { compressPng: sinon.stub().yields() }
-    this.handler = SandboxedModule.require(modulePath, {
+    fs = {
+      createReadStream: sinon.stub().returns(readStream)
+    }
+
+    FileHandler = SandboxedModule.require(modulePath, {
       requires: {
-        'settings-sharelatex': this.settings,
+        'settings-sharelatex': settings,
-        './PersistorManager': this.PersistorManager,
+        './PersistorManager': PersistorManager,
-        './LocalFileWriter': this.LocalFileWriter,
+        './LocalFileWriter': LocalFileWriter,
-        './FileConverter': this.FileConverter,
+        './FileConverter': FileConverter,
-        './KeyBuilder': this.keyBuilder,
+        './KeyBuilder': KeyBuilder,
-        './ImageOptimiser': this.ImageOptimiser,
+        './ImageOptimiser': ImageOptimiser,
+        fs: fs,
         'logger-sharelatex': {
           log() {},
           err() {}
         }
-      }
+      },
+      globals: { console }
     })
-    this.bucket = 'my_bucket'
-    this.key = 'key/here'
-    this.stubbedPath = '/var/somewhere/path'
-    this.format = 'png'
-    return (this.formattedStubbedPath = `${this.stubbedPath}.${this.format}`)
   })
 
   describe('insertFile', function() {
-    beforeEach(function() {
+    const stream = 'stream'
-      this.stream = {}
-      this.PersistorManager.deleteDirectory.callsArgWith(2)
-      return this.PersistorManager.sendStream.callsArgWith(3)
-    })
 
     it('should send file to the filestore', function(done) {
-      return this.handler.insertFile(this.bucket, this.key, this.stream, () => {
+      FileHandler.insertFile(bucket, key, stream, err => {
-        this.PersistorManager.sendStream
+        expect(err).not.to.exist
-          .calledWith(this.bucket, this.key, this.stream)
+        expect(PersistorManager.sendStream).to.have.been.calledWith(
-          .should.equal(true)
+          bucket,
-        return done()
+          key,
+          stream
+        )
+        done()
       })
     })
 
-    return it('should delete the convetedKey folder', function(done) {
+    it('should delete the convertedKey folder', function(done) {
-      this.keyBuilder.getConvertedFolderKey.returns(this.stubbedConvetedKey)
+      FileHandler.insertFile(bucket, key, stream, err => {
-      return this.handler.insertFile(this.bucket, this.key, this.stream, () => {
+        expect(err).not.to.exist
-        this.PersistorManager.deleteDirectory
+        expect(PersistorManager.deleteDirectory).to.have.been.calledWith(
-          .calledWith(this.bucket, this.stubbedConvetedKey)
+          bucket,
-          .should.equal(true)
+          convertedFolderKey
-        return done()
+        )
+        done()
       })
     })
   })
 
   describe('deleteFile', function() {
-    beforeEach(function() {
-      this.keyBuilder.getConvertedFolderKey.returns(this.stubbedConvetedKey)
-      this.PersistorManager.deleteFile.callsArgWith(2)
-      return this.PersistorManager.deleteDirectory.callsArgWith(2)
-    })
-
     it('should tell the filestore manager to delete the file', function(done) {
-      return this.handler.deleteFile(this.bucket, this.key, () => {
+      FileHandler.deleteFile(bucket, key, err => {
-        this.PersistorManager.deleteFile
+        expect(err).not.to.exist
-          .calledWith(this.bucket, this.key)
+        expect(PersistorManager.deleteFile).to.have.been.calledWith(bucket, key)
-          .should.equal(true)
+        done()
-        return done()
       })
     })
 
-    return it('should tell the filestore manager to delete the cached foler', function(done) {
+    it('should tell the filestore manager to delete the cached folder', function(done) {
-      return this.handler.deleteFile(this.bucket, this.key, () => {
+      FileHandler.deleteFile(bucket, key, err => {
-        this.PersistorManager.deleteDirectory
+        expect(err).not.to.exist
-          .calledWith(this.bucket, this.stubbedConvetedKey)
+        expect(PersistorManager.deleteDirectory).to.have.been.calledWith(
-          .should.equal(true)
+          bucket,
-        return done()
+          convertedFolderKey
+        )
+        done()
       })
     })
   })
 
   describe('getFile', function() {
-    beforeEach(function() {
+    it('should return the source stream no format or style are defined', function(done) {
-      this.handler._getStandardFile = sinon.stub().callsArgWith(3)
+      FileHandler.getFile(bucket, key, null, (err, stream) => {
-      return (this.handler._getConvertedFile = sinon.stub().callsArgWith(3))
+        expect(err).not.to.exist
-    })
+        expect(stream).to.equal(sourceStream)
-
+        done()
-    it('should call _getStandardFile if no format or style are defined', function(done) {
-      return this.handler.getFile(this.bucket, this.key, null, () => {
-        this.handler._getStandardFile.called.should.equal(true)
-        this.handler._getConvertedFile.called.should.equal(false)
-        return done()
       })
     })
 
-    it('should pass options to _getStandardFile', function(done) {
+    it('should pass options through to PersistorManager', function(done) {
       const options = { start: 0, end: 8 }
-      return this.handler.getFile(this.bucket, this.key, options, () => {
+      FileHandler.getFile(bucket, key, options, err => {
-        expect(this.handler._getStandardFile.lastCall.args[2].start).to.equal(0)
+        expect(err).not.to.exist
-        expect(this.handler._getStandardFile.lastCall.args[2].end).to.equal(8)
+        expect(PersistorManager.getFileStream).to.have.been.calledWith(
-        return done()
+          bucket,
+          key,
+          options
+        )
+        done()
       })
     })
 
-    return it('should call _getConvertedFile if a format is defined', function(done) {
+    describe('when a format is defined', function() {
-      return this.handler.getFile(
+      let result
-        this.bucket,
-        this.key,
-        { format: 'png' },
-        () => {
-          this.handler._getStandardFile.called.should.equal(false)
-          this.handler._getConvertedFile.called.should.equal(true)
-          return done()
-        }
-      )
-    })
-  })
 
-  describe('_getStandardFile', function() {
+      describe('when the file is not cached', function() {
-    beforeEach(function() {
+        beforeEach(function(done) {
-      this.fileStream = { on() {} }
+          FileHandler.getFile(bucket, key, { format: 'png' }, (err, stream) => {
-      return this.PersistorManager.getFileStream.callsArgWith(
+            result = { err, stream }
-        3,
+            done()
-        'err',
+          })
-        this.fileStream
+        })
-      )
-    })
 
-    it('should get the stream', function(done) {
+        it('should convert the file', function() {
-      return this.handler.getFile(this.bucket, this.key, null, () => {
+          expect(FileConverter.convert).to.have.been.called
-        this.PersistorManager.getFileStream
+          expect(ImageOptimiser.compressPng).to.have.been.called
-          .calledWith(this.bucket, this.key)
+        })
-          .should.equal(true)
+
-        return done()
+        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
+          )
+        })
+      })
+
+      describe('when the file is cached', function() {
+        beforeEach(function(done) {
+          PersistorManager.checkIfFileExists = sinon.stub().yields(null, true)
+          FileHandler.getFile(bucket, key, { format: 'png' }, (err, stream) => {
+            result = { err, stream }
+            done()
+          })
+        })
+
+        it('should not convert the file', function() {
+          expect(FileConverter.convert).not.to.have.been.called
+          expect(ImageOptimiser.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
+          )
+        })
       })
     })
 
-    it('should return the stream and error', function(done) {
+    describe('when a style is defined', function() {
-      return this.handler.getFile(
+      it('generates a thumbnail when requested', function(done) {
-        this.bucket,
+        FileHandler.getFile(bucket, key, { style: 'thumbnail' }, err => {
-        this.key,
+          expect(err).not.to.exist
-        null,
+          expect(FileConverter.thumbnail).to.have.been.called
-        (err, stream) => {
+          expect(FileConverter.preview).not.to.have.been.called
-          err.should.equal('err')
+          done()
-          stream.should.equal(this.fileStream)
+        })
-          return done()
-        }
-      )
-    })
-
-    return it('should pass options to PersistorManager', function(done) {
-      return this.handler.getFile(
-        this.bucket,
-        this.key,
-        { start: 0, end: 8 },
-        () => {
-          expect(
-            this.PersistorManager.getFileStream.lastCall.args[2].start
-          ).to.equal(0)
-          expect(
-            this.PersistorManager.getFileStream.lastCall.args[2].end
-          ).to.equal(8)
-          return done()
-        }
-      )
-    })
-  })
-
-  describe('_getConvertedFile', function() {
-    it('should getFileStream if it does exists', function(done) {
-      this.PersistorManager.checkIfFileExists.callsArgWith(2, null, true)
-      this.PersistorManager.getFileStream.callsArgWith(3)
-      return this.handler._getConvertedFile(this.bucket, this.key, {}, () => {
-        this.PersistorManager.getFileStream
-          .calledWith(this.bucket)
-          .should.equal(true)
-        return done()
       })
-    })
 
-    return it('should call _getConvertedFileAndCache if it does exists', function(done) {
+      it('generates a preview when requested', function(done) {
-      this.PersistorManager.checkIfFileExists.callsArgWith(2, null, false)
+        FileHandler.getFile(bucket, key, { style: 'preview' }, err => {
-      this.handler._getConvertedFileAndCache = sinon.stub().callsArgWith(4)
+          expect(err).not.to.exist
-      return this.handler._getConvertedFile(this.bucket, this.key, {}, () => {
+          expect(FileConverter.thumbnail).not.to.have.been.called
-        this.handler._getConvertedFileAndCache
+          expect(FileConverter.preview).to.have.been.called
-          .calledWith(this.bucket, this.key)
+          done()
-          .should.equal(true)
+        })
-        return done()
       })
     })
   })
 
-  describe('_getConvertedFileAndCache', () =>
+  describe('getDirectorySize', function() {
-    it('should _convertFile ', function(done) {
+    it('should call the filestore manager to get directory size', function(done) {
-      this.stubbedStream = { something: 'here' }
+      FileHandler.getDirectorySize(bucket, key, err => {
-      this.localStream = {
+        expect(err).not.to.exist
-        on() {}
+        expect(PersistorManager.directorySize).to.have.been.calledWith(
-      }
+          bucket,
-      this.PersistorManager.sendFile = sinon.stub().callsArgWith(3)
+          key
-      this.LocalFileWriter.getStream = sinon
+        )
-        .stub()
+        done()
-        .callsArgWith(1, null, this.localStream)
-      this.convetedKey = this.key + 'converted'
-      this.handler._convertFile = sinon
-        .stub()
-        .callsArgWith(3, null, this.stubbedPath)
-      this.ImageOptimiser.compressPng = sinon.stub().callsArgWith(1)
-      return this.handler._getConvertedFileAndCache(
-        this.bucket,
-        this.key,
-        this.convetedKey,
-        {},
-        (err, fsStream) => {
-          this.handler._convertFile.called.should.equal(true)
-          this.PersistorManager.sendFile
-            .calledWith(this.bucket, this.convetedKey, this.stubbedPath)
-            .should.equal(true)
-          this.ImageOptimiser.compressPng
-            .calledWith(this.stubbedPath)
-            .should.equal(true)
-          this.LocalFileWriter.getStream
-            .calledWith(this.stubbedPath)
-            .should.equal(true)
-          fsStream.should.equal(this.localStream)
-          return done()
-        }
-      )
-    }))
-
-  describe('_convertFile', function() {
-    beforeEach(function() {
-      this.FileConverter.convert.callsArgWith(
-        2,
-        null,
-        this.formattedStubbedPath
-      )
-      this.FileConverter.thumbnail.callsArgWith(
-        1,
-        null,
-        this.formattedStubbedPath
-      )
-      this.FileConverter.preview.callsArgWith(
-        1,
-        null,
-        this.formattedStubbedPath
-      )
-      this.handler._writeS3FileToDisk = sinon
-        .stub()
-        .callsArgWith(3, null, this.stubbedPath)
-      return this.LocalFileWriter.deleteFile.callsArgWith(1)
-    })
-
-    it('should call thumbnail on the writer path if style was thumbnail was specified', function(done) {
-      return this.handler._convertFile(
-        this.bucket,
-        this.key,
-        { style: 'thumbnail' },
-        (err, path) => {
-          path.should.equal(this.formattedStubbedPath)
-          this.FileConverter.thumbnail
-            .calledWith(this.stubbedPath)
-            .should.equal(true)
-          this.LocalFileWriter.deleteFile
-            .calledWith(this.stubbedPath)
-            .should.equal(true)
-          return done()
-        }
-      )
-    })
-
-    it('should call preview on the writer path if style was preview was specified', function(done) {
-      return this.handler._convertFile(
-        this.bucket,
-        this.key,
-        { style: 'preview' },
-        (err, path) => {
-          path.should.equal(this.formattedStubbedPath)
-          this.FileConverter.preview
-            .calledWith(this.stubbedPath)
-            .should.equal(true)
-          this.LocalFileWriter.deleteFile
-            .calledWith(this.stubbedPath)
-            .should.equal(true)
-          return done()
-        }
-      )
-    })
-
-    return it('should call convert on the writer path if a format was specified', function(done) {
-      return this.handler._convertFile(
-        this.bucket,
-        this.key,
-        { format: this.format },
-        (err, path) => {
-          path.should.equal(this.formattedStubbedPath)
-          this.FileConverter.convert
-            .calledWith(this.stubbedPath, this.format)
-            .should.equal(true)
-          this.LocalFileWriter.deleteFile
-            .calledWith(this.stubbedPath)
-            .should.equal(true)
-          return done()
-        }
-      )
-    })
-  })
-
-  return describe('getDirectorySize', function() {
-    beforeEach(function() {
-      return this.PersistorManager.directorySize.callsArgWith(2)
-    })
-
-    return it('should call the filestore manager to get directory size', function(done) {
-      return this.handler.getDirectorySize(this.bucket, this.key, () => {
-        this.PersistorManager.directorySize
-          .calledWith(this.bucket, this.key)
-          .should.equal(true)
-        return done()
       })
     })
   })

services/filestore/test/unit/js/LocalFileWriterTests.js

@@ -1,120 +1,79 @@
-/* eslint-disable
-    handle-callback-err,
-    no-return-assign,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-
-const { assert } = require('chai')
 const sinon = require('sinon')
 const chai = require('chai')
-const should = chai.should()
 const { expect } = chai
 const modulePath = '../../../app/js/LocalFileWriter.js'
 const SandboxedModule = require('sandboxed-module')
+chai.use(require('sinon-chai'))
 
 describe('LocalFileWriter', function() {
+  const writeStream = 'writeStream'
+  const readStream = 'readStream'
+  const settings = { path: { uploadFolder: '/uploads' } }
+  const fsPath = '/uploads/wombat'
+  const filename = 'wombat'
+  let stream, fs, LocalFileWriter
+
   beforeEach(function() {
-    this.writeStream = {
+    fs = {
-      on(type, cb) {
+      createWriteStream: sinon.stub().returns(writeStream),
-        if (type === 'finish') {
+      unlink: sinon.stub().yields()
-          return cb()
-        }
-      }
     }
-    this.readStream = { on() {} }
+    stream = {
-    this.fs = {
+      pipeline: sinon.stub().yields()
-      createWriteStream: sinon.stub().returns(this.writeStream),
-      createReadStream: sinon.stub().returns(this.readStream),
-      unlink: sinon.stub()
     }
-    this.settings = {
+
-      path: {
+    LocalFileWriter = SandboxedModule.require(modulePath, {
-        uploadFolder: 'somewhere'
-      }
-    }
-    this.writer = SandboxedModule.require(modulePath, {
       requires: {
-        fs: this.fs,
+        fs,
+        stream,
         'logger-sharelatex': {
           log() {},
           err() {}
         },
-        'settings-sharelatex': this.settings,
+        'settings-sharelatex': settings,
         'metrics-sharelatex': {
           inc: sinon.stub(),
           Timer: sinon.stub().returns({ done: sinon.stub() })
         }
       }
     })
-
-    return (this.stubbedFsPath = 'something/uploads/eio2k1j3')
   })
 
-  describe('writeStrem', function() {
+  describe('writeStream', function() {
-    beforeEach(function() {
+    it('writes the stream to the upload folder', function(done) {
-      return (this.writer._getPath = sinon.stub().returns(this.stubbedFsPath))
+      LocalFileWriter.writeStream(readStream, filename, (err, path) => {
-    })
+        expect(err).not.to.exist
-
+        expect(fs.createWriteStream).to.have.been.calledWith(fsPath)
-    it('write the stream to ./uploads', function(done) {
+        expect(stream.pipeline).to.have.been.calledWith(readStream, writeStream)
-      const stream = {
+        expect(path).to.equal(fsPath)
-        pipe: dest => {
+        done()
-          dest.should.equal(this.writeStream)
-          return done()
-        },
-        on() {}
-      }
-      return this.writer.writeStream(stream, null, () => {})
-    })
-
-    return it('should send the path in the callback', function(done) {
-      const stream = {
-        pipe: dest => {},
-        on(type, cb) {
-          if (type === 'end') {
-            return cb()
-          }
-        }
-      }
-      return this.writer.writeStream(stream, null, (err, fsPath) => {
-        fsPath.should.equal(this.stubbedFsPath)
-        return done()
       })
     })
   })
 
-  describe('getStream', function() {
+  describe('deleteFile', function() {
-    it('should read the stream from the file ', function(done) {
-      return this.writer.getStream(this.stubbedFsPath, (err, stream) => {
-        this.fs.createReadStream
-          .calledWith(this.stubbedFsPath)
-          .should.equal(true)
-        return done()
-      })
-    })
-
-    return it('should send the stream in the callback', function(done) {
-      return this.writer.getStream(this.stubbedFsPath, (err, readStream) => {
-        readStream.should.equal(this.readStream)
-        return done()
-      })
-    })
-  })
-
-  return describe('delete file', () =>
     it('should unlink the file', function(done) {
-      const error = 'my error'
+      LocalFileWriter.deleteFile(fsPath, err => {
-      this.fs.unlink.callsArgWith(1, error)
+        expect(err).not.to.exist
-      return this.writer.deleteFile(this.stubbedFsPath, err => {
+        expect(fs.unlink).to.have.been.calledWith(fsPath)
-        this.fs.unlink.calledWith(this.stubbedFsPath).should.equal(true)
+        done()
-        err.should.equal(error)
-        return done()
       })
-    }))
+    })
+
+    it('should not do anything if called with an empty path', function(done) {
+      fs.unlink = sinon.stub().yields(new Error('failed to reticulate splines'))
+      LocalFileWriter.deleteFile(fsPath, err => {
+        expect(err).to.exist
+        done()
+      })
+    })
+
+    it('should not call unlink with an empty path', function(done) {
+      LocalFileWriter.deleteFile('', err => {
+        expect(err).not.to.exist
+        expect(fs.unlink).not.to.have.been.called
+        done()
+      })
+    })
+  })
 })