From a6fc45c7c7829827df97c8d6b4de635ab12aa1e7 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 17 Mar 2021 15:31:05 -0400 Subject: [PATCH 01/20] tests: Increase import/export test timeouts --- src/tests/backend/specs/api/importexport.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/tests/backend/specs/api/importexport.js b/src/tests/backend/specs/api/importexport.js index 6b2fb3ac90f..6c74cd9770b 100644 --- a/src/tests/backend/specs/api/importexport.js +++ b/src/tests/backend/specs/api/importexport.js @@ -226,6 +226,8 @@ const testImports = { }; describe(__filename, function () { + this.timeout(1000); + before(async function () { agent = await common.init(); }); Object.keys(testImports).forEach((testName) => { @@ -238,7 +240,6 @@ describe(__filename, function () { }); } 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'); @@ -248,7 +249,6 @@ describe(__filename, function () { }); it('setHTML', function (done) { - this.timeout(150); agent.get(`${endPoint('setHTML')}&padID=${testPadId}` + `&html=${encodeURIComponent(test.input)}`) .expect((res) => { @@ -259,7 +259,6 @@ describe(__filename, function () { }); it('getHTML', function (done) { - this.timeout(150); agent.get(`${endPoint('getHTML')}&padID=${testPadId}`) .expect((res) => { const gotHtml = res.body.data.html; @@ -283,7 +282,6 @@ describe(__filename, function () { }); it('getText', function (done) { - this.timeout(100); agent.get(`${endPoint('getText')}&padID=${testPadId}`) .expect((res) => { const gotText = res.body.data.text; From 0eb756f60d2069d1c936d6f46311fabf61fa50b0 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 17 Mar 2021 15:37:12 -0400 Subject: [PATCH 02/20] tests: Promisify import/export tests --- src/tests/backend/specs/api/importexport.js | 43 +++++++++++---------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/src/tests/backend/specs/api/importexport.js b/src/tests/backend/specs/api/importexport.js index 6c74cd9770b..1626d3e6e13 100644 --- a/src/tests/backend/specs/api/importexport.js +++ b/src/tests/backend/specs/api/importexport.js @@ -239,27 +239,30 @@ describe(__filename, function () { done(); }); } - it('createPad', function (done) { - agent.get(`${endPoint('createPad')}&padID=${testPadId}`) + + it('createPad', async function () { + await agent.get(`${endPoint('createPad')}&padID=${testPadId}`) + .expect(200) + .expect('Content-Type', /json/) .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) { - agent.get(`${endPoint('setHTML')}&padID=${testPadId}` + - `&html=${encodeURIComponent(test.input)}`) + it('setHTML', async function () { + await agent.get(`${endPoint('setHTML')}&padID=${testPadId}` + + `&html=${encodeURIComponent(test.input)}`) + .expect(200) + .expect('Content-Type', /json/) .expect((res) => { if (res.body.code !== 0) throw new Error(`Error:${testName}`); - }) - .expect('Content-Type', /json/) - .expect(200, done); + }); }); - it('getHTML', function (done) { - agent.get(`${endPoint('getHTML')}&padID=${testPadId}`) + it('getHTML', async function () { + await agent.get(`${endPoint('getHTML')}&padID=${testPadId}`) + .expect(200) + .expect('Content-Type', /json/) .expect((res) => { const gotHtml = res.body.data.html; if (gotHtml !== test.wantHTML) { @@ -276,13 +279,13 @@ describe(__filename, function () { Which is a different version of the originally imported one: ${test.input}`); } - }) - .expect('Content-Type', /json/) - .expect(200, done); + }); }); - it('getText', function (done) { - agent.get(`${endPoint('getText')}&padID=${testPadId}`) + it('getText', async function () { + await agent.get(`${endPoint('getText')}&padID=${testPadId}`) + .expect(200) + .expect('Content-Type', /json/) .expect((res) => { const gotText = res.body.data.text; if (gotText !== test.wantText) { @@ -299,9 +302,7 @@ describe(__filename, function () { Which is a different version of the originally imported one: ${test.input}`); } - }) - .expect('Content-Type', /json/) - .expect(200, done); + }); }); }); }); From 79b0fe0a3d019785d097f93a7a2a2ce2f9651333 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 17 Mar 2021 15:38:10 -0400 Subject: [PATCH 03/20] tests: Use `assert` to simplify import/export tests --- src/tests/backend/specs/api/importexport.js | 61 +++++---------------- 1 file changed, 13 insertions(+), 48 deletions(-) diff --git a/src/tests/backend/specs/api/importexport.js b/src/tests/backend/specs/api/importexport.js index 1626d3e6e13..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; @@ -241,68 +242,32 @@ describe(__filename, function () { } it('createPad', async function () { - await agent.get(`${endPoint('createPad')}&padID=${testPadId}`) + const res = await agent.get(`${endPoint('createPad')}&padID=${testPadId}`) .expect(200) - .expect('Content-Type', /json/) - .expect((res) => { - if (res.body.code !== 0) throw new Error('Unable to create new Pad'); - }); + .expect('Content-Type', /json/); + assert.equal(res.body.code, 0); }); it('setHTML', async function () { - await agent.get(`${endPoint('setHTML')}&padID=${testPadId}` + + const res = await agent.get(`${endPoint('setHTML')}&padID=${testPadId}` + `&html=${encodeURIComponent(test.input)}`) .expect(200) - .expect('Content-Type', /json/) - .expect((res) => { - if (res.body.code !== 0) throw new Error(`Error:${testName}`); - }); + .expect('Content-Type', /json/); + assert.equal(res.body.code, 0); }); it('getHTML', async function () { - await agent.get(`${endPoint('getHTML')}&padID=${testPadId}`) + const res = await agent.get(`${endPoint('getHTML')}&padID=${testPadId}`) .expect(200) - .expect('Content-Type', /json/) - .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/); + assert.equal(res.body.data.html, test.wantHTML); }); it('getText', async function () { - await agent.get(`${endPoint('getText')}&padID=${testPadId}`) + const res = await agent.get(`${endPoint('getText')}&padID=${testPadId}`) .expect(200) - .expect('Content-Type', /json/) - .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)} - - Which is a different version of the originally imported one: - ${test.input}`); - } - }); + .expect('Content-Type', /json/); + assert.equal(res.body.data.text, test.wantText); }); }); }); From 2c88ce6d813a682b8acdfb3cfe62b92fcf981e91 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 17 Mar 2021 18:17:07 -0400 Subject: [PATCH 04/20] import/export: Delete unnecessary comments --- src/node/handler/ExportHandler.js | 15 ++++-------- src/node/utils/Abiword.js | 40 +++---------------------------- src/node/utils/LibreOffice.js | 25 ++----------------- 3 files changed, 9 insertions(+), 71 deletions(-) diff --git a/src/node/handler/ExportHandler.js b/src/node/handler/ExportHandler.js index fbb9e57da60..f84c53ad1a1 100644 --- a/src/node/handler/ExportHandler.js +++ b/src/node/handler/ExportHandler.js @@ -33,17 +33,10 @@ 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 convertor = + settings.soffice != null ? require('../utils/LibreOffice') + : settings.abiword != null ? require('../utils/Abiword') + : null; const tempDirectory = os.tmpdir(); diff --git a/src/node/utils/Abiword.js b/src/node/utils/Abiword.js index b93646cd59a..79e4954150e 100644 --- a/src/node/utils/Abiword.js +++ b/src/node/utils/Abiword.js @@ -32,30 +32,16 @@ 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.stdout.on('data', (data) => { stdoutBuffer += data.toString(); }); + abiword.stderr.on('data', (data) => { stdoutBuffer += data.toString(); }); abiword.on('exit', (code) => { if (code !== 0) { return callback(`Abiword died with exit code ${code}`); } - if (stdoutBuffer !== '') { console.log(stdoutBuffer); } - callback(); }); }; @@ -67,45 +53,27 @@ if (os.type().indexOf('Windows') > -1) { // 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}`); }); - - // 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 stdoutBuffer = ''; - - // call the callback with the error message - // skip the first prompt if (stdoutCallback != null && !firstPrompt) { stdoutCallback(err); stdoutCallback = null; } - firstPrompt = false; } }); @@ -114,7 +82,6 @@ if (os.type().indexOf('Windows') > -1) { doConvertTask = (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'); @@ -126,7 +93,6 @@ if (os.type().indexOf('Windows') > -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}); diff --git a/src/node/utils/LibreOffice.js b/src/node/utils/LibreOffice.js index 914d79a4bbf..3c18df59c4e 100644 --- a/src/node/utils/LibreOffice.js +++ b/src/node/utils/LibreOffice.js @@ -31,10 +31,6 @@ const doConvertTask = (task, callback) => { 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}` @@ -57,32 +53,18 @@ const doConvertTask = (task, callback) => { 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.stdout.on('data', (data) => { stdoutBuffer += data.toString(); }); + 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}`; @@ -91,10 +73,7 @@ const doConvertTask = (task, callback) => { fs.rename(sourcePath, task.destFile, callback); }, ], (err) => { - // Invoke the callback for the local queue callback(); - - // Invoke the callback for the task task.callback(err); }); }; From 2f089e8f286baa2873c6f5f79f732a7befde2cea Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 17 Mar 2021 18:23:32 -0400 Subject: [PATCH 05/20] Abiword: Reset stdout buffer when starting abiword --- src/node/utils/Abiword.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/node/utils/Abiword.js b/src/node/utils/Abiword.js index 79e4954150e..c7e45e86d62 100644 --- a/src/node/utils/Abiword.js +++ b/src/node/utils/Abiword.js @@ -29,10 +29,9 @@ 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) => { const abiword = spawn(settings.abiword, [`--to=${task.destFile}`, task.srcFile]); + let stdoutBuffer = ''; abiword.stdout.on('data', (data) => { stdoutBuffer += data.toString(); }); abiword.stderr.on('data', (data) => { stdoutBuffer += data.toString(); }); abiword.on('exit', (code) => { From 0f0b64e14137cac9041d89cca39b0484ea1dc893 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 17 Mar 2021 18:35:16 -0400 Subject: [PATCH 06/20] ExportHandler: Replace unnecessary exception with `return` --- src/node/handler/ExportHandler.js | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/node/handler/ExportHandler.js b/src/node/handler/ExportHandler.js index f84c53ad1a1..ea8dd446607 100644 --- a/src/node/handler/ExportHandler.js +++ b/src/node/handler/ExportHandler.js @@ -43,7 +43,7 @@ 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; @@ -78,7 +78,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 @@ -121,11 +121,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; - } - }); -}; From 478d7cfcd7739f7d871172b2cdffb208dd05d628 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 17 Mar 2021 18:28:39 -0400 Subject: [PATCH 07/20] import/export: Use Error objects for errors, not strings --- src/node/handler/ExportHandler.js | 2 +- src/node/utils/Abiword.js | 8 +++----- src/node/utils/LibreOffice.js | 3 ++- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/node/handler/ExportHandler.js b/src/node/handler/ExportHandler.js index ea8dd446607..a13906b42d1 100644 --- a/src/node/handler/ExportHandler.js +++ b/src/node/handler/ExportHandler.js @@ -102,7 +102,7 @@ exports.doExport = async (req, res, padId, readOnlyId, type) => { // @TODO no Promise interface for convertors (yet) await new Promise((resolve, reject) => { convertor.convertFile(srcFile, destFile, type, (err) => { - err ? reject('convertFailed') : resolve(); + err ? reject(new Error('convertFailed')) : resolve(); }); }); } diff --git a/src/node/utils/Abiword.js b/src/node/utils/Abiword.js index c7e45e86d62..f77b69cf2a7 100644 --- a/src/node/utils/Abiword.js +++ b/src/node/utils/Abiword.js @@ -35,9 +35,7 @@ if (os.type().indexOf('Windows') > -1) { abiword.stdout.on('data', (data) => { stdoutBuffer += data.toString(); }); abiword.stderr.on('data', (data) => { stdoutBuffer += data.toString(); }); abiword.on('exit', (code) => { - if (code !== 0) { - return callback(`Abiword died with exit code ${code}`); - } + if (code !== 0) return callback(new Error(`Abiword died with exit code ${code}`)); if (stdoutBuffer !== '') { console.log(stdoutBuffer); } @@ -61,13 +59,13 @@ if (os.type().indexOf('Windows') > -1) { abiword.stderr.on('data', (data) => { stdoutBuffer += data.toString(); }); abiword.on('exit', (code) => { spawnAbiword(); - stdoutCallback(`Abiword died with exit code ${code}`); + stdoutCallback(new Error(`Abiword died with exit code ${code}`)); }); abiword.stdout.on('data', (data) => { stdoutBuffer += data.toString(); // we're searching for the prompt, cause this means everything we need is in the buffer if (stdoutBuffer.search('AbiWord:>') !== -1) { - const err = stdoutBuffer.search('OK') !== -1 ? null : stdoutBuffer; + const err = stdoutBuffer.search('OK') !== -1 ? null : new Error(stdoutBuffer); stdoutBuffer = ''; if (stdoutCallback != null && !firstPrompt) { stdoutCallback(err); diff --git a/src/node/utils/LibreOffice.js b/src/node/utils/LibreOffice.js index 3c18df59c4e..04d330e35ac 100644 --- a/src/node/utils/LibreOffice.js +++ b/src/node/utils/LibreOffice.js @@ -59,7 +59,8 @@ const doConvertTask = (task, callback) => { soffice.on('exit', (code) => { clearTimeout(hangTimeout); if (code !== 0) { - return callback(`LibreOffice died with exit code ${code} and message: ${stdoutBuffer}`); + return callback( + new Error(`LibreOffice died with exit code ${code} and message: ${stdoutBuffer}`)); } callback(); }); From 8b900a44e1ad9589a1d9feaf80b965a33edb5a30 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 17 Mar 2021 18:40:42 -0400 Subject: [PATCH 08/20] ExportHandler: Pass the error unmodified --- src/node/handler/ExportHandler.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/node/handler/ExportHandler.js b/src/node/handler/ExportHandler.js index a13906b42d1..5b1581a5143 100644 --- a/src/node/handler/ExportHandler.js +++ b/src/node/handler/ExportHandler.js @@ -100,11 +100,7 @@ exports.doExport = async (req, res, padId, readOnlyId, type) => { // 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(new Error('convertFailed')) : resolve(); - }); - }); + await util.promisify(convertor.convertFile)(srcFile, destFile, type); } // send the file From db28add475f17821ff7d3f159aaf8072c3243a29 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 17 Mar 2021 18:53:07 -0400 Subject: [PATCH 09/20] import/export: Spelling fix: "convertor" -> "converter" --- src/node/handler/ExportHandler.js | 8 ++++---- src/node/handler/ImportHandler.js | 26 +++++++++++++------------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/node/handler/ExportHandler.js b/src/node/handler/ExportHandler.js index 5b1581a5143..38215f9c130 100644 --- a/src/node/handler/ExportHandler.js +++ b/src/node/handler/ExportHandler.js @@ -33,7 +33,7 @@ const util = require('util'); const fsp_writeFile = util.promisify(fs.writeFile); const fsp_unlink = util.promisify(fs.unlink); -const convertor = +const converter = settings.soffice != null ? require('../utils/LibreOffice') : settings.abiword != null ? require('../utils/Abiword') : null; @@ -91,7 +91,7 @@ exports.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 @@ -99,8 +99,8 @@ exports.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 util.promisify(convertor.convertFile)(srcFile, destFile, type); + // @TODO no Promise interface for converters (yet) + await util.promisify(converter.convertFile)(srcFile, destFile, type); } // send the file diff --git a/src/node/handler/ImportHandler.js b/src/node/handler/ImportHandler.js index d97ce91ef75..5dbbf492c63 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,18 +170,18 @@ 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) + // @TODO - no Promise interface for converters (yet) await new Promise((resolve, reject) => { - convertor.convertFile(srcFile, destFile, exportExtension, (err) => { + converter.convertFile(srcFile, destFile, exportExtension, (err) => { // catch convert errors if (err) { logger.warn(`Converting Error: ${err.stack || err}`); @@ -193,7 +193,7 @@ const doImport = async (req, res, padId) => { } } - if (!useConvertor && !directDatabaseAccess) { + if (!useConverter && !directDatabaseAccess) { // Read the file with no encoding for raw buffer access. const buf = await fs.readFile(destFile); @@ -224,7 +224,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) { From de0e7b0fad2c8d563390dc96617340808eee7d13 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 17 Mar 2021 18:54:57 -0400 Subject: [PATCH 10/20] import/export: On export error return 500 instead of crashing --- src/node/handler/ExportHandler.js | 9 ++++----- src/node/hooks/express/importexport.js | 2 +- src/tests/backend/specs/export.js | 26 ++++++++++++++++++++++++++ 3 files changed, 31 insertions(+), 6 deletions(-) create mode 100644 src/tests/backend/specs/export.js diff --git a/src/node/handler/ExportHandler.js b/src/node/handler/ExportHandler.js index 38215f9c130..7dbcf7e2d51 100644 --- a/src/node/handler/ExportHandler.js +++ b/src/node/handler/ExportHandler.js @@ -33,11 +33,6 @@ const util = require('util'); const fsp_writeFile = util.promisify(fs.writeFile); const fsp_unlink = util.promisify(fs.unlink); -const converter = - settings.soffice != null ? require('../utils/LibreOffice') - : settings.abiword != null ? require('../utils/Abiword') - : null; - const tempDirectory = os.tmpdir(); /** @@ -99,6 +94,10 @@ exports.doExport = async (req, res, padId, readOnlyId, type) => { if (result.length > 0) { // console.log("export handled by plugin", destFile); } else { + const converter = + settings.soffice != null ? require('../utils/LibreOffice') + : settings.abiword != null ? require('../utils/Abiword') + : null; // @TODO no Promise interface for converters (yet) await util.promisify(converter.convertFile)(srcFile, destFile, type); } 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/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); + }); +}); From bcaebb684bb052145d3a855e74d18d411f756676 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 17 Mar 2021 19:05:14 -0400 Subject: [PATCH 11/20] Abiword: Don't call the callback if null --- src/node/utils/Abiword.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node/utils/Abiword.js b/src/node/utils/Abiword.js index f77b69cf2a7..d21a9783d6e 100644 --- a/src/node/utils/Abiword.js +++ b/src/node/utils/Abiword.js @@ -59,7 +59,7 @@ if (os.type().indexOf('Windows') > -1) { abiword.stderr.on('data', (data) => { stdoutBuffer += data.toString(); }); abiword.on('exit', (code) => { spawnAbiword(); - stdoutCallback(new Error(`Abiword died with exit code ${code}`)); + if (stdoutCallback != null) stdoutCallback(new Error(`Abiword died with exit code ${code}`)); }); abiword.stdout.on('data', (data) => { stdoutBuffer += data.toString(); From 545ac5a10fd9bb80142bba71031d000cca03f9b4 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 17 Mar 2021 19:06:50 -0400 Subject: [PATCH 12/20] Abiword: Reduce log spam --- src/node/utils/Abiword.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/node/utils/Abiword.js b/src/node/utils/Abiword.js index d21a9783d6e..debb77db9a2 100644 --- a/src/node/utils/Abiword.js +++ b/src/node/utils/Abiword.js @@ -81,7 +81,6 @@ if (os.type().indexOf('Windows') > -1) { abiword.stdin.write(`convert ${task.srcFile} ${task.destFile} ${task.type}\n`); stdoutCallback = (err) => { callback(); - console.log('queue continue'); try { task.callback(err); } catch (e) { From b5b957abd11dc9f0d4caa8c2d0b391df2d7cc2bf Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 17 Mar 2021 19:09:53 -0400 Subject: [PATCH 13/20] Abiword: Fix logging of conversion failure --- src/node/utils/Abiword.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/node/utils/Abiword.js b/src/node/utils/Abiword.js index debb77db9a2..348c76a0d48 100644 --- a/src/node/utils/Abiword.js +++ b/src/node/utils/Abiword.js @@ -81,11 +81,8 @@ if (os.type().indexOf('Windows') > -1) { abiword.stdin.write(`convert ${task.srcFile} ${task.destFile} ${task.type}\n`); stdoutCallback = (err) => { callback(); - 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); + task.callback(err); }; }; From aba6e53d2da56dd6786a60ea44ed4dc40a3cbb22 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 17 Mar 2021 19:14:36 -0400 Subject: [PATCH 14/20] Abiword: Use the async-provided callback to signal errors This avoids having two callbacks, which improves readability. --- src/node/utils/Abiword.js | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/node/utils/Abiword.js b/src/node/utils/Abiword.js index 348c76a0d48..ca80967f243 100644 --- a/src/node/utils/Abiword.js +++ b/src/node/utils/Abiword.js @@ -77,17 +77,15 @@ if (os.type().indexOf('Windows') > -1) { }; spawnAbiword(); - doConvertTask = (task, callback) => { + const queue = async.queue((task, callback) => { abiword.stdin.write(`convert ${task.srcFile} ${task.destFile} ${task.type}\n`); stdoutCallback = (err) => { - callback(); if (err != null) console.error('Abiword File failed to convert', err); - task.callback(err); + callback(err); }; - }; + }, 1); - const queue = async.queue(doConvertTask, 1); exports.convertFile = (srcFile, destFile, type, callback) => { - queue.push({srcFile, destFile, type, callback}); + queue.push({srcFile, destFile, type}, callback); }; } From ae87c3e2aaa2d8d9ac6f5c4098a3fe567376843c Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 18 Mar 2021 01:55:29 -0400 Subject: [PATCH 15/20] LibreOffice: Add missing `fileExtension` property on intermediate step --- src/node/utils/LibreOffice.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/node/utils/LibreOffice.js b/src/node/utils/LibreOffice.js index 04d330e35ac..a263f3d2faa 100644 --- a/src/node/utils/LibreOffice.js +++ b/src/node/utils/LibreOffice.js @@ -113,6 +113,7 @@ exports.convertFile = (srcFile, destFile, type, callback) => { srcFile, destFile: destFile.replace(/\.doc$/, '.odt'), type: 'odt', + fileExtension: 'odt', callback: () => { queue.push( { From 3e0ea9df9fbe7b771495053f0ce57a81a5735f9d Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 18 Mar 2021 00:13:38 -0400 Subject: [PATCH 16/20] LibreOffice: Use consistent intermediate filename --- src/node/utils/LibreOffice.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/node/utils/LibreOffice.js b/src/node/utils/LibreOffice.js index a263f3d2faa..f3485310d1a 100644 --- a/src/node/utils/LibreOffice.js +++ b/src/node/utils/LibreOffice.js @@ -109,15 +109,16 @@ 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') { + const intermediateFile = destFile.replace(/\.doc$/, '.odt'); queue.push({ srcFile, - destFile: destFile.replace(/\.doc$/, '.odt'), + destFile: intermediateFile, type: 'odt', fileExtension: 'odt', callback: () => { queue.push( { - srcFile: srcFile.replace(/\.html$/, '.odt'), + srcFile: intermediateFile, destFile, type, callback, From 62de5c7aa50233d8e64db4a5883591dd8abe3e8e Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 18 Mar 2021 00:19:28 -0400 Subject: [PATCH 17/20] LibreOffice: Use `async.series` to properly handle conversion errors --- src/node/utils/LibreOffice.js | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/src/node/utils/LibreOffice.js b/src/node/utils/LibreOffice.js index f3485310d1a..168c376ec44 100644 --- a/src/node/utils/LibreOffice.js +++ b/src/node/utils/LibreOffice.js @@ -110,23 +110,22 @@ exports.convertFile = (srcFile, destFile, type, callback) => { // to avoid `Error: no export filter for /tmp/xxxx.doc` error if (type === 'doc') { const intermediateFile = destFile.replace(/\.doc$/, '.odt'); - queue.push({ - srcFile, - destFile: intermediateFile, - type: 'odt', - fileExtension: 'odt', - callback: () => { - queue.push( - { - srcFile: intermediateFile, - destFile, - type, - callback, - fileExtension, - } - ); - }, - }); + async.series([ + (callback) => queue.push({ + srcFile, + destFile: intermediateFile, + type: 'odt', + fileExtension: 'odt', + callback, + }), + (callback) => queue.push({ + srcFile: intermediateFile, + destFile, + type, + callback, + fileExtension, + }), + ], callback); } else { queue.push({srcFile, destFile, type, callback, fileExtension}); } From 9c53342fd9d084a7f8678b66eaf9953b2063bddd Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 17 Mar 2021 19:14:36 -0400 Subject: [PATCH 18/20] LibreOffice: Use the async-provided callback to signal errors This avoids having two callbacks, which improves readability. --- src/node/utils/LibreOffice.js | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/node/utils/LibreOffice.js b/src/node/utils/LibreOffice.js index 168c376ec44..7d5416dcf3a 100644 --- a/src/node/utils/LibreOffice.js +++ b/src/node/utils/LibreOffice.js @@ -73,10 +73,7 @@ const doConvertTask = (task, callback) => { libreOfficeLogger.debug(`Renaming ${sourcePath} to ${task.destFile}`); fs.rename(sourcePath, task.destFile, callback); }, - ], (err) => { - callback(); - task.callback(err); - }); + ], callback); }; // Conversion tasks will be queued up, so we don't overload the system @@ -116,17 +113,15 @@ exports.convertFile = (srcFile, destFile, type, callback) => { destFile: intermediateFile, type: 'odt', fileExtension: 'odt', - callback, - }), + }, callback), (callback) => queue.push({ srcFile: intermediateFile, destFile, type, - callback, fileExtension, - }), + }, callback), ], callback); } else { - queue.push({srcFile, destFile, type, callback, fileExtension}); + queue.push({srcFile, destFile, type, fileExtension}, callback); } }; From 944cdfe0bc1d6580f06f0e97c5ef31ad85627454 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 18 Mar 2021 01:01:47 -0400 Subject: [PATCH 19/20] import/export: Promisify Abiword and LibreOffice conversion --- src/node/handler/ExportHandler.js | 3 +- src/node/handler/ImportHandler.js | 17 ++--- src/node/utils/Abiword.js | 28 ++++---- src/node/utils/LibreOffice.js | 110 +++++++++++++----------------- 4 files changed, 67 insertions(+), 91 deletions(-) diff --git a/src/node/handler/ExportHandler.js b/src/node/handler/ExportHandler.js index 7dbcf7e2d51..0bf75a17f11 100644 --- a/src/node/handler/ExportHandler.js +++ b/src/node/handler/ExportHandler.js @@ -98,8 +98,7 @@ exports.doExport = async (req, res, padId, readOnlyId, type) => { settings.soffice != null ? require('../utils/LibreOffice') : settings.abiword != null ? require('../utils/Abiword') : null; - // @TODO no Promise interface for converters (yet) - await util.promisify(converter.convertFile)(srcFile, destFile, type); + await converter.convertFile(srcFile, destFile, type); } // send the file diff --git a/src/node/handler/ImportHandler.js b/src/node/handler/ImportHandler.js index 5dbbf492c63..acaaf092707 100644 --- a/src/node/handler/ImportHandler.js +++ b/src/node/handler/ImportHandler.js @@ -179,17 +179,12 @@ const doImport = async (req, res, padId) => { // if no converter only rename await fs.rename(srcFile, destFile); } else { - // @TODO - no Promise interface for converters (yet) - await new Promise((resolve, reject) => { - converter.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'); + } } } diff --git a/src/node/utils/Abiword.js b/src/node/utils/Abiword.js index ca80967f243..07abf26c019 100644 --- a/src/node/utils/Abiword.js +++ b/src/node/utils/Abiword.js @@ -24,28 +24,24 @@ 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) { - doConvertTask = (task, callback) => { - const abiword = spawn(settings.abiword, [`--to=${task.destFile}`, task.srcFile]); + 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(); }); - abiword.on('exit', (code) => { - if (code !== 0) return callback(new Error(`Abiword died with exit code ${code}`)); - if (stdoutBuffer !== '') { - console.log(stdoutBuffer); - } - callback(); + 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 @@ -85,7 +81,7 @@ if (os.type().indexOf('Windows') > -1) { }; }, 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 7d5416dcf3a..d30af5b223e 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,57 +27,55 @@ 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([ - (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 = ''; - soffice.stdout.on('data', (data) => { stdoutBuffer += data.toString(); }); - soffice.stderr.on('data', (data) => { stdoutBuffer += data.toString(); }); - soffice.on('exit', (code) => { - clearTimeout(hangTimeout); - if (code !== 0) { - return callback( - new Error(`LibreOffice died with exit code ${code} and message: ${stdoutBuffer}`)); - } - 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 = ''; + 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) { + return reject( + new Error(`LibreOffice died with exit code ${code} and message: ${stdoutBuffer}`)); + } + resolve(); + }); + }); - (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); - }, - ], 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}`); + 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 @@ -87,7 +85,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; @@ -107,21 +105,9 @@ exports.convertFile = (srcFile, destFile, type, callback) => { // to avoid `Error: no export filter for /tmp/xxxx.doc` error if (type === 'doc') { const intermediateFile = destFile.replace(/\.doc$/, '.odt'); - async.series([ - (callback) => queue.push({ - srcFile, - destFile: intermediateFile, - type: 'odt', - fileExtension: 'odt', - }, callback), - (callback) => queue.push({ - srcFile: intermediateFile, - destFile, - type, - fileExtension, - }, callback), - ], callback); + await queue.pushAsync({srcFile, destFile: intermediateFile, type: 'odt', fileExtension: 'odt'}); + await queue.pushAsync({srcFile: intermediateFile, destFile, type, fileExtension}); } else { - queue.push({srcFile, destFile, type, fileExtension}, callback); + await queue.pushAsync({srcFile, destFile, type, fileExtension}); } }; From fd1164c91ec92ccc6eba193547e7c422d37fb18c Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 18 Mar 2021 01:58:25 -0400 Subject: [PATCH 20/20] LibreOffice: Log conversion errors --- src/node/utils/LibreOffice.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/node/utils/LibreOffice.js b/src/node/utils/LibreOffice.js index d30af5b223e..5fcfa3accf9 100644 --- a/src/node/utils/LibreOffice.js +++ b/src/node/utils/LibreOffice.js @@ -57,8 +57,10 @@ const doConvertTask = async (task) => { soffice.on('exit', (code) => { clearTimeout(hangTimeout); if (code !== 0) { - return reject( - new Error(`LibreOffice died with exit code ${code} and message: ${stdoutBuffer}`)); + const err = + new Error(`LibreOffice died with exit code ${code} and message: ${stdoutBuffer}`); + libreOfficeLogger.error(err.stack); + return reject(err); } resolve(); });