From 8ef9fafe226913d271d36656dbbb84152e00df91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Wed, 31 Jul 2024 10:53:47 +0200 Subject: [PATCH 1/3] fix(core): Restore missing log event `n8n.workflow.failed` --- packages/cli/src/InternalHooks.ts | 2 +- .../cli/src/WorkflowExecuteAdditionalData.ts | 12 +++++ packages/cli/src/WorkflowRunner.ts | 6 +++ .../audit-event-relay.service.test.ts | 51 ++++++++++++++++++- .../src/eventbus/audit-event-relay.service.ts | 33 ++++++++++-- packages/cli/src/eventbus/event.types.ts | 5 ++ .../executions/execution-recovery.service.ts | 6 +++ 7 files changed, 109 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/InternalHooks.ts b/packages/cli/src/InternalHooks.ts index 7912054085824..c64bc89348e64 100644 --- a/packages/cli/src/InternalHooks.ts +++ b/packages/cli/src/InternalHooks.ts @@ -175,7 +175,7 @@ export class InternalHooks { runData.status = 'canceled'; } - telemetryProperties.success = !!runData?.finished; + telemetryProperties.success = !!runData?.finished; // xyz // const executionStatus: ExecutionStatus = runData?.status ?? 'unknown'; const executionStatus: ExecutionStatus = runData diff --git a/packages/cli/src/WorkflowExecuteAdditionalData.ts b/packages/cli/src/WorkflowExecuteAdditionalData.ts index 754cfea693c3d..a5685615892e9 100644 --- a/packages/cli/src/WorkflowExecuteAdditionalData.ts +++ b/packages/cli/src/WorkflowExecuteAdditionalData.ts @@ -651,6 +651,12 @@ function hookFunctionsSaveWorker(): IWorkflowExecuteHooks { executionId, success: runData.status === 'success', isManual: runData.mode === 'manual', + lastNodeExecuted: runData?.data.resultData.lastNodeExecuted, + errorNodeType: + runData?.data.resultData.error && 'node' in runData?.data.resultData.error + ? runData?.data.resultData.error.node?.type + : undefined, + errorMessage: runData?.data.resultData.error?.message.toString(), }); }, async function (this: WorkflowHooks, fullRunData: IRun) { @@ -940,6 +946,12 @@ async function executeWorkflow( success: data.status === 'success', isManual: data.mode === 'manual', userId: additionalData.userId, + lastNodeExecuted: data?.data.resultData.lastNodeExecuted, + errorNodeType: + data?.data.resultData.error && 'node' in data?.data.resultData.error + ? data?.data.resultData.error.node?.type + : undefined, + errorMessage: data?.data.resultData.error?.message.toString(), }); // subworkflow either finished, or is in status waiting due to a wait node, both cases are considered successes here diff --git a/packages/cli/src/WorkflowRunner.ts b/packages/cli/src/WorkflowRunner.ts index f8faf55fbe49d..0bf62e582a5d0 100644 --- a/packages/cli/src/WorkflowRunner.ts +++ b/packages/cli/src/WorkflowRunner.ts @@ -173,6 +173,12 @@ export class WorkflowRunner { success: executionData?.status === 'success', isManual: data.executionMode === 'manual', userId: data.userId, + lastNodeExecuted: executionData?.data.resultData.lastNodeExecuted, + errorNodeType: + executionData?.data.resultData.error && 'node' in executionData?.data.resultData.error + ? executionData?.data.resultData.error.node?.type + : undefined, + errorMessage: executionData?.data.resultData.error?.message.toString(), }); if (this.externalHooks.exists('workflow.postExecute')) { try { diff --git a/packages/cli/src/eventbus/__tests__/audit-event-relay.service.test.ts b/packages/cli/src/eventbus/__tests__/audit-event-relay.service.test.ts index 80392206079b7..6dbb7fc58ff2d 100644 --- a/packages/cli/src/eventbus/__tests__/audit-event-relay.service.test.ts +++ b/packages/cli/src/eventbus/__tests__/audit-event-relay.service.test.ts @@ -2,12 +2,13 @@ import { mock } from 'jest-mock-extended'; import { AuditEventRelay } from '../audit-event-relay.service'; import type { MessageEventBus } from '../MessageEventBus/MessageEventBus'; import type { Event } from '../event.types'; -import type { EventService } from '../event.service'; +import { EventService } from '../event.service'; describe('AuditorService', () => { const eventBus = mock(); - const eventService = mock(); + const eventService = new EventService(); const auditor = new AuditEventRelay(eventService, eventBus); + auditor.init(); afterEach(() => { jest.clearAllMocks(); @@ -80,4 +81,50 @@ describe('AuditorService', () => { }, }); }); + + it('should log on `workflow-post-execute` for successful execution', () => { + const payload = { + executionId: 'some-id', + success: true, + userId: 'some-id', + workflowId: 'some-id', + isManual: true, + workflowName: 'some-name', + metadata: {}, + lastNodeExecuted: 'some-node', + errorNodeType: 'some-type', + errorMessage: 'some-message', + }; + + eventService.emit('workflow-post-execute', payload); + + const { lastNodeExecuted: _, errorNodeType: __, errorMessage: ___, ...rest } = payload; + + expect(eventBus.sendWorkflowEvent).toHaveBeenCalledWith({ + eventName: 'n8n.workflow.success', + payload: rest, + }); + }); + + it('should handle `workflow-post-execute` event for unsuccessful execution', () => { + const payload = { + executionId: 'some-id', + success: false, + userId: 'some-id', + workflowId: 'some-id', + isManual: true, + workflowName: 'some-name', + metadata: {}, + lastNodeExecuted: 'some-node', + errorNodeType: 'some-type', + errorMessage: 'some-message', + }; + + eventService.emit('workflow-post-execute', payload); + + expect(eventBus.sendWorkflowEvent).toHaveBeenCalledWith({ + eventName: 'n8n.workflow.failed', + payload, + }); + }); }); diff --git a/packages/cli/src/eventbus/audit-event-relay.service.ts b/packages/cli/src/eventbus/audit-event-relay.service.ts index 9c73494520dbe..449f8de248b7f 100644 --- a/packages/cli/src/eventbus/audit-event-relay.service.ts +++ b/packages/cli/src/eventbus/audit-event-relay.service.ts @@ -121,10 +121,37 @@ export class AuditEventRelay { }); } - private workflowPostExecute(event: Event['workflow-post-execute']) { + private workflowPostExecute({ + executionId, + success, + userId, + workflowId, + isManual, + workflowName, + metadata, + lastNodeExecuted, + errorNodeType, + errorMessage, + }: Event['workflow-post-execute']) { + const payload: Event['workflow-post-execute'] = { + executionId, + success, + userId, + workflowId, + isManual, + workflowName, + metadata, + }; + + if (!success) { + payload.lastNodeExecuted = lastNodeExecuted; + payload.errorNodeType = errorNodeType; + payload.errorMessage = errorMessage; + } + void this.eventBus.sendWorkflowEvent({ - eventName: 'n8n.workflow.success', - payload: event, + eventName: success ? 'n8n.workflow.success' : 'n8n.workflow.failed', + payload, }); } diff --git a/packages/cli/src/eventbus/event.types.ts b/packages/cli/src/eventbus/event.types.ts index 225f9aca8c873..661304444021d 100644 --- a/packages/cli/src/eventbus/event.types.ts +++ b/packages/cli/src/eventbus/event.types.ts @@ -46,6 +46,11 @@ export type Event = { isManual: boolean; workflowName: string; metadata?: Record; + + // failed case + lastNodeExecuted?: string; + errorNodeType?: string; + errorMessage?: string; }; 'node-pre-execute': { diff --git a/packages/cli/src/executions/execution-recovery.service.ts b/packages/cli/src/executions/execution-recovery.service.ts index 05431a6d9c9d5..919c9a4c04c2b 100644 --- a/packages/cli/src/executions/execution-recovery.service.ts +++ b/packages/cli/src/executions/execution-recovery.service.ts @@ -296,6 +296,12 @@ export class ExecutionRecoveryService { executionId: execution.id, success: execution.status === 'success', isManual: execution.mode === 'manual', + lastNodeExecuted: execution.data.resultData.lastNodeExecuted, + errorNodeType: + execution.data.resultData.error && 'node' in execution.data.resultData.error + ? execution.data.resultData.error.node?.type + : undefined, + errorMessage: execution.data.resultData.error?.message.toString(), }); const externalHooks = getWorkflowHooksMain( From 9b2a0ef67b34e66bf75b6b48ce5e55a722a4ff15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Wed, 31 Jul 2024 10:55:19 +0200 Subject: [PATCH 2/3] Remove stray log --- packages/cli/src/InternalHooks.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli/src/InternalHooks.ts b/packages/cli/src/InternalHooks.ts index c64bc89348e64..7912054085824 100644 --- a/packages/cli/src/InternalHooks.ts +++ b/packages/cli/src/InternalHooks.ts @@ -175,7 +175,7 @@ export class InternalHooks { runData.status = 'canceled'; } - telemetryProperties.success = !!runData?.finished; // xyz + telemetryProperties.success = !!runData?.finished; // const executionStatus: ExecutionStatus = runData?.status ?? 'unknown'; const executionStatus: ExecutionStatus = runData From bfd340a59131ff5719fb1e5ea2f2e23df0c3d89c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Wed, 31 Jul 2024 11:53:15 +0200 Subject: [PATCH 3/3] Apply feedback --- .../cli/src/WorkflowExecuteAdditionalData.ts | 14 +----- packages/cli/src/WorkflowRunner.ts | 7 +-- .../audit-event-relay.service.test.ts | 42 +++++++++++----- .../src/eventbus/audit-event-relay.service.ts | 48 ++++++++----------- packages/cli/src/eventbus/event.types.ts | 8 +--- .../executions/execution-recovery.service.ts | 7 +-- 6 files changed, 56 insertions(+), 70 deletions(-) diff --git a/packages/cli/src/WorkflowExecuteAdditionalData.ts b/packages/cli/src/WorkflowExecuteAdditionalData.ts index a5685615892e9..98ad9acde2936 100644 --- a/packages/cli/src/WorkflowExecuteAdditionalData.ts +++ b/packages/cli/src/WorkflowExecuteAdditionalData.ts @@ -651,12 +651,7 @@ function hookFunctionsSaveWorker(): IWorkflowExecuteHooks { executionId, success: runData.status === 'success', isManual: runData.mode === 'manual', - lastNodeExecuted: runData?.data.resultData.lastNodeExecuted, - errorNodeType: - runData?.data.resultData.error && 'node' in runData?.data.resultData.error - ? runData?.data.resultData.error.node?.type - : undefined, - errorMessage: runData?.data.resultData.error?.message.toString(), + runData, }); }, async function (this: WorkflowHooks, fullRunData: IRun) { @@ -946,12 +941,7 @@ async function executeWorkflow( success: data.status === 'success', isManual: data.mode === 'manual', userId: additionalData.userId, - lastNodeExecuted: data?.data.resultData.lastNodeExecuted, - errorNodeType: - data?.data.resultData.error && 'node' in data?.data.resultData.error - ? data?.data.resultData.error.node?.type - : undefined, - errorMessage: data?.data.resultData.error?.message.toString(), + runData: data, }); // subworkflow either finished, or is in status waiting due to a wait node, both cases are considered successes here diff --git a/packages/cli/src/WorkflowRunner.ts b/packages/cli/src/WorkflowRunner.ts index 0bf62e582a5d0..3318dd283cd60 100644 --- a/packages/cli/src/WorkflowRunner.ts +++ b/packages/cli/src/WorkflowRunner.ts @@ -173,12 +173,7 @@ export class WorkflowRunner { success: executionData?.status === 'success', isManual: data.executionMode === 'manual', userId: data.userId, - lastNodeExecuted: executionData?.data.resultData.lastNodeExecuted, - errorNodeType: - executionData?.data.resultData.error && 'node' in executionData?.data.resultData.error - ? executionData?.data.resultData.error.node?.type - : undefined, - errorMessage: executionData?.data.resultData.error?.message.toString(), + runData: executionData, }); if (this.externalHooks.exists('workflow.postExecute')) { try { diff --git a/packages/cli/src/eventbus/__tests__/audit-event-relay.service.test.ts b/packages/cli/src/eventbus/__tests__/audit-event-relay.service.test.ts index 6dbb7fc58ff2d..145bf2929f07a 100644 --- a/packages/cli/src/eventbus/__tests__/audit-event-relay.service.test.ts +++ b/packages/cli/src/eventbus/__tests__/audit-event-relay.service.test.ts @@ -3,6 +3,7 @@ import { AuditEventRelay } from '../audit-event-relay.service'; import type { MessageEventBus } from '../MessageEventBus/MessageEventBus'; import type { Event } from '../event.types'; import { EventService } from '../event.service'; +import type { INode, IRun } from 'n8n-workflow'; describe('AuditorService', () => { const eventBus = mock(); @@ -83,7 +84,7 @@ describe('AuditorService', () => { }); it('should log on `workflow-post-execute` for successful execution', () => { - const payload = { + const payload = mock({ executionId: 'some-id', success: true, userId: 'some-id', @@ -91,14 +92,12 @@ describe('AuditorService', () => { isManual: true, workflowName: 'some-name', metadata: {}, - lastNodeExecuted: 'some-node', - errorNodeType: 'some-type', - errorMessage: 'some-message', - }; + runData: mock({ data: { resultData: {} } }), + }); eventService.emit('workflow-post-execute', payload); - const { lastNodeExecuted: _, errorNodeType: __, errorMessage: ___, ...rest } = payload; + const { runData: _, ...rest } = payload; expect(eventBus.sendWorkflowEvent).toHaveBeenCalledWith({ eventName: 'n8n.workflow.success', @@ -107,7 +106,21 @@ describe('AuditorService', () => { }); it('should handle `workflow-post-execute` event for unsuccessful execution', () => { - const payload = { + const runData = mock({ + data: { + resultData: { + lastNodeExecuted: 'some-node', + // @ts-expect-error Partial mock + error: { + node: mock({ type: 'some-type' }), + message: 'some-message', + }, + errorMessage: 'some-message', + }, + }, + }) as unknown as IRun; + + const event = { executionId: 'some-id', success: false, userId: 'some-id', @@ -115,16 +128,21 @@ describe('AuditorService', () => { isManual: true, workflowName: 'some-name', metadata: {}, - lastNodeExecuted: 'some-node', - errorNodeType: 'some-type', - errorMessage: 'some-message', + runData, }; - eventService.emit('workflow-post-execute', payload); + eventService.emit('workflow-post-execute', event); + + const { runData: _, ...rest } = event; expect(eventBus.sendWorkflowEvent).toHaveBeenCalledWith({ eventName: 'n8n.workflow.failed', - payload, + payload: { + ...rest, + lastNodeExecuted: 'some-node', + errorNodeType: 'some-type', + errorMessage: 'some-message', + }, }); }); }); diff --git a/packages/cli/src/eventbus/audit-event-relay.service.ts b/packages/cli/src/eventbus/audit-event-relay.service.ts index 449f8de248b7f..56f4dd95cd1e6 100644 --- a/packages/cli/src/eventbus/audit-event-relay.service.ts +++ b/packages/cli/src/eventbus/audit-event-relay.service.ts @@ -121,37 +121,29 @@ export class AuditEventRelay { }); } - private workflowPostExecute({ - executionId, - success, - userId, - workflowId, - isManual, - workflowName, - metadata, - lastNodeExecuted, - errorNodeType, - errorMessage, - }: Event['workflow-post-execute']) { - const payload: Event['workflow-post-execute'] = { - executionId, - success, - userId, - workflowId, - isManual, - workflowName, - metadata, - }; - - if (!success) { - payload.lastNodeExecuted = lastNodeExecuted; - payload.errorNodeType = errorNodeType; - payload.errorMessage = errorMessage; + private workflowPostExecute(event: Event['workflow-post-execute']) { + const { runData, ...rest } = event; + + if (event.success) { + void this.eventBus.sendWorkflowEvent({ + eventName: 'n8n.workflow.success', + payload: rest, + }); + + return; } void this.eventBus.sendWorkflowEvent({ - eventName: success ? 'n8n.workflow.success' : 'n8n.workflow.failed', - payload, + eventName: 'n8n.workflow.failed', + payload: { + ...rest, + lastNodeExecuted: runData?.data.resultData.lastNodeExecuted, + errorNodeType: + runData?.data.resultData.error && 'node' in runData?.data.resultData.error + ? runData?.data.resultData.error.node?.type + : undefined, + errorMessage: runData?.data.resultData.error?.message.toString(), + }, }); } diff --git a/packages/cli/src/eventbus/event.types.ts b/packages/cli/src/eventbus/event.types.ts index 661304444021d..513c659d3e2a3 100644 --- a/packages/cli/src/eventbus/event.types.ts +++ b/packages/cli/src/eventbus/event.types.ts @@ -1,4 +1,4 @@ -import type { AuthenticationMethod, IWorkflowBase } from 'n8n-workflow'; +import type { AuthenticationMethod, IRun, IWorkflowBase } from 'n8n-workflow'; import type { IWorkflowExecutionDataProcess } from '@/Interfaces'; import type { ProjectRole } from '@/databases/entities/ProjectRelation'; import type { GlobalRole } from '@/databases/entities/User'; @@ -46,11 +46,7 @@ export type Event = { isManual: boolean; workflowName: string; metadata?: Record; - - // failed case - lastNodeExecuted?: string; - errorNodeType?: string; - errorMessage?: string; + runData?: IRun; }; 'node-pre-execute': { diff --git a/packages/cli/src/executions/execution-recovery.service.ts b/packages/cli/src/executions/execution-recovery.service.ts index 919c9a4c04c2b..b72fc490dddbb 100644 --- a/packages/cli/src/executions/execution-recovery.service.ts +++ b/packages/cli/src/executions/execution-recovery.service.ts @@ -296,12 +296,7 @@ export class ExecutionRecoveryService { executionId: execution.id, success: execution.status === 'success', isManual: execution.mode === 'manual', - lastNodeExecuted: execution.data.resultData.lastNodeExecuted, - errorNodeType: - execution.data.resultData.error && 'node' in execution.data.resultData.error - ? execution.data.resultData.error.node?.type - : undefined, - errorMessage: execution.data.resultData.error?.message.toString(), + runData: execution, }); const externalHooks = getWorkflowHooksMain(