Merge pull request #65 from overleaf/spd-decaf-cleanup-2

Cleanup and promisify FileConverter

services/filestore/app/js/FileConverter.js

@@ -1,133 +1,105 @@
-/* eslint-disable
-    camelcase,
-*/
-// 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 _ = require('underscore')
 const metrics = require('metrics-sharelatex')
 const logger = require('logger-sharelatex')
-const safe_exec = require('./SafeExec')
-const approvedFormats = ['png']
 const Settings = require('settings-sharelatex')
+const { callbackify } = require('util')
 
-const fourtySeconds = 40 * 1000
+const safeExec = require('./SafeExec').promises
+const { ConversionError } = require('./Errors')
 
-const childProcessOpts = {
+const APPROVED_FORMATS = ['png']
-  killSignal: 'SIGTERM',
+const FOURTY_SECONDS = 40 * 1000
-  timeout: fourtySeconds
+const KILL_SIGNAL = 'SIGTERM'
-}
 
 module.exports = {
-  convert(sourcePath, requestedFormat, callback) {
+  convert: callbackify(convert),
-    logger.log({ sourcePath, requestedFormat }, 'converting file format')
+  thumbnail: callbackify(thumbnail),
-    const timer = new metrics.Timer('imageConvert')
+  preview: callbackify(preview),
-    const destPath = `${sourcePath}.${requestedFormat}`
+  promises: {
-    sourcePath = `${sourcePath}[0]`
+    convert,
-    if (!_.include(approvedFormats, requestedFormat)) {
+    thumbnail,
-      const err = new Error('invalid format requested')
+    preview
-      return callback(err)
-    }
-    const width = '600x'
-    let command = [
-      'convert',
-      '-define',
-      `pdf:fit-page=${width}`,
-      '-flatten',
-      '-density',
-      '300',
-      sourcePath,
-      destPath
-    ]
-    command = Settings.commands.convertCommandPrefix.concat(command)
-    return safe_exec(command, childProcessOpts, function(err, stdout, stderr) {
-      timer.done()
-      if (err != null) {
-        logger.err(
-          { err, stderr, sourcePath, requestedFormat, destPath },
-          'something went wrong converting file'
-        )
-      } else {
-        logger.log(
-          { sourcePath, requestedFormat, destPath },
-          'finished converting file'
-        )
-      }
-      return callback(err, destPath)
-    })
-  },
-
-  thumbnail(sourcePath, callback) {
-    const destPath = `${sourcePath}.png`
-    sourcePath = `${sourcePath}[0]`
-    const width = '260x'
-    let command = [
-      'convert',
-      '-flatten',
-      '-background',
-      'white',
-      '-density',
-      '300',
-      '-define',
-      `pdf:fit-page=${width}`,
-      sourcePath,
-      '-resize',
-      width,
-      destPath
-    ]
-    logger.log({ sourcePath, destPath, command }, 'thumbnail convert file')
-    command = Settings.commands.convertCommandPrefix.concat(command)
-    return safe_exec(command, childProcessOpts, function(err, stdout, stderr) {
-      if (err != null) {
-        logger.err(
-          { err, stderr, sourcePath },
-          'something went wrong converting file to thumbnail'
-        )
-      } else {
-        logger.log({ sourcePath, destPath }, 'finished thumbnailing file')
-      }
-      return callback(err, destPath)
-    })
-  },
-
-  preview(sourcePath, callback) {
-    logger.log({ sourcePath }, 'preview convert file')
-    const destPath = `${sourcePath}.png`
-    sourcePath = `${sourcePath}[0]`
-    const width = '548x'
-    let command = [
-      'convert',
-      '-flatten',
-      '-background',
-      'white',
-      '-density',
-      '300',
-      '-define',
-      `pdf:fit-page=${width}`,
-      sourcePath,
-      '-resize',
-      width,
-      destPath
-    ]
-    command = Settings.commands.convertCommandPrefix.concat(command)
-    return safe_exec(command, childProcessOpts, function(err, stdout, stderr) {
-      if (err != null) {
-        logger.err(
-          { err, stderr, sourcePath, destPath },
-          'something went wrong converting file to preview'
-        )
-      } else {
-        logger.log(
-          { sourcePath, destPath },
-          'finished converting file to preview'
-        )
-      }
-      return callback(err, destPath)
-    })
   }
 }
+
+async function convert(sourcePath, requestedFormat) {
+  const width = '600x'
+  return _convert(sourcePath, requestedFormat, [
+    'convert',
+    '-define',
+    `pdf:fit-page=${width}`,
+    '-flatten',
+    '-density',
+    '300',
+    `${sourcePath}[0]`
+  ])
+}
+
+async function thumbnail(sourcePath) {
+  const width = '260x'
+  return convert(sourcePath, 'png', [
+    'convert',
+    '-flatten',
+    '-background',
+    'white',
+    '-density',
+    '300',
+    '-define',
+    `pdf:fit-page=${width}`,
+    `${sourcePath}[0]`,
+    '-resize',
+    width
+  ])
+}
+
+async function preview(sourcePath) {
+  const width = '548x'
+  return convert(sourcePath, 'png', [
+    'convert',
+    '-flatten',
+    '-background',
+    'white',
+    '-density',
+    '300',
+    '-define',
+    `pdf:fit-page=${width}`,
+    `${sourcePath}[0]`,
+    '-resize',
+    width
+  ])
+}
+
+async function _convert(sourcePath, requestedFormat, command) {
+  logger.log({ sourcePath, requestedFormat }, 'converting file format')
+
+  if (!APPROVED_FORMATS.includes(requestedFormat)) {
+    throw new ConversionError({
+      message: 'invalid format requested',
+      info: { format: requestedFormat }
+    })
+  }
+
+  const timer = new metrics.Timer('imageConvert')
+  const destPath = `${sourcePath}.${requestedFormat}`
+
+  command.push(destPath)
+  command = Settings.commands.convertCommandPrefix.concat(command)
+
+  try {
+    await safeExec(command, {
+      killSignal: KILL_SIGNAL,
+      timeout: FOURTY_SECONDS
+    })
+  } catch (err) {
+    throw new ConversionError({
+      message: 'something went wrong converting file',
+      info: { stderr: err.stderr, sourcePath, requestedFormat, destPath }
+    }).withCause(err)
+  }
+
+  timer.done()
+  logger.log(
+    { sourcePath, requestedFormat, destPath },
+    'finished converting file'
+  )
+  return destPath
+}

services/filestore/test/unit/js/FileConverterTests.js

@@ -1,29 +1,30 @@
-/* eslint-disable
-    handle-callback-err,
-    no-return-assign,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
-/*
- * decaffeinate suggestions:
- * DS102: Remove unnecessary code created because of implicit returns
- * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
- */
-const { assert } = require('chai')
 const sinon = require('sinon')
 const chai = require('chai')
-const should = chai.should()
 const { expect } = chai
-const modulePath = '../../../app/js/FileConverter.js'
 const SandboxedModule = require('sandboxed-module')
 
+const modulePath = '../../../app/js/FileConverter.js'
+
 describe('FileConverter', function() {
+  let SafeExec, FileConverter
+  const sourcePath = '/data/wombat.eps'
+  const destPath = '/tmp/dest.png'
+  const format = 'png'
+  const errorMessage = 'guru meditation error'
+  const Settings = {
+    commands: {
+      convertCommandPrefix: []
+    }
+  }
+
   beforeEach(function() {
-    this.safe_exec = sinon.stub()
+    SafeExec = {
-    this.converter = SandboxedModule.require(modulePath, {
+      promises: sinon.stub().resolves(destPath)
+    }
+
+    FileConverter = SandboxedModule.require(modulePath, {
       requires: {
-        './SafeExec': this.safe_exec,
+        './SafeExec': SafeExec,
         'logger-sharelatex': {
           log() {},
           err() {}
@@ -32,86 +33,75 @@ describe('FileConverter', function() {
           inc: sinon.stub(),
           Timer: sinon.stub().returns({ done: sinon.stub() })
         },
-        'settings-sharelatex': (this.Settings = {
+        'settings-sharelatex': Settings
-          commands: {
-            convertCommandPrefix: []
-          }
-        })
       }
     })
-
-    this.sourcePath = '/this/path/here.eps'
-    this.format = 'png'
-    return (this.error = 'Error')
   })
 
   describe('convert', function() {
-    it('should convert the source to the requested format', function(done) {
+    it('should convert the source to the requested format', async function() {
-      this.safe_exec.callsArgWith(2)
+      await FileConverter.promises.convert(sourcePath, format)
-      return this.converter.convert(this.sourcePath, this.format, err => {
+      const args = SafeExec.promises.args[0][0]
-        const args = this.safe_exec.args[0][0]
+      expect(args).to.include(`${sourcePath}[0]`)
-        args.indexOf(`${this.sourcePath}[0]`).should.not.equal(-1)
+      expect(args).to.include(`${sourcePath}.${format}`)
-        args.indexOf(`${this.sourcePath}.${this.format}`).should.not.equal(-1)
-        return done()
-      })
     })
 
-    it('should return the dest path', function(done) {
+    it('should return the dest path', async function() {
-      this.safe_exec.callsArgWith(2)
+      const destPath = await FileConverter.promises.convert(sourcePath, format)
-      return this.converter.convert(
+      destPath.should.equal(`${sourcePath}.${format}`)
-        this.sourcePath,
-        this.format,
-        (err, destPath) => {
-          destPath.should.equal(`${this.sourcePath}.${this.format}`)
-          return done()
-        }
-      )
     })
 
-    it('should return the error from convert', function(done) {
+    it('should wrap the error from convert', async function() {
-      this.safe_exec.callsArgWith(2, this.error)
+      SafeExec.promises.rejects(errorMessage)
-      return this.converter.convert(this.sourcePath, this.format, err => {
+      try {
-        err.should.equal(this.error)
+        await FileConverter.promises.convert(sourcePath, format)
-        return done()
+        expect('error should have been thrown').not.to.exist
-      })
+      } catch (err) {
+        expect(err.name).to.equal('ConversionError')
+        expect(err.cause.toString()).to.equal(errorMessage)
+      }
     })
 
-    it('should not accapt an non aproved format', function(done) {
+    it('should not accept an non approved format', async function() {
-      this.safe_exec.callsArgWith(2)
+      try {
-      return this.converter.convert(this.sourcePath, 'ahhhhh', err => {
+        await FileConverter.promises.convert(sourcePath, 'potato')
-        expect(err).to.exist
+        expect('error should have been thrown').not.to.exist
-        return done()
+      } catch (err) {
-      })
+        expect(err.name).to.equal('ConversionError')
+      }
     })
 
-    return it('should prefix the command with Settings.commands.convertCommandPrefix', function(done) {
+    it('should prefix the command with Settings.commands.convertCommandPrefix', async function() {
-      this.safe_exec.callsArgWith(2)
+      Settings.commands.convertCommandPrefix = ['nice']
-      this.Settings.commands.convertCommandPrefix = ['nice']
+      await FileConverter.promises.convert(sourcePath, format)
-      return this.converter.convert(this.sourcePath, this.format, err => {
+    })
-        const command = this.safe_exec.args[0][0]
+
-        command[0].should.equal('nice')
+    it('should convert the file when called as a callback', function(done) {
-        return done()
+      FileConverter.convert(sourcePath, format, (err, destPath) => {
+        expect(err).not.to.exist
+        destPath.should.equal(`${sourcePath}.${format}`)
+
+        const args = SafeExec.promises.args[0][0]
+        expect(args).to.include(`${sourcePath}[0]`)
+        expect(args).to.include(`${sourcePath}.${format}`)
+        done()
       })
     })
   })
 
-  describe('thumbnail', () =>
+  describe('thumbnail', function() {
-    it('should call converter resize with args', function(done) {
+    it('should call converter resize with args', async function() {
-      this.safe_exec.callsArgWith(2)
+      await FileConverter.promises.thumbnail(sourcePath)
-      return this.converter.thumbnail(this.sourcePath, err => {
+      const args = SafeExec.promises.args[0][0]
-        const args = this.safe_exec.args[0][0]
+      expect(args).to.include(`${sourcePath}[0]`)
-        args.indexOf(`${this.sourcePath}[0]`).should.not.equal(-1)
+    })
-        return done()
+  })
-      })
-    }))
 
-  return describe('preview', () =>
+  describe('preview', function() {
-    it('should call converter resize with args', function(done) {
+    it('should call converter resize with args', async function() {
-      this.safe_exec.callsArgWith(2)
+      await FileConverter.promises.preview(sourcePath)
-      return this.converter.preview(this.sourcePath, err => {
+      const args = SafeExec.promises.args[0][0]
-        const args = this.safe_exec.args[0][0]
+      expect(args).to.include(`${sourcePath}[0]`)
-        args.indexOf(`${this.sourcePath}[0]`).should.not.equal(-1)
+    })
-        return done()
+  })
-      })
-    }))
 })