From b6a1ea6a3025145bffda321d30bcefc9b214081a Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Thu, 19 Dec 2019 10:41:41 +0000 Subject: [PATCH] Cleanup and promisify FileConverter --- services/filestore/app/js/FileConverter.js | 231 ++++++++---------- .../test/unit/js/FileConverterTests.js | 154 ++++++------ 2 files changed, 179 insertions(+), 206 deletions(-) diff --git a/services/filestore/app/js/FileConverter.js b/services/filestore/app/js/FileConverter.js index 04b599728a..0cb5c709fc 100644 --- a/services/filestore/app/js/FileConverter.js +++ b/services/filestore/app/js/FileConverter.js @@ -1,133 +1,116 @@ -/* 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 = { - killSignal: 'SIGTERM', - timeout: fourtySeconds -} +const APPROVED_FORMATS = ['png'] +const FOURTY_SECONDS = 40 * 1000 +const KILL_SIGNAL = 'SIGTERM' module.exports = { - convert(sourcePath, requestedFormat, callback) { - logger.log({ sourcePath, requestedFormat }, 'converting file format') - const timer = new metrics.Timer('imageConvert') - const destPath = `${sourcePath}.${requestedFormat}` - sourcePath = `${sourcePath}[0]` - if (!_.include(approvedFormats, requestedFormat)) { - const err = new Error('invalid format requested') - 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) - }) + convert: callbackify(convert), + thumbnail: callbackify(thumbnail), + preview: callbackify(preview), + promises: { + convert, + thumbnail, + preview } } + +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) { + logger.err( + { + err, + stderr: err.stderr, + command, + sourcePath, + requestedFormat, + destPath + }, + 'something went wrong converting file' + ) + 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 +} diff --git a/services/filestore/test/unit/js/FileConverterTests.js b/services/filestore/test/unit/js/FileConverterTests.js index 89faacd1d1..7a7ba12ee2 100644 --- a/services/filestore/test/unit/js/FileConverterTests.js +++ b/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() - this.converter = SandboxedModule.require(modulePath, { + SafeExec = { + 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 = { - commands: { - convertCommandPrefix: [] - } - }) + 'settings-sharelatex': Settings } }) - - 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) { - this.safe_exec.callsArgWith(2) - return this.converter.convert(this.sourcePath, this.format, err => { - const args = this.safe_exec.args[0][0] - args.indexOf(`${this.sourcePath}[0]`).should.not.equal(-1) - args.indexOf(`${this.sourcePath}.${this.format}`).should.not.equal(-1) - return done() - }) + it('should convert the source to the requested format', async function() { + await FileConverter.promises.convert(sourcePath, format) + const args = SafeExec.promises.args[0][0] + expect(args).to.include(`${sourcePath}[0]`) + expect(args).to.include(`${sourcePath}.${format}`) }) - it('should return the dest path', function(done) { - this.safe_exec.callsArgWith(2) - return this.converter.convert( - this.sourcePath, - this.format, - (err, destPath) => { - destPath.should.equal(`${this.sourcePath}.${this.format}`) - return done() - } - ) + it('should return the dest path', async function() { + const destPath = await FileConverter.promises.convert(sourcePath, format) + destPath.should.equal(`${sourcePath}.${format}`) }) - it('should return the error from convert', function(done) { - this.safe_exec.callsArgWith(2, this.error) - return this.converter.convert(this.sourcePath, this.format, err => { - err.should.equal(this.error) - return done() - }) + it('should wrap the error from convert', async function() { + SafeExec.promises.rejects(errorMessage) + try { + await FileConverter.promises.convert(sourcePath, format) + 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) { - this.safe_exec.callsArgWith(2) - return this.converter.convert(this.sourcePath, 'ahhhhh', err => { - expect(err).to.exist - return done() - }) + it('should not accept an non approved format', async function() { + try { + await FileConverter.promises.convert(sourcePath, 'potato') + expect('error should have been thrown').not.to.exist + } catch (err) { + expect(err.name).to.equal('ConversionError') + } }) - return it('should prefix the command with Settings.commands.convertCommandPrefix', function(done) { - this.safe_exec.callsArgWith(2) - this.Settings.commands.convertCommandPrefix = ['nice'] - return this.converter.convert(this.sourcePath, this.format, err => { - const command = this.safe_exec.args[0][0] - command[0].should.equal('nice') - return done() + it('should prefix the command with Settings.commands.convertCommandPrefix', async function() { + Settings.commands.convertCommandPrefix = ['nice'] + await FileConverter.promises.convert(sourcePath, format) + }) + + it('should convert the file when called as a callback', function(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', () => - it('should call converter resize with args', function(done) { - this.safe_exec.callsArgWith(2) - return this.converter.thumbnail(this.sourcePath, err => { - const args = this.safe_exec.args[0][0] - args.indexOf(`${this.sourcePath}[0]`).should.not.equal(-1) - return done() - }) - })) + describe('thumbnail', function() { + it('should call converter resize with args', async function() { + await FileConverter.promises.thumbnail(sourcePath) + const args = SafeExec.promises.args[0][0] + expect(args).to.include(`${sourcePath}[0]`) + }) + }) - return describe('preview', () => - it('should call converter resize with args', function(done) { - this.safe_exec.callsArgWith(2) - return this.converter.preview(this.sourcePath, err => { - const args = this.safe_exec.args[0][0] - args.indexOf(`${this.sourcePath}[0]`).should.not.equal(-1) - return done() - }) - })) + describe('preview', function() { + it('should call converter resize with args', async function() { + await FileConverter.promises.preview(sourcePath) + const args = SafeExec.promises.args[0][0] + expect(args).to.include(`${sourcePath}[0]`) + }) + }) })