Merge pull request #69 from overleaf/spd-decaf-cleanup-6

Decaf cleanup and promisification for FSPersistorManager

services/filestore/app/js/FSPersistorManager.js

@@ -1,206 +1,229 @@
-/* eslint-disable
-    handle-callback-err,
-    no-unreachable,
-    node/no-deprecated-api,
-*/
-// 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 logger = require('logger-sharelatex')
 const fs = require('fs')
+const glob = require('glob')
+const logger = require('logger-sharelatex')
 const path = require('path')
-const LocalFileWriter = require('./LocalFileWriter')
-const Errors = require('./Errors')
 const rimraf = require('rimraf')
-const _ = require('underscore')
+const Stream = require('stream')
+const { promisify, callbackify } = require('util')
+
+const LocalFileWriter = require('./LocalFileWriter').promises
+const { NotFoundError, ReadError, WriteError } = require('./Errors')
+
+const pipeline = promisify(Stream.pipeline)
+const fsUnlink = promisify(fs.unlink)
+const fsOpen = promisify(fs.open)
+const fsStat = promisify(fs.stat)
+const fsGlob = promisify(glob)
+const rmrf = promisify(rimraf)
 
 const filterName = key => key.replace(/\//g, '_')
 
-module.exports = {
-  sendFile(location, target, source, callback) {
-    if (callback == null) {
-      callback = function(err) {}
-    }
-    const filteredTarget = filterName(target)
-    logger.log({ location, target: filteredTarget, source }, 'sending file')
-    const done = _.once(function(err) {
-      if (err != null) {
-        logger.err(
-          { err, location, target: filteredTarget, source },
-          'Error on put of file'
-        )
-      }
-      return callback(err)
-    })
-    // actually copy the file (instead of moving it) to maintain consistent behaviour
-    // between the different implementations
+async function sendFile(location, target, source) {
+  const filteredTarget = filterName(target)
+  logger.log({ location, target: filteredTarget, source }, 'sending file')
+
+  // actually copy the file (instead of moving it) to maintain consistent behaviour
+  // between the different implementations
+  try {
     const sourceStream = fs.createReadStream(source)
-    sourceStream.on('error', done)
     const targetStream = fs.createWriteStream(`${location}/${filteredTarget}`)
-    targetStream.on('error', done)
-    targetStream.on('finish', () => done())
-    return sourceStream.pipe(targetStream)
-  },
-
-  sendStream(location, target, sourceStream, callback) {
-    if (callback == null) {
-      callback = function(err) {}
-    }
-    logger.log({ location, target }, 'sending file stream')
-    sourceStream.on('error', err =>
-      logger.err({ location, target, err: err('error on stream to send') })
+    await pipeline(sourceStream, targetStream)
+  } catch (err) {
+    throw _wrapError(
+      err,
+      'failed to copy the specified file',
+      { location, target, source },
+      WriteError
     )
-    return LocalFileWriter.writeStream(sourceStream, null, (err, fsPath) => {
-      if (err != null) {
-        logger.err(
-          { location, target, fsPath, err },
-          'something went wrong writing stream to disk'
-        )
-        return callback(err)
-      }
-      return this.sendFile(location, target, fsPath, (
-        err // delete the temporary file created above and return the original error
-      ) => LocalFileWriter.deleteFile(fsPath, () => callback(err)))
-    })
-  },
-
-  // opts may be {start: Number, end: Number}
-  getFileStream(location, name, opts, callback) {
-    if (callback == null) {
-      callback = function(err, res) {}
-    }
-    const filteredName = filterName(name)
-    logger.log({ location, filteredName }, 'getting file')
-    return fs.open(`${location}/${filteredName}`, 'r', function(err, fd) {
-      if (err != null) {
-        logger.err(
-          { err, location, filteredName: name },
-          'Error reading from file'
-        )
-        if (err.code === 'ENOENT') {
-          return callback(new Errors.NotFoundError(err.message), null)
-        } else {
-          return callback(err, null)
-        }
-      }
-      opts.fd = fd
-      const sourceStream = fs.createReadStream(null, opts)
-      return callback(null, sourceStream)
-    })
-  },
-
-  getFileSize(location, filename, callback) {
-    const fullPath = path.join(location, filterName(filename))
-    return fs.stat(fullPath, function(err, stats) {
-      if (err != null) {
-        if (err.code === 'ENOENT') {
-          logger.log({ location, filename }, 'file not found')
-          callback(new Errors.NotFoundError(err.message))
-        } else {
-          logger.err({ err, location, filename }, 'failed to stat file')
-          callback(err)
-        }
-        return
-      }
-      return callback(null, stats.size)
-    })
-  },
+  }
+}
 
-  copyFile(location, fromName, toName, callback) {
-    if (callback == null) {
-      callback = function(err) {}
-    }
-    const filteredFromName = filterName(fromName)
-    const filteredToName = filterName(toName)
-    logger.log(
-      { location, fromName: filteredFromName, toName: filteredToName },
-      'copying file'
+async function sendStream(location, target, sourceStream) {
+  logger.log({ location, target }, 'sending file stream')
+
+  const fsPath = await LocalFileWriter.writeStream(sourceStream)
+
+  try {
+    await sendFile(location, target, fsPath)
+  } finally {
+    await LocalFileWriter.deleteFile(fsPath)
+  }
+}
+
+// opts may be {start: Number, end: Number}
+async function getFileStream(location, name, opts) {
+  const filteredName = filterName(name)
+  logger.log({ location, filteredName }, 'getting file')
+
+  try {
+    opts.fd = await fsOpen(`${location}/${filteredName}`, 'r')
+  } catch (err) {
+    logger.err({ err, location, filteredName: name }, 'Error reading from file')
+
+    throw _wrapError(
+      err,
+      'failed to open file for streaming',
+      { location, filteredName, opts },
+      ReadError
     )
+  }
+
+  return fs.createReadStream(null, opts)
+}
+
+async function getFileSize(location, filename) {
+  const fullPath = path.join(location, filterName(filename))
+
+  try {
+    const stat = await fsStat(fullPath)
+    return stat.size
+  } catch (err) {
+    logger.err({ err, location, filename }, 'failed to stat file')
+
+    throw _wrapError(
+      err,
+      'failed to stat file',
+      { location, filename },
+      ReadError
+    )
+  }
+}
+
+async function copyFile(location, fromName, toName) {
+  const filteredFromName = filterName(fromName)
+  const filteredToName = filterName(toName)
+  logger.log({ location, filteredFromName, filteredToName }, 'copying file')
+
+  try {
     const sourceStream = fs.createReadStream(`${location}/${filteredFromName}`)
-    sourceStream.on('error', function(err) {
-      logger.err(
-        { err, location, key: filteredFromName },
-        'Error reading from file'
-      )
-      return callback(err)
-    })
     const targetStream = fs.createWriteStream(`${location}/${filteredToName}`)
-    targetStream.on('error', function(err) {
-      logger.err(
-        { err, location, key: filteredToName },
-        'Error writing to file'
-      )
-      return callback(err)
-    })
-    targetStream.on('finish', () => callback(null))
-    return sourceStream.pipe(targetStream)
-  },
-
-  deleteFile(location, name, callback) {
-    const filteredName = filterName(name)
-    logger.log({ location, filteredName }, 'delete file')
-    return fs.unlink(`${location}/${filteredName}`, function(err) {
-      if (err != null) {
-        logger.err({ err, location, filteredName }, 'Error on delete.')
-        return callback(err)
-      } else {
-        return callback()
-      }
-    })
-  },
+    await pipeline(sourceStream, targetStream)
+  } catch (err) {
+    throw _wrapError(
+      err,
+      'failed to copy file',
+      { location, filteredFromName, filteredToName },
+      WriteError
+    )
+  }
+}
 
-  deleteDirectory(location, name, callback) {
-    if (callback == null) {
-      callback = function(err) {}
-    }
-    const filteredName = filterName(name.replace(/\/$/, ''))
-    return rimraf(`${location}/${filteredName}`, function(err) {
-      if (err != null) {
-        logger.err({ err, location, filteredName }, 'Error on rimraf rmdir.')
-        return callback(err)
-      } else {
-        return callback()
-      }
-    })
-  },
+async function deleteFile(location, name) {
+  const filteredName = filterName(name)
+  logger.log({ location, filteredName }, 'delete file')
+  try {
+    await fsUnlink(`${location}/${filteredName}`)
+  } catch (err) {
+    throw _wrapError(
+      err,
+      'failed to delete file',
+      { location, filteredName },
+      WriteError
+    )
+  }
+}
+
+// this is only called internally for clean-up by `FileHandler` and isn't part of the external API
+async function deleteDirectory(location, name) {
+  const filteredName = filterName(name.replace(/\/$/, ''))
+
+  logger.log({ location, filteredName }, 'deleting directory')
+
+  try {
+    await rmrf(`${location}/${filteredName}`)
+  } catch (err) {
+    throw _wrapError(
+      err,
+      'failed to delete directory',
+      { location, filteredName },
+      WriteError
+    )
+  }
+}
 
-  checkIfFileExists(location, name, callback) {
-    if (callback == null) {
-      callback = function(err, exists) {}
+async function checkIfFileExists(location, name) {
+  const filteredName = filterName(name)
+  try {
+    const stat = await fsStat(`${location}/${filteredName}`)
+    return !!stat
+  } catch (err) {
+    if (err.code === 'ENOENT') {
+      return false
     }
-    const filteredName = filterName(name)
-    logger.log({ location, filteredName }, 'checking if file exists')
-    return fs.exists(`${location}/${filteredName}`, function(exists) {
-      logger.log({ location, filteredName, exists }, 'checked if file exists')
-      return callback(null, exists)
-    })
-  },
-
-  directorySize(location, name, callback) {
-    const filteredName = filterName(name.replace(/\/$/, ''))
-    logger.log({ location, filteredName }, 'get project size in file system')
-    return fs.readdir(`${location}/${filteredName}`, function(err, files) {
-      if (err != null) {
-        logger.err(
-          { err, location, filteredName },
-          'something went wrong listing prefix in aws'
-        )
-        return callback(err)
+    throw _wrapError(
+      err,
+      'failed to stat file',
+      { location, filteredName },
+      ReadError
+    )
+  }
+}
+
+// note, does not recurse into subdirectories, as we use a flattened directory structure
+async function directorySize(location, name) {
+  const filteredName = filterName(name.replace(/\/$/, ''))
+  let size = 0
+
+  try {
+    const files = await fsGlob(`${location}/${filteredName}_*`)
+    for (const file of files) {
+      try {
+        const stat = await fsStat(file)
+        if (stat.isFile()) {
+          size += stat.size
+        }
+      } catch (err) {
+        // ignore files that may have just been deleted
+        if (err.code !== 'ENOENT') {
+          throw err
+        }
       }
-      let totalSize = 0
-      _.each(files, function(entry) {
-        const fd = fs.openSync(`${location}/${filteredName}/${entry}`, 'r')
-        const fileStats = fs.fstatSync(fd)
-        totalSize += fileStats.size
-        return fs.closeSync(fd)
-      })
-      logger.log({ totalSize }, 'total size', { files })
-      return callback(null, totalSize)
-    })
+    }
+  } catch (err) {
+    throw _wrapError(
+      err,
+      'failed to get directory size',
+      { location, name },
+      ReadError
+    )
+  }
+
+  return size
+}
+
+function _wrapError(error, message, params, ErrorType) {
+  if (error.code === 'ENOENT') {
+    return new NotFoundError({
+      message: 'no such file or directory',
+      info: params
+    }).withCause(error)
+  } else {
+    return new ErrorType({
+      message: message,
+      info: params
+    }).withCause(error)
+  }
+}
+
+module.exports = {
+  sendFile: callbackify(sendFile),
+  sendStream: callbackify(sendStream),
+  getFileStream: callbackify(getFileStream),
+  getFileSize: callbackify(getFileSize),
+  copyFile: callbackify(copyFile),
+  deleteFile: callbackify(deleteFile),
+  deleteDirectory: callbackify(deleteDirectory),
+  checkIfFileExists: callbackify(checkIfFileExists),
+  directorySize: callbackify(directorySize),
+  promises: {
+    sendFile,
+    sendStream,
+    getFileStream,
+    getFileSize,
+    copyFile,
+    deleteFile,
+    deleteDirectory,
+    checkIfFileExists,
+    directorySize
   }
 }

services/filestore/app/js/LocalFileWriter.js

@@ -32,6 +32,8 @@ async function writeStream(stream, key) {
     logger.log({ fsPath }, 'finished writing file locally')
     return fsPath
   } catch (err) {
+    await deleteFile(fsPath)
+
     logger.err({ err, fsPath }, 'problem writing file locally')
     throw new WriteError({
       message: 'problem writing file locally',
@@ -45,7 +47,16 @@ async function deleteFile(fsPath) {
     return
   }
   logger.log({ fsPath }, 'removing local temp file')
-  await promisify(fs.unlink)(fsPath)
+  try {
+    await promisify(fs.unlink)(fsPath)
+  } catch (err) {
+    if (err.code !== 'ENOENT') {
+      throw new WriteError({
+        message: 'failed to delete file',
+        info: { fsPath }
+      }).withCause(err)
+    }
+  }
 }
 
 function _getPath(key) {

services/filestore/npm-shrinkwrap.json

@@ -473,7 +473,7 @@
     "@sinonjs/text-encoding": {
       "version": "0.7.1",
       "resolved": "https://registry.npmjs.org/@sinonjs/text-encoding/-/text-encoding-0.7.1.tgz",
-      "integrity": "sha1-jaXGUwkVZT86Hzj9XxAdjD+AecU=",
+      "integrity": "sha512-+iTbntw2IZPb/anVDbypzfQa+ay64MW0Zo8aJ8gZPWMMK6/OubMVb6lUPMagqjOPnmtauXnFCACVl3O7ogjeqQ==",
       "dev": true
     },
     "@types/caseless": {
@@ -2189,14 +2189,14 @@
       "integrity": "sha1-uKLHAUu1zUFTTpg7XKFgo3RwhGk="
     },
     "glob": {
-      "version": "6.0.4",
-      "resolved": "https://registry.npmjs.org/glob/-/glob-6.0.4.tgz",
-      "integrity": "sha1-DwiGD2oVUSey+t1PnOJLGqtuTSI=",
-      "optional": true,
+      "version": "7.1.6",
+      "resolved": "https://registry.npmjs.org/glob/-/glob-7.1.6.tgz",
+      "integrity": "sha512-LwaxwyZ72Lk7vZINtNNrywX0ZuLyStrdDtabefZKAY5ZGJhVtgdznluResxNmPitE0SAO+O26sWTHeKSI2wMBA==",
       "requires": {
+        "fs.realpath": "^1.0.0",
         "inflight": "^1.0.4",
         "inherits": "2",
-        "minimatch": "2 || 3",
+        "minimatch": "^3.0.4",
         "once": "^1.3.0",
         "path-is-absolute": "^1.0.0"
       }
@@ -2671,7 +2671,7 @@
     "just-extend": {
       "version": "4.0.2",
       "resolved": "https://registry.npmjs.org/just-extend/-/just-extend-4.0.2.tgz",
-      "integrity": "sha1-8/R/ffyg+YnFVBCn68iFSwcQivw=",
+      "integrity": "sha512-FrLwOgm+iXrPV+5zDU6Jqu4gCRXbWEQg2O3SKONsWE4w7AXFRkryS53bpWdaL9cNol+AmR3AEYz6kn+o0fCPnw==",
       "dev": true
     },
     "jwa": {
@@ -3259,6 +3259,21 @@
           "optional": true,
           "requires": {
             "glob": "^6.0.1"
+          },
+          "dependencies": {
+            "glob": {
+              "version": "6.0.4",
+              "resolved": "https://registry.npmjs.org/glob/-/glob-6.0.4.tgz",
+              "integrity": "sha1-DwiGD2oVUSey+t1PnOJLGqtuTSI=",
+              "optional": true,
+              "requires": {
+                "inflight": "^1.0.4",
+                "inherits": "2",
+                "minimatch": "2 || 3",
+                "once": "^1.3.0",
+                "path-is-absolute": "^1.0.0"
+              }
+            }
           }
         }
       }

services/filestore/package.json

@@ -26,6 +26,7 @@
     "body-parser": "^1.2.0",
     "express": "^4.2.0",
     "fs-extra": "^1.0.0",
+    "glob": "^7.1.6",
     "heapdump": "^0.3.2",
     "knox": "~0.9.1",
     "logger-sharelatex": "^1.7.0",

services/filestore/test/acceptance/js/FilestoreTests.js

@@ -56,6 +56,7 @@ if (process.env.AWS_ACCESS_KEY_ID) {
 describe('Filestore', function() {
   this.timeout(1000 * 10)
   const filestoreUrl = `http://localhost:${Settings.internal.filestore.port}`
+  const directoryName = 'directory'
 
   // redefine the test suite for every available backend
   Object.keys(BackendSettings).forEach(backend => {
@@ -113,11 +114,11 @@ describe('Filestore', function() {
 
         beforeEach(async function() {
           fileId = Math.random()
-          fileUrl = `${filestoreUrl}/project/acceptance_tests/file/${fileId}`
+          fileUrl = `${filestoreUrl}/project/acceptance_tests/file/${directoryName}%2F${fileId}`
 
           const writeStream = request.post(fileUrl)
           const readStream = fs.createReadStream(localFileReadPath)
-          // consume the result to ensure the http request has been fully processed
+          // hack to consume the result to ensure the http request has been fully processed
           const resultStream = fs.createWriteStream('/dev/null')
           await pipeline(readStream, writeStream, resultStream)
         })
@@ -176,14 +177,14 @@ describe('Filestore', function() {
         it('should be able to copy files', async function() {
           const newProjectID = 'acceptance_tests_copyied_project'
           const newFileId = Math.random()
-          const newFileUrl = `${filestoreUrl}/project/${newProjectID}/file/${newFileId}`
+          const newFileUrl = `${filestoreUrl}/project/${newProjectID}/file/${directoryName}%2F${newFileId}`
           const opts = {
             method: 'put',
             uri: newFileUrl,
             json: {
               source: {
                 project_id: 'acceptance_tests',
-                file_id: fileId
+                file_id: `${directoryName}/${fileId}`
               }
             }
           }
@@ -223,6 +224,70 @@ describe('Filestore', function() {
         }
       })
 
+      describe('with multiple files', function() {
+        let fileIds, fileUrls, project
+        const directoryName = 'directory'
+        const localFileReadPaths = [
+          '/tmp/filestore_acceptance_tests_file_read_1.txt',
+          '/tmp/filestore_acceptance_tests_file_read_2.txt'
+        ]
+        const constantFileContents = [
+          [
+            'hello world',
+            `line 2 goes here ${Math.random()}`,
+            'there are 3 lines in all'
+          ].join('\n'),
+          [
+            `for reference: ${Math.random()}`,
+            'cats are the best animals',
+            'wombats are a close second'
+          ].join('\n')
+        ]
+
+        before(async function() {
+          return Promise.all([
+            fsWriteFile(localFileReadPaths[0], constantFileContents[0]),
+            fsWriteFile(localFileReadPaths[1], constantFileContents[1])
+          ])
+        })
+
+        beforeEach(async function() {
+          project = `acceptance_tests_${Math.random()}`
+          fileIds = [Math.random(), Math.random()]
+          fileUrls = [
+            `${filestoreUrl}/project/${project}/file/${directoryName}%2F${fileIds[0]}`,
+            `${filestoreUrl}/project/${project}/file/${directoryName}%2F${fileIds[1]}`
+          ]
+
+          const writeStreams = [
+            request.post(fileUrls[0]),
+            request.post(fileUrls[1])
+          ]
+          const readStreams = [
+            fs.createReadStream(localFileReadPaths[0]),
+            fs.createReadStream(localFileReadPaths[1])
+          ]
+          // hack to consume the result to ensure the http request has been fully processed
+          const resultStreams = [
+            fs.createWriteStream('/dev/null'),
+            fs.createWriteStream('/dev/null')
+          ]
+          return Promise.all([
+            pipeline(readStreams[0], writeStreams[0], resultStreams[0]),
+            pipeline(readStreams[1], writeStreams[1], resultStreams[1])
+          ])
+        })
+
+        it('should get the directory size', async function() {
+          const response = await rp.get(
+            `${filestoreUrl}/project/${project}/size`
+          )
+          expect(parseInt(JSON.parse(response.body)['total bytes'])).to.equal(
+            constantFileContents[0].length + constantFileContents[1].length
+          )
+        })
+      })
+
       describe('with a pdf file', function() {
         let fileId, fileUrl, localFileSize
         const localFileReadPath = Path.resolve(
@@ -232,7 +297,7 @@ describe('Filestore', function() {
 
         beforeEach(async function() {
           fileId = Math.random()
-          fileUrl = `${filestoreUrl}/project/acceptance_tests/file/${fileId}`
+          fileUrl = `${filestoreUrl}/project/acceptance_tests/file/${directoryName}%2F${fileId}`
           const stat = await fsStat(localFileReadPath)
           localFileSize = stat.size
           const writeStream = request.post(fileUrl)

services/filestore/test/unit/js/FSPersistorManagerTests.js

@@ -1,502 +1,328 @@
-/* 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
- * DS207: Consider shorter variations of null checks
- * 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
 const { expect } = chai
-const modulePath = '../../../app/js/FSPersistorManager.js'
 const SandboxedModule = require('sandboxed-module')
-const fs = require('fs')
-const response = require('response')
+const Errors = require('../../../app/js/Errors')
+
+chai.use(require('sinon-chai'))
+chai.use(require('chai-as-promised'))
+
+const modulePath = '../../../app/js/FSPersistorManager.js'
 
 describe('FSPersistorManagerTests', function() {
+  const stat = { size: 4, isFile: sinon.stub().returns(true) }
+  const fd = 1234
+  const readStream = 'readStream'
+  const writeStream = 'writeStream'
+  const remoteStream = 'remoteStream'
+  const tempFile = '/tmp/potato.txt'
+  const location = '/foo'
+  const error = new Error('guru meditation error')
+
+  const files = ['animals/wombat.tex', 'vegetables/potato.tex']
+  const globs = [`${location}/${files[0]}`, `${location}/${files[1]}`]
+  const filteredFilenames = ['animals_wombat.tex', 'vegetables_potato.tex']
+  let fs, rimraf, stream, LocalFileWriter, FSPersistorManager, glob
+
   beforeEach(function() {
-    this.Fs = {
-      rename: sinon.stub(),
-      createReadStream: sinon.stub(),
-      createWriteStream: sinon.stub(),
-      unlink: sinon.stub(),
-      rmdir: sinon.stub(),
-      exists: sinon.stub(),
-      readdir: sinon.stub(),
-      open: sinon.stub(),
-      openSync: sinon.stub(),
-      fstatSync: sinon.stub(),
-      closeSync: sinon.stub(),
-      stat: sinon.stub()
+    fs = {
+      createReadStream: sinon.stub().returns(readStream),
+      createWriteStream: sinon.stub().returns(writeStream),
+      unlink: sinon.stub().yields(),
+      open: sinon.stub().yields(null, fd),
+      stat: sinon.stub().yields(null, stat)
     }
-    this.Rimraf = sinon.stub()
-    this.LocalFileWriter = {
-      writeStream: sinon.stub(),
-      deleteFile: sinon.stub()
+    glob = sinon.stub().yields(null, globs)
+    rimraf = sinon.stub().yields()
+    stream = { pipeline: sinon.stub().yields() }
+    LocalFileWriter = {
+      promises: {
+        writeStream: sinon.stub().resolves(tempFile),
+        deleteFile: sinon.stub().resolves()
+      }
     }
-    this.requires = {
-      './LocalFileWriter': this.LocalFileWriter,
-      fs: this.Fs,
-      'logger-sharelatex': {
-        log() {},
-        err() {}
+    FSPersistorManager = SandboxedModule.require(modulePath, {
+      requires: {
+        './LocalFileWriter': LocalFileWriter,
+        'logger-sharelatex': {
+          log() {},
+          err() {}
+        },
+        './Errors': Errors,
+        fs,
+        glob,
+        rimraf,
+        stream
       },
-      response: response,
-      rimraf: this.Rimraf,
-      './Errors': (this.Errors = { NotFoundError: sinon.stub() })
-    }
-    this.location = '/tmp'
-    this.name1 = '530f2407e7ef165704000007/530f838b46d9a9e859000008'
-    this.name1Filtered = '530f2407e7ef165704000007_530f838b46d9a9e859000008'
-    this.name2 = 'second_file'
-    this.error = 'error_message'
-    return (this.FSPersistorManager = SandboxedModule.require(modulePath, {
-      requires: this.requires
-    }))
+      globals: { console }
+    })
   })
 
   describe('sendFile', function() {
-    beforeEach(function() {
-      return (this.Fs.createReadStream = sinon.stub().returns({
-        on() {},
-        pipe() {}
-      }))
-    })
-
-    it('should copy the file', function(done) {
-      this.Fs.createWriteStream = sinon.stub().returns({
-        on(event, handler) {
-          if (event === 'finish') {
-            return process.nextTick(handler)
-          }
-        }
-      })
-      return this.FSPersistorManager.sendFile(
-        this.location,
-        this.name1,
-        this.name2,
-        err => {
-          this.Fs.createReadStream.calledWith(this.name2).should.equal(true)
-          this.Fs.createWriteStream
-            .calledWith(`${this.location}/${this.name1Filtered}`)
-            .should.equal(true)
-          return done()
-        }
+    const localFilesystemPath = '/path/to/local/file'
+    it('should copy the file', async function() {
+      await FSPersistorManager.promises.sendFile(
+        location,
+        files[0],
+        localFilesystemPath
       )
+      expect(fs.createReadStream).to.have.been.calledWith(localFilesystemPath)
+      expect(fs.createWriteStream).to.have.been.calledWith(
+        `${location}/${filteredFilenames[0]}`
+      )
+      expect(stream.pipeline).to.have.been.calledWith(readStream, writeStream)
     })
 
-    return it('should return an error if the file cannot be stored', function(done) {
-      this.Fs.createWriteStream = sinon.stub().returns({
-        on: (event, handler) => {
-          if (event === 'error') {
-            return process.nextTick(() => {
-              return handler(this.error)
-            })
-          }
-        }
-      })
-      return this.FSPersistorManager.sendFile(
-        this.location,
-        this.name1,
-        this.name2,
-        err => {
-          this.Fs.createReadStream.calledWith(this.name2).should.equal(true)
-          this.Fs.createWriteStream
-            .calledWith(`${this.location}/${this.name1Filtered}`)
-            .should.equal(true)
-          err.should.equal(this.error)
-          return done()
-        }
-      )
+    it('should return an error if the file cannot be stored', async function() {
+      stream.pipeline.yields(error)
+      await expect(
+        FSPersistorManager.promises.sendFile(
+          location,
+          files[0],
+          localFilesystemPath
+        )
+      ).to.eventually.be.rejected.and.have.property('cause', error)
     })
   })
 
   describe('sendStream', function() {
-    beforeEach(function() {
-      this.FSPersistorManager.sendFile = sinon.stub().callsArgWith(3)
-      this.LocalFileWriter.writeStream.callsArgWith(2, null, this.name1)
-      this.LocalFileWriter.deleteFile.callsArg(1)
-      return (this.SourceStream = { on() {} })
+    it('should send the stream to LocalFileWriter', async function() {
+      await FSPersistorManager.promises.sendStream(
+        location,
+        files[0],
+        remoteStream
+      )
+      expect(LocalFileWriter.promises.writeStream).to.have.been.calledWith(
+        remoteStream
+      )
     })
 
-    it('should sent stream to LocalFileWriter', function(done) {
-      return this.FSPersistorManager.sendStream(
-        this.location,
-        this.name1,
-        this.SourceStream,
-        () => {
-          this.LocalFileWriter.writeStream
-            .calledWith(this.SourceStream)
-            .should.equal(true)
-          return done()
-        }
+    it('should delete the temporary file', async function() {
+      await FSPersistorManager.promises.sendStream(
+        location,
+        files[0],
+        remoteStream
+      )
+      expect(LocalFileWriter.promises.deleteFile).to.have.been.calledWith(
+        tempFile
       )
     })
 
-    it('should return the error from LocalFileWriter', function(done) {
-      this.LocalFileWriter.writeStream.callsArgWith(2, this.error)
-      return this.FSPersistorManager.sendStream(
-        this.location,
-        this.name1,
-        this.SourceStream,
-        err => {
-          err.should.equal(this.error)
-          return done()
-        }
-      )
+    it('should return the error from LocalFileWriter', async function() {
+      LocalFileWriter.promises.writeStream.rejects(error)
+      await expect(
+        FSPersistorManager.promises.sendStream(location, files[0], remoteStream)
+      ).to.eventually.be.rejectedWith(error)
     })
 
-    return it('should send the file to the filestore', function(done) {
-      this.LocalFileWriter.writeStream.callsArgWith(2)
-      return this.FSPersistorManager.sendStream(
-        this.location,
-        this.name1,
-        this.SourceStream,
-        err => {
-          this.FSPersistorManager.sendFile.called.should.equal(true)
-          return done()
-        }
+    it('should send the temporary file to the filestore', async function() {
+      await FSPersistorManager.promises.sendStream(
+        location,
+        files[0],
+        remoteStream
       )
+      expect(fs.createReadStream).to.have.been.calledWith(tempFile)
     })
   })
 
   describe('getFileStream', function() {
-    beforeEach(function() {
-      return (this.opts = {})
-    })
-
-    it('should use correct file location', function(done) {
-      this.FSPersistorManager.getFileStream(
-        this.location,
-        this.name1,
-        this.opts,
-        (err, res) => {}
+    it('should use correct file location', async function() {
+      await FSPersistorManager.promises.getFileStream(location, files[0], {})
+      expect(fs.open).to.have.been.calledWith(
+        `${location}/${filteredFilenames[0]}`
       )
-      this.Fs.open
-        .calledWith(`${this.location}/${this.name1Filtered}`)
-        .should.equal(true)
-      return done()
     })
 
-    describe('with start and end options', function() {
-      beforeEach(function() {
-        this.fd = 2019
-        this.opts_in = { start: 0, end: 8 }
-        this.opts = { start: 0, end: 8, fd: this.fd }
-        return this.Fs.open.callsArgWith(2, null, this.fd)
+    it('should pass the options to createReadStream', async function() {
+      await FSPersistorManager.promises.getFileStream(location, files[0], {
+        start: 0,
+        end: 8
       })
-
-      return it('should pass the options to createReadStream', function(done) {
-        this.FSPersistorManager.getFileStream(
-          this.location,
-          this.name1,
-          this.opts_in,
-          (err, res) => {}
-        )
-        this.Fs.createReadStream.calledWith(null, this.opts).should.equal(true)
-        return done()
+      expect(fs.createReadStream).to.have.been.calledWith(null, {
+        start: 0,
+        end: 8,
+        fd
       })
     })
 
-    return describe('error conditions', function() {
-      describe('when the file does not exist', function() {
-        beforeEach(function() {
-          this.fakeCode = 'ENOENT'
-          const err = new Error()
-          err.code = this.fakeCode
-          return this.Fs.open.callsArgWith(2, err, null)
-        })
-
-        return it('should give a NotFoundError', function(done) {
-          return this.FSPersistorManager.getFileStream(
-            this.location,
-            this.name1,
-            this.opts,
-            (err, res) => {
-              expect(res).to.equal(null)
-              expect(err).to.not.equal(null)
-              expect(err instanceof this.Errors.NotFoundError).to.equal(true)
-              return done()
-            }
-          )
-        })
-      })
+    it('should give a NotFoundError if the file does not exist', async function() {
+      const err = new Error()
+      err.code = 'ENOENT'
+      fs.open.yields(err)
 
-      return describe('when some other error happens', function() {
-        beforeEach(function() {
-          this.fakeCode = 'SOMETHINGHORRIBLE'
-          const err = new Error()
-          err.code = this.fakeCode
-          return this.Fs.open.callsArgWith(2, err, null)
-        })
-
-        return it('should give an Error', function(done) {
-          return this.FSPersistorManager.getFileStream(
-            this.location,
-            this.name1,
-            this.opts,
-            (err, res) => {
-              expect(res).to.equal(null)
-              expect(err).to.not.equal(null)
-              expect(err instanceof Error).to.equal(true)
-              return done()
-            }
-          )
-        })
-      })
+      await expect(
+        FSPersistorManager.promises.getFileStream(location, files[0], {})
+      )
+        .to.eventually.be.rejected.and.be.an.instanceOf(Errors.NotFoundError)
+        .and.have.property('cause', err)
+    })
+
+    it('should wrap any other error', async function() {
+      fs.open.yields(error)
+      await expect(
+        FSPersistorManager.promises.getFileStream(location, files[0], {})
+      )
+        .to.eventually.be.rejectedWith('failed to open file for streaming')
+        .and.be.an.instanceOf(Errors.ReadError)
+        .and.have.property('cause', error)
     })
   })
 
   describe('getFileSize', function() {
-    it('should return the file size', function(done) {
-      const expectedFileSize = 75382
-      this.Fs.stat.yields(new Error('fs.stat got unexpected arguments'))
-      this.Fs.stat
-        .withArgs(`${this.location}/${this.name1Filtered}`)
-        .yields(null, { size: expectedFileSize })
-
-      return this.FSPersistorManager.getFileSize(
-        this.location,
-        this.name1,
-        (err, fileSize) => {
-          if (err != null) {
-            return done(err)
-          }
-          expect(fileSize).to.equal(expectedFileSize)
-          return done()
-        }
-      )
+    const badFilename = 'neenaw.tex'
+    const size = 65536
+    const noentError = new Error('not found')
+    noentError.code = 'ENOENT'
+
+    beforeEach(function() {
+      fs.stat
+        .yields(error)
+        .withArgs(`${location}/${filteredFilenames[0]}`)
+        .yields(null, { size })
+        .withArgs(`${location}/${badFilename}`)
+        .yields(noentError)
     })
 
-    it('should throw a NotFoundError if the file does not exist', function(done) {
-      const error = new Error()
-      error.code = 'ENOENT'
-      this.Fs.stat.yields(error)
-
-      return this.FSPersistorManager.getFileSize(
-        this.location,
-        this.name1,
-        (err, fileSize) => {
-          expect(err).to.be.instanceof(this.Errors.NotFoundError)
-          return done()
-        }
-      )
+    it('should return the file size', async function() {
+      expect(
+        await FSPersistorManager.promises.getFileSize(location, files[0])
+      ).to.equal(size)
     })
 
-    return it('should rethrow any other error', function(done) {
-      const error = new Error()
-      this.Fs.stat.yields(error)
-
-      return this.FSPersistorManager.getFileSize(
-        this.location,
-        this.name1,
-        (err, fileSize) => {
-          expect(err).to.equal(error)
-          return done()
-        }
-      )
+    it('should throw a NotFoundError if the file does not exist', async function() {
+      await expect(
+        FSPersistorManager.promises.getFileSize(location, badFilename)
+      ).to.eventually.be.rejected.and.be.an.instanceOf(Errors.NotFoundError)
     })
-  })
 
-  describe('copyFile', function() {
-    beforeEach(function() {
-      this.ReadStream = {
-        on() {},
-        pipe: sinon.stub()
-      }
-      this.WriteStream = { on() {} }
-      this.Fs.createReadStream.returns(this.ReadStream)
-      return this.Fs.createWriteStream.returns(this.WriteStream)
+    it('should wrap any other error', async function() {
+      await expect(FSPersistorManager.promises.getFileSize(location, 'raccoon'))
+        .to.eventually.be.rejected.and.be.an.instanceOf(Errors.ReadError)
+        .and.have.property('cause', error)
     })
+  })
 
-    it('Should open the source for reading', function(done) {
-      this.FSPersistorManager.copyFile(
-        this.location,
-        this.name1,
-        this.name2,
-        function() {}
+  describe('copyFile', function() {
+    it('Should open the source for reading', async function() {
+      await FSPersistorManager.promises.copyFile(location, files[0], files[1])
+      expect(fs.createReadStream).to.have.been.calledWith(
+        `${location}/${filteredFilenames[0]}`
       )
-      this.Fs.createReadStream
-        .calledWith(`${this.location}/${this.name1Filtered}`)
-        .should.equal(true)
-      return done()
     })
 
-    it('Should open the target for writing', function(done) {
-      this.FSPersistorManager.copyFile(
-        this.location,
-        this.name1,
-        this.name2,
-        function() {}
+    it('Should open the target for writing', async function() {
+      await FSPersistorManager.promises.copyFile(location, files[0], files[1])
+      expect(fs.createWriteStream).to.have.been.calledWith(
+        `${location}/${filteredFilenames[1]}`
       )
-      this.Fs.createWriteStream
-        .calledWith(`${this.location}/${this.name2}`)
-        .should.equal(true)
-      return done()
     })
 
-    return it('Should pipe the source to the target', function(done) {
-      this.FSPersistorManager.copyFile(
-        this.location,
-        this.name1,
-        this.name2,
-        function() {}
-      )
-      this.ReadStream.pipe.calledWith(this.WriteStream).should.equal(true)
-      return done()
+    it('Should pipe the source to the target', async function() {
+      await FSPersistorManager.promises.copyFile(location, files[0], files[1])
+      expect(stream.pipeline).to.have.been.calledWith(readStream, writeStream)
     })
   })
 
   describe('deleteFile', function() {
-    beforeEach(function() {
-      return this.Fs.unlink.callsArgWith(1, this.error)
-    })
-
-    it('Should call unlink with correct options', function(done) {
-      return this.FSPersistorManager.deleteFile(
-        this.location,
-        this.name1,
-        err => {
-          this.Fs.unlink
-            .calledWith(`${this.location}/${this.name1Filtered}`)
-            .should.equal(true)
-          return done()
-        }
+    it('Should call unlink with correct options', async function() {
+      await FSPersistorManager.promises.deleteFile(location, files[0])
+      expect(fs.unlink).to.have.been.calledWith(
+        `${location}/${filteredFilenames[0]}`
       )
     })
 
-    return it('Should propogate the error', function(done) {
-      return this.FSPersistorManager.deleteFile(
-        this.location,
-        this.name1,
-        err => {
-          err.should.equal(this.error)
-          return done()
-        }
-      )
+    it('Should propagate the error', async function() {
+      fs.unlink.yields(error)
+      await expect(
+        FSPersistorManager.promises.deleteFile(location, files[0])
+      ).to.eventually.be.rejected.and.have.property('cause', error)
     })
   })
 
   describe('deleteDirectory', function() {
-    beforeEach(function() {
-      return this.Rimraf.callsArgWith(1, this.error)
-    })
-
-    it('Should call rmdir(rimraf) with correct options', function(done) {
-      return this.FSPersistorManager.deleteDirectory(
-        this.location,
-        this.name1,
-        err => {
-          this.Rimraf.calledWith(
-            `${this.location}/${this.name1Filtered}`
-          ).should.equal(true)
-          return done()
-        }
+    it('Should call rmdir(rimraf) with correct options', async function() {
+      await FSPersistorManager.promises.deleteDirectory(location, files[0])
+      expect(rimraf).to.have.been.calledWith(
+        `${location}/${filteredFilenames[0]}`
       )
     })
 
-    return it('Should propogate the error', function(done) {
-      return this.FSPersistorManager.deleteDirectory(
-        this.location,
-        this.name1,
-        err => {
-          err.should.equal(this.error)
-          return done()
-        }
-      )
+    it('Should propagate the error', async function() {
+      rimraf.yields(error)
+      await expect(
+        FSPersistorManager.promises.deleteDirectory(location, files[0])
+      ).to.eventually.be.rejected.and.have.property('cause', error)
     })
   })
 
   describe('checkIfFileExists', function() {
+    const badFilename = 'pototo'
+    const noentError = new Error('not found')
+    noentError.code = 'ENOENT'
+
     beforeEach(function() {
-      return this.Fs.exists.callsArgWith(1, true)
+      fs.stat
+        .yields(error)
+        .withArgs(`${location}/${filteredFilenames[0]}`)
+        .yields(null, {})
+        .withArgs(`${location}/${badFilename}`)
+        .yields(noentError)
     })
 
-    it('Should call exists with correct options', function(done) {
-      return this.FSPersistorManager.checkIfFileExists(
-        this.location,
-        this.name1,
-        exists => {
-          this.Fs.exists
-            .calledWith(`${this.location}/${this.name1Filtered}`)
-            .should.equal(true)
-          return done()
-        }
+    it('Should call stat with correct options', async function() {
+      await FSPersistorManager.promises.checkIfFileExists(location, files[0])
+      expect(fs.stat).to.have.been.calledWith(
+        `${location}/${filteredFilenames[0]}`
       )
     })
 
-    // fs.exists simply returns false on any error, so...
-    it('should not return an error', function(done) {
-      return this.FSPersistorManager.checkIfFileExists(
-        this.location,
-        this.name1,
-        (err, exists) => {
-          expect(err).to.be.null
-          return done()
-        }
-      )
+    it('Should return true for existing files', async function() {
+      expect(
+        await FSPersistorManager.promises.checkIfFileExists(location, files[0])
+      ).to.equal(true)
     })
 
-    it('Should return true for existing files', function(done) {
-      this.Fs.exists.callsArgWith(1, true)
-      return this.FSPersistorManager.checkIfFileExists(
-        this.location,
-        this.name1,
-        (err, exists) => {
-          exists.should.be.true
-          return done()
-        }
-      )
+    it('Should return false for non-existing files', async function() {
+      expect(
+        await FSPersistorManager.promises.checkIfFileExists(
+          location,
+          badFilename
+        )
+      ).to.equal(false)
     })
 
-    return it('Should return false for non-existing files', function(done) {
-      this.Fs.exists.callsArgWith(1, false)
-      return this.FSPersistorManager.checkIfFileExists(
-        this.location,
-        this.name1,
-        (err, exists) => {
-          exists.should.be.false
-          return done()
-        }
+    it('should wrap the error if there is a problem', async function() {
+      await expect(
+        FSPersistorManager.promises.checkIfFileExists(location, 'llama')
       )
+        .to.eventually.be.rejected.and.be.an.instanceOf(Errors.ReadError)
+        .and.have.property('cause', error)
     })
   })
 
-  return describe('directorySize', function() {
-    it('should propogate the error', function(done) {
-      this.Fs.readdir.callsArgWith(1, this.error)
-      return this.FSPersistorManager.directorySize(
-        this.location,
-        this.name1,
-        (err, totalsize) => {
-          err.should.equal(this.error)
-          return done()
-        }
+  describe('directorySize', function() {
+    it('should wrap the error', async function() {
+      glob.yields(error)
+      await expect(
+        FSPersistorManager.promises.directorySize(location, files[0])
       )
+        .to.eventually.be.rejected.and.be.an.instanceOf(Errors.ReadError)
+        .and.include({ cause: error })
+        .and.have.property('info')
+        .which.includes({ location, name: files[0] })
     })
 
-    return it('should sum directory files size', function(done) {
-      this.Fs.readdir.callsArgWith(1, null, [
-        { file1: 'file1' },
-        { file2: 'file2' }
-      ])
-      this.Fs.fstatSync.returns({ size: 1024 })
-      return this.FSPersistorManager.directorySize(
-        this.location,
-        this.name1,
-        (err, totalsize) => {
-          expect(totalsize).to.equal(2048)
-          return done()
-        }
+    it('should filter the directory name', async function() {
+      await FSPersistorManager.promises.directorySize(location, files[0])
+      expect(glob).to.have.been.calledWith(
+        `${location}/${filteredFilenames[0]}_*`
       )
     })
+
+    it('should sum directory files size', async function() {
+      expect(
+        await FSPersistorManager.promises.directorySize(location, files[0])
+      ).to.equal(stat.size * files.length)
+    })
   })
 })

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

@@ -49,6 +49,26 @@ describe('LocalFileWriter', function() {
         done()
       })
     })
+
+    describe('when there is an error', function() {
+      const error = new Error('not enough ketchup')
+      beforeEach(function() {
+        stream.pipeline.yields(error)
+      })
+
+      it('should wrap the error', function() {
+        LocalFileWriter.writeStream(readStream, filename, err => {
+          expect(err).to.exist
+          expect(err.cause).to.equal(error)
+        })
+      })
+
+      it('should delete the temporary file', function() {
+        LocalFileWriter.writeStream(readStream, filename, () => {
+          expect(fs.unlink).to.have.been.calledWith(fsPath)
+        })
+      })
+    })
   })
 
   describe('deleteFile', function() {
@@ -60,18 +80,30 @@ describe('LocalFileWriter', function() {
       })
     })
 
-    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
+    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()
       })
     })
 
-    it('should not call unlink with an empty path', function(done) {
-      LocalFileWriter.deleteFile('', err => {
+    it('should not throw a error if the file does not exist', function(done) {
+      const error = new Error('file not found')
+      error.code = 'ENOENT'
+      fs.unlink = sinon.stub().yields(error)
+      LocalFileWriter.deleteFile(fsPath, err => {
         expect(err).not.to.exist
-        expect(fs.unlink).not.to.have.been.called
+        done()
+      })
+    })
+
+    it('should wrap the error', function(done) {
+      const error = new Error('failed to reticulate splines')
+      fs.unlink = sinon.stub().yields(error)
+      LocalFileWriter.deleteFile(fsPath, err => {
+        expect(err).to.exist
+        expect(err.cause).to.equal(error)
         done()
       })
     })