Merge pull request #66 from overleaf/spd-decaf-cleanup-3

Clean up and promisify health-check controller and KeyBuilder

services/filestore/app.js

@@ -89,82 +89,82 @@ Metrics.injectMetricsRoute(app)
 
 app.head(
   '/project/:project_id/file/:file_id',
-  keyBuilder.userFileKey,
+  keyBuilder.userFileKeyMiddleware,
   fileController.getFileHead
 )
 app.get(
   '/project/:project_id/file/:file_id',
-  keyBuilder.userFileKey,
+  keyBuilder.userFileKeyMiddleware,
   fileController.getFile
 )
 app.post(
   '/project/:project_id/file/:file_id',
-  keyBuilder.userFileKey,
+  keyBuilder.userFileKeyMiddleware,
   fileController.insertFile
 )
 app.put(
   '/project/:project_id/file/:file_id',
-  keyBuilder.userFileKey,
+  keyBuilder.userFileKeyMiddleware,
   bodyParser.json(),
   fileController.copyFile
 )
 app.del(
   '/project/:project_id/file/:file_id',
-  keyBuilder.userFileKey,
+  keyBuilder.userFileKeyMiddleware,
   fileController.deleteFile
 )
 
 app.head(
   '/template/:template_id/v/:version/:format',
-  keyBuilder.templateFileKey,
+  keyBuilder.templateFileKeyMiddleware,
   fileController.getFileHead
 )
 app.get(
   '/template/:template_id/v/:version/:format',
-  keyBuilder.templateFileKey,
+  keyBuilder.templateFileKeyMiddleware,
   fileController.getFile
 )
 app.get(
   '/template/:template_id/v/:version/:format/:sub_type',
-  keyBuilder.templateFileKey,
+  keyBuilder.templateFileKeyMiddleware,
   fileController.getFile
 )
 app.post(
   '/template/:template_id/v/:version/:format',
-  keyBuilder.templateFileKey,
+  keyBuilder.templateFileKeyMiddleware,
   fileController.insertFile
 )
 
 app.head(
   '/project/:project_id/public/:public_file_id',
-  keyBuilder.publicFileKey,
+  keyBuilder.publicFileKeyMiddleware,
   fileController.getFileHead
 )
 app.get(
   '/project/:project_id/public/:public_file_id',
-  keyBuilder.publicFileKey,
+  keyBuilder.publicFileKeyMiddleware,
   fileController.getFile
 )
 app.post(
   '/project/:project_id/public/:public_file_id',
-  keyBuilder.publicFileKey,
+  keyBuilder.publicFileKeyMiddleware,
   fileController.insertFile
 )
 app.put(
   '/project/:project_id/public/:public_file_id',
-  keyBuilder.publicFileKey,
+  keyBuilder.publicFileKeyMiddleware,
   bodyParser.json(),
   fileController.copyFile
 )
 app.del(
   '/project/:project_id/public/:public_file_id',
-  keyBuilder.publicFileKey,
+  keyBuilder.publicFileKeyMiddleware,
   fileController.deleteFile
 )
 
 app.get(
   '/project/:project_id/size',
-  keyBuilder.publicProjectKey,
+  keyBuilder.publicProjectKeyMiddleware,
   fileController.directorySize
 )
 

services/filestore/app/js/Errors.js

