Merge pull request #1877 from overleaf/em-filestore-range-request

 Get file size before truncating files for preview

GitOrigin-RevId: 0822691d75bd8bfe3d6cfd23f9ca4b1c3be20585

services/web/app/src/Features/FileStore/FileStoreController.js

@@ -1,76 +1,90 @@
-/* eslint-disable
-    camelcase,
-    max-len,
-    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 logger = require('logger-sharelatex')
+
 const FileStoreHandler = require('./FileStoreHandler')
 const ProjectLocator = require('../Project/ProjectLocator')
-const _ = require('underscore')
-
-const is_mobile_safari = user_agent =>
-  user_agent &&
-  (user_agent.indexOf('iPhone') >= 0 || user_agent.indexOf('iPad') >= 0)
-
-const is_html = function(file) {
-  const ends_with = ext =>
-    file.name != null &&
-    file.name.length > ext.length &&
-    file.name.lastIndexOf(ext) === file.name.length - ext.length
-
-  return ends_with('.html') || ends_with('.htm') || ends_with('.xhtml')
-}
+const Errors = require('../Errors/Errors')
 
 module.exports = {
   getFile(req, res) {
-    const project_id = req.params.Project_id
-    const file_id = req.params.File_id
+    const projectId = req.params.Project_id
+    const fileId = req.params.File_id
     const queryString = req.query
-    const user_agent = req.get('User-Agent')
-    logger.log({ project_id, file_id, queryString }, 'file download')
-    return ProjectLocator.findElement(
-      { project_id, element_id: file_id, type: 'file' },
+    const userAgent = req.get('User-Agent')
+    logger.log({ projectId, fileId, queryString }, 'file download')
+    ProjectLocator.findElement(
+      { project_id: projectId, element_id: fileId, type: 'file' },
       function(err, file) {
-        if (err != null) {
+        if (err) {
           logger.err(
-            { err, project_id, file_id, queryString },
+            { err, projectId, fileId, queryString },
             'error finding element for downloading file'
           )
           return res.sendStatus(500)
         }
-        return FileStoreHandler.getFileStream(
-          project_id,
-          file_id,
-          queryString,
-          function(err, stream) {
-            if (err != null) {
-              logger.err(
-                { err, project_id, file_id, queryString },
-                'error getting file stream for downloading file'
-              )
-              return res.sendStatus(500)
-            }
-            // mobile safari will try to render html files, prevent this
-            if (is_mobile_safari(user_agent) && is_html(file)) {
-              logger.log(
-                { filename: file.name, user_agent },
-                'sending html file to mobile-safari as plain text'
-              )
-              res.setHeader('Content-Type', 'text/plain')
-            }
-            res.setContentDisposition('attachment', { filename: file.name })
-            return stream.pipe(res)
+        FileStoreHandler.getFileStream(projectId, fileId, queryString, function(
+          err,
+          stream
+        ) {
+          if (err) {
+            logger.err(
+              { err, projectId, fileId, queryString },
+              'error getting file stream for downloading file'
+            )
+            return res.sendStatus(500)
+          }
+          // mobile safari will try to render html files, prevent this
+          if (isMobileSafari(userAgent) && isHtml(file)) {
+            logger.log(
+              { filename: file.name, userAgent },
+              'sending html file to mobile-safari as plain text'
+            )
+            res.setHeader('Content-Type', 'text/plain')
           }
-        )
+          res.setContentDisposition('attachment', { filename: file.name })
+          stream.pipe(res)
+        })
       }
     )
+  },
+
+  getFileHead(req, res) {
+    const projectId = req.params.Project_id
+    const fileId = req.params.File_id
+    FileStoreHandler.getFileSize(projectId, fileId, (err, fileSize) => {
+      if (err) {
+        if (err instanceof Errors.NotFoundError) {
+          res.status(404).end()
+        } else {
+          logger.err({ err, projectId, fileId }, 'error getting file size')
+          res.status(500).end()
+        }
+        return
+      }
+      res.set('Content-Length', fileSize)
+      res.status(200).end()
+    })
   }
 }
+
+function isHtml(file) {
+  return (
+    fileEndsWith(file, '.html') ||
+    fileEndsWith(file, '.htm') ||
+    fileEndsWith(file, '.xhtml')
+  )
+}
+
+function fileEndsWith(file, ext) {
+  return (
+    file.name != null &&
+    file.name.length > ext.length &&
+    file.name.lastIndexOf(ext) === file.name.length - ext.length
+  )
+}
+
+function isMobileSafari(userAgent) {
+  return (
+    userAgent &&
+    (userAgent.indexOf('iPhone') >= 0 || userAgent.indexOf('iPad') >= 0)
+  )
+}

services/web/app/src/Features/FileStore/FileStoreHandler.js

@@ -1,18 +1,4 @@
-/* eslint-disable
-    camelcase,
-    handle-callback-err,
-    max-len,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS101: Remove unnecessary use of Array.from
- * 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 FileStoreHandler
+const _ = require('underscore')
 const logger = require('logger-sharelatex')
 const fs = require('fs')
 const request = require('request')
@@ -20,95 +6,84 @@ const settings = require('settings-sharelatex')
 const Async = require('async')
 const FileHashManager = require('./FileHashManager')
 const { File } = require('../../models/File')
+const Errors = require('../Errors/Errors')
 
-const oneMinInMs = 60 * 1000
-const fiveMinsInMs = oneMinInMs * 5
+const ONE_MIN_IN_MS = 60 * 1000
+const FIVE_MINS_IN_MS = ONE_MIN_IN_MS * 5
 
-module.exports = FileStoreHandler = {
+const FileStoreHandler = {
   RETRY_ATTEMPTS: 3,
 
-  uploadFileFromDisk(project_id, file_args, fsPath, callback) {
-    if (callback == null) {
-      callback = function(error, url, fileRef) {}
-    }
-    return fs.lstat(fsPath, function(err, stat) {
-      if (err != null) {
-        logger.err({ err, project_id, file_args, fsPath }, 'error stating file')
+  uploadFileFromDisk(projectId, fileArgs, fsPath, callback) {
+    fs.lstat(fsPath, function(err, stat) {
+      if (err) {
+        logger.err({ err, projectId, fileArgs, fsPath }, 'error stating file')
         callback(err)
       }
-      if (stat == null) {
+      if (!stat) {
         logger.err(
-          { project_id, file_args, fsPath },
+          { projectId, fileArgs, fsPath },
           'stat is not available, can not check file from disk'
         )
         return callback(new Error('error getting stat, not available'))
       }
       if (!stat.isFile()) {
         logger.log(
-          { project_id, file_args, fsPath },
-          'tried to upload symlink, not contining'
+          { projectId, fileArgs, fsPath },
+          'tried to upload symlink, not continuing'
         )
         return callback(new Error('can not upload symlink'))
       }
-      return Async.retry(
+      Async.retry(
         FileStoreHandler.RETRY_ATTEMPTS,
         (cb, results) =>
           FileStoreHandler._doUploadFileFromDisk(
-            project_id,
-            file_args,
+            projectId,
+            fileArgs,
             fsPath,
             cb
           ),
         function(err, result) {
-          if (err != null) {
+          if (err) {
             logger.err(
-              { err, project_id, file_args },
+              { err, projectId, fileArgs },
               'Error uploading file, retries failed'
             )
             return callback(err)
           }
-          return callback(err, result.url, result.fileRef)
+          callback(err, result.url, result.fileRef)
         }
       )
     })
   },
 
-  _doUploadFileFromDisk(project_id, file_args, fsPath, callback) {
-    if (callback == null) {
-      callback = function(err, result) {}
-    }
-    const _cb = callback
-    callback = function(err, ...result) {
-      callback = function() {} // avoid double callbacks
-      return _cb(err, ...Array.from(result))
-    }
+  _doUploadFileFromDisk(projectId, fileArgs, fsPath, callback) {
+    const callbackOnce = _.once(callback)
 
-    return FileHashManager.computeHash(fsPath, function(err, hashValue) {
-      if (err != null) {
-        return callback(err)
+    FileHashManager.computeHash(fsPath, function(err, hashValue) {
+      if (err) {
+        return callbackOnce(err)
       }
-      const fileRef = new File(
-        Object.assign({}, file_args, { hash: hashValue })
-      )
-      const file_id = fileRef._id
+      const fileRef = new File(Object.assign({}, fileArgs, { hash: hashValue }))
+      const fileId = fileRef._id
       logger.log(
-        { project_id, file_id, fsPath, hash: hashValue, fileRef },
+        { projectId, fileId, fsPath, hash: hashValue, fileRef },
         'uploading file from disk'
       )
       const readStream = fs.createReadStream(fsPath)
       readStream.on('error', function(err) {
         logger.err(
-          { err, project_id, file_id, fsPath },
+          { err, projectId, fileId, fsPath },
           'something went wrong on the read stream of uploadFileFromDisk'
         )
-        return callback(err)
+        callbackOnce(err)
       })
-      return readStream.on('open', function() {
-        const url = FileStoreHandler._buildUrl(project_id, file_id)
+      readStream.on('open', function() {
+        const url = FileStoreHandler._buildUrl(projectId, fileId)
         const opts = {
           method: 'post',
           uri: url,
-          timeout: fiveMinsInMs,
+          timeout: FIVE_MINS_IN_MS,
           headers: {
             'X-File-Hash-From-Web': hashValue
           } // send the hash to the filestore as a custom header so it can be checked
@@ -116,10 +91,10 @@ module.exports = FileStoreHandler = {
         const writeStream = request(opts)
         writeStream.on('error', function(err) {
           logger.err(
-            { err, project_id, file_id, fsPath },
+            { err, projectId, fileId, fsPath },
             'something went wrong on the write stream of uploadFileFromDisk'
           )
-          return callback(err)
+          callbackOnce(err)
         })
         writeStream.on('response', function(response) {
           if (![200, 201].includes(response.statusCode)) {
@@ -132,19 +107,18 @@ module.exports = FileStoreHandler = {
               { err, statusCode: response.statusCode },
               'error uploading to filestore'
             )
-            return callback(err)
-          } else {
-            return callback(null, { url, fileRef })
+            return callbackOnce(err)
           }
+          callbackOnce(null, { url, fileRef })
         }) // have to pass back an object because async.retry only accepts a single result argument
-        return readStream.pipe(writeStream)
+        readStream.pipe(writeStream)
       })
     })
   },
 
-  getFileStream(project_id, file_id, query, callback) {
+  getFileStream(projectId, fileId, query, callback) {
     logger.log(
-      { project_id, file_id, query },
+      { projectId, fileId, query },
       'getting file stream from file store'
     )
     let queryString = ''
@@ -153,8 +127,8 @@ module.exports = FileStoreHandler = {
     }
     const opts = {
       method: 'get',
-      uri: `${this._buildUrl(project_id, file_id)}${queryString}`,
-      timeout: fiveMinsInMs,
+      uri: `${this._buildUrl(projectId, fileId)}${queryString}`,
+      timeout: FIVE_MINS_IN_MS,
       headers: {}
     }
     if (query != null && query['range'] != null) {
@@ -166,24 +140,49 @@ module.exports = FileStoreHandler = {
     const readStream = request(opts)
     readStream.on('error', err =>
       logger.err(
-        { err, project_id, file_id, query, opts },
+        { err, projectId, fileId, query, opts },
         'error in file stream'
       )
     )
     return callback(null, readStream)
   },
 
-  deleteFile(project_id, file_id, callback) {
-    logger.log({ project_id, file_id }, 'telling file store to delete file')
+  getFileSize(projectId, fileId, callback) {
+    const url = this._buildUrl(projectId, fileId)
+    request.head(url, (err, res) => {
+      if (err) {
+        logger.err(
+          { err, projectId, fileId },
+          'failed to get file size from filestore'
+        )
+        return callback(err)
+      }
+      if (res.statusCode === 404) {
+        return callback(new Errors.NotFoundError('file not found in filestore'))
+      }
+      if (res.statusCode !== 200) {
+        logger.err(
+          { projectId, fileId, statusCode: res.statusCode },
+          'filestore returned non-200 response'
+        )
+        return callback(new Error('filestore returned non-200 response'))
+      }
+      const fileSize = res.headers['content-length']
+      callback(null, fileSize)
+    })
+  },
+
+  deleteFile(projectId, fileId, callback) {
+    logger.log({ projectId, fileId }, 'telling file store to delete file')
     const opts = {
       method: 'delete',
-      uri: this._buildUrl(project_id, file_id),
-      timeout: fiveMinsInMs
+      uri: this._buildUrl(projectId, fileId),
+      timeout: FIVE_MINS_IN_MS
     }
     return request(opts, function(err, response) {
-      if (err != null) {
+      if (err) {
         logger.err(
-          { err, project_id, file_id },
+          { err, projectId, fileId },
           'something went wrong deleting file from filestore'
         )
       }
@@ -191,26 +190,26 @@ module.exports = FileStoreHandler = {
     })
   },
 
-  copyFile(oldProject_id, oldFile_id, newProject_id, newFile_id, callback) {
+  copyFile(oldProjectId, oldFileId, newProjectId, newFileId, callback) {
     logger.log(
-      { oldProject_id, oldFile_id, newProject_id, newFile_id },
+      { oldProjectId, oldFileId, newProjectId, newFileId },
       'telling filestore to copy a file'
     )
     const opts = {
       method: 'put',
       json: {
         source: {
-          project_id: oldProject_id,
-          file_id: oldFile_id
+          project_id: oldProjectId,
+          file_id: oldFileId
         }
       },
-      uri: this._buildUrl(newProject_id, newFile_id),
-      timeout: fiveMinsInMs
+      uri: this._buildUrl(newProjectId, newFileId),
+      timeout: FIVE_MINS_IN_MS
     }
     return request(opts, function(err, response) {
-      if (err != null) {
+      if (err) {
         logger.err(
-          { err, oldProject_id, oldFile_id, newProject_id, newFile_id },
+          { err, oldProjectId, oldFileId, newProjectId, newFileId },
           'something went wrong telling filestore api to copy file'
         )
         return callback(err)
@@ -230,9 +229,9 @@ module.exports = FileStoreHandler = {
     })
   },
 
-  _buildUrl(project_id, file_id) {
-    return `${
-      settings.apis.filestore.url
-    }/project/${project_id}/file/${file_id}`
+  _buildUrl(projectId, fileId) {
+    return `${settings.apis.filestore.url}/project/${projectId}/file/${fileId}`
   }
 }
+
+module.exports = FileStoreHandler

services/web/app/src/router.js

@@ -1,25 +1,8 @@
-/* eslint-disable
-    camelcase,
-    max-len,
-    no-return-assign,
-    no-unused-vars,
-    no-useless-escape,
-*/
-// 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 Router
 const AdminController = require('./Features/ServerAdmin/AdminController')
 const ErrorController = require('./Features/Errors/ErrorController')
 const ProjectController = require('./Features/Project/ProjectController')
 const ProjectApiController = require('./Features/Project/ProjectApiController')
 const SpellingController = require('./Features/Spelling/SpellingController')
