diff --git a/src/node/handler/ExportHandler.js b/src/node/handler/ExportHandler.js index fbb9e57da60..0bf75a17f11 100644 --- a/src/node/handler/ExportHandler.js +++ b/src/node/handler/ExportHandler.js @@ -33,24 +33,12 @@ const util = require('util'); const fsp_writeFile = util.promisify(fs.writeFile); const fsp_unlink = util.promisify(fs.unlink); -let convertor = null; - -// load abiword only if it is enabled -if (settings.abiword != null) { - convertor = require('../utils/Abiword'); -} - -// Use LibreOffice if an executable has been defined in the settings -if (settings.soffice != null) { - convertor = require('../utils/LibreOffice'); -} - const tempDirectory = os.tmpdir(); /** * do a requested export */ -const doExport = async (req, res, padId, readOnlyId, type) => { +exports.doExport = async (req, res, padId, readOnlyId, type) => { // avoid naming the read-only file as the original pad's id let fileName = readOnlyId ? readOnlyId : padId; @@ -85,7 +73,7 @@ const doExport = async (req, res, padId, readOnlyId, type) => { const newHTML = await hooks.aCallFirst('exportHTMLSend', html); if (newHTML.length) html = newHTML; res.send(html); - throw 'stop'; + return; } // else write the html export to a file @@ -98,7 +86,7 @@ const doExport = async (req, res, padId, readOnlyId, type) => { html = null; await TidyHtml.tidy(srcFile); - // send the convert job to the convertor (abiword, libreoffice, ..) + // send the convert job to the converter (abiword, libreoffice, ..) const destFile = `${tempDirectory}/etherpad_export_${randNum}.${type}`; // Allow plugins to overwrite the convert in export process @@ -106,12 +94,11 @@ const doExport = async (req, res, padId, readOnlyId, type) => { if (result.length > 0) { // console.log("export handled by plugin", destFile); } else { - // @TODO no Promise interface for convertors (yet) - await new Promise((resolve, reject) => { - convertor.convertFile(srcFile, destFile, type, (err) => { - err ? reject('convertFailed') : resolve(); - }); - }); + const converter = + settings.soffice != null ? require('../utils/LibreOffice') + : settings.abiword != null ? require('../utils/Abiword') + : null; + await converter.convertFile(srcFile, destFile, type); } // send the file @@ -128,11 +115,3 @@ const doExport = async (req, res, padId, readOnlyId, type) => { await fsp_unlink(destFile); } }; - -exports.doExport = (req, res, padId, readOnlyId, type) => { - doExport(req, res, padId, readOnlyId, type).catch((err) => { - if (err !== 'stop') { - throw err; - } - }); -}; diff --git a/src/node/handler/ImportHandler.js b/src/node/handler/ImportHandler.js index d97ce91ef75..acaaf092707 100644 --- a/src/node/handler/ImportHandler.js +++ b/src/node/handler/ImportHandler.js @@ -55,17 +55,17 @@ const rm = async (path) => { } }; -let convertor = null; +let converter = null; let exportExtension = 'htm'; // load abiword only if it is enabled and if soffice is disabled if (settings.abiword != null && settings.soffice == null) { - convertor = require('../utils/Abiword'); + converter = require('../utils/Abiword'); } // load soffice only if it is enabled if (settings.soffice != null) { - convertor = require('../utils/LibreOffice'); + converter = require('../utils/LibreOffice'); exportExtension = 'html'; } @@ -80,8 +80,8 @@ const doImport = async (req, res, padId) => { // set html in the pad const randNum = Math.floor(Math.random() * 0xFFFFFFFF); - // setting flag for whether to use convertor or not - let useConvertor = (convertor != null); + // setting flag for whether to use converter or not + let useConverter = (converter != null); const form = new formidable.IncomingForm(); form.keepExtensions = true; @@ -170,30 +170,25 @@ const doImport = async (req, res, padId) => { // convert file to html if necessary if (!importHandledByPlugin && !directDatabaseAccess) { if (fileIsTXT) { - // Don't use convertor for text files - useConvertor = false; + // Don't use converter for text files + useConverter = false; } // See https://github.com/ether/etherpad-lite/issues/2572 - if (fileIsHTML || !useConvertor) { - // if no convertor only rename + if (fileIsHTML || !useConverter) { + // if no converter only rename await fs.rename(srcFile, destFile); } else { - // @TODO - no Promise interface for convertors (yet) - await new Promise((resolve, reject) => { - convertor.convertFile(srcFile, destFile, exportExtension, (err) => { - // catch convert errors - if (err) { - logger.warn(`Converting Error: ${err.stack || err}`); - return reject(new ImportError('convertFailed')); - } - resolve(); - }); - }); + try { + await converter.convertFile(srcFile, destFile, exportExtension); + } catch (err) { + logger.warn(`Converting Error: ${err.stack || err}`); + throw new ImportError('convertFailed'); + } } } - if (!useConvertor && !directDatabaseAccess) { + if (!useConverter && !directDatabaseAccess) { // Read the file with no encoding for raw buffer access. const buf = await fs.readFile(destFile); @@ -224,7 +219,7 @@ const doImport = async (req, res, padId) => { // change text of the pad and broadcast the changeset if (!directDatabaseAccess) { - if (importHandledByPlugin || useConvertor || fileIsHTML) { + if (importHandledByPlugin || useConverter || fileIsHTML) { try { await importHtml.setPadHTML(pad, text); } catch (err) { diff --git a/src/node/hooks/express/importexport.js b/src/node/hooks/express/importexport.js index d6f287c6bbe..ab8d6037612 100644 --- a/src/node/hooks/express/importexport.js +++ b/src/node/hooks/express/importexport.js @@ -60,7 +60,7 @@ exports.expressCreateServer = (hookName, args, cb) => { } console.log(`Exporting pad "${req.params.pad}" in ${req.params.type} format`); - exportHandler.doExport(req, res, padId, readOnlyId, req.params.type); + await exportHandler.doExport(req, res, padId, readOnlyId, req.params.type); } })().catch((err) => next(err || new Error(err))); }); diff --git a/src/node/utils/Abiword.js b/src/node/utils/Abiword.js index b93646cd59a..07abf26c019 100644 --- a/src/node/utils/Abiword.js +++ b/src/node/utils/Abiword.js @@ -24,111 +24,64 @@ const async = require('async'); const settings = require('./Settings'); const os = require('os'); -let doConvertTask; - // on windows we have to spawn a process for each convertion, // cause the plugin abicommand doesn't exist on this platform if (os.type().indexOf('Windows') > -1) { - let stdoutBuffer = ''; - - doConvertTask = (task, callback) => { - // span an abiword process to perform the conversion - const abiword = spawn(settings.abiword, [`--to=${task.destFile}`, task.srcFile]); - - // delegate the processing of stdout to another function - abiword.stdout.on('data', (data) => { - // add data to buffer - stdoutBuffer += data.toString(); - }); - - // append error messages to the buffer - abiword.stderr.on('data', (data) => { - stdoutBuffer += data.toString(); - }); - - // throw exceptions if abiword is dieing - abiword.on('exit', (code) => { - if (code !== 0) { - return callback(`Abiword died with exit code ${code}`); - } - - if (stdoutBuffer !== '') { - console.log(stdoutBuffer); - } - - callback(); + exports.convertFile = async (srcFile, destFile, type) => { + const abiword = spawn(settings.abiword, [`--to=${destFile}`, srcFile]); + let stdoutBuffer = ''; + abiword.stdout.on('data', (data) => { stdoutBuffer += data.toString(); }); + abiword.stderr.on('data', (data) => { stdoutBuffer += data.toString(); }); + await new Promise((resolve, reject) => { + abiword.on('exit', (code) => { + if (code !== 0) return reject(new Error(`Abiword died with exit code ${code}`)); + if (stdoutBuffer !== '') { + console.log(stdoutBuffer); + } + resolve(); + }); }); }; - - exports.convertFile = (srcFile, destFile, type, callback) => { - doConvertTask({srcFile, destFile, type}, callback); - }; // on unix operating systems, we can start abiword with abicommand and // communicate with it via stdin/stdout // thats much faster, about factor 10 } else { - // spawn the abiword process let abiword; let stdoutCallback = null; const spawnAbiword = () => { abiword = spawn(settings.abiword, ['--plugin', 'AbiCommand']); let stdoutBuffer = ''; let firstPrompt = true; - - // append error messages to the buffer - abiword.stderr.on('data', (data) => { - stdoutBuffer += data.toString(); - }); - - // abiword died, let's restart abiword and return an error with the callback + abiword.stderr.on('data', (data) => { stdoutBuffer += data.toString(); }); abiword.on('exit', (code) => { spawnAbiword(); - stdoutCallback(`Abiword died with exit code ${code}`); + if (stdoutCallback != null) stdoutCallback(new Error(`Abiword died with exit code ${code}`)); }); - - // delegate the processing of stdout to a other function abiword.stdout.on('data', (data) => { - // add data to buffer stdoutBuffer += data.toString(); - // we're searching for the prompt, cause this means everything we need is in the buffer if (stdoutBuffer.search('AbiWord:>') !== -1) { - // filter the feedback message - const err = stdoutBuffer.search('OK') !== -1 ? null : stdoutBuffer; - - // reset the buffer + const err = stdoutBuffer.search('OK') !== -1 ? null : new Error(stdoutBuffer); stdoutBuffer = ''; - - // call the callback with the error message - // skip the first prompt if (stdoutCallback != null && !firstPrompt) { stdoutCallback(err); stdoutCallback = null; } - firstPrompt = false; } }); }; spawnAbiword(); - doConvertTask = (task, callback) => { + const queue = async.queue((task, callback) => { abiword.stdin.write(`convert ${task.srcFile} ${task.destFile} ${task.type}\n`); - // create a callback that calls the task callback and the caller callback stdoutCallback = (err) => { - callback(); - console.log('queue continue'); - try { - task.callback(err); - } catch (e) { - console.error('Abiword File failed to convert', e); - } + if (err != null) console.error('Abiword File failed to convert', err); + callback(err); }; - }; + }, 1); - // Queue with the converts we have to do - const queue = async.queue(doConvertTask, 1); - exports.convertFile = (srcFile, destFile, type, callback) => { - queue.push({srcFile, destFile, type, callback}); + exports.convertFile = async (srcFile, destFile, type) => { + await queue.pushAsync({srcFile, destFile, type}); }; } diff --git a/src/node/utils/LibreOffice.js b/src/node/utils/LibreOffice.js index 914d79a4bbf..5fcfa3accf9 100644 --- a/src/node/utils/LibreOffice.js +++ b/src/node/utils/LibreOffice.js @@ -18,7 +18,7 @@ */ const async = require('async'); -const fs = require('fs'); +const fs = require('fs').promises; const log4js = require('log4js'); const os = require('os'); const path = require('path'); @@ -27,80 +27,57 @@ const spawn = require('child_process').spawn; const libreOfficeLogger = log4js.getLogger('LibreOffice'); -const doConvertTask = (task, callback) => { +const doConvertTask = async (task) => { const tmpDir = os.tmpdir(); - async.series([ - /* - * use LibreOffice to convert task.srcFile to another format, given in - * task.type - */ - (callback) => { - libreOfficeLogger.debug( - `Converting ${task.srcFile} to format ${task.type}. The result will be put in ${tmpDir}` - ); - const soffice = spawn(settings.soffice, [ - '--headless', - '--invisible', - '--nologo', - '--nolockcheck', - '--writer', - '--convert-to', - task.type, - task.srcFile, - '--outdir', - tmpDir, - ]); - // Soffice/libreoffice is buggy and often hangs. - // To remedy this we kill the spawned process after a while. - const hangTimeout = setTimeout(() => { - soffice.stdin.pause(); // required to kill hanging threads - soffice.kill(); - }, 120000); - - let stdoutBuffer = ''; - - // Delegate the processing of stdout to another function - soffice.stdout.on('data', (data) => { - stdoutBuffer += data.toString(); - }); - - // Append error messages to the buffer - soffice.stderr.on('data', (data) => { - stdoutBuffer += data.toString(); - }); - - soffice.on('exit', (code) => { - clearTimeout(hangTimeout); - if (code !== 0) { - // Throw an exception if libreoffice failed - return callback(`LibreOffice died with exit code ${code} and message: ${stdoutBuffer}`); - } - - // if LibreOffice exited succesfully, go on with processing - callback(); - }); - }, - - // Move the converted file to the correct place - (callback) => { - const filename = path.basename(task.srcFile); - const sourceFile = `${filename.substr(0, filename.lastIndexOf('.'))}.${task.fileExtension}`; - const sourcePath = path.join(tmpDir, sourceFile); - libreOfficeLogger.debug(`Renaming ${sourcePath} to ${task.destFile}`); - fs.rename(sourcePath, task.destFile, callback); - }, - ], (err) => { - // Invoke the callback for the local queue - callback(); - - // Invoke the callback for the task - task.callback(err); + libreOfficeLogger.debug( + `Converting ${task.srcFile} to format ${task.type}. The result will be put in ${tmpDir}`); + const soffice = spawn(settings.soffice, [ + '--headless', + '--invisible', + '--nologo', + '--nolockcheck', + '--writer', + '--convert-to', + task.type, + task.srcFile, + '--outdir', + tmpDir, + ]); + // Soffice/libreoffice is buggy and often hangs. + // To remedy this we kill the spawned process after a while. + const hangTimeout = setTimeout(() => { + soffice.stdin.pause(); // required to kill hanging threads + soffice.kill(); + }, 120000); + let stdoutBuffer = ''; + soffice.stdout.on('data', (data) => { stdoutBuffer += data.toString(); }); + soffice.stderr.on('data', (data) => { stdoutBuffer += data.toString(); }); + await new Promise((resolve, reject) => { + soffice.on('exit', (code) => { + clearTimeout(hangTimeout); + if (code !== 0) { + const err = + new Error(`LibreOffice died with exit code ${code} and message: ${stdoutBuffer}`); + libreOfficeLogger.error(err.stack); + return reject(err); + } + resolve(); + }); }); + + const filename = path.basename(task.srcFile); + const sourceFile = `${filename.substr(0, filename.lastIndexOf('.'))}.${task.fileExtension}`; + const sourcePath = path.join(tmpDir, sourceFile); + libreOfficeLogger.debug(`Renaming ${sourcePath} to ${task.destFile}`); + await fs.rename(sourcePath, task.destFile); }; // Conversion tasks will be queued up, so we don't overload the system -const queue = async.queue(doConvertTask, 1); +const queue = async.queue( + // For some reason util.callbackify() throws "TypeError [ERR_INVALID_ARG_TYPE]: The last + // argument must be of type Function. Received type object" on Node.js 10.x. + (task, cb) => doConvertTask(task).then(() => cb(), (err) => cb(err || new Error(err))), 1); /** * Convert a file from one type to another @@ -110,7 +87,7 @@ const queue = async.queue(doConvertTask, 1); * @param {String} type The type to convert into * @param {Function} callback Standard callback function */ -exports.convertFile = (srcFile, destFile, type, callback) => { +exports.convertFile = async (srcFile, destFile, type) => { // Used for the moving of the file, not the conversion const fileExtension = type; @@ -129,23 +106,10 @@ exports.convertFile = (srcFile, destFile, type, callback) => { // we need to convert to odt first, then to doc // to avoid `Error: no export filter for /tmp/xxxx.doc` error if (type === 'doc') { - queue.push({ - srcFile, - destFile: destFile.replace(/\.doc$/, '.odt'), - type: 'odt', - callback: () => { - queue.push( - { - srcFile: srcFile.replace(/\.html$/, '.odt'), - destFile, - type, - callback, - fileExtension, - } - ); - }, - }); + const intermediateFile = destFile.replace(/\.doc$/, '.odt'); + await queue.pushAsync({srcFile, destFile: intermediateFile, type: 'odt', fileExtension: 'odt'}); + await queue.pushAsync({srcFile: intermediateFile, destFile, type, fileExtension}); } else { - queue.push({srcFile, destFile, type, callback, fileExtension}); + await queue.pushAsync({srcFile, destFile, type, fileExtension}); } }; diff --git a/src/tests/backend/specs/api/importexport.js b/src/tests/backend/specs/api/importexport.js index 6b2fb3ac90f..ceced447fd1 100644 --- a/src/tests/backend/specs/api/importexport.js +++ b/src/tests/backend/specs/api/importexport.js @@ -6,6 +6,7 @@ * TODO: unify those two files, and merge in a single one. */ +const assert = require('assert').strict; const common = require('../../common'); let agent; @@ -226,6 +227,8 @@ const testImports = { }; describe(__filename, function () { + this.timeout(1000); + before(async function () { agent = await common.init(); }); Object.keys(testImports).forEach((testName) => { @@ -237,73 +240,34 @@ describe(__filename, function () { done(); }); } - it('createPad', function (done) { - this.timeout(200); - agent.get(`${endPoint('createPad')}&padID=${testPadId}`) - .expect((res) => { - if (res.body.code !== 0) throw new Error('Unable to create new Pad'); - }) - .expect('Content-Type', /json/) - .expect(200, done); - }); - it('setHTML', function (done) { - this.timeout(150); - agent.get(`${endPoint('setHTML')}&padID=${testPadId}` + - `&html=${encodeURIComponent(test.input)}`) - .expect((res) => { - if (res.body.code !== 0) throw new Error(`Error:${testName}`); - }) - .expect('Content-Type', /json/) - .expect(200, done); + it('createPad', async function () { + const res = await agent.get(`${endPoint('createPad')}&padID=${testPadId}`) + .expect(200) + .expect('Content-Type', /json/); + assert.equal(res.body.code, 0); }); - it('getHTML', function (done) { - this.timeout(150); - agent.get(`${endPoint('getHTML')}&padID=${testPadId}`) - .expect((res) => { - const gotHtml = res.body.data.html; - if (gotHtml !== test.wantHTML) { - throw new Error(`HTML received from export is not the one we were expecting. - Test Name: - ${testName} - - Got: - ${JSON.stringify(gotHtml)} - - Want: - ${JSON.stringify(test.wantHTML)} - - Which is a different version of the originally imported one: - ${test.input}`); - } - }) - .expect('Content-Type', /json/) - .expect(200, done); + it('setHTML', async function () { + const res = await agent.get(`${endPoint('setHTML')}&padID=${testPadId}` + + `&html=${encodeURIComponent(test.input)}`) + .expect(200) + .expect('Content-Type', /json/); + assert.equal(res.body.code, 0); }); - it('getText', function (done) { - this.timeout(100); - agent.get(`${endPoint('getText')}&padID=${testPadId}`) - .expect((res) => { - const gotText = res.body.data.text; - if (gotText !== test.wantText) { - throw new Error(`Text received from export is not the one we were expecting. - Test Name: - ${testName} - - Got: - ${JSON.stringify(gotText)} - - Want: - ${JSON.stringify(test.wantText)} + it('getHTML', async function () { + const res = await agent.get(`${endPoint('getHTML')}&padID=${testPadId}`) + .expect(200) + .expect('Content-Type', /json/); + assert.equal(res.body.data.html, test.wantHTML); + }); - Which is a different version of the originally imported one: - ${test.input}`); - } - }) - .expect('Content-Type', /json/) - .expect(200, done); + it('getText', async function () { + const res = await agent.get(`${endPoint('getText')}&padID=${testPadId}`) + .expect(200) + .expect('Content-Type', /json/); + assert.equal(res.body.data.text, test.wantText); }); }); }); diff --git a/src/tests/backend/specs/export.js b/src/tests/backend/specs/export.js new file mode 100644 index 00000000000..d2fcde131dc --- /dev/null +++ b/src/tests/backend/specs/export.js @@ -0,0 +1,26 @@ +'use strict'; + +const common = require('../common'); +const padManager = require('../../../node/db/PadManager'); +const settings = require('../../../node/utils/Settings'); + +describe(__filename, function () { + let agent; + const settingsBackup = {}; + + before(async function () { + agent = await common.init(); + settingsBackup.soffice = settings.soffice; + await padManager.getPad('testExportPad', 'test content'); + }); + + after(async function () { + Object.assign(settings, settingsBackup); + }); + + it('returns 500 on export error', async function () { + settings.soffice = 'false'; // '/bin/false' doesn't work on Windows + await agent.get('/p/testExportPad/export/doc') + .expect(500); + }); +});