@@ -20,6 +20,7 @@ class BackwardCompatibleError extends OError {
 class NotFoundError extends BackwardCompatibleError {}
 class WriteError extends BackwardCompatibleError {}
 class ReadError extends BackwardCompatibleError {}
+class HealthCheckError extends BackwardCompatibleError {}
 class ConversionsDisabledError extends BackwardCompatibleError {}
 class ConversionError extends BackwardCompatibleError {}
 
@@ -44,5 +45,6 @@ module.exports = {
   ConversionsDisabledError,
   WriteError,
   ReadError,
-  ConversionError
+  ConversionError,
+  HealthCheckError
 }

services/filestore/app/js/HealthCheckController.js

@@ -1,80 +1,72 @@
-// TODO: This file was created by bulk-decaffeinate.
-// Sanity-check the conversion and remove this comment.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * DS207: Consider shorter variations of null checks
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
 const fs = require('fs-extra')
 const path = require('path')
-const async = require('async')
-const fileConverter = require('./FileConverter')
-const keyBuilder = require('./KeyBuilder')
-const fileController = require('./FileController')
 const logger = require('logger-sharelatex')
-const settings = require('settings-sharelatex')
+const Settings = require('settings-sharelatex')
 const streamBuffers = require('stream-buffers')
-const _ = require('underscore')
+const { promisify } = require('util')
+const Stream = require('stream')
 
-const checkCanStoreFiles = function(callback) {
-  callback = _.once(callback)
-  const req = { params: {}, query: {}, headers: {} }
-  req.params.project_id = settings.health_check.project_id
-  req.params.file_id = settings.health_check.file_id
-  const myWritableStreamBuffer = new streamBuffers.WritableStreamBuffer({
+const pipeline = promisify(Stream.pipeline)
+const fsCopy = promisify(fs.copy)
+const fsUnlink = promisify(fs.unlink)
+
+const { HealthCheckError } = require('./Errors')
+const FileConverter = require('./FileConverter').promises
+const FileHandler = require('./FileHandler').promises
+
+async function checkCanGetFiles() {
+  if (!Settings.health_check) {
+    return
+  }
+
+  const projectId = Settings.health_check.project_id
+  const fileId = Settings.health_check.file_id
+  const key = `${projectId}/${fileId}`
+  const bucket = Settings.filestore.stores.user_files
+
+  const buffer = new streamBuffers.WritableStreamBuffer({
     initialSize: 100
   })
-  const res = {
-    send(code) {
-      if (code !== 200) {
-        return callback(new Error(`non-200 code from getFile: ${code}`))
-      }
-    }
+
+  const sourceStream = await FileHandler.getFile(bucket, key, {})
+  try {
+    await pipeline(sourceStream, buffer)
+  } catch (err) {
+    throw new HealthCheckError('failed to get health-check file').withCause(err)
+  }
+
+  if (!buffer.size()) {
+    throw new HealthCheckError('no bytes written to download stream')
   }
-  myWritableStreamBuffer.send = res.send
-  return keyBuilder.userFileKey(req, res, function() {
-    fileController.getFile(req, myWritableStreamBuffer)
-    return myWritableStreamBuffer.on('close', function() {
-      if (myWritableStreamBuffer.size() > 0) {
-        return callback()
-      } else {
-        const err = 'no data in write stream buffer for health check'
-        logger.err({ err }, 'error performing health check')
-        return callback(err)
-      }
-    })
-  })
 }
 
-const checkFileConvert = function(callback) {
-  if (!settings.enableConversions) {
-    return callback()
+async function checkFileConvert() {
+  if (!Settings.enableConversions) {
+    return
+  }
+
+  const imgPath = path.join(Settings.path.uploadFolder, '/tiny.pdf')
+
+  let resultPath
+  try {
+    await fsCopy('./tiny.pdf', imgPath)
+    resultPath = await FileConverter.thumbnail(imgPath)
+  } finally {
+    if (resultPath) {
+      await fsUnlink(resultPath)
+    }
+    await fsUnlink(imgPath)
   }
-  const imgPath = path.join(settings.path.uploadFolder, '/tiny.pdf')
-  return async.waterfall(
-    [
-      cb => fs.copy('./tiny.pdf', imgPath, cb),
-      cb => fileConverter.thumbnail(imgPath, cb),
-      (resultPath, cb) => fs.unlink(resultPath, cb),
-      cb => fs.unlink(imgPath, cb)
-    ],
-    callback
-  )
 }
 
 module.exports = {
   check(req, res) {
     logger.log({}, 'performing health check')
-    return async.parallel([checkFileConvert, checkCanStoreFiles], function(
-      err
-    ) {
-      if (err != null) {
+    Promise.all([checkCanGetFiles(), checkFileConvert()])
+      .then(() => res.send(200))
+      .catch(err => {
         logger.err({ err }, 'Health check: error running')
-        return res.send(500)
-      } else {
-        return res.send(200)
-      }
-    })
+        res.send(500)
+      })
   }
 }

services/filestore/app/js/KeyBuilder.js

@@ -1,71 +1,78 @@
-/* eslint-disable
-    camelcase,
-    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 settings = require('settings-sharelatex')
 
 module.exports = {
-  getConvertedFolderKey(key) {
-    return (key = `${key}-converted-cache/`)
-  },
-
-  addCachingToKey(key, opts) {
-    key = this.getConvertedFolderKey(key)
-    if (opts.format != null && opts.style == null) {
-      key = `${key}format-${opts.format}`
-    }
-    if (opts.style != null && opts.format == null) {
-      key = `${key}style-${opts.style}`
-    }
-    if (opts.style != null && opts.format != null) {
-      key = `${key}format-${opts.format}-style-${opts.style}`
-    }
-    return key
-  },
-
-  userFileKey(req, res, next) {
-    const { project_id, file_id } = req.params
-    req.key = `${project_id}/${file_id}`
-    req.bucket = settings.filestore.stores.user_files
-    return next()
-  },
-
-  publicFileKey(req, res, next) {
-    const { project_id, public_file_id } = req.params
-    if (settings.filestore.stores.public_files == null) {
-      return res.status(501).send('public files not available')
-    } else {
-      req.key = `${project_id}/${public_file_id}`
-      req.bucket = settings.filestore.stores.public_files
-      return next()
-    }
-  },
-
-  templateFileKey(req, res, next) {
-    const { template_id, format, version, sub_type } = req.params
-    req.key = `${template_id}/v/${version}/${format}`
-    if (sub_type != null) {
-      req.key = `${req.key}/${sub_type}`
-    }
-    req.bucket = settings.filestore.stores.template_files
-    req.version = version
-    const opts = req.query
-    return next()
-  },
-
-  publicProjectKey(req, res, next) {
-    const { project_id } = req.params
-    req.project_id = project_id
-    req.bucket = settings.filestore.stores.user_files
-    return next()
-  }
+  getConvertedFolderKey,
+  addCachingToKey,
+  userFileKeyMiddleware,
+  publicFileKeyMiddleware,
+  publicProjectKeyMiddleware,
+  templateFileKeyMiddleware
+}
+
+function getConvertedFolderKey(key) {
+  return `${key}-converted-cache/`
+}
+
+function addCachingToKey(key, opts) {
+  key = this.getConvertedFolderKey(key)
+
+  if (opts.format && !opts.style) {
+    key = `${key}format-${opts.format}`
+  }
+  if (opts.style && !opts.format) {
+    key = `${key}style-${opts.style}`
+  }
+  if (opts.style && opts.format) {
+    key = `${key}format-${opts.format}-style-${opts.style}`
+  }
+
+  return key
+}
+
+function userFileKeyMiddleware(req, res, next) {
+  const { project_id: projectId, file_id: fileId } = req.params
+  req.key = `${projectId}/${fileId}`
+  req.bucket = settings.filestore.stores.user_files
+  next()
+}
+
+function publicFileKeyMiddleware(req, res, next) {
+  if (settings.filestore.stores.public_files == null) {
+    return res.status(501).send('public files not available')
+  }
+
+  const { project_id: projectId, public_file_id: publicFileId } = req.params
+  req.key = `${projectId}/${publicFileId}`
+  req.bucket = settings.filestore.stores.public_files
+
+  next()
+}
+
+function templateFileKeyMiddleware(req, res, next) {
+  const {
+    template_id: templateId,
+    format,
+    version,
+    sub_type: subType
+  } = req.params
+
+  req.key = `${templateId}/v/${version}/${format}`
+
+  if (subType) {
+    req.key = `${req.key}/${subType}`
+  }
+
+  req.bucket = settings.filestore.stores.template_files
+  req.version = version
+
+  next()
+}
+
+function publicProjectKeyMiddleware(req, res, next) {
+  const { project_id: projectId } = req.params
+
+  req.project_id = projectId
+  req.bucket = settings.filestore.stores.user_files
+
+  next()
 }