From e79679c023d127458227d904dbdb4824a755b956 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Wed, 12 Apr 2023 14:58:05 +0200 Subject: [PATCH] fix(HTTP Request Node): Show detailed error message in the UI again (#5959) --- packages/core/jest.config.js | 5 +- packages/core/src/NodeExecuteFunctions.ts | 23 ++-- .../core/test/NodeExecuteFunctions.test.ts | 105 ++++++++++++++++-- packages/core/test/WorkflowExecute.test.ts | 25 +---- packages/core/test/setup.ts | 5 + packages/core/test/utils.ts | 14 +++ .../HttpRequest/V3/HttpRequestV3.node.ts | 2 +- 7 files changed, 141 insertions(+), 38 deletions(-) create mode 100644 packages/core/test/setup.ts create mode 100644 packages/core/test/utils.ts diff --git a/packages/core/jest.config.js b/packages/core/jest.config.js index 6d9cddf364..a066fe093c 100644 --- a/packages/core/jest.config.js +++ b/packages/core/jest.config.js @@ -1,2 +1,5 @@ /** @type {import('jest').Config} */ -module.exports = require('../../jest.config'); +module.exports = { + ...require('../../jest.config'), + globalSetup: '/test/setup.ts', +}; diff --git a/packages/core/src/NodeExecuteFunctions.ts b/packages/core/src/NodeExecuteFunctions.ts index e5db3e8d6c..1962b38d3d 100644 --- a/packages/core/src/NodeExecuteFunctions.ts +++ b/packages/core/src/NodeExecuteFunctions.ts @@ -578,7 +578,13 @@ function digestAuthAxiosConfig( return axiosConfig; } -async function proxyRequestToAxios( +type ConfigObject = { + auth?: { sendImmediately: boolean }; + resolveWithFullResponse?: boolean; + simple?: boolean; +}; + +export async function proxyRequestToAxios( workflow: Workflow, additionalData: IWorkflowExecuteAdditionalData, node: INode, @@ -598,12 +604,6 @@ async function proxyRequestToAxios( maxBodyLength: Infinity, maxContentLength: Infinity, }; - - type ConfigObject = { - auth?: { sendImmediately: boolean }; - resolveWithFullResponse?: boolean; - simple?: boolean; - }; let configObject: ConfigObject; if (uriOrObject !== undefined && typeof uriOrObject === 'string') { axiosConfig.url = uriOrObject; @@ -707,12 +707,17 @@ async function proxyRequestToAxios( } const message = `${response.status as number} - ${JSON.stringify(responseData)}`; - throw Object.assign(new Error(message, { cause: error }), { + throw Object.assign(error, { + message, statusCode: response.status, options: pick(config ?? {}, ['url', 'method', 'data', 'headers']), + error: responseData, + config: undefined, + request: undefined, + response: pick(response, ['headers', 'status', 'statusText']), }); } else { - throw Object.assign(new Error(error.message, { cause: error }), { + throw Object.assign(error, { options: pick(config ?? {}, ['url', 'method', 'data', 'headers']), }); } diff --git a/packages/core/test/NodeExecuteFunctions.test.ts b/packages/core/test/NodeExecuteFunctions.test.ts index 731db0061a..aff7ba0366 100644 --- a/packages/core/test/NodeExecuteFunctions.test.ts +++ b/packages/core/test/NodeExecuteFunctions.test.ts @@ -1,15 +1,28 @@ +import nock from 'nock'; import { join } from 'path'; import { tmpdir } from 'os'; import { readFileSync, mkdtempSync } from 'fs'; - -import { IBinaryData, ITaskDataConnections } from 'n8n-workflow'; +import { mock } from 'jest-mock-extended'; +import type { + IBinaryData, + INode, + ITaskDataConnections, + IWorkflowExecuteAdditionalData, + Workflow, + WorkflowHooks, +} from 'n8n-workflow'; import { BinaryDataManager } from '@/BinaryDataManager'; -import * as NodeExecuteFunctions from '@/NodeExecuteFunctions'; +import { + setBinaryDataBuffer, + getBinaryDataBuffer, + proxyRequestToAxios, +} from '@/NodeExecuteFunctions'; +import { initLogger } from './utils'; const temporaryDir = mkdtempSync(join(tmpdir(), 'n8n')); describe('NodeExecuteFunctions', () => { - describe(`test binary data helper methods`, () => { + describe('test binary data helper methods', () => { // Reset BinaryDataManager for each run. This is a dirty operation, as individual managers are not cleaned. beforeEach(() => { BinaryDataManager.instance = undefined; @@ -27,7 +40,7 @@ describe('NodeExecuteFunctions', () => { // Set our binary data buffer let inputData: Buffer = Buffer.from('This is some binary data', 'utf8'); - let setBinaryDataBufferResponse: IBinaryData = await NodeExecuteFunctions.setBinaryDataBuffer( + let setBinaryDataBufferResponse: IBinaryData = await setBinaryDataBuffer( { mimeType: 'txt', data: 'This should be overwritten by the actual payload in the response', @@ -56,7 +69,7 @@ describe('NodeExecuteFunctions', () => { ]); // Now, lets fetch our data! The item will be item index 0. - let getBinaryDataBufferResponse: Buffer = await NodeExecuteFunctions.getBinaryDataBuffer( + let getBinaryDataBufferResponse: Buffer = await getBinaryDataBuffer( taskDataConnectionsInput, 0, 'data', @@ -78,7 +91,7 @@ describe('NodeExecuteFunctions', () => { // Set our binary data buffer let inputData: Buffer = Buffer.from('This is some binary data', 'utf8'); - let setBinaryDataBufferResponse: IBinaryData = await NodeExecuteFunctions.setBinaryDataBuffer( + let setBinaryDataBufferResponse: IBinaryData = await setBinaryDataBuffer( { mimeType: 'txt', data: 'This should be overwritten with the name of the configured data manager', @@ -114,7 +127,7 @@ describe('NodeExecuteFunctions', () => { ]); // Now, lets fetch our data! The item will be item index 0. - let getBinaryDataBufferResponse: Buffer = await NodeExecuteFunctions.getBinaryDataBuffer( + let getBinaryDataBufferResponse: Buffer = await getBinaryDataBuffer( taskDataConnectionsInput, 0, 'data', @@ -124,4 +137,80 @@ describe('NodeExecuteFunctions', () => { expect(getBinaryDataBufferResponse).toEqual(inputData); }); }); + + describe('proxyRequestToAxios', () => { + const baseUrl = 'http://example.de'; + const workflow = mock(); + const hooks = mock(); + const additionalData = mock({ hooks }); + const node = mock(); + + beforeEach(() => { + initLogger(); + hooks.executeHookFunctions.mockClear(); + }); + + test('should not throw if the response status is 200', async () => { + nock(baseUrl).get('/test').reply(200); + await proxyRequestToAxios(workflow, additionalData, node, `${baseUrl}/test`); + expect(hooks.executeHookFunctions).toHaveBeenCalledWith('nodeFetchedData', [ + workflow.id, + node, + ]); + }); + + test('should throw if the response status is 403', async () => { + const headers = { 'content-type': 'text/plain' }; + nock(baseUrl).get('/test').reply(403, 'Forbidden', headers); + try { + await proxyRequestToAxios(workflow, additionalData, node, `${baseUrl}/test`); + } catch (error) { + expect(error.statusCode).toEqual(403); + expect(error.request).toBeUndefined(); + expect(error.response).toMatchObject({ headers, status: 403 }); + expect(error.options).toMatchObject({ + headers: { Accept: '*/*' }, + method: 'get', + url: 'http://example.de/test', + }); + expect(error.config).toBeUndefined(); + expect(error.message).toEqual('403 - "Forbidden"'); + } + expect(hooks.executeHookFunctions).not.toHaveBeenCalled(); + }); + + test('should not throw if the response status is 404, but `simple` option is set to `false`', async () => { + nock(baseUrl).get('/test').reply(404, 'Not Found'); + const response = await proxyRequestToAxios(workflow, additionalData, node, { + url: `${baseUrl}/test`, + simple: false, + }); + + expect(response).toEqual('Not Found'); + expect(hooks.executeHookFunctions).toHaveBeenCalledWith('nodeFetchedData', [ + workflow.id, + node, + ]); + }); + + test('should return full response when `resolveWithFullResponse` is set to true', async () => { + nock(baseUrl).get('/test').reply(404, 'Not Found'); + const response = await proxyRequestToAxios(workflow, additionalData, node, { + url: `${baseUrl}/test`, + resolveWithFullResponse: true, + simple: false, + }); + + expect(response).toMatchObject({ + body: 'Not Found', + headers: {}, + statusCode: 404, + statusMessage: null, + }); + expect(hooks.executeHookFunctions).toHaveBeenCalledWith('nodeFetchedData', [ + workflow.id, + node, + ]); + }); + }); }); diff --git a/packages/core/test/WorkflowExecute.test.ts b/packages/core/test/WorkflowExecute.test.ts index b80a7e0a5b..5a79bac723 100644 --- a/packages/core/test/WorkflowExecute.test.ts +++ b/packages/core/test/WorkflowExecute.test.ts @@ -1,17 +1,14 @@ -import { - createDeferredPromise, - IConnections, - ILogger, - INode, - IRun, - LoggerProxy, - Workflow, -} from 'n8n-workflow'; +import { createDeferredPromise, IConnections, INode, IRun, Workflow } from 'n8n-workflow'; import { WorkflowExecute } from '@/WorkflowExecute'; import * as Helpers from './Helpers'; +import { initLogger } from './utils'; describe('WorkflowExecute', () => { + beforeAll(() => { + initLogger(); + }); + describe('run', () => { const tests: Array<{ description: string; @@ -1352,18 +1349,8 @@ describe('WorkflowExecute', () => { }, ]; - const fakeLogger = { - log: () => {}, - debug: () => {}, - verbose: () => {}, - info: () => {}, - warn: () => {}, - error: () => {}, - } as ILogger; - const executionMode = 'manual'; const nodeTypes = Helpers.NodeTypes(); - LoggerProxy.init(fakeLogger); for (const testData of tests) { test(testData.description, async () => { diff --git a/packages/core/test/setup.ts b/packages/core/test/setup.ts new file mode 100644 index 0000000000..49629bf321 --- /dev/null +++ b/packages/core/test/setup.ts @@ -0,0 +1,5 @@ +import nock from 'nock'; + +export default async () => { + nock.disableNetConnect(); +}; diff --git a/packages/core/test/utils.ts b/packages/core/test/utils.ts new file mode 100644 index 0000000000..1d6abbf50e --- /dev/null +++ b/packages/core/test/utils.ts @@ -0,0 +1,14 @@ +import { ILogger, LoggerProxy } from 'n8n-workflow'; + +const fakeLogger = { + log: () => {}, + debug: () => {}, + verbose: () => {}, + info: () => {}, + warn: () => {}, + error: () => {}, +} as ILogger; + +export const initLogger = () => { + LoggerProxy.init(fakeLogger); +}; diff --git a/packages/nodes-base/nodes/HttpRequest/V3/HttpRequestV3.node.ts b/packages/nodes-base/nodes/HttpRequest/V3/HttpRequestV3.node.ts index ca2585b7f4..4e51690e2b 100644 --- a/packages/nodes-base/nodes/HttpRequest/V3/HttpRequestV3.node.ts +++ b/packages/nodes-base/nodes/HttpRequest/V3/HttpRequestV3.node.ts @@ -1369,7 +1369,7 @@ export class HttpRequestV3 implements INodeType { if (autoDetectResponseFormat && response.reason.error instanceof Buffer) { response.reason.error = Buffer.from(response.reason.error as Buffer).toString(); } - throw new NodeApiError(this.getNode(), response.reason as JsonObject); + throw new NodeApiError(this.getNode(), response as JsonObject); } else { // Return the actual reason as error returnItems.push({