diff --git a/services/clsi/app/js/ConversionController.js b/services/clsi/app/js/ConversionController.js index 6bb37a36a7..d6af00638f 100644 --- a/services/clsi/app/js/ConversionController.js +++ b/services/clsi/app/js/ConversionController.js @@ -11,6 +11,7 @@ import RequestParser from './RequestParser.js' import { pipeline } from 'node:stream/promises' import Settings from '@overleaf/settings' import Path from 'node:path' +import { ConversionError } from './Errors.js' const CONVERSION_CONFIGS = { docx: { extension: 'docx' }, @@ -37,6 +38,23 @@ async function convertDocumentToLaTeX(req, res) { path, conversionType ) + } catch (err) { + if (err instanceof ConversionError) { + if (err.isUserFacing) { + return res.status(422).json({ + error: err.stderr, + exitCode: err.exitCode, + }) + } else { + logger.warn( + { err, conversionType, stderr: err.stderr }, + 'Conversion failed with non-user-facing error' + ) + return res.status(422).json({}) + } + } else { + throw err + } } finally { await fs.unlink(path).catch(() => {}) } @@ -124,6 +142,23 @@ async function convertProjectToDocument(req, res) { const readStream = fsSync.createReadStream(documentPath) await pipeline(readStream, res) } + } catch (err) { + if (err instanceof ConversionError) { + if (err.isUserFacing) { + return res.status(422).json({ + error: err.stderr, + exitCode: err.exitCode, + }) + } else { + logger.warn( + { err, type, stderr: err.stderr }, + 'Conversion failed with non-user-facing error' + ) + return res.status(422).json({}) + } + } else { + throw err + } } finally { await fs.rm(conversionDir, { recursive: true, force: true }).catch(() => {}) } diff --git a/services/clsi/app/js/ConversionManager.js b/services/clsi/app/js/ConversionManager.js index 32825aecc9..6f37a69002 100644 --- a/services/clsi/app/js/ConversionManager.js +++ b/services/clsi/app/js/ConversionManager.js @@ -5,6 +5,7 @@ import Path from 'node:path' import CommandRunner from './CommandRunner.js' import LockManager from './LockManager.js' import OError from '@overleaf/o-error' +import { ConversionError } from './Errors.js' const CONVERSION_CONFIGS = { docx: { @@ -72,7 +73,8 @@ async function convertToLaTeX( null ) if (exitCodePandoc !== 0) { - throw new OError('Non-zero exit code from pandoc', { + throw new ConversionError('Non-zero exit code from pandoc', { + type: conversionType, exitCode: exitCodePandoc, stderr: stderrPandoc, }) @@ -112,6 +114,9 @@ async function convertToLaTeX( } catch (error) { // Clean up the conversion directory on error to avoid leaving failed conversions around await fs.rm(conversionDir, { force: true, recursive: true }).catch(() => {}) + if (error instanceof ConversionError) { + throw error + } throw new OError('pandoc conversion failed').withCause(error) } @@ -204,10 +209,9 @@ async function convertLaTeXToDocumentInDir( ) if (exitCode !== 0) { - throw new OError('pandoc latex-to-document conversion failed', { + throw new ConversionError('pandoc latex-to-document conversion failed', { type, exitCode, - stdout, stderr, }) } @@ -251,10 +255,9 @@ async function convertLaTeXToDocumentInDir( ) if (exitCode !== 0) { - throw new OError('pandoc latex-to-document conversion failed', { + throw new ConversionError('pandoc latex-to-document conversion failed', { type, exitCode, - stdout, stderr, }) } diff --git a/services/clsi/app/js/Errors.js b/services/clsi/app/js/Errors.js index bf20bb3c90..949f7eb88d 100644 --- a/services/clsi/app/js/Errors.js +++ b/services/clsi/app/js/Errors.js @@ -35,6 +35,37 @@ export class TooManyCompileRequestsError extends OError {} export class InvalidParameter extends OError {} export class MissingUpdatesError extends OError {} +export class ConversionError extends OError { + static USER_FACING_ERRORS = new Set([ + 1, // IO error + 23, // Unsupported extension + 24, // Citeproc error + 25, // Other bibliography error + 44, // Malformed XML error + 63, // Generic error (e.g. malformed docx container) + 64, // Parse error + 91, // Macro loop + 92, // UTF8 decoding error + 94, // Unsupported char set + 95, // Input not text + 97, // Missing data file + 98, // Missing metadata file + 99, // Missing file + ]) + + isUserFacing + stderr + exitCode + + constructor(message, { type, stderr, exitCode }) { + const isUserFacingError = ConversionError.USER_FACING_ERRORS.has(exitCode) + super(message, { exitCode, type }) + this.isUserFacing = isUserFacingError + this.stderr = stderr + this.exitCode = exitCode + } +} + export default { QueueLimitReachedError, TimedOutError, @@ -45,4 +76,5 @@ export default { TooManyCompileRequestsError, InvalidParameter, MissingUpdatesError, + ConversionError, } diff --git a/services/clsi/test/acceptance/js/ConversionTests.js b/services/clsi/test/acceptance/js/ConversionTests.js index a855c34cd3..ae7337c42d 100644 --- a/services/clsi/test/acceptance/js/ConversionTests.js +++ b/services/clsi/test/acceptance/js/ConversionTests.js @@ -28,7 +28,7 @@ describe('Conversions', function () { const outputStream = fs.createWriteStream( '/tmp/clsi_acceptance_tests_' + crypto.randomUUID() + '.zip' ) - const stream = await Client.convertDocument(sourcePath, 'docx') + const { stream } = await Client.convertDocument(sourcePath, 'docx') await pipeline(stream, outputStream) await new Promise((resolve, reject) => { @@ -75,13 +75,14 @@ describe('Conversions', function () { }) }) - it('should fail when file is not a docx', async function () { + it('should fail with 422 and surface pandoc stderr when file is not a docx', async function () { const sourcePath = Path.join( import.meta.dirname, '../fixtures/minimal.pdf' ) - await expect(Client.convertDocument(sourcePath, 'docx')).to.eventually.be - .rejected + const { status, body } = await Client.convertDocument(sourcePath, 'docx') + expect(status).to.equal(422) + expect(body.error).to.include("couldn't unpack docx container") }) }) diff --git a/services/clsi/test/acceptance/js/helpers/Client.js b/services/clsi/test/acceptance/js/helpers/Client.js index d9b0a81eb7..67f78161f1 100644 --- a/services/clsi/test/acceptance/js/helpers/Client.js +++ b/services/clsi/test/acceptance/js/helpers/Client.js @@ -33,10 +33,24 @@ function compile(projectId, data) { async function convertDocument(path, type) { const formData = new FormData() formData.append('qqfile', fs.createReadStream(path)) - return await fetchStream(`${host}/convert/document-to-latex?type=${type}`, { - method: 'POST', - body: formData, - }) + try { + const stream = await fetchStream( + `${host}/convert/document-to-latex?type=${type}`, + { + method: 'POST', + body: formData, + } + ) + return { status: 200, stream, body: null } + } catch (err) { + if (!err.response) throw err + let body = err.body + const contentType = err.response.headers.get?.('content-type') ?? '' + if (contentType.includes('application/json')) { + body = JSON.parse(body) + } + return { status: err.response.status, stream: null, body } + } } async function convertProjectToDocument( diff --git a/services/clsi/test/unit/js/ConversionController.test.js b/services/clsi/test/unit/js/ConversionController.test.js index 94138ab1f3..69a6c784a5 100644 --- a/services/clsi/test/unit/js/ConversionController.test.js +++ b/services/clsi/test/unit/js/ConversionController.test.js @@ -2,6 +2,7 @@ import sinon from 'sinon' import { vi, describe, it, beforeEach, expect } from 'vitest' import Path from 'node:path' import { PassThrough } from 'node:stream' +import * as Errors from '../../../app/js/Errors.js' const MODULE_PATH = Path.join( import.meta.dirname, @@ -98,6 +99,8 @@ describe('ConversionController', function () { default: ctx.ConversionOutputCleaner, })) + vi.doMock('../../../app/js/Errors', () => Errors) + ctx.res = new PassThrough() ctx.res.attachment = sinon.stub() ctx.res.setHeader = sinon.stub() @@ -278,6 +281,73 @@ describe('ConversionController', function () { sinon.assert.calledWith(ctx.fs.rm, ctx.conversionDir) }) }) + + describe('on a user-facing ConversionError', function () { + beforeEach(async function (ctx) { + ctx.jsonStub = sinon.stub() + ctx.res.status = sinon.stub().returns({ json: ctx.jsonStub }) + ctx.req = { + file: { path: '/path/to/uploaded/file.docx' }, + query: { type: 'docx' }, + } + ctx.ConversionManager.promises.convertToLaTeXWithLock.rejects( + new Errors.ConversionError('Non-zero exit code from pandoc', { + type: 'docx', + exitCode: 64, + stderr: 'parse error at line 5', + }) + ) + + await ctx.ConversionController.convertDocumentToLaTeX( + ctx.req, + ctx.res + ) + }) + + it('should return 422 with the pandoc stderr in the response body', function (ctx) { + sinon.assert.calledWith(ctx.res.status, 422) + sinon.assert.calledWith(ctx.jsonStub, { + error: 'parse error at line 5', + exitCode: 64, + }) + }) + + it('should remove the uploaded file', function (ctx) { + sinon.assert.calledWith(ctx.fs.unlink, ctx.req.file.path) + }) + }) + + describe('on a non-user-facing ConversionError', function () { + beforeEach(async function (ctx) { + ctx.jsonStub = sinon.stub() + ctx.res.status = sinon.stub().returns({ json: ctx.jsonStub }) + ctx.req = { + file: { path: '/path/to/uploaded/file.docx' }, + query: { type: 'docx' }, + } + ctx.ConversionManager.promises.convertToLaTeXWithLock.rejects( + new Errors.ConversionError('Non-zero exit code from pandoc', { + type: 'docx', + exitCode: 62, + stderr: 'internal pandoc bug', + }) + ) + + await ctx.ConversionController.convertDocumentToLaTeX( + ctx.req, + ctx.res + ) + }) + + it('should return 422 without surfacing stderr', function (ctx) { + sinon.assert.calledWith(ctx.res.status, 422) + sinon.assert.calledWith(ctx.jsonStub, {}) + }) + + it('should remove the uploaded file', function (ctx) { + sinon.assert.calledWith(ctx.fs.unlink, ctx.req.file.path) + }) + }) }) }) @@ -531,5 +601,88 @@ describe('ConversionController', function () { }) }) }) + + describe('when conversion fails with a user-facing ConversionError', function () { + beforeEach(async function (ctx) { + ctx.next = sinon.stub() + ctx.jsonStub = sinon.stub() + ctx.res.status = sinon.stub().returns({ json: ctx.jsonStub }) + ctx.ConversionManager.promises.convertLaTeXToDocumentInDirWithLock.rejects( + new Errors.ConversionError( + 'pandoc latex-to-document conversion failed', + { + type: 'docx', + exitCode: 64, + stderr: 'parse error at line 5', + } + ) + ) + + await ctx.ConversionController.convertProjectToDocument( + ctx.req, + ctx.res, + ctx.next + ) + }) + + it('should return 422 with the pandoc stderr in the response body', function (ctx) { + sinon.assert.calledWith(ctx.res.status, 422) + sinon.assert.calledWith(ctx.jsonStub, { + error: 'parse error at line 5', + exitCode: 64, + }) + }) + + it('should not call next', function (ctx) { + sinon.assert.notCalled(ctx.next) + }) + + it('should still clean up the conversion directory', function (ctx) { + sinon.assert.calledWith(ctx.fs.rm, sinon.match(uuidDirPattern), { + recursive: true, + force: true, + }) + }) + }) + + describe('when conversion fails with a non-user-facing ConversionError', function () { + beforeEach(async function (ctx) { + ctx.next = sinon.stub() + ctx.jsonStub = sinon.stub() + ctx.res.status = sinon.stub().returns({ json: ctx.jsonStub }) + ctx.ConversionManager.promises.convertLaTeXToDocumentInDirWithLock.rejects( + new Errors.ConversionError( + 'pandoc latex-to-document conversion failed', + { + type: 'docx', + exitCode: 62, + stderr: 'internal pandoc bug', + } + ) + ) + + await ctx.ConversionController.convertProjectToDocument( + ctx.req, + ctx.res, + ctx.next + ) + }) + + it('should return 422 without surfacing stderr', function (ctx) { + sinon.assert.calledWith(ctx.res.status, 422) + sinon.assert.calledWith(ctx.jsonStub, {}) + }) + + it('should not call next', function (ctx) { + sinon.assert.notCalled(ctx.next) + }) + + it('should still clean up the conversion directory', function (ctx) { + sinon.assert.calledWith(ctx.fs.rm, sinon.match(uuidDirPattern), { + recursive: true, + force: true, + }) + }) + }) }) }) diff --git a/services/clsi/test/unit/js/ConversionManager.test.js b/services/clsi/test/unit/js/ConversionManager.test.js index b65cde0f18..71d22fb33b 100644 --- a/services/clsi/test/unit/js/ConversionManager.test.js +++ b/services/clsi/test/unit/js/ConversionManager.test.js @@ -233,7 +233,7 @@ describe('ConversionManager', function () { ctx.inputPath, 'docx' ) - ).to.be.rejectedWith('pandoc conversion failed') + ).to.be.rejectedWith('Non-zero exit code from pandoc') }) it('should remove the entire conversion directory', function (ctx) {