-const EditorController = require('./Features/Editor/EditorController')
 const EditorRouter = require('./Features/Editor/EditorRouter')
 const Settings = require('settings-sharelatex')
 const TpdsController = require('./Features/ThirdPartyDataStore/TpdsController')
@@ -52,7 +35,6 @@ const ChatController = require('./Features/Chat/ChatController')
 const BlogController = require('./Features/Blog/BlogController')
 const Modules = require('./infrastructure/Modules')
 const RateLimiterMiddleware = require('./Features/Security/RateLimiterMiddleware')
-const CooldownMiddleware = require('./Features/Cooldown/CooldownMiddleware')
 const RealTimeProxyRouter = require('./Features/RealTimeProxy/RealTimeProxyRouter')
 const InactiveProjectController = require('./Features/InactiveData/InactiveProjectController')
 const ContactRouter = require('./Features/Contacts/ContactRouter')
@@ -74,7 +56,7 @@ const UserMembershipRouter = require('./Features/UserMembership/UserMembershipRo
 const logger = require('logger-sharelatex')
 const _ = require('underscore')
 
-module.exports = Router = class Router {
+module.exports = class Router {
   constructor(webRouter, privateApiRouter, publicApiRouter) {
     if (!Settings.allowPublicAccess) {
       webRouter.all('*', AuthenticationController.requireGlobalLogin)
@@ -295,6 +277,11 @@ module.exports = Router = class Router {
       AuthorizationMiddleware.ensureUserCanReadProject,
       ProjectController.loadEditor
     )
+    webRouter.head(
+      '/Project/:Project_id/file/:File_id',
+      AuthorizationMiddleware.ensureUserCanReadProject,
+      FileStoreController.getFileHead
+    )
     webRouter.get(
       '/Project/:Project_id/file/:File_id',
       AuthorizationMiddleware.ensureUserCanReadProject,
@@ -338,11 +325,11 @@ module.exports = Router = class Router {
 
     // PDF Download button
     webRouter.get(
-      /^\/download\/project\/([^\/]*)\/output\/output\.pdf$/,
+      /^\/download\/project\/([^/]*)\/output\/output\.pdf$/,
       function(req, res, next) {
         const params = { Project_id: req.params[0] }
         req.params = params
-        return next()
+        next()
       },
       AuthorizationMiddleware.ensureUserCanReadProject,
       CompileController.downloadPdf
@@ -350,14 +337,14 @@ module.exports = Router = class Router {
 
     // PDF Download button for specific build
     webRouter.get(
-      /^\/download\/project\/([^\/]*)\/build\/([0-9a-f-]+)\/output\/output\.pdf$/,
+      /^\/download\/project\/([^/]*)\/build\/([0-9a-f-]+)\/output\/output\.pdf$/,
       function(req, res, next) {
         const params = {
           Project_id: req.params[0],
           build_id: req.params[1]
         }
         req.params = params
-        return next()
+        next()
       },
       AuthorizationMiddleware.ensureUserCanReadProject,
       CompileController.downloadPdf
@@ -365,21 +352,21 @@ module.exports = Router = class Router {
 
     // Used by the pdf viewers
     webRouter.get(
-      /^\/project\/([^\/]*)\/output\/(.*)$/,
+      /^\/project\/([^/]*)\/output\/(.*)$/,
       function(req, res, next) {
         const params = {
           Project_id: req.params[0],
           file: req.params[1]
         }
         req.params = params
-        return next()
+        next()
       },
       AuthorizationMiddleware.ensureUserCanReadProject,
       CompileController.getFileFromClsi
     )
     // direct url access to output files for a specific build (query string not required)
     webRouter.get(
-      /^\/project\/([^\/]*)\/build\/([0-9a-f-]+)\/output\/(.*)$/,
+      /^\/project\/([^/]*)\/build\/([0-9a-f-]+)\/output\/(.*)$/,
       function(req, res, next) {
         const params = {
           Project_id: req.params[0],
@@ -387,7 +374,7 @@ module.exports = Router = class Router {
           file: req.params[2]
         }
         req.params = params
-        return next()
+        next()
       },
       AuthorizationMiddleware.ensureUserCanReadProject,
       CompileController.getFileFromClsi
@@ -395,7 +382,7 @@ module.exports = Router = class Router {
 
     // direct url access to output files for user but no build, to retrieve files when build fails
     webRouter.get(
-      /^\/project\/([^\/]*)\/user\/([0-9a-f-]+)\/output\/(.*)$/,
+      /^\/project\/([^/]*)\/user\/([0-9a-f-]+)\/output\/(.*)$/,
       function(req, res, next) {
         const params = {
           Project_id: req.params[0],
@@ -403,7 +390,7 @@ module.exports = Router = class Router {
           file: req.params[2]
         }
         req.params = params
-        return next()
+        next()
       },
       AuthorizationMiddleware.ensureUserCanReadProject,
       CompileController.getFileFromClsi
@@ -411,7 +398,7 @@ module.exports = Router = class Router {
 
     // direct url access to output files for a specific user and build (query string not required)
     webRouter.get(
-      /^\/project\/([^\/]*)\/user\/([0-9a-f]+)\/build\/([0-9a-f-]+)\/output\/(.*)$/,
+      /^\/project\/([^/]*)\/user\/([0-9a-f]+)\/build\/([0-9a-f-]+)\/output\/(.*)$/,
       function(req, res, next) {
         const params = {
           Project_id: req.params[0],
@@ -420,7 +407,7 @@ module.exports = Router = class Router {
           file: req.params[3]
         }
         req.params = params
-        return next()
+        next()
       },
       AuthorizationMiddleware.ensureUserCanReadProject,
       CompileController.getFileFromClsi
@@ -691,14 +678,14 @@ module.exports = Router = class Router {
     )
 
     webRouter.get(
-      /^\/internal\/project\/([^\/]*)\/output\/(.*)$/,
+      /^\/internal\/project\/([^/]*)\/output\/(.*)$/,
       function(req, res, next) {
         const params = {
           Project_id: req.params[0],
           file: req.params[1]
         }
         req.params = params
-        return next()
+        next()
       },
       AuthenticationController.httpAuth,
       CompileController.getFileFromClsi
@@ -826,7 +813,7 @@ module.exports = Router = class Router {
       CompileController.compileSubmission
     )
     publicApiRouter.get(
-      /^\/api\/clsi\/compile\/([^\/]*)\/build\/([0-9a-f-]+)\/output\/(.*)$/,
+      /^\/api\/clsi\/compile\/([^/]*)\/build\/([0-9a-f-]+)\/output\/(.*)$/,
       function(req, res, next) {
         const params = {
           submission_id: req.params[0],
@@ -834,7 +821,7 @@ module.exports = Router = class Router {
           file: req.params[2]
         }
         req.params = params
-        return next()
+        next()
       },
       AuthenticationController.httpAuth,
       CompileController.getFileFromClsiWithoutUser
@@ -853,9 +840,9 @@ module.exports = Router = class Router {
     webRouter.get('/chrome', function(req, res, next) {
       // Match v1 behaviour - this is used for a Chrome web app
       if (AuthenticationController.isUserLoggedIn(req)) {
-        return res.redirect('/project')
+        res.redirect('/project')
       } else {
-        return res.redirect('/register')
+        res.redirect('/register')
       }
     })
 
@@ -951,33 +938,33 @@ module.exports = Router = class Router {
       '/status/compiler/:Project_id',
       AuthorizationMiddleware.ensureUserCanReadProject,
       function(req, res) {
-        const project_id = req.params.Project_id
+        const projectId = req.params.Project_id
         const sendRes = _.once(function(statusCode, message) {
           res.status(statusCode)
           res.send(message)
-          return ClsiCookieManager.clearServerId(project_id)
+          ClsiCookieManager.clearServerId(projectId)
         }) // force every compile to a new server
         // set a timeout
         var handler = setTimeout(function() {
           sendRes(500, 'Compiler timed out')
-          return (handler = null)
+          handler = null
         }, 10000)
         // use a valid user id for testing
-        const test_user_id = '123456789012345678901234'
+        const testUserId = '123456789012345678901234'
         // run the compile
-        return CompileManager.compile(project_id, test_user_id, {}, function(
+        CompileManager.compile(projectId, testUserId, {}, function(
           error,
           status
         ) {
-          if (handler != null) {
+          if (handler) {
             clearTimeout(handler)
           }
-          if (error != null) {
-            return sendRes(500, `Compiler returned error ${error.message}`)
+          if (error) {
+            sendRes(500, `Compiler returned error ${error.message}`)
           } else if (status === 'success') {
-            return sendRes(200, 'Compiler returned in less than 10 seconds')
+            sendRes(200, 'Compiler returned in less than 10 seconds')
           } else {
-            return sendRes(500, `Compiler returned failure ${status}`)
+            sendRes(500, `Compiler returned failure ${status}`)
           }
         })
       }
@@ -985,7 +972,7 @@ module.exports = Router = class Router {
 
     webRouter.get('/no-cache', function(req, res, next) {
       res.header('Cache-Control', 'max-age=0')
-      return res.sendStatus(404)
+      res.sendStatus(404)
     })
 
     webRouter.get('/oops-express', (req, res, next) =>
@@ -1002,7 +989,7 @@ module.exports = Router = class Router {
 
     privateApiRouter.get('/opps-small', function(req, res, next) {
       logger.err('test error occured')
-      return res.send()
+      res.send()
     })
 
     webRouter.post('/error/client', function(req, res, next) {
@@ -1011,7 +998,7 @@ module.exports = Router = class Router {
         'client side error'
       )
       metrics.inc('client-side-error')
-      return res.sendStatus(204)
+      res.sendStatus(204)
     })
 
     webRouter.get(

services/web/public/src/ide/binary-files/controllers/BinaryFileController.js

@@ -1,19 +1,3 @@
-/* eslint-disable
-    camelcase,
-    max-len,
-    no-return-assign,
-    no-undef,
-    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
- * DS103: Rewrite code to no longer use __guard__
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 define(['base', 'moment'], (App, moment) =>
   App.controller('BinaryFileController', [
     '$scope',
@@ -24,25 +8,29 @@ define(['base', 'moment'], (App, moment) =>
     'ide',
     'waitFor',
     function($scope, $rootScope, $http, $timeout, $element, ide, waitFor) {
-      let loadTextFileFilePreview, setHeight
-      const TWO_MEGABYTES = 2 * 1024 * 1024
+      const MAX_FILE_SIZE = 2 * 1024 * 1024
+      const MAX_URL_LENGTH = 60
+      const FRONT_OF_URL_LENGTH = 35
+      const FILLER = '...'
+      const TAIL_OF_URL_LENGTH =
+        MAX_URL_LENGTH - FRONT_OF_URL_LENGTH - FILLER.length
 
       const textExtensions = ['bib', 'tex', 'txt', 'cls', 'sty']
       const imageExtensions = ['png', 'jpg', 'jpeg', 'gif']
       const previewableExtensions = []
 
       const extension = file =>
-        __guard__(file.name.split('.').pop(), x => x.toLowerCase())
+        file.name
+          .split('.')
+          .pop()
+          .toLowerCase()
 
-      $scope.isTextFile = () => {
-        return textExtensions.indexOf(extension($scope.openFile)) > -1
-      }
-      $scope.isImageFile = () => {
-        return imageExtensions.indexOf(extension($scope.openFile)) > -1
-      }
-      $scope.isPreviewableFile = () => {
-        return previewableExtensions.indexOf(extension($scope.openFile)) > -1
-      }
+      $scope.isTextFile = () =>
+        textExtensions.indexOf(extension($scope.openFile)) > -1
+      $scope.isImageFile = () =>
+        imageExtensions.indexOf(extension($scope.openFile)) > -1
+      $scope.isPreviewableFile = () =>
+        previewableExtensions.indexOf(extension($scope.openFile)) > -1
       $scope.isUnpreviewableFile = () =>
         !$scope.isTextFile() &&
         !$scope.isImageFile() &&
@@ -58,11 +46,6 @@ define(['base', 'moment'], (App, moment) =>
       $scope.refreshing = false
       $scope.refreshError = null
 
-      const MAX_URL_LENGTH = 60
-      const FRONT_OF_URL_LENGTH = 35
-      const FILLER = '...'
-      const TAIL_OF_URL_LENGTH =
-        MAX_URL_LENGTH - FRONT_OF_URL_LENGTH - FILLER.length
       $scope.displayUrl = function(url) {
         if (url == null) {
           return
@@ -71,23 +54,22 @@ define(['base', 'moment'], (App, moment) =>
           const front = url.slice(0, FRONT_OF_URL_LENGTH)
           const tail = url.slice(url.length - TAIL_OF_URL_LENGTH)
           return front + FILLER + tail
-        } else {
-          return url
         }
+        return url
       }
 
       $scope.refreshFile = function(file) {
         $scope.refreshing = true
         $scope.refreshError = null
-        return ide.fileTreeManager
+        ide.fileTreeManager
           .refreshLinkedFile(file)
           .then(function(response) {
             const { data } = response
-            const { new_file_id } = data
+            const newFileId = data.new_file_id
             $timeout(
               () =>
                 waitFor(
-                  () => ide.fileTreeManager.findEntityById(new_file_id),
+                  () => ide.fileTreeManager.findEntityById(newFileId),
                   5000
                 )
                   .then(newFile => ide.binaryFilesManager.openFile(newFile))
@@ -95,7 +77,7 @@ define(['base', 'moment'], (App, moment) =>
 
               0
             )
-            return ($scope.refreshError = null)
+            $scope.refreshError = null
           })
           .catch(response => ($scope.refreshError = response.data))
           .finally(() => {
@@ -116,7 +98,7 @@ define(['base', 'moment'], (App, moment) =>
       $scope.failedLoad = false
       window.sl_binaryFilePreviewError = () => {
         $scope.failedLoad = true
-        return $scope.$apply()
+        $scope.$apply()
       }
 
       // Callback fired when the `img` tag is done loading,
@@ -124,69 +106,92 @@ define(['base', 'moment'], (App, moment) =>
       $scope.imgLoaded = false
       window.sl_binaryFilePreviewLoaded = () => {
         $scope.imgLoaded = true
-        return $scope.$apply()
+        $scope.$apply()
       }
-      ;(loadTextFileFilePreview = function() {
-        if (!$scope.isTextFile()) {
-          return
+
+      if ($scope.isTextFile()) {
+        loadTextFilePreview()
+      }
+
+      function loadTextFilePreview() {
+        const url = `/project/${window.project_id}/file/${$scope.openFile.id}`
+        let truncated = false
+        displayPreviewLoading()
+        getFileSize(url)
+          .then(fileSize => {
+            const opts = {}
+            if (fileSize > MAX_FILE_SIZE) {
+              truncated = true
+              opts.maxSize = MAX_FILE_SIZE
+            }
+            return getFileContents(url, opts)
+          })
+          .then(contents => {
+            const displayedContents = truncated
+              ? truncateFileContents(contents)
+              : contents
+            displayPreview(displayedContents, truncated)
+          })
+          .catch(err => {
+            console.error(err)
+            displayPreviewError()
+          })
+      }
+
+      function getFileSize(url) {
+        return $http.head(url).then(response => {
+          const size = parseInt(response.headers('Content-Length'), 10)
+          if (isNaN(size)) {
+            throw new Error('Could not parse Content-Length header')
+          }
+          return size
+        })
+      }
+
+      function getFileContents(url, opts = {}) {
+        const { maxSize } = opts
+        if (maxSize != null) {
+          url += `?range=0-${maxSize}`
         }
-        const url = `/project/${project_id}/file/${
-          $scope.openFile.id
-        }?range=0-${TWO_MEGABYTES}`
+        return $http
+          .get(url, {
+            transformResponse: null // Don't parse JSON
+          })
+          .then(response => {
+            return response.data
+          })
+      }
+
+      function truncateFileContents(contents) {
+        return contents.replace(/\n.*$/, '')
+      }
+
+      function displayPreviewLoading() {
         $scope.textPreview.data = null
         $scope.textPreview.loading = true
         $scope.textPreview.shouldShowDots = false
         $scope.$apply()
-        return $http({
-          url,
-          method: 'GET',
-          transformResponse: null // Don't parse JSON
-        })
-          .then(function(response) {
-            let { data } = response
-            $scope.textPreview.error = false
-            // show dots when payload is closs to cutoff
-            if (data.length >= TWO_MEGABYTES - 200) {
-              $scope.textPreview.shouldShowDots = true
-              // remove last partial line
-              data = __guardMethod__(data, 'replace', o =>
-                o.replace(/\n.*$/, '')
-              )
-            }
-            $scope.textPreview.data = data
-            return $timeout(setHeight, 0)
-          })
-          .catch(function(error) {
-            console.error(error)
-            $scope.textPreview.error = true
-            return ($scope.textPreview.loading = false)
-          })
-      })()
+      }
 
-      return (setHeight = function() {
+      function displayPreview(contents, truncated) {
+        $scope.textPreview.error = false
+        $scope.textPreview.data = contents
+        $scope.textPreview.shouldShowDots = truncated
+        $timeout(setPreviewHeight, 0)
+      }
+
+      function displayPreviewError() {
+        $scope.textPreview.error = true
+        $scope.textPreview.loading = false
+      }
+
+      function setPreviewHeight() {
         const $preview = $element.find('.text-preview .scroll-container')
         const $footer = $element.find('.binary-file-footer')
         const maxHeight = $element.height() - $footer.height() - 14 // borders + margin
         $preview.css({ 'max-height': maxHeight })
         // Don't show the preview until we've set the height, otherwise we jump around
-        return ($scope.textPreview.loading = false)
-      })
+        $scope.textPreview.loading = false
+      }
     }
   ]))
-
-function __guard__(value, transform) {
-  return typeof value !== 'undefined' && value !== null
-    ? transform(value)
-    : undefined
-}
-function __guardMethod__(obj, methodName, transform) {
-  if (
-    typeof obj !== 'undefined' &&
-    obj !== null &&
-    typeof obj[methodName] === 'function'
-  ) {
-    return transform(obj, methodName)
-  } else {
-    return undefined
-  }
-}

services/web/test/unit/src/FileStore/FileStoreControllerTests.js

@@ -1,29 +1,19 @@
-/* eslint-disable
-    max-len,
-    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 { expect } = require('chai')
 const sinon = require('sinon')
-const chai = require('chai')
-const should = chai.should()
-const { expect } = chai
-const modulePath =
-  '../../../../app/src/Features/FileStore/FileStoreController.js'
 const SandboxedModule = require('sandboxed-module')
 
+const MODULE_PATH =
+  '../../../../app/src/Features/FileStore/FileStoreController.js'
+
 describe('FileStoreController', function() {
   beforeEach(function() {
-    this.FileStoreHandler = { getFileStream: sinon.stub() }
+    this.FileStoreHandler = {
+      getFileStream: sinon.stub(),
+      getFileSize: sinon.stub()
+    }
     this.ProjectLocator = { findElement: sinon.stub() }
-    this.controller = SandboxedModule.require(modulePath, {
+    this.Errors = { NotFoundError: sinon.stub() }
+    this.controller = SandboxedModule.require(MODULE_PATH, {
       requires: {
         'settings-sharelatex': this.settings,
         'logger-sharelatex': (this.logger = {
@@ -31,16 +21,17 @@ describe('FileStoreController', function() {
           err: sinon.stub()
         }),
         '../Project/ProjectLocator': this.ProjectLocator,
+        '../Errors/Errors': this.Errors,
         './FileStoreHandler': this.FileStoreHandler
       }
     })
     this.stream = {}
-    this.project_id = '2k3j1lk3j21lk3j'
-    this.file_id = '12321kklj1lk3jk12'
+    this.projectId = '2k3j1lk3j21lk3j'
+    this.fileId = '12321kklj1lk3jk12'
     this.req = {
       params: {
-        Project_id: this.project_id,
-        File_id: this.file_id
+        Project_id: this.projectId,
+        File_id: this.fileId
       },
       query: 'query string here',
       get(key) {
@@ -48,16 +39,18 @@ describe('FileStoreController', function() {
       }
     }
     this.res = {
+      set: sinon.stub().returnsThis(),
       setHeader: sinon.stub(),
-      setContentDisposition: sinon.stub()
+      setContentDisposition: sinon.stub(),
+      status: sinon.stub().returnsThis()
     }
-    return (this.file = { name: 'myfile.png' })
+    this.file = { name: 'myfile.png' }
   })
 
-  return describe('getFile', function() {
+  describe('getFile', function() {
     beforeEach(function() {
       this.FileStoreHandler.getFileStream.callsArgWith(3, null, this.stream)
-      return this.ProjectLocator.findElement.callsArgWith(1, null, this.file)
+      this.ProjectLocator.findElement.callsArgWith(1, null, this.file)
     })
 
     it('should call the file store handler with the project_id file_id and any query string', function(done) {
@@ -69,30 +62,30 @@ describe('FileStoreController', function() {
             this.req.query
           )
           .should.equal(true)
-        return done()
+        done()
       }
-      return this.controller.getFile(this.req, this.res)
+      this.controller.getFile(this.req, this.res)
     })
 
     it('should pipe to res', function(done) {
       this.stream.pipe = des => {
         des.should.equal(this.res)
-        return done()
+        done()
       }
-      return this.controller.getFile(this.req, this.res)
+      this.controller.getFile(this.req, this.res)
     })
 
     it('should get the file from the db', function(done) {
       this.stream.pipe = des => {
         const opts = {
-          project_id: this.project_id,
-          element_id: this.file_id,
+          project_id: this.projectId,
+          element_id: this.fileId,
           type: 'file'
         }
         this.ProjectLocator.findElement.calledWith(opts).should.equal(true)
-        return done()
+        done()
       }
-      return this.controller.getFile(this.req, this.res)
+      this.controller.getFile(this.req, this.res)
     })
 
     it('should set the Content-Disposition header', function(done) {
@@ -100,112 +93,156 @@ describe('FileStoreController', function() {
         this.res.setContentDisposition
           .calledWith('attachment', { filename: this.file.name })
           .should.equal(true)
-        return done()
+        done()
       }
-      return this.controller.getFile(this.req, this.res)
+      this.controller.getFile(this.req, this.res)
     })
 
     // Test behaviour around handling html files
-    ;['.html', '.htm', '.xhtml'].forEach(extension =>
+    ;['.html', '.htm', '.xhtml'].forEach(extension => {
       describe(`with a '${extension}' file extension`, function() {
         beforeEach(function() {
           this.file.name = `bad${extension}`
-          return (this.req.get = key => {
+          this.req.get = key => {
             if (key === 'User-Agent') {
               return 'A generic browser'
             }
-          })
+          }
         })
 
-        describe('from a non-ios browser', () =>
+        describe('from a non-ios browser', function() {
           it('should not set Content-Type', function(done) {
             this.stream.pipe = des => {
               this.res.setHeader
                 .calledWith('Content-Type', 'text/plain')
                 .should.equal(false)
-              return done()
+              done()
             }
-            return this.controller.getFile(this.req, this.res)
-          }))
+            this.controller.getFile(this.req, this.res)
+          })
+        })
 
         describe('from an iPhone', function() {
           beforeEach(function() {
-            return (this.req.get = key => {
+            this.req.get = key => {
               if (key === 'User-Agent') {
                 return 'An iPhone browser'
               }
-            })
+            }
           })
 
-          return it("should set Content-Type to 'text/plain'", function(done) {
+          it("should set Content-Type to 'text/plain'", function(done) {
             this.stream.pipe = des => {
               this.res.setHeader
                 .calledWith('Content-Type', 'text/plain')
                 .should.equal(true)
-              return done()
+              done()
             }
-            return this.controller.getFile(this.req, this.res)
+            this.controller.getFile(this.req, this.res)
           })
         })
 
-        return describe('from an iPad', function() {
+        describe('from an iPad', function() {
           beforeEach(function() {
-            return (this.req.get = key => {
+            this.req.get = key => {
               if (key === 'User-Agent') {
                 return 'An iPad browser'
               }
-            })
+            }
           })
 
-          return it("should set Content-Type to 'text/plain'", function(done) {
+          it("should set Content-Type to 'text/plain'", function(done) {
             this.stream.pipe = des => {
               this.res.setHeader
                 .calledWith('Content-Type', 'text/plain')
                 .should.equal(true)
-              return done()
+              done()
             }
-            return this.controller.getFile(this.req, this.res)
+            this.controller.getFile(this.req, this.res)
           })
         })
       })
-    )
-
-    // None of these should trigger the iOS/html logic
-    return [
-      'x.html-is-rad',
+    })
+    ;[
+      // None of these should trigger the iOS/html logic
+      ('x.html-is-rad',
       'html.pdf',
       '.html-is-good-for-hidden-files',
-      'somefile'
-    ].forEach(filename =>
+      'somefile')
+    ].forEach(filename => {
       describe(`with filename as '${filename}'`, function() {
         beforeEach(function() {
           this.user_agent = 'A generic browser'
           this.file.name = filename
-          return (this.req.get = key => {
+          this.req.get = key => {
             if (key === 'User-Agent') {
               return this.user_agent
             }
-          })
+          }
         })
-
-        return ['iPhone', 'iPad', 'Firefox', 'Chrome'].forEach(browser =>
+        ;[('iPhone', 'iPad', 'Firefox', 'Chrome')].forEach(browser => {
           describe(`downloaded from ${browser}`, function() {
             beforeEach(function() {
-              return (this.user_agent = `Some ${browser} thing`)
+              this.user_agent = `Some ${browser} thing`
             })
 
-            return it('Should not set the Content-type', function(done) {
+            it('Should not set the Content-type', function(done) {
               this.stream.pipe = des => {
                 this.res.setHeader
                   .calledWith('Content-Type', 'text/plain')
                   .should.equal(false)
-                return done()
+                done()
               }
-              return this.controller.getFile(this.req, this.res)
+              this.controller.getFile(this.req, this.res)
             })
           })
-        )
+        })
       })
-    )
+    })
+  })
+
+  describe('getFileHead', function() {
+    it('reports the file size', function(done) {
+      const expectedFileSize = 99393
+      this.FileStoreHandler.getFileSize.yields(
+        new Error('getFileSize: unexpected arguments')
+      )
+      this.FileStoreHandler.getFileSize
+        .withArgs(this.projectId, this.fileId)
+        .yields(null, expectedFileSize)
+
+      this.res.end = () => {
+        expect(this.res.status.lastCall.args).to.deep.equal([200])
+        expect(this.res.set.lastCall.args).to.deep.equal([
+          'Content-Length',
+          expectedFileSize
+        ])
+        done()
+      }
+
+      this.controller.getFileHead(this.req, this.res)
+    })
+
+    it('returns 404 on NotFoundError', function(done) {
+      this.FileStoreHandler.getFileSize.yields(new this.Errors.NotFoundError())
+
+      this.res.end = () => {
+        expect(this.res.status.lastCall.args).to.deep.equal([404])
+        done()
+      }
+
+      this.controller.getFileHead(this.req, this.res)
+    })
+
+    it('returns 500 on error', function(done) {
+      this.FileStoreHandler.getFileSize.yields(new Error('boom!'))
+
+      this.res.end = () => {
+        expect(this.res.status.lastCall.args).to.deep.equal([500])
+        done()
+      }
+
+      this.controller.getFileHead(this.req, this.res)
+    })
   })
 })

services/web/test/unit/src/FileStore/FileStoreHandlerTests.js

@@ -1,34 +1,18 @@
-/* eslint-disable
-    handle-callback-err,
-    max-len,
-    mocha/no-identical-title,
-    no-return-assign,
-    no-unused-vars,
-    standard/no-callback-literal,
-*/
-// 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.should()
-const { expect } = chai
-const modulePath = '../../../../app/src/Features/FileStore/FileStoreHandler.js'
+const { expect } = require('chai')
 const SandboxedModule = require('sandboxed-module')
 
+const MODULE_PATH = '../../../../app/src/Features/FileStore/FileStoreHandler.js'
+
 describe('FileStoreHandler', function() {
   beforeEach(function() {
-    let File
     this.fs = {
       createReadStream: sinon.stub(),
       lstat: sinon.stub().callsArgWith(1, null, {
-        isFile: () => true,
+        isFile() {
+          return true
+        },
         isDirectory() {
           return false
         }
@@ -38,17 +22,26 @@ describe('FileStoreHandler', function() {
       my: 'writeStream',
       on(type, cb) {
         if (type === 'response') {
-          return cb({ statusCode: 200 })
+          // eslint-disable-next-line standard/no-callback-literal
+          cb({ statusCode: 200 })
         }
       }
     }
     this.readStream = { my: 'readStream', on: sinon.stub() }
     this.request = sinon.stub()
+    this.request.head = sinon.stub()
+    this.filestoreUrl = 'http://filestore.sharelatex.test'
     this.settings = {
-      apis: { filestore: { url: 'http//filestore.sharelatex.test' } }
+      apis: { filestore: { url: this.filestoreUrl } }
     }
     this.hashValue = '0123456789'
-    this.FileModel = File = class File {
+    this.fileArgs = { name: 'upload-filename' }
+    this.fileId = 'file_id_here'
+    this.projectId = '1312312312'
+    this.fsPath = 'uploads/myfile.eps'
+    this.getFileUrl = (projectId, fileId) =>
+      `${this.filestoreUrl}/project/${projectId}/file/${fileId}`
+    this.FileModel = class File {
       constructor(options) {
         ;({ name: this.name, hash: this.hash } = options)
         this._id = 'file_id_here'
@@ -58,36 +51,35 @@ describe('FileStoreHandler', function() {
         }
       }
     }
-    this.handler = SandboxedModule.require(modulePath, {
+    this.Errors = {
+      NotFoundError: sinon.stub()
+    }
+    this.logger = {
+      log: sinon.stub(),
+      err: sinon.stub()
+    }
+    this.FileHashManager = {
+      computeHash: sinon.stub().callsArgWith(1, null, this.hashValue)
+    }
+    this.handler = SandboxedModule.require(MODULE_PATH, {
       requires: {
         'settings-sharelatex': this.settings,
         request: this.request,
-        'logger-sharelatex': (this.logger = {
-          log: sinon.stub(),
-          err: sinon.stub()
-        }),
-        './FileHashManager': (this.FileHashManager = {
-          computeHash: sinon.stub().callsArgWith(1, null, this.hashValue)
-        }),
+        'logger-sharelatex': this.logger,
+        './FileHashManager': this.FileHashManager,
         // FIXME: need to stub File object here
         '../../models/File': {
           File: this.FileModel
         },
+        '../Errors/Errors': this.Errors,
         fs: this.fs
       }
     })
-    this.file_args = { name: 'upload-filename' }
-    this.file_id = 'file_id_here'
-    this.project_id = '1312312312'
-    this.fsPath = 'uploads/myfile.eps'
-    return (this.handler._buildUrl = sinon
-      .stub()
-      .returns('http://filestore.stubbedBuilder.com'))
   })
 
   describe('uploadFileFromDisk', function() {
     beforeEach(function() {
-      return this.request.returns(this.writeStream)
+      this.request.returns(this.writeStream)
     })
 
     it('should create read stream', function(done) {
@@ -95,17 +87,17 @@ describe('FileStoreHandler', function() {
         pipe() {},
         on(type, cb) {
           if (type === 'open') {
-            return cb()
+            cb()
           }
         }
       })
-      return this.handler.uploadFileFromDisk(
-        this.project_id,
-        this.file_args,
+      this.handler.uploadFileFromDisk(
+        this.projectId,
+        this.fileArgs,
         this.fsPath,
         () => {
           this.fs.createReadStream.calledWith(this.fsPath).should.equal(true)
-          return done()
+          done()
         }
       )
     })
@@ -115,147 +107,127 @@ describe('FileStoreHandler', function() {
       this.fs.createReadStream.returns({
         on(type, cb) {
           if (type === 'open') {
-            return cb()
+            cb()
           }
         },
         pipe: o => {
           this.writeStream.should.equal(o)
-          return done()
+          done()
         }
       })
-      return this.handler.uploadFileFromDisk(
-        this.project_id,
-        this.file_args,
+      this.handler.uploadFileFromDisk(
+        this.projectId,
+        this.fileArgs,
         this.fsPath,
         () => {}
       )
     })
 
     it('should pass the correct options to request', function(done) {
+      const fileUrl = this.getFileUrl(this.projectId, this.fileId)
       this.fs.createReadStream.returns({
         pipe() {},
         on(type, cb) {
           if (type === 'open') {
-            return cb()
+            cb()
           }
         }
       })
-      return this.handler.uploadFileFromDisk(
-        this.project_id,
-        this.file_args,
+      this.handler.uploadFileFromDisk(
+        this.projectId,
+        this.fileArgs,
         this.fsPath,
         () => {
           this.request.args[0][0].method.should.equal('post')
-          this.request.args[0][0].uri.should.equal(this.handler._buildUrl())
-          return done()
-        }
-      )
-    })
-
-    it('builds the correct url', function(done) {
-      this.fs.createReadStream.returns({
-        pipe() {},
-        on(type, cb) {
-          if (type === 'open') {
-            return cb()
-          }
-        }
-      })
-      return this.handler.uploadFileFromDisk(
-        this.project_id,
-        this.file_args,
-        this.fsPath,
-        () => {
-          this.handler._buildUrl
-            .calledWith(this.project_id, this.file_id)
-            .should.equal(true)
-          return done()
+          this.request.args[0][0].uri.should.equal(fileUrl)
+          done()
         }
       )
     })
 
     it('should callback with the url and fileRef', function(done) {
+      const fileUrl = this.getFileUrl(this.projectId, this.fileId)
       this.fs.createReadStream.returns({
         pipe() {},
         on(type, cb) {
           if (type === 'open') {
-            return cb()
+            cb()
           }
         }
       })
-      return this.handler.uploadFileFromDisk(
-        this.project_id,
-        this.file_args,
+      this.handler.uploadFileFromDisk(
+        this.projectId,
+        this.fileArgs,
         this.fsPath,
         (err, url, fileRef) => {
           expect(err).to.not.exist
-          expect(url).to.equal(this.handler._buildUrl())
-          expect(fileRef._id).to.equal(this.file_id)
+          expect(url).to.equal(fileUrl)
+          expect(fileRef._id).to.equal(this.fileId)
           expect(fileRef.hash).to.equal(this.hashValue)
-          return done()
+          done()
         }
       )
     })
 
     describe('symlink', function() {
-      beforeEach(function() {
-        return (this.fs.lstat = sinon.stub().callsArgWith(1, null, {
-          isFile: () => false,
+      it('should not read file if it is symlink', function(done) {
+        this.fs.lstat = sinon.stub().callsArgWith(1, null, {
+          isFile() {
+            return false
+          },
           isDirectory() {
             return false
           }
-        }))
-      })
+        })
 
-      return it('should not read file if it is symlink', function(done) {
-        return this.handler.uploadFileFromDisk(
-          this.project_id,
-          this.file_args,
+        this.handler.uploadFileFromDisk(
+          this.projectId,
+          this.fileArgs,
           this.fsPath,
           () => {
             this.fs.createReadStream.called.should.equal(false)
-            return done()
+            done()
           }
         )
       })
-    })
 
-    describe('symlink', () =>
       it('should not read file stat returns nothing', function(done) {
         this.fs.lstat = sinon.stub().callsArgWith(1, null, null)
-        return this.handler.uploadFileFromDisk(
-          this.project_id,
-          this.file_args,
+        this.handler.uploadFileFromDisk(
+          this.projectId,
+          this.fileArgs,
           this.fsPath,
           () => {
             this.fs.createReadStream.called.should.equal(false)
-            return done()
+            done()
           }
         )
-      }))
+      })
+    })
 
-    return describe('when upload fails', function() {
+    describe('when upload fails', function() {
       beforeEach(function() {
-        return (this.writeStream.on = function(type, cb) {
+        this.writeStream.on = function(type, cb) {
           if (type === 'response') {
-            return cb({ statusCode: 500 })
+            // eslint-disable-next-line standard/no-callback-literal
+            cb({ statusCode: 500 })
           }
-        })
+        }
       })
 
-      return it('should callback with an error', function(done) {
+      it('should callback with an error', function(done) {
         this.fs.createReadStream.callCount = 0
         this.fs.createReadStream.returns({
           pipe() {},
           on(type, cb) {
             if (type === 'open') {
-              return cb()
+              cb()
             }
           }
         })
-        return this.handler.uploadFileFromDisk(
-          this.project_id,
-          this.file_args,
+        this.handler.uploadFileFromDisk(
+          this.projectId,
+          this.fileArgs,
           this.fsPath,
           err => {
             expect(err).to.exist
@@ -263,7 +235,7 @@ describe('FileStoreHandler', function() {
             expect(this.fs.createReadStream.callCount).to.equal(
               this.handler.RETRY_ATTEMPTS
             )
-            return done()
+            done()
           }
         )
       })
@@ -272,31 +244,23 @@ describe('FileStoreHandler', function() {
 
   describe('deleteFile', function() {
     it('should send a delete request to filestore api', function(done) {
+      const fileUrl = this.getFileUrl(this.projectId, this.fileId)
       this.request.callsArgWith(1, null)
-      return this.handler.deleteFile(this.project_id, this.file_id, err => {
+
+      this.handler.deleteFile(this.projectId, this.fileId, err => {
         assert.equal(err, undefined)
         this.request.args[0][0].method.should.equal('delete')
-        this.request.args[0][0].uri.should.equal(this.handler._buildUrl())
-        return done()
+        this.request.args[0][0].uri.should.equal(fileUrl)
+        done()
       })
     })
 
     it('should return the error if there is one', function(done) {
       const error = 'my error'
       this.request.callsArgWith(1, error)
-      return this.handler.deleteFile(this.project_id, this.file_id, err => {
+      this.handler.deleteFile(this.projectId, this.fileId, err => {
         assert.equal(err, error)
-        return done()
-      })
-    })
-
-    return it('builds the correct url', function(done) {
-      this.request.callsArgWith(1, null)
-      return this.handler.deleteFile(this.project_id, this.file_id, err => {
-        this.handler._buildUrl
-          .calledWith(this.project_id, this.file_id)
-          .should.equal(true)
-        return done()
+        done()
       })
     })
   })
@@ -304,155 +268,198 @@ describe('FileStoreHandler', function() {
   describe('getFileStream', function() {
     beforeEach(function() {
       this.query = {}
-      return this.request.returns(this.readStream)
+      this.request.returns(this.readStream)
     })
 
     it('should get the stream with the correct params', function(done) {
-      return this.handler.getFileStream(
-        this.project_id,
-        this.file_id,
+      const fileUrl = this.getFileUrl(this.projectId, this.fileId)
+      this.handler.getFileStream(
+        this.projectId,
+        this.fileId,
         this.query,
         (err, stream) => {
+          if (err) {
+            return done(err)
+          }
           this.request.args[0][0].method.should.equal('get')
-          this.request.args[0][0].uri.should.equal(this.handler._buildUrl())
-          return done()
+          this.request.args[0][0].uri.should.equal(fileUrl)
+          done()
         }
       )
     })
 
     it('should get stream from request', function(done) {
-      return this.handler.getFileStream(
-        this.project_id,
-        this.file_id,
+      this.handler.getFileStream(
+        this.projectId,
+        this.fileId,
         this.query,
         (err, stream) => {
+          if (err) {
+            return done(err)
+          }
           stream.should.equal(this.readStream)
-          return done()
-        }
-      )
-    })
-
-    it('builds the correct url', function(done) {
-      return this.handler.getFileStream(
-        this.project_id,
-        this.file_id,
-        this.query,
-        (err, stream) => {
-          this.handler._buildUrl
-            .calledWith(this.project_id, this.file_id)
-            .should.equal(true)
-          return done()
+          done()
         }
       )
     })
 
     it('should add an error handler', function(done) {
-      return this.handler.getFileStream(
-        this.project_id,
-        this.file_id,
+      this.handler.getFileStream(
+        this.projectId,
+        this.fileId,
         this.query,
         (err, stream) => {
+          if (err) {
+            return done(err)
+          }
           stream.on.calledWith('error').should.equal(true)
-          return done()
+          done()
         }
       )
     })
 
-    return describe('when range is specified in query', function() {
+    describe('when range is specified in query', function() {
       beforeEach(function() {
-        return (this.query = { range: '0-10' })
+        this.query = { range: '0-10' }
       })
 
       it('should add a range header', function(done) {
-        return this.handler.getFileStream(
-          this.project_id,
-          this.file_id,
+        this.handler.getFileStream(
+          this.projectId,
+          this.fileId,
           this.query,
           (err, stream) => {
+            if (err) {
+              return done(err)
+            }
             this.request.callCount.should.equal(1)
             const { headers } = this.request.firstCall.args[0]
             expect(headers).to.have.keys('range')
             expect(headers['range']).to.equal('bytes=0-10')
-            return done()
+            done()
           }
         )
       })
 
-      return describe('when range is invalid', () =>
-        ['0-', '-100', 'one-two', 'nonsense'].forEach(r => {
+      describe('when range is invalid', function() {
+        ;['0-', '-100', 'one-two', 'nonsense'].forEach(r => {
           beforeEach(function() {
-            return (this.query = { range: `${r}` })
+            this.query = { range: `${r}` }
           })
 
-          return it(`should not add a range header for '${r}'`, function(done) {
-            return this.handler.getFileStream(
-              this.project_id,
-              this.file_id,
+          it(`should not add a range header for '${r}'`, function(done) {
+            this.handler.getFileStream(
+              this.projectId,
+              this.fileId,
               this.query,
               (err, stream) => {
+                if (err) {
+                  return done(err)
+                }
                 this.request.callCount.should.equal(1)
                 const { headers } = this.request.firstCall.args[0]
                 expect(headers).to.not.have.keys('range')
-                return done()
+                done()
               }
             )
           })
-        }))
+        })
+      })
     })
   })
 
-  return describe('copyFile', function() {
+  describe('getFileSize', function() {
+    it('returns the file size reported by filestore', function(done) {
+      const expectedFileSize = 32432
+      const fileUrl = this.getFileUrl(this.projectId, this.fileId)
+      this.request.head.yields(
+        new Error('request.head() received unexpected arguments')
+      )
+      this.request.head.withArgs(fileUrl).yields(null, {
+        statusCode: 200,
+        headers: {
+          'content-length': expectedFileSize
+        }
+      })
+
+      this.handler.getFileSize(this.projectId, this.fileId, (err, fileSize) => {
+        if (err) {
+          return done(err)
+        }
+        expect(fileSize).to.equal(expectedFileSize)
+        done()
+      })
+    })
+
+    it('throws a NotFoundError on a 404 from filestore', function(done) {
+      this.request.head.yields(null, { statusCode: 404 })
+
+      this.handler.getFileSize(this.projectId, this.fileId, err => {
+        expect(err).to.be.instanceof(this.Errors.NotFoundError)
+        done()
+      })
+    })
+
+    it('throws an error on a non-200 from filestore', function(done) {
+      this.request.head.yields(null, { statusCode: 500 })
+
+      this.handler.getFileSize(this.projectId, this.fileId, err => {
+        expect(err).to.be.instanceof(Error)
+        done()
+      })
+    })
+
+    it('rethrows errors from filestore', function(done) {
+      this.request.head.yields(new Error())
+
+      this.handler.getFileSize(this.projectId, this.fileId, err => {
+        expect(err).to.be.instanceof(Error)
+        done()
+      })
+    })
+  })
+
+  describe('copyFile', function() {
     beforeEach(function() {
-      this.newProject_id = 'new project'
-      return (this.newFile_id = 'new file id')
+      this.newProjectId = 'new project'
+      this.newFileId = 'new file id'
     })
 
     it('should post json', function(done) {
+      const newFileUrl = this.getFileUrl(this.newProjectId, this.newFileId)
       this.request.callsArgWith(1, null, { statusCode: 200 })
 
-      return this.handler.copyFile(
-        this.project_id,
-        this.file_id,
-        this.newProject_id,
-        this.newFile_id,
+      this.handler.copyFile(
+        this.projectId,
+        this.fileId,
+        this.newProjectId,
+        this.newFileId,
         () => {
           this.request.args[0][0].method.should.equal('put')
-          this.request.args[0][0].uri.should.equal(this.handler._buildUrl())
+          this.request.args[0][0].uri.should.equal(newFileUrl)
           this.request.args[0][0].json.source.project_id.should.equal(
-            this.project_id
+            this.projectId
           )
-          this.request.args[0][0].json.source.file_id.should.equal(this.file_id)
-          return done()
-        }
-      )
-    })
-
-    it('builds the correct url', function(done) {
-      this.request.callsArgWith(1, null, { statusCode: 200 })
-      return this.handler.copyFile(
-        this.project_id,
-        this.file_id,
-        this.newProject_id,
-        this.newFile_id,
-        () => {
-          this.handler._buildUrl
-            .calledWith(this.newProject_id, this.newFile_id)
-            .should.equal(true)
-          return done()
+          this.request.args[0][0].json.source.file_id.should.equal(this.fileId)
+          done()
         }
       )
     })
 
     it('returns the url', function(done) {
+      const expectedUrl = this.getFileUrl(this.newProjectId, this.newFileId)
       this.request.callsArgWith(1, null, { statusCode: 200 })
-      return this.handler.copyFile(
-        this.project_id,
-        this.file_id,
-        this.newProject_id,
-        this.newFile_id,
+      this.handler.copyFile(
+        this.projectId,
+        this.fileId,
+        this.newProjectId,
+        this.newFileId,
         (err, url) => {
-          url.should.equal('http://filestore.stubbedBuilder.com')
-          return done()
+          if (err) {
+            return done(err)
+          }
+          url.should.equal(expectedUrl)
+          done()
         }
       )
     })
@@ -460,31 +467,31 @@ describe('FileStoreHandler', function() {
     it('should return the err', function(done) {
       const error = 'errrror'
       this.request.callsArgWith(1, error)
-      return this.handler.copyFile(
-        this.project_id,
-        this.file_id,
-        this.newProject_id,
-        this.newFile_id,
+      this.handler.copyFile(
+        this.projectId,
+        this.fileId,
+        this.newProjectId,
+        this.newFileId,
         err => {
           err.should.equal(error)
-          return done()
+          done()
         }
       )
     })
 
-    return it('should return an error for a non-success statusCode', function(done) {
+    it('should return an error for a non-success statusCode', function(done) {
       this.request.callsArgWith(1, null, { statusCode: 500 })
-      return this.handler.copyFile(
-        this.project_id,
-        this.file_id,
-        this.newProject_id,
-        this.newFile_id,
+      this.handler.copyFile(
+        this.projectId,
+        this.fileId,
+        this.newProjectId,
+        this.newFileId,
         err => {
           err.should.be.an('error')
           err.message.should.equal(
             'non-ok response from filestore for copyFile: 500'
           )
-          return done()
+          done()
         }
       )
     })