fix(AWS S3 Node): Fix File upload, and add node tests (#6153)

packages/nodes-base/credentials/Aws.credentials.ts

@@ -354,7 +354,7 @@ export class Aws implements ICredentialType {
 			});
 		}
 
-		if (body && typeof body === 'object' && !isObjectEmpty(body)) {
+		if (body && typeof body === 'object' && isObjectEmpty(body)) {
 			body = '';
 		}
 

packages/nodes-base/jest.config.js

@@ -2,4 +2,7 @@
 module.exports = {
 	...require('../../jest.config'),
 	collectCoverageFrom: ['credentials/**/*.ts', 'nodes/**/*.ts', 'utils/**/*.ts'],
+	moduleNameMapper: {
+		'^@test/(.*)$': '<rootDir>/test/$1',
+	},
 };

packages/nodes-base/nodes/Aws/S3/test/AwsS3.file.upload.workflow.json

@@ -0,0 +1,97 @@
+{
+	"name": "Test S3 upload",
+	"nodes": [
+		{
+			"parameters": {},
+			"id": "8f35d24b-1493-43a4-846f-bacb577bfcb2",
+			"name": "When clicking \"Execute Workflow\"",
+			"type": "n8n-nodes-base.manualTrigger",
+			"typeVersion": 1,
+			"position": [540, 340]
+		},
+		{
+			"parameters": {
+				"mode": "jsonToBinary",
+				"options": {}
+			},
+			"id": "eae2946a-1a1e-47e9-9fd6-e32119b13ec0",
+			"name": "Move Binary Data",
+			"type": "n8n-nodes-base.moveBinaryData",
+			"typeVersion": 1,
+			"position": [900, 340]
+		},
+		{
+			"parameters": {
+				"operation": "upload",
+				"bucketName": "bucket",
+				"fileName": "binary.json",
+				"additionalFields": {}
+			},
+			"id": "6f21fa3f-ede1-44b1-8182-a2c07152f666",
+			"name": "AWS S3",
+			"type": "n8n-nodes-base.awsS3",
+			"typeVersion": 1,
+			"position": [1080, 340],
+			"credentials": {
+				"aws": {
+					"id": "1",
+					"name": "AWS account"
+				}
+			}
+		},
+		{
+			"parameters": {
+				"jsCode": "return [{ key: \"value\" }];"
+			},
+			"id": "e12f1876-cfd1-47a4-a21b-d478452683bc",
+			"name": "Code",
+			"type": "n8n-nodes-base.code",
+			"typeVersion": 1,
+			"position": [720, 340]
+		}
+	],
+	"connections": {
+		"When clicking \"Execute Workflow\"": {
+			"main": [
+				[
+					{
+						"node": "Code",
+						"type": "main",
+						"index": 0
+					}
+				]
+			]
+		},
+		"Move Binary Data": {
+			"main": [
+				[
+					{
+						"node": "AWS S3",
+						"type": "main",
+						"index": 0
+					}
+				]
+			]
+		},
+		"Code": {
+			"main": [
+				[
+					{
+						"node": "Move Binary Data",
+						"type": "main",
+						"index": 0
+					}
+				]
+			]
+		}
+	},
+	"pinData": {
+		"AWS S3": [
+			{
+				"json": {
+					"success": true
+				}
+			}
+		]
+	}
+}

packages/nodes-base/nodes/Aws/S3/test/AwsS3.node.test.ts

@@ -0,0 +1,48 @@
+import nock from 'nock';
+import { getWorkflowFilenames, initBinaryDataManager, testWorkflows } from '@test/nodes/Helpers';
+
+const workflows = getWorkflowFilenames(__dirname);
+
+describe('Test S3 Node', () => {
+	describe('File Upload', () => {
+		let mock: nock.Scope;
+		const now = 1683028800000;
+
+		beforeAll(async () => {
+			jest.useFakeTimers({ doNotFake: ['nextTick'], now });
+
+			await initBinaryDataManager();
+
+			nock.disableNetConnect();
+			mock = nock('https://bucket.s3.eu-central-1.amazonaws.com');
+		});
+
+		beforeEach(async () => {
+			mock.get('/?location').reply(
+				200,
+				`<?xml version="1.0" encoding="UTF-8"?>
+				<LocationConstraint>
+					<LocationConstraint>eu-central-1</LocationConstraint>
+				</LocationConstraint>`,
+				{
+					'content-type': 'application/xml',
+				},
+			);
+
+			mock
+				.put('/binary.json')
+				.matchHeader(
+					'X-Amz-Content-Sha256',
+					'e43abcf3375244839c012f9633f95862d232a95b00d5bc7348b3098b9fed7f32',
+				)
+				.once()
+				.reply(200, { success: true });
+		});
+
+		afterAll(() => {
+			nock.restore();
+		});
+
+		testWorkflows(workflows);
+	});
+});

packages/nodes-base/test/nodes/FakeCredentialsMap.ts

@@ -19,4 +19,9 @@ export const FAKE_CREDENTIALS_DATA: IDataObject = {
 		label: 'GitHub:john-doe',
 		secret: 'BVDRSBXQB2ZEL5HE',
 	},
+	aws: {
+		region: 'eu-central-1',
+		accessKeyId: 'key',
+		secretAccessKey: 'secret',
+	},
 };

packages/nodes-base/test/nodes/Helpers.ts

@@ -1,7 +1,15 @@
 import { readFileSync, readdirSync, mkdtempSync } from 'fs';
+import path from 'path';
+import { tmpdir } from 'os';
+import { isEmpty } from 'lodash';
+import { get } from 'lodash';
 import { BinaryDataManager, Credentials, constructExecutionMetaData } from 'n8n-core';
 import type {
+	CredentialLoadingDetails,
 	ICredentialDataDecryptedObject,
+	ICredentialType,
+	ICredentialTypeData,
+	ICredentialTypes,
 	IDataObject,
 	IDeferredPromise,
 	IExecuteFunctions,
@@ -20,18 +28,16 @@ import type {
 	IVersionedNodeType,
 	IWorkflowBase,
 	IWorkflowExecuteAdditionalData,
-	LoadingDetails,
+	NodeLoadingDetails,
 } from 'n8n-workflow';
 import { ICredentialsHelper, LoggerProxy, NodeHelpers, WorkflowHooks } from 'n8n-workflow';
 import { executeWorkflow } from './ExecuteWorkflow';
 import type { WorkflowTestData } from './types';
-import path from 'path';
-import { tmpdir } from 'os';
-import { isEmpty } from 'lodash';
-import { get } from 'lodash';
 
 import { FAKE_CREDENTIALS_DATA } from './FakeCredentialsMap';
 
+const baseDir = path.resolve(__dirname, '../..');
+
 const getFakeDecryptedCredentials = (
 	nodeCredentials: INodeCredentialsDetails,
 	type: string,
@@ -48,12 +54,56 @@ const getFakeDecryptedCredentials = (
 	return {};
 };
 
+export const readJsonFileSync = <T = any>(filePath: string) =>
+	JSON.parse(readFileSync(path.join(baseDir, filePath), 'utf-8')) as T;
+
+const knownCredentials = readJsonFileSync<Record<string, CredentialLoadingDetails>>(
+	'dist/known/credentials.json',
+);
+
+const knownNodes = readJsonFileSync<Record<string, NodeLoadingDetails>>('dist/known/nodes.json');
+
+class CredentialType implements ICredentialTypes {
+	credentialTypes: ICredentialTypeData = {};
+
+	addCredential(credentialTypeName: string, credentialType: ICredentialType) {
+		this.credentialTypes[credentialTypeName] = {
+			sourcePath: '',
+			type: credentialType,
+		};
+	}
+
+	recognizes(credentialType: string): boolean {
+		return credentialType in this.credentialTypes;
+	}
+
+	getByName(credentialType: string): ICredentialType {
+		return this.credentialTypes[credentialType].type;
+	}
+
+	getNodeTypesToTestWith(type: string): string[] {
+		return knownCredentials[type]?.nodesToTestWith ?? [];
+	}
+
+	getParentTypes(typeName: string): string[] {
+		return [];
+	}
+}
+
 export class CredentialsHelper extends ICredentialsHelper {
+	constructor(private credentialTypes: ICredentialTypes) {
+		super('');
+	}
+
 	async authenticate(
 		credentials: ICredentialDataDecryptedObject,
 		typeName: string,
 		requestParams: IHttpRequestOptions,
 	): Promise<IHttpRequestOptions> {
+		const credentialType = this.credentialTypes.getByName(typeName);
+		if (typeof credentialType.authenticate === 'function') {
+			return credentialType.authenticate(credentials, requestParams);
+		}
 		return requestParams;
 	}
 
@@ -119,7 +169,7 @@ export function WorkflowExecuteAdditionalData(
 	};
 
 	return {
-		credentialsHelper: new CredentialsHelper(''),
+		credentialsHelper: new CredentialsHelper(credentialTypes),
 		hooks: new WorkflowHooks(hookFunctions, 'trigger', '1', workflowData),
 		executeWorkflow: async (workflowInfo: IExecuteWorkflowInfo): Promise<any> => {},
 		sendMessageToUI: (message: string) => {},
@@ -134,7 +184,7 @@ export function WorkflowExecuteAdditionalData(
 	};
 }
 
-class NodeTypesClass implements INodeTypes {
+class NodeTypes implements INodeTypes {
 	nodeTypes: INodeTypeData = {};
 
 	getByName(nodeType: string): INodeType | IVersionedNodeType {
@@ -152,7 +202,6 @@ class NodeTypesClass implements INodeTypes {
 			...this.nodeTypes,
 			...loadedNode,
 		};
-		//Object.assign(this.nodeTypes, loadedNode);
 	}
 
 	getByNameAndVersion(nodeType: string, version?: number): INodeType {
@@ -160,24 +209,6 @@ class NodeTypesClass implements INodeTypes {
 	}
 }
 
-let nodeTypesInstance: NodeTypesClass | undefined;
-
-export function NodeTypes(): NodeTypesClass {
-	if (nodeTypesInstance === undefined) {
-		nodeTypesInstance = new NodeTypesClass();
-	}
-	return nodeTypesInstance;
-}
-
-let knownNodes: Record<string, LoadingDetails> | null = null;
-
-const loadKnownNodes = (): Record<string, LoadingDetails> => {
-	if (knownNodes === null) {
-		knownNodes = JSON.parse(readFileSync('dist/known/nodes.json').toString());
-	}
-	return knownNodes!;
-};
-
 export function createTemporaryDir(prefix = 'n8n') {
 	return mkdtempSync(path.join(tmpdir(), prefix));
 }
@@ -194,18 +225,31 @@ export async function initBinaryDataManager(mode: 'default' | 'filesystem' = 'de
 	return temporaryDir;
 }
 
+const credentialTypes = new CredentialType();
+
 export function setup(testData: WorkflowTestData[] | WorkflowTestData) {
 	if (!Array.isArray(testData)) {
 		testData = [testData];
 	}
 
-	const knownNodes = loadKnownNodes();
+	const nodeTypes = new NodeTypes();
 
-	const nodeTypes = NodeTypes();
-	const nodeNames = Array.from(
-		new Set(testData.flatMap((data) => data.input.workflowData.nodes.map((n) => n.type))),
-	);
+	const nodes = [...new Set(testData.flatMap((data) => data.input.workflowData.nodes))];
+	const credentialNames = nodes
+		.filter((n) => n.credentials)
+		.flatMap(({ credentials }) => Object.keys(credentials!));
+	for (const credentialName of credentialNames) {
+		const loadInfo = knownCredentials[credentialName];
+		if (!loadInfo) {
+			throw new Error(`Unknown credential type: ${credentialName}`);
+		}
+		const sourcePath = loadInfo.sourcePath.replace(/^dist\//, './').replace(/\.js$/, '.ts');
+		const nodeSourcePath = path.join(baseDir, sourcePath);
+		const credential = new (require(nodeSourcePath)[loadInfo.className])() as ICredentialType;
+		credentialTypes.addCredential(credentialName, credential);
+	}
 
+	const nodeNames = nodes.map((n) => n.type);
 	for (const nodeName of nodeNames) {
 		if (!nodeName.startsWith('n8n-nodes-base.')) {
 			throw new Error(`Unknown node type: ${nodeName}`);
@@ -215,7 +259,7 @@ export function setup(testData: WorkflowTestData[] | WorkflowTestData) {
 			throw new Error(`Unknown node type: ${nodeName}`);
 		}
 		const sourcePath = loadInfo.sourcePath.replace(/^dist\//, './').replace(/\.js$/, '.ts');
-		const nodeSourcePath = path.join(process.cwd(), sourcePath);
+		const nodeSourcePath = path.join(baseDir, sourcePath);
 		const node = new (require(nodeSourcePath)[loadInfo.className])() as INodeType;
 		nodeTypes.addNode(nodeName, node);
 	}
@@ -234,6 +278,12 @@ export function setup(testData: WorkflowTestData[] | WorkflowTestData) {
 
 export function getResultNodeData(result: IRun, testData: WorkflowTestData) {
 	return Object.keys(testData.output.nodeData).map((nodeName) => {
+		const error = result.data.resultData.error;
+		// If there was an error running the workflow throw it for easier debugging
+		// and to surface all issues
+		if (error?.cause) throw error.cause;
+		if (error) throw error;
+
 		if (result.data.resultData.runData[nodeName] === undefined) {
 			// log errors from other nodes
 			Object.keys(result.data.resultData.runData).forEach((key) => {
@@ -262,10 +312,6 @@ export function getResultNodeData(result: IRun, testData: WorkflowTestData) {
 	});
 }
 
-export function readJsonFileSync(path: string) {
-	return JSON.parse(readFileSync(path, 'utf-8'));
-}
-
 export const equalityTest = async (testData: WorkflowTestData, types: INodeTypes) => {
 	// execute workflow
 	const { result } = await executeWorkflow(testData, types);
@@ -298,7 +344,7 @@ export const workflowToTests = (workflowFiles: string[]) => {
 	const testCases: WorkflowTestData[] = [];
 	for (const filePath of workflowFiles) {
 		const description = filePath.replace('.json', '');
-		const workflowData = readJsonFileSync(filePath);
+		const workflowData = readJsonFileSync<IWorkflowBase>(filePath);
 		if (workflowData.pinData === undefined) {
 			throw new Error('Workflow data does not contain pinData');
 		}
@@ -331,7 +377,7 @@ export const getWorkflowFilenames = (dirname: string) => {
 	const filenames = readdirSync(dirname);
 	const testFolder = dirname.split(`${path.sep}nodes-base${path.sep}`)[1];
 	filenames.forEach((file) => {
-		if (file.includes('.json')) {
+		if (file.endsWith('.json')) {
 			workflows.push(path.join(testFolder, file));
 		}
 	});

packages/nodes-base/tsconfig.json

@@ -4,6 +4,9 @@
 		"lib": ["dom", "es2020", "es2022.error"],
 		"types": ["node", "jest"],
 		"noEmit": true,
+		"paths": {
+			"@test/*": ["./test/*"]
+		},
 		// TODO: remove all options below this line
 		"noImplicitReturns": false,
 		"noUnusedLocals": false,

packages/workflow/src/NodeErrors.ts

@@ -373,7 +373,7 @@ export class NodeApiError extends NodeError {
 				this.message = STATUS_CODE_MESSAGES['5XX'];
 				break;
 			default:
-				this.message = UNKNOWN_ERROR_MESSAGE;
+				this.message = this.message || UNKNOWN_ERROR_MESSAGE;
 		}
 		if (this.node.type === 'n8n-nodes-base.noOp' && this.message === UNKNOWN_ERROR_MESSAGE) {
 			this.message = `${UNKNOWN_ERROR_MESSAGE_CRED} - ${this.httpCode}`;