From 7288492797e4f7831f904336e70b4b3c58ebbeda Mon Sep 17 00:00:00 2001 From: Eugene Molodkin Date: Tue, 22 Oct 2024 12:13:18 +0200 Subject: [PATCH 1/5] fix(HTTP Request Tool Node): Fix the undefined response issue --- .../nodes/tools/ToolHttpRequest/ToolHttpRequest.node.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/ToolHttpRequest.node.ts b/packages/@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/ToolHttpRequest.node.ts index aa294edadd612..d16ecd2f98e4c 100644 --- a/packages/@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/ToolHttpRequest.node.ts +++ b/packages/@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/ToolHttpRequest.node.ts @@ -281,7 +281,10 @@ export class ToolHttpRequest implements INodeType { 'User-Agent': undefined, }, body: {}, + // We will need a full response object later to extract the headers and check the response's content type. + // As we may use different underlying implementations for HTTP request, we specify two options. returnFullResponse: true, + resolveWithFullResponse: true, }; const authentication = this.getNodeParameter('authentication', itemIndex, 'none') as From 9afca172a88295c7bca86d402806b2aa1a74d7cf Mon Sep 17 00:00:00 2001 From: Eugene Molodkin Date: Tue, 22 Oct 2024 15:02:37 +0200 Subject: [PATCH 2/5] wip: Migrate requests with authentication to a newer helper function --- .../@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/utils.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/utils.ts b/packages/@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/utils.ts index e637251a74c7f..cd12ac00eb5d8 100644 --- a/packages/@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/utils.ts +++ b/packages/@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/utils.ts @@ -109,12 +109,11 @@ const predefinedCredentialRequest = async (ctx: IExecuteFunctions, itemIndex: nu const additionalOptions = getOAuth2AdditionalParameters(predefinedType); return async (options: IHttpRequestOptions) => { - return await ctx.helpers.requestWithAuthentication.call( + return await ctx.helpers.httpRequestWithAuthentication.call( ctx, predefinedType, options, additionalOptions && { oauth2: additionalOptions }, - itemIndex, ); }; }; From d024c500212fe29f3af904740ba1becb558ec88d Mon Sep 17 00:00:00 2001 From: Eugene Molodkin Date: Tue, 22 Oct 2024 15:03:07 +0200 Subject: [PATCH 3/5] wip: remove unnecessary flag --- .../nodes/tools/ToolHttpRequest/ToolHttpRequest.node.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/ToolHttpRequest.node.ts b/packages/@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/ToolHttpRequest.node.ts index d16ecd2f98e4c..2fbac434746d4 100644 --- a/packages/@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/ToolHttpRequest.node.ts +++ b/packages/@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/ToolHttpRequest.node.ts @@ -282,9 +282,7 @@ export class ToolHttpRequest implements INodeType { }, body: {}, // We will need a full response object later to extract the headers and check the response's content type. - // As we may use different underlying implementations for HTTP request, we specify two options. returnFullResponse: true, - resolveWithFullResponse: true, }; const authentication = this.getNodeParameter('authentication', itemIndex, 'none') as From 4d685631da3269add7b77479347396563da38c1c Mon Sep 17 00:00:00 2001 From: Eugene Molodkin Date: Tue, 22 Oct 2024 15:03:39 +0200 Subject: [PATCH 4/5] wip: Refactor tests --- .../test/ToolHttpRequest.node.test.ts | 323 ++++++++++++------ 1 file changed, 219 insertions(+), 104 deletions(-) diff --git a/packages/@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/test/ToolHttpRequest.node.test.ts b/packages/@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/test/ToolHttpRequest.node.test.ts index 161aa140f54de..6fb0888fa01ea 100644 --- a/packages/@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/test/ToolHttpRequest.node.test.ts +++ b/packages/@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/test/ToolHttpRequest.node.test.ts @@ -1,165 +1,280 @@ -import get from 'lodash/get'; -import type { IDataObject, IExecuteFunctions } from 'n8n-workflow'; +import type { IExecuteFunctions } from 'n8n-workflow'; import { jsonParse } from 'n8n-workflow'; import type { N8nTool } from '../../../../utils/N8nTool'; import { ToolHttpRequest } from '../ToolHttpRequest.node'; -const createExecuteFunctionsMock = (parameters: IDataObject, requestMock: any) => { - const nodeParameters = parameters; - - return { - getNodeParameter(parameter: string) { - return get(nodeParameters, parameter); - }, - getNode() { - return { - name: 'HTTP Request', - }; - }, - getInputData() { - return [{ json: {} }]; - }, - getWorkflow() { - return { - name: 'Test Workflow', - }; - }, - continueOnFail() { - return false; - }, - addInputData() { - return { index: 0 }; - }, - addOutputData() { - return; - }, - helpers: { - httpRequest: requestMock, - }, - } as unknown as IExecuteFunctions; -}; - describe('ToolHttpRequest', () => { let httpTool: ToolHttpRequest; - let mockRequest: jest.Mock; + let executeFunctions: IExecuteFunctions; describe('Binary response', () => { beforeEach(() => { httpTool = new ToolHttpRequest(); - mockRequest = jest.fn(); + executeFunctions = { + addInputData: jest.fn().mockResolvedValue({ index: 0 }), + addOutputData: jest.fn(), + getInputData: jest.fn(), + getNodeParameter: jest.fn(), + getNode: jest.fn(() => { + return { + type: 'n8n-nodes-base.httpRequest', + name: 'HTTP Request', + typeVersion: 1.1, + }; + }), + getCredentials: jest.fn(), + helpers: { + httpRequest: jest.fn(), + httpRequestWithAuthentication: jest.fn(), + request: jest.fn(), + requestOAuth1: jest.fn( + async () => + await Promise.resolve({ + statusCode: 200, + headers: { 'content-type': 'application/json' }, + body: Buffer.from(JSON.stringify({ success: true })), + }), + ), + requestOAuth2: jest.fn( + async () => + await Promise.resolve({ + statusCode: 200, + headers: { 'content-type': 'application/json' }, + body: Buffer.from(JSON.stringify({ success: true })), + }), + ), + requestWithAuthentication: jest.fn(), + requestWithAuthenticationPaginated: jest.fn(), + assertBinaryData: jest.fn(), + getBinaryStream: jest.fn(), + getBinaryMetadata: jest.fn(), + binaryToString: jest.fn((buffer: Buffer) => { + return buffer.toString(); + }), + prepareBinaryData: jest.fn(), + }, + getContext: jest.fn(), + sendMessageToUI: jest.fn(), + continueOnFail: jest.fn(), + getMode: jest.fn(), + } as unknown as IExecuteFunctions; }); it('should return the error when receiving a binary response', async () => { - mockRequest.mockResolvedValue({ + (executeFunctions.helpers.httpRequest as jest.Mock).mockResolvedValue({ body: Buffer.from(''), headers: { 'content-type': 'image/jpeg', }, }); - const { response } = await httpTool.supplyData.call( - createExecuteFunctionsMock( - { - method: 'GET', - url: 'https://httpbin.org/image/jpeg', - options: {}, - placeholderDefinitions: { - values: [], - }, - }, - mockRequest, - ), - 0, - ); + (executeFunctions.getNodeParameter as jest.Mock).mockImplementation((paramName: string) => { + switch (paramName) { + case 'method': + return 'GET'; + case 'url': + return 'https://httpbin.org/image/jpeg'; + case 'options': + return {}; + case 'placeholderDefinitions.values': + return []; + default: + return undefined; + } + }); - const res = await (response as N8nTool).invoke(''); + const { response } = await httpTool.supplyData.call(executeFunctions, 0); + const res = await (response as N8nTool).invoke({}); + expect(executeFunctions.helpers.httpRequest as jest.Mock).toHaveBeenCalled(); expect(res).toContain('error'); expect(res).toContain('Binary data is not supported'); }); it('should return the response text when receiving a text response', async () => { - mockRequest.mockResolvedValue({ + (executeFunctions.helpers.httpRequest as jest.Mock).mockResolvedValue({ body: 'Hello World', headers: { 'content-type': 'text/plain', }, }); - const { response } = await httpTool.supplyData.call( - createExecuteFunctionsMock( - { - method: 'GET', - url: 'https://httpbin.org/text/plain', - options: {}, - placeholderDefinitions: { - values: [], - }, - }, - mockRequest, - ), - 0, - ); + (executeFunctions.getNodeParameter as jest.Mock).mockImplementation((paramName: string) => { + switch (paramName) { + case 'method': + return 'GET'; + case 'url': + return 'https://httpbin.org/text/plain'; + case 'options': + return {}; + case 'placeholderDefinitions.values': + return []; + default: + return undefined; + } + }); - const res = await (response as N8nTool).invoke(''); + const { response } = await httpTool.supplyData.call(executeFunctions, 0); + + const res = await (response as N8nTool).invoke({}); + expect(executeFunctions.helpers.httpRequest as jest.Mock).toHaveBeenCalled(); expect(res).toEqual('Hello World'); }); it('should return the response text when receiving a text response with a charset', async () => { - mockRequest.mockResolvedValue({ + (executeFunctions.helpers.httpRequest as jest.Mock).mockResolvedValue({ body: 'こんにちは世界', headers: { 'content-type': 'text/plain; charset=iso-2022-jp', }, }); - const { response } = await httpTool.supplyData.call( - createExecuteFunctionsMock( - { - method: 'GET', - url: 'https://httpbin.org/text/plain', - options: {}, - placeholderDefinitions: { - values: [], - }, - }, - mockRequest, - ), - 0, - ); + (executeFunctions.getNodeParameter as jest.Mock).mockImplementation((paramName: string) => { + switch (paramName) { + case 'method': + return 'GET'; + case 'url': + return 'https://httpbin.org/text/plain'; + case 'options': + return {}; + case 'placeholderDefinitions.values': + return []; + default: + return undefined; + } + }); - const res = await (response as N8nTool).invoke(''); + const { response } = await httpTool.supplyData.call(executeFunctions, 0); + + const res = await (response as N8nTool).invoke({}); + expect(executeFunctions.helpers.httpRequest as jest.Mock).toHaveBeenCalled(); expect(res).toEqual('こんにちは世界'); }); it('should return the response object when receiving a JSON response', async () => { const mockJson = { hello: 'world' }; - mockRequest.mockResolvedValue({ - body: mockJson, + (executeFunctions.helpers.httpRequest as jest.Mock).mockResolvedValue({ + body: JSON.stringify(mockJson), headers: { 'content-type': 'application/json', }, }); - const { response } = await httpTool.supplyData.call( - createExecuteFunctionsMock( - { - method: 'GET', - url: 'https://httpbin.org/json', - options: {}, - placeholderDefinitions: { - values: [], - }, - }, - mockRequest, - ), - 0, - ); + (executeFunctions.getNodeParameter as jest.Mock).mockImplementation((paramName: string) => { + switch (paramName) { + case 'method': + return 'GET'; + case 'url': + return 'https://httpbin.org/json'; + case 'options': + return {}; + case 'placeholderDefinitions.values': + return []; + default: + return undefined; + } + }); - const res = await (response as N8nTool).invoke(''); + const { response } = await httpTool.supplyData.call(executeFunctions, 0); + + const res = await (response as N8nTool).invoke({}); + expect(executeFunctions.helpers.httpRequest as jest.Mock).toHaveBeenCalled(); expect(jsonParse(res)).toEqual(mockJson); }); + + it('should handle authentication with predefined credentials', async () => { + (executeFunctions.helpers.httpRequestWithAuthentication as jest.Mock).mockResolvedValue({ + body: 'Hello World', + headers: { + 'content-type': 'text/plain', + }, + }); + + (executeFunctions.getNodeParameter as jest.Mock).mockImplementation((paramName: string) => { + switch (paramName) { + case 'method': + return 'GET'; + case 'url': + return 'https://httpbin.org/text/plain'; + case 'authentication': + return 'predefinedCredentialType'; + case 'nodeCredentialType': + return 'linearApi'; + case 'options': + return {}; + case 'placeholderDefinitions.values': + return []; + default: + return undefined; + } + }); + + const { response } = await httpTool.supplyData.call(executeFunctions, 0); + + const res = await (response as N8nTool).invoke({}); + + expect(res).toEqual('Hello World'); + + expect( + executeFunctions.helpers.httpRequestWithAuthentication as jest.Mock, + ).toHaveBeenCalledWith( + 'linearApi', + expect.objectContaining({ + returnFullResponse: true, + }), + undefined, + ); + }); + + it('should handle authentication with generic credentials', async () => { + (executeFunctions.helpers.httpRequest as jest.Mock).mockResolvedValue({ + body: 'Hello World', + headers: { + 'content-type': 'text/plain', + }, + }); + + (executeFunctions.getNodeParameter as jest.Mock).mockImplementation((paramName: string) => { + switch (paramName) { + case 'method': + return 'GET'; + case 'url': + return 'https://httpbin.org/text/plain'; + case 'authentication': + return 'genericCredentialType'; + case 'genericAuthType': + return 'httpBasicAuth'; + case 'options': + return {}; + case 'placeholderDefinitions.values': + return []; + default: + return undefined; + } + }); + + (executeFunctions.getCredentials as jest.Mock).mockResolvedValue({ + user: 'username', + password: 'password', + }); + + const { response } = await httpTool.supplyData.call(executeFunctions, 0); + + const res = await (response as N8nTool).invoke({}); + + expect(res).toEqual('Hello World'); + + expect(executeFunctions.helpers.httpRequest as jest.Mock).toHaveBeenCalledWith( + expect.objectContaining({ + returnFullResponse: true, + auth: expect.objectContaining({ + username: 'username', + password: 'password', + }), + }), + ); + }); }); }); From a59a4d7064e5c9b5d4747e70ab70f0cf1ffe3ffd 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: Tue, 22 Oct 2024 15:39:19 +0200 Subject: [PATCH 5/5] simplify test --- .../test/ToolHttpRequest.node.test.ts | 104 ++++++------------ 1 file changed, 32 insertions(+), 72 deletions(-) diff --git a/packages/@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/test/ToolHttpRequest.node.test.ts b/packages/@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/test/ToolHttpRequest.node.test.ts index 6fb0888fa01ea..f18a2437e336a 100644 --- a/packages/@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/test/ToolHttpRequest.node.test.ts +++ b/packages/@n8n/nodes-langchain/nodes/tools/ToolHttpRequest/test/ToolHttpRequest.node.test.ts @@ -1,75 +1,37 @@ -import type { IExecuteFunctions } from 'n8n-workflow'; +import { mock } from 'jest-mock-extended'; +import type { IExecuteFunctions, INode } from 'n8n-workflow'; import { jsonParse } from 'n8n-workflow'; import type { N8nTool } from '../../../../utils/N8nTool'; import { ToolHttpRequest } from '../ToolHttpRequest.node'; describe('ToolHttpRequest', () => { - let httpTool: ToolHttpRequest; - let executeFunctions: IExecuteFunctions; + const httpTool = new ToolHttpRequest(); + const helpers = mock(); + const executeFunctions = mock({ helpers }); describe('Binary response', () => { beforeEach(() => { - httpTool = new ToolHttpRequest(); - executeFunctions = { - addInputData: jest.fn().mockResolvedValue({ index: 0 }), - addOutputData: jest.fn(), - getInputData: jest.fn(), - getNodeParameter: jest.fn(), - getNode: jest.fn(() => { - return { - type: 'n8n-nodes-base.httpRequest', - name: 'HTTP Request', - typeVersion: 1.1, - }; + jest.resetAllMocks(); + executeFunctions.getNode.mockReturnValue( + mock({ + type: 'n8n-nodes-base.httpRequest', + name: 'HTTP Request', + typeVersion: 1.1, }), - getCredentials: jest.fn(), - helpers: { - httpRequest: jest.fn(), - httpRequestWithAuthentication: jest.fn(), - request: jest.fn(), - requestOAuth1: jest.fn( - async () => - await Promise.resolve({ - statusCode: 200, - headers: { 'content-type': 'application/json' }, - body: Buffer.from(JSON.stringify({ success: true })), - }), - ), - requestOAuth2: jest.fn( - async () => - await Promise.resolve({ - statusCode: 200, - headers: { 'content-type': 'application/json' }, - body: Buffer.from(JSON.stringify({ success: true })), - }), - ), - requestWithAuthentication: jest.fn(), - requestWithAuthenticationPaginated: jest.fn(), - assertBinaryData: jest.fn(), - getBinaryStream: jest.fn(), - getBinaryMetadata: jest.fn(), - binaryToString: jest.fn((buffer: Buffer) => { - return buffer.toString(); - }), - prepareBinaryData: jest.fn(), - }, - getContext: jest.fn(), - sendMessageToUI: jest.fn(), - continueOnFail: jest.fn(), - getMode: jest.fn(), - } as unknown as IExecuteFunctions; + ); + executeFunctions.addInputData.mockReturnValue({ index: 0 }); }); it('should return the error when receiving a binary response', async () => { - (executeFunctions.helpers.httpRequest as jest.Mock).mockResolvedValue({ + helpers.httpRequest.mockResolvedValue({ body: Buffer.from(''), headers: { 'content-type': 'image/jpeg', }, }); - (executeFunctions.getNodeParameter as jest.Mock).mockImplementation((paramName: string) => { + executeFunctions.getNodeParameter.mockImplementation((paramName: string) => { switch (paramName) { case 'method': return 'GET'; @@ -87,20 +49,20 @@ describe('ToolHttpRequest', () => { const { response } = await httpTool.supplyData.call(executeFunctions, 0); const res = await (response as N8nTool).invoke({}); - expect(executeFunctions.helpers.httpRequest as jest.Mock).toHaveBeenCalled(); + expect(helpers.httpRequest).toHaveBeenCalled(); expect(res).toContain('error'); expect(res).toContain('Binary data is not supported'); }); it('should return the response text when receiving a text response', async () => { - (executeFunctions.helpers.httpRequest as jest.Mock).mockResolvedValue({ + helpers.httpRequest.mockResolvedValue({ body: 'Hello World', headers: { 'content-type': 'text/plain', }, }); - (executeFunctions.getNodeParameter as jest.Mock).mockImplementation((paramName: string) => { + executeFunctions.getNodeParameter.mockImplementation((paramName: string) => { switch (paramName) { case 'method': return 'GET'; @@ -118,19 +80,19 @@ describe('ToolHttpRequest', () => { const { response } = await httpTool.supplyData.call(executeFunctions, 0); const res = await (response as N8nTool).invoke({}); - expect(executeFunctions.helpers.httpRequest as jest.Mock).toHaveBeenCalled(); + expect(helpers.httpRequest).toHaveBeenCalled(); expect(res).toEqual('Hello World'); }); it('should return the response text when receiving a text response with a charset', async () => { - (executeFunctions.helpers.httpRequest as jest.Mock).mockResolvedValue({ + helpers.httpRequest.mockResolvedValue({ body: 'こんにちは世界', headers: { 'content-type': 'text/plain; charset=iso-2022-jp', }, }); - (executeFunctions.getNodeParameter as jest.Mock).mockImplementation((paramName: string) => { + executeFunctions.getNodeParameter.mockImplementation((paramName: string) => { switch (paramName) { case 'method': return 'GET'; @@ -148,21 +110,21 @@ describe('ToolHttpRequest', () => { const { response } = await httpTool.supplyData.call(executeFunctions, 0); const res = await (response as N8nTool).invoke({}); - expect(executeFunctions.helpers.httpRequest as jest.Mock).toHaveBeenCalled(); + expect(helpers.httpRequest).toHaveBeenCalled(); expect(res).toEqual('こんにちは世界'); }); it('should return the response object when receiving a JSON response', async () => { const mockJson = { hello: 'world' }; - (executeFunctions.helpers.httpRequest as jest.Mock).mockResolvedValue({ + helpers.httpRequest.mockResolvedValue({ body: JSON.stringify(mockJson), headers: { 'content-type': 'application/json', }, }); - (executeFunctions.getNodeParameter as jest.Mock).mockImplementation((paramName: string) => { + executeFunctions.getNodeParameter.mockImplementation((paramName: string) => { switch (paramName) { case 'method': return 'GET'; @@ -180,19 +142,19 @@ describe('ToolHttpRequest', () => { const { response } = await httpTool.supplyData.call(executeFunctions, 0); const res = await (response as N8nTool).invoke({}); - expect(executeFunctions.helpers.httpRequest as jest.Mock).toHaveBeenCalled(); + expect(helpers.httpRequest).toHaveBeenCalled(); expect(jsonParse(res)).toEqual(mockJson); }); it('should handle authentication with predefined credentials', async () => { - (executeFunctions.helpers.httpRequestWithAuthentication as jest.Mock).mockResolvedValue({ + helpers.httpRequestWithAuthentication.mockResolvedValue({ body: 'Hello World', headers: { 'content-type': 'text/plain', }, }); - (executeFunctions.getNodeParameter as jest.Mock).mockImplementation((paramName: string) => { + executeFunctions.getNodeParameter.mockImplementation((paramName: string) => { switch (paramName) { case 'method': return 'GET'; @@ -217,9 +179,7 @@ describe('ToolHttpRequest', () => { expect(res).toEqual('Hello World'); - expect( - executeFunctions.helpers.httpRequestWithAuthentication as jest.Mock, - ).toHaveBeenCalledWith( + expect(helpers.httpRequestWithAuthentication).toHaveBeenCalledWith( 'linearApi', expect.objectContaining({ returnFullResponse: true, @@ -229,14 +189,14 @@ describe('ToolHttpRequest', () => { }); it('should handle authentication with generic credentials', async () => { - (executeFunctions.helpers.httpRequest as jest.Mock).mockResolvedValue({ + helpers.httpRequest.mockResolvedValue({ body: 'Hello World', headers: { 'content-type': 'text/plain', }, }); - (executeFunctions.getNodeParameter as jest.Mock).mockImplementation((paramName: string) => { + executeFunctions.getNodeParameter.mockImplementation((paramName: string) => { switch (paramName) { case 'method': return 'GET'; @@ -255,7 +215,7 @@ describe('ToolHttpRequest', () => { } }); - (executeFunctions.getCredentials as jest.Mock).mockResolvedValue({ + executeFunctions.getCredentials.mockResolvedValue({ user: 'username', password: 'password', }); @@ -266,7 +226,7 @@ describe('ToolHttpRequest', () => { expect(res).toEqual('Hello World'); - expect(executeFunctions.helpers.httpRequest as jest.Mock).toHaveBeenCalledWith( + expect(helpers.httpRequest).toHaveBeenCalledWith( expect.objectContaining({ returnFullResponse: true, auth: expect.objectContaining({