From f1d452f130c929dbdc96d4a60e12ddd4432d4151 Mon Sep 17 00:00:00 2001 From: pedr Date: Wed, 20 Aug 2025 03:46:15 -0300 Subject: [PATCH] Server: Fixes #12983: Not handling correctly non JSON error responses from Transcribe (#12986) --- .../server/src/routes/api/transcribe.test.ts | 126 ++++++++++-------- packages/server/src/routes/api/transcribe.ts | 20 ++- 2 files changed, 90 insertions(+), 56 deletions(-) diff --git a/packages/server/src/routes/api/transcribe.test.ts b/packages/server/src/routes/api/transcribe.test.ts index d73bbb2ab4..86c120069f 100644 --- a/packages/server/src/routes/api/transcribe.test.ts +++ b/packages/server/src/routes/api/transcribe.test.ts @@ -18,6 +18,23 @@ type JobWithResult = { state: string; }; +type TranscribeJobResponse = { + jobId: string; +}; + +type TranscribeResponse = TranscribeJobResponse | JobWithResult | string; + +type FetchResponse = { + json?: ()=> Promise; + text?: ()=> Promise; + status: number; +}; + +const fetchSpyOn = (response: FetchResponse) => { + jest.spyOn(global, 'fetch').mockImplementation( + jest.fn(() => Promise.resolve(response)) as jest.Mock, + ); +}; describe('api_transcribe', () => { @@ -41,16 +58,12 @@ describe('api_transcribe', () => { test('should create job', async () => { const { session } = await createUserAndSession(1); - - jest.spyOn(global, 'fetch').mockImplementation( - jest.fn(() => Promise.resolve( - { - json: () => Promise.resolve( - { jobId: '608626f1-cad9-4b07-a02e-ec427c47147f' }, - ), - status: 200, - })) as jest.Mock, - ); + fetchSpyOn({ + json: () => Promise.resolve( + { jobId: '608626f1-cad9-4b07-a02e-ec427c47147f' }, + ), + status: 200, + }); const fileContent = await readFile(`${testAssetDir}/htr_example.png`); const tempFilePath = await makeTempFileWithContent(fileContent); const response = await postApi(session.id, 'transcribe', {}, @@ -65,15 +78,12 @@ describe('api_transcribe', () => { test('should create job and return response eventually', async () => { const { session } = await createUserAndSession(1); - jest.spyOn(global, 'fetch').mockImplementation( - jest.fn(() => Promise.resolve( - { - json: () => Promise.resolve( - { jobId: '608626f1-cad9-4b07-a02e-ec427c47147f' }, - ), - status: 200, - })) as jest.Mock, - ); + fetchSpyOn({ + json: () => Promise.resolve( + { jobId: '608626f1-cad9-4b07-a02e-ec427c47147f' }, + ), + status: 200, + }); const fileContent = await readFile(`${testAssetDir}/htr_example.png`); const tempFilePath = await makeTempFileWithContent(fileContent); @@ -85,19 +95,16 @@ describe('api_transcribe', () => { expect(postResponse.jobId).not.toBe(undefined); - jest.spyOn(global, 'fetch').mockImplementation( - jest.fn(() => Promise.resolve( + fetchSpyOn({ + json: () => Promise.resolve( { - json: (): Promise => Promise.resolve( - { - id: '608626f1-cad9-4b07-a02e-ec427c47147f', - state: 'completed', - result: { result: 'transcription' }, - }, - ), - status: 200, - })) as jest.Mock, - ); + id: '608626f1-cad9-4b07-a02e-ec427c47147f', + state: 'completed', + result: { result: 'transcription' }, + }, + ), + status: 200, + }); const getResponse = await getApi(session.id, `transcribe/${postResponse.jobId}`, {}); expect(getResponse.id).toBe(postResponse.jobId); @@ -108,13 +115,10 @@ describe('api_transcribe', () => { test('should throw a error if API returns error 400', async () => { const { session } = await createUserAndSession(1); - jest.spyOn(global, 'fetch').mockImplementation( - jest.fn(() => Promise.resolve( - { - json: () => Promise.resolve(''), - status: 400, - })) as jest.Mock, - ); + fetchSpyOn({ + json: () => Promise.resolve(''), + status: 400, + }); const fileContent = await readFile(`${testAssetDir}/htr_example.png`); const tempFilePath = await makeTempFileWithContent(fileContent); @@ -131,13 +135,10 @@ describe('api_transcribe', () => { test('should throw error if API returns error 500', async () => { const { session } = await createUserAndSession(1); - jest.spyOn(global, 'fetch').mockImplementation( - jest.fn(() => Promise.resolve( - { - json: () => Promise.resolve(''), - status: 500, - })) as jest.Mock, - ); + fetchSpyOn({ + json: () => Promise.resolve(''), + status: 500, + }); const fileContent = await readFile(`${testAssetDir}/htr_example.png`); const tempFilePath = await makeTempFileWithContent(fileContent); @@ -153,13 +154,10 @@ describe('api_transcribe', () => { test('should throw 500 error is something unexpected', async () => { const { session } = await createUserAndSession(1); - jest.spyOn(global, 'fetch').mockImplementation( - jest.fn(() => Promise.resolve( - { - json: () => Promise.reject(new Error('Something went wrong')), - status: 200, - })) as jest.Mock, - ); + fetchSpyOn({ + json: () => Promise.reject(new Error('Something went wrong')), + status: 200, + }); const fileContent = await readFile(`${testAssetDir}/htr_example.png`); const tempFilePath = await makeTempFileWithContent(fileContent); @@ -174,4 +172,28 @@ describe('api_transcribe', () => { expect(error.message.startsWith('POST /api/transcribe {"status":500,"body":{"error":"Something went wrong"')).toBe(true); }); + test.each([ + ['non-json', 'Something went wrong'], + ['json', JSON.stringify({ error: 'Something went wrong' })], + ])('should be able to handle %s error responses', async (_type: string, result: string) => { + const { session } = await createUserAndSession(1); + + fetchSpyOn({ + text: () => Promise.resolve(result), + status: 500, + }); + + const fileContent = await readFile(`${testAssetDir}/htr_example.png`); + const tempFilePath = await makeTempFileWithContent(fileContent); + const error = await expectThrow(() => + postApi(session.id, 'transcribe', {}, + { + filePath: tempFilePath, + }, + )); + + const body = JSON.parse(error.message.split('POST /api/transcribe ')[1]); + expect(error.httpCode).toBe(502); + expect(body.body.error).toBe('Something went wrong'); + }); }); diff --git a/packages/server/src/routes/api/transcribe.ts b/packages/server/src/routes/api/transcribe.ts index 56df2d6a17..8056d221bc 100644 --- a/packages/server/src/routes/api/transcribe.ts +++ b/packages/server/src/routes/api/transcribe.ts @@ -17,6 +17,18 @@ const isHtrSupported = () => { return config().TRANSCRIBE_ENABLED; }; +const parseResponseSafely = async (response: Response) => { + const text = await response.text(); + + try { + return JSON.parse(text); + } catch (parseError) { + const truncatedText = text.substring(0, 1000); + + return { error: truncatedText }; + } +}; + router.get('api/transcribe/:id', async (path: SubPath, _ctx: AppContext) => { if (!isHtrSupported()) { throw new ErrorNotImplemented('HTR feature is not enabled in this server'); @@ -36,8 +48,8 @@ router.get('api/transcribe/:id', async (path: SubPath, _ctx: AppContext) => { const responseJson = await response.json(); throw new ErrorBadRequest(responseJson.error); } else if (response.status >= 500) { - const responseJson = await response.json(); - throw new ErrorBadGateway(responseJson.error); + const responseParsed = await parseResponseSafely(response); + throw new ErrorBadGateway(responseParsed.error); } const responseJson = await response.json(); @@ -77,8 +89,8 @@ router.post('api/transcribe', async (_path: SubPath, ctx: AppContext) => { const responseJson = await response.json(); throw new ErrorBadRequest(responseJson.error); } else if (response.status >= 500) { - const responseJson = await response.json(); - throw new ErrorBadGateway(responseJson.error); + const responseParsed = await parseResponseSafely(response); + throw new ErrorBadGateway(responseParsed.error); } const responseJson = await response.json();