fix(core): Don't create additional nodeExecuteBefore message (#14958)

packages/core/src/execution-engine/__tests__/workflow-execute.test.ts

@@ -191,6 +191,160 @@ describe('WorkflowExecute', () => {
 		}
 	});
 
+	describe('v0 hook order', () => {
+		const executionMode = 'manual';
+		const executionOrder = 'v0';
+		const nodeTypes = Helpers.NodeTypes();
+
+		test("don't run hooks for siblings of the destination node", async () => {
+			// ARRANGE
+			const trigger = createNodeData({ name: 'trigger', type: 'n8n-nodes-base.manualTrigger' });
+			const node1 = createNodeData({ name: 'node1' });
+			const node2 = createNodeData({ name: 'node2' });
+			const workflowInstance = new DirectedGraph()
+				.addNodes(trigger, node1, node2)
+				.addConnections({ from: trigger, to: node1 }, { from: trigger, to: node2 })
+				.toWorkflow({ name: '', active: false, nodeTypes, settings: { executionOrder } });
+
+			const additionalData = Helpers.WorkflowExecuteAdditionalData(createDeferredPromise<IRun>());
+			const runHookSpy = jest.spyOn(additionalData.hooks!, 'runHook');
+
+			const workflowExecute = new WorkflowExecute(additionalData, executionMode);
+
+			// ACT
+			await workflowExecute.run(workflowInstance, trigger, 'node1');
+
+			// ASSERT
+			const workflowHooks = runHookSpy.mock.calls.filter(
+				(call) => call[0] === 'workflowExecuteBefore' || call[0] === 'workflowExecuteAfter',
+			);
+			const nodeHooks = runHookSpy.mock.calls.filter(
+				(call) => call[0] === 'nodeExecuteBefore' || call[0] === 'nodeExecuteAfter',
+			);
+
+			expect(workflowHooks.map((hook) => hook[0])).toEqual([
+				'workflowExecuteBefore',
+				'workflowExecuteAfter',
+			]);
+
+			expect(nodeHooks.map((hook) => ({ name: hook[0], node: hook[1][0] }))).toEqual([
+				{ name: 'nodeExecuteBefore', node: 'trigger' },
+				{ name: 'nodeExecuteAfter', node: 'trigger' },
+				{ name: 'nodeExecuteBefore', node: 'node1' },
+				{ name: 'nodeExecuteAfter', node: 'node1' },
+			]);
+		});
+
+		test("don't run hooks if a node does not have input data", async () => {
+			// ARRANGE
+			const trigger = createNodeData({ name: 'trigger', type: 'n8n-nodes-base.manualTrigger' });
+			const workflowInstance = new DirectedGraph()
+				.addNodes(trigger)
+				.toWorkflow({ name: '', active: false, nodeTypes, settings: { executionOrder } });
+
+			const additionalData = Helpers.WorkflowExecuteAdditionalData(createDeferredPromise<IRun>());
+			const runHookSpy = jest.spyOn(additionalData.hooks!, 'runHook');
+
+			const workflowExecute = new WorkflowExecute(additionalData, executionMode);
+			jest.spyOn(workflowExecute, 'ensureInputData').mockReturnValue(false);
+
+			// ACT
+			await workflowExecute.run(workflowInstance, trigger);
+
+			// ASSERT
+			const workflowHooks = runHookSpy.mock.calls.filter(
+				(call) => call[0] === 'workflowExecuteBefore' || call[0] === 'workflowExecuteAfter',
+			);
+			const nodeHooks = runHookSpy.mock.calls.filter(
+				(call) => call[0] === 'nodeExecuteBefore' || call[0] === 'nodeExecuteAfter',
+			);
+
+			expect(workflowHooks.map((hook) => hook[0])).toEqual([
+				'workflowExecuteBefore',
+				'workflowExecuteAfter',
+			]);
+
+			expect(nodeHooks).toHaveLength(0);
+		});
+	});
+
+	describe('v1 hook order', () => {
+		const executionMode = 'manual';
+		const executionOrder = 'v1';
+		const nodeTypes = Helpers.NodeTypes();
+
+		test("don't run hooks for siblings of the destination node", async () => {
+			// ARRANGE
+			const trigger = createNodeData({ name: 'trigger', type: 'n8n-nodes-base.manualTrigger' });
+			const node1 = createNodeData({ name: 'node1' });
+			const node2 = createNodeData({ name: 'node2' });
+			const workflowInstance = new DirectedGraph()
+				.addNodes(trigger, node1, node2)
+				.addConnections({ from: trigger, to: node1 }, { from: trigger, to: node2 })
+				.toWorkflow({ name: '', active: false, nodeTypes, settings: { executionOrder } });
+
+			const additionalData = Helpers.WorkflowExecuteAdditionalData(createDeferredPromise<IRun>());
+			const runHookSpy = jest.spyOn(additionalData.hooks!, 'runHook');
+
+			const workflowExecute = new WorkflowExecute(additionalData, executionMode);
+
+			// ACT
+			await workflowExecute.run(workflowInstance, trigger, 'node1');
+
+			// ASSERT
+			const workflowHooks = runHookSpy.mock.calls.filter(
+				(call) => call[0] === 'workflowExecuteBefore' || call[0] === 'workflowExecuteAfter',
+			);
+			const nodeHooks = runHookSpy.mock.calls.filter(
+				(call) => call[0] === 'nodeExecuteBefore' || call[0] === 'nodeExecuteAfter',
+			);
+
+			expect(workflowHooks.map((hook) => hook[0])).toEqual([
+				'workflowExecuteBefore',
+				'workflowExecuteAfter',
+			]);
+
+			expect(nodeHooks.map((hook) => ({ name: hook[0], node: hook[1][0] }))).toEqual([
+				{ name: 'nodeExecuteBefore', node: 'trigger' },
+				{ name: 'nodeExecuteAfter', node: 'trigger' },
+				{ name: 'nodeExecuteBefore', node: 'node1' },
+				{ name: 'nodeExecuteAfter', node: 'node1' },
+			]);
+		});
+
+		test("don't run hooks if a node does not have input data", async () => {
+			// ARRANGE
+			const trigger = createNodeData({ name: 'trigger', type: 'n8n-nodes-base.manualTrigger' });
+			const workflowInstance = new DirectedGraph()
+				.addNodes(trigger)
+				.toWorkflow({ name: '', active: false, nodeTypes, settings: { executionOrder } });
+
+			const additionalData = Helpers.WorkflowExecuteAdditionalData(createDeferredPromise<IRun>());
+			const runHookSpy = jest.spyOn(additionalData.hooks!, 'runHook');
+
+			const workflowExecute = new WorkflowExecute(additionalData, executionMode);
+			jest.spyOn(workflowExecute, 'ensureInputData').mockReturnValue(false);
+
+			// ACT
+			await workflowExecute.run(workflowInstance, trigger);
+
+			// ASSERT
+			const workflowHooks = runHookSpy.mock.calls.filter(
+				(call) => call[0] === 'workflowExecuteBefore' || call[0] === 'workflowExecuteAfter',
+			);
+			const nodeHooks = runHookSpy.mock.calls.filter(
+				(call) => call[0] === 'nodeExecuteBefore' || call[0] === 'nodeExecuteAfter',
+			);
+
+			expect(workflowHooks.map((hook) => hook[0])).toEqual([
+				'workflowExecuteBefore',
+				'workflowExecuteAfter',
+			]);
+
+			expect(nodeHooks).toHaveLength(0);
+		});
+	});
+
 	//run tests on json files from specified directory, default 'workflows'
 	//workflows must have pinned data that would be used to test output after execution
 	describe('run test workflows', () => {

packages/core/src/execution-engine/workflow-execute.ts

@@ -1433,20 +1433,12 @@ export class WorkflowExecute {
 					}
 					executionData.data = newTaskDataConnections;
 
-					Logger.debug(`Start processing node "${executionNode.name}"`, {
-						node: executionNode.name,
-						workflowId: workflow.id,
-					});
-					await hooks.runHook('nodeExecuteBefore', [executionNode.name, taskStartedData]);
-
 					// Get the index of the current run
 					runIndex = 0;
 					if (this.runExecutionData.resultData.runData.hasOwnProperty(executionNode.name)) {
 						runIndex = this.runExecutionData.resultData.runData[executionNode.name].length;
 					}
-
 					currentExecutionTry = `${executionNode.name}:${runIndex}`;
-
 					if (currentExecutionTry === lastExecutionTry) {
 						throw new ApplicationError(
 							'Stopped execution because it seems to be in an endless loop',
@@ -1469,6 +1461,11 @@ export class WorkflowExecute {
 						continue executionLoop;
 					}
 
+					Logger.debug(`Start executing node "${executionNode.name}"`, {
+						node: executionNode.name,
+						workflowId: workflow.id,
+					});
+					await hooks.runHook('nodeExecuteBefore', [executionNode.name, taskStartedData]);
 					let maxTries = 1;
 					if (executionData.node.retryOnFail === true) {
 						// TODO: Remove the hardcoded default-values here and also in NodeSettings.vue

packages/core/test/helpers/index.ts

@@ -54,7 +54,15 @@ export function WorkflowExecuteAdditionalData(
 ): IWorkflowExecuteAdditionalData {
 	const hooks = new ExecutionLifecycleHooks('trigger', '1', mock());
 	hooks.addHandler('workflowExecuteAfter', (fullRunData) => waitPromise.resolve(fullRunData));
-	return mock<IWorkflowExecuteAdditionalData>({ hooks, currentNodeExecutionIndex: 0 });
+	return mock<IWorkflowExecuteAdditionalData>({
+		hooks,
+		currentNodeExecutionIndex: 0,
+		// Not setting this to undefined would set it to a mock which would trigger
+		// conditions in the WorkflowExecute which only check if a property exists,
+		// e.g. `if (!this.additionalData.restartExecutionId)`. This would for
+		// example skip running the `workflowExecuteBefore` hook in the tests.
+		restartExecutionId: undefined,
+	});
 }
 
 const preparePinData = (pinData: IDataObject) => {