From 78450e9943796a4ac2c07a486616fb7d0b86b254 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: Mon, 10 Mar 2025 15:30:14 +0100 Subject: [PATCH 1/5] improve the errors on permission checker --- .../__tests__/permission-checker.test.ts | 108 ++++++++++++++++++ .../src/user-management/permission-checker.ts | 34 ++++-- packages/core/src/errors/error-reporter.ts | 2 +- .../src/errors/credential-access-error.ts | 25 ---- packages/workflow/src/errors/index.ts | 1 - 5 files changed, 135 insertions(+), 35 deletions(-) create mode 100644 packages/cli/src/user-management/__tests__/permission-checker.test.ts delete mode 100644 packages/workflow/src/errors/credential-access-error.ts diff --git a/packages/cli/src/user-management/__tests__/permission-checker.test.ts b/packages/cli/src/user-management/__tests__/permission-checker.test.ts new file mode 100644 index 0000000000000..945e079328dc9 --- /dev/null +++ b/packages/cli/src/user-management/__tests__/permission-checker.test.ts @@ -0,0 +1,108 @@ +import { mock } from 'jest-mock-extended'; +import type { INode } from 'n8n-workflow'; + +import type { Project } from '@/databases/entities/project'; +import type { User } from '@/databases/entities/user'; +import type { SharedCredentialsRepository } from '@/databases/repositories/shared-credentials.repository'; +import type { OwnershipService } from '@/services/ownership.service'; +import type { ProjectService } from '@/services/project.service.ee'; + +import { PermissionChecker } from '../permission-checker'; + +describe('PermissionChecker', () => { + const sharedCredentialsRepository = mock(); + const ownershipService = mock(); + const projectService = mock(); + const permissionChecker = new PermissionChecker( + sharedCredentialsRepository, + ownershipService, + projectService, + ); + + const workflowId = 'workflow123'; + const credentialId = 'cred123'; + const personalProject = mock({ + id: 'personal-project', + name: 'Personal Project', + type: 'personal', + }); + + const node = mock({ + name: 'Test Node', + credentials: { + someCredential: { + id: credentialId, + name: 'Test Credential', + }, + }, + disabled: false, + }); + + beforeEach(async () => { + jest.resetAllMocks(); + + node.credentials!.someCredential.id = credentialId; + ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(personalProject); + projectService.findProjectsWorkflowIsIn.mockResolvedValueOnce([personalProject.id]); + }); + + it('should throw if a node has a credential without an id', async () => { + node.credentials!.someCredential.id = null; + + await expect(permissionChecker.check(workflowId, [node])).rejects.toThrow( + 'Node "Test Node" uses invalid credential', + ); + + expect(projectService.findProjectsWorkflowIsIn).toHaveBeenCalledWith(workflowId); + expect(sharedCredentialsRepository.getFilteredAccessibleCredentials).not.toHaveBeenCalled(); + }); + + it('should throw if a credential is not accessible', async () => { + ownershipService.getPersonalProjectOwnerCached.mockResolvedValueOnce(null); + sharedCredentialsRepository.getFilteredAccessibleCredentials.mockResolvedValueOnce([]); + + await expect(permissionChecker.check(workflowId, [node])).rejects.toThrow( + 'Node "Test Node" does not have access to the credential', + ); + + expect(projectService.findProjectsWorkflowIsIn).toHaveBeenCalledWith(workflowId); + expect(sharedCredentialsRepository.getFilteredAccessibleCredentials).toHaveBeenCalledWith( + [personalProject.id], + [credentialId], + ); + }); + + it('should not throw an error if the workflow has no credentials', async () => { + await expect(permissionChecker.check(workflowId, [])).resolves.not.toThrow(); + + expect(projectService.findProjectsWorkflowIsIn).toHaveBeenCalledWith(workflowId); + expect(sharedCredentialsRepository.getFilteredAccessibleCredentials).not.toHaveBeenCalled(); + }); + + it('should not throw an error if all credentials are accessible', async () => { + ownershipService.getPersonalProjectOwnerCached.mockResolvedValueOnce(null); + sharedCredentialsRepository.getFilteredAccessibleCredentials.mockResolvedValueOnce([ + credentialId, + ]); + + await expect(permissionChecker.check(workflowId, [node])).resolves.not.toThrow(); + + expect(projectService.findProjectsWorkflowIsIn).toHaveBeenCalledWith(workflowId); + expect(sharedCredentialsRepository.getFilteredAccessibleCredentials).toHaveBeenCalledWith( + [personalProject.id], + [credentialId], + ); + }); + + it('should skip credential checks if the home project owner has global scope', async () => { + const projectOwner = mock({ + hasGlobalScope: (scope) => scope === 'credential:list', + }); + ownershipService.getPersonalProjectOwnerCached.mockResolvedValueOnce(projectOwner); + + await expect(permissionChecker.check(workflowId, [node])).resolves.not.toThrow(); + + expect(projectService.findProjectsWorkflowIsIn).not.toHaveBeenCalled(); + expect(sharedCredentialsRepository.getFilteredAccessibleCredentials).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/cli/src/user-management/permission-checker.ts b/packages/cli/src/user-management/permission-checker.ts index 72ac867061052..cdf0911f806b7 100644 --- a/packages/cli/src/user-management/permission-checker.ts +++ b/packages/cli/src/user-management/permission-checker.ts @@ -1,11 +1,34 @@ import { Service } from '@n8n/di'; import type { INode } from 'n8n-workflow'; -import { CredentialAccessError, NodeOperationError } from 'n8n-workflow'; +import { UserError } from 'n8n-workflow'; +import type { Project } from '@/databases/entities/project'; import { SharedCredentialsRepository } from '@/databases/repositories/shared-credentials.repository'; import { OwnershipService } from '@/services/ownership.service'; import { ProjectService } from '@/services/project.service.ee'; +class InvalidCredentialError extends UserError { + override description = 'Please recreate the credential.'; + + constructor(readonly node: INode) { + super(`Node "${node.name}" uses invalid credential`); + } +} + +class InaccessibleCredentialError extends UserError { + override description = + this.project.type === 'personal' + ? 'Please recreate the credential or ask its owner to share it with you.' + : `Please make sure that the credential is shared with the project "${this.project.name}"`; + + constructor( + readonly node: INode, + private project: Project, + ) { + super(`Node "${node.name}" does not have access to the credential`); + } +} + @Service() export class PermissionChecker { constructor( @@ -42,7 +65,7 @@ export class PermissionChecker { for (const credentialsId of workflowCredIds) { if (!accessible.includes(credentialsId)) { const nodeToFlag = credIdsToNodes[credentialsId][0]; - throw new CredentialAccessError(nodeToFlag, credentialsId, workflowId); + throw new InaccessibleCredentialError(nodeToFlag, homeProject); } } } @@ -52,12 +75,7 @@ export class PermissionChecker { if (node.disabled || !node.credentials) return map; Object.values(node.credentials).forEach((cred) => { - if (!cred.id) { - throw new NodeOperationError(node, 'Node uses invalid credential', { - description: 'Please recreate the credential.', - level: 'warning', - }); - } + if (!cred.id) throw new InvalidCredentialError(node); map[cred.id] = map[cred.id] ? [...map[cred.id], node] : [node]; }); diff --git a/packages/core/src/errors/error-reporter.ts b/packages/core/src/errors/error-reporter.ts index 50e900cce80f3..4dd726a2b203f 100644 --- a/packages/core/src/errors/error-reporter.ts +++ b/packages/core/src/errors/error-reporter.ts @@ -190,7 +190,7 @@ export class ErrorReporter { 'cause' in originalException && originalException.cause instanceof Error && 'level' in originalException.cause && - originalException.cause.level === 'warning' + (originalException.cause.level === 'warning' || originalException.cause.level === 'info') ) { // handle underlying errors propagating from dependencies like ai-assistant-sdk return null; diff --git a/packages/workflow/src/errors/credential-access-error.ts b/packages/workflow/src/errors/credential-access-error.ts deleted file mode 100644 index 6de0c3ec96be8..0000000000000 --- a/packages/workflow/src/errors/credential-access-error.ts +++ /dev/null @@ -1,25 +0,0 @@ -import { ExecutionBaseError } from './abstract/execution-base.error'; -import type { INode } from '../Interfaces'; - -export class CredentialAccessError extends ExecutionBaseError { - override readonly description = - 'Please recreate the credential or ask its owner to share it with you.'; - - override readonly level = 'warning'; - - constructor( - readonly node: INode, - credentialId: string, - workflowId: string, - ) { - super('Node has no access to credential', { - tags: { - nodeType: node.type, - }, - extra: { - credentialId, - workflowId, - }, - }); - } -} diff --git a/packages/workflow/src/errors/index.ts b/packages/workflow/src/errors/index.ts index fd501dc56659c..db3245608e90c 100644 --- a/packages/workflow/src/errors/index.ts +++ b/packages/workflow/src/errors/index.ts @@ -5,7 +5,6 @@ export { UnexpectedError, type UnexpectedErrorOptions } from './base/unexpected. export { UserError, type UserErrorOptions } from './base/user.error'; export { ApplicationError } from './application.error'; export { ExpressionError } from './expression.error'; -export { CredentialAccessError } from './credential-access-error'; export { ExecutionCancelledError } from './execution-cancelled.error'; export { NodeApiError } from './node-api.error'; export { NodeOperationError } from './node-operation.error'; From a688e95ec908c207dd784b575004c177e5dabbfe 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: Mon, 10 Mar 2025 15:48:09 +0100 Subject: [PATCH 2/5] render subworkflow permission-checking errors --- .../cli/src/__tests__/workflow-helper.test.ts | 43 ++++++++++ .../src/workflow-execute-additional-data.ts | 28 +++---- packages/cli/src/workflow-helpers.ts | 78 ++++++++++--------- 3 files changed, 98 insertions(+), 51 deletions(-) create mode 100644 packages/cli/src/__tests__/workflow-helper.test.ts diff --git a/packages/cli/src/__tests__/workflow-helper.test.ts b/packages/cli/src/__tests__/workflow-helper.test.ts new file mode 100644 index 0000000000000..7537ab53f3214 --- /dev/null +++ b/packages/cli/src/__tests__/workflow-helper.test.ts @@ -0,0 +1,43 @@ +import { mock } from 'jest-mock-extended'; +import type { INode, WorkflowExecuteMode } from 'n8n-workflow'; +import { NodeOperationError } from 'n8n-workflow'; + +import { generateFailedExecutionFromError } from '@/workflow-helpers'; + +describe('generateFailedExecutionFromError', () => { + const mode: WorkflowExecuteMode = 'manual'; + const node = mock({ name: 'Test Node' }); + const error = new NodeOperationError(node, 'Test error message'); + + it('should generate a failed execution with error details', () => { + const startTime = Date.now(); + + const result = generateFailedExecutionFromError(mode, error, node, startTime); + + expect(result.mode).toBe(mode); + expect(result.status).toBe('error'); + expect(result.startedAt).toBeInstanceOf(Date); + expect(result.stoppedAt).toBeInstanceOf(Date); + expect(result.data.resultData.error?.message).toEqual(error.message); + + const taskData = result.data.resultData.runData[node.name][0]; + expect(taskData.error?.message).toEqual(error.message); + expect(taskData.startTime).toBe(startTime); + expect(taskData.executionStatus).toBe('error'); + expect(result.data.resultData.lastNodeExecuted).toBe(node.name); + expect(result.data.executionData?.nodeExecutionStack[0].node).toEqual(node); + }); + + it('should generate a failed execution without node details if node is undefined', () => { + const result = generateFailedExecutionFromError(mode, error, undefined); + + expect(result.mode).toBe(mode); + expect(result.status).toBe('error'); + expect(result.startedAt).toBeInstanceOf(Date); + expect(result.stoppedAt).toBeInstanceOf(Date); + expect(result.data.resultData.error?.message).toEqual(error.message); + expect(result.data.resultData.runData).toEqual({}); + expect(result.data.resultData.lastNodeExecuted).toBeUndefined(); + expect(result.data.executionData).toBeUndefined(); + }); +}); diff --git a/packages/cli/src/workflow-execute-additional-data.ts b/packages/cli/src/workflow-execute-additional-data.ts index 43b478703eeb8..7d4c96a0f45b5 100644 --- a/packages/cli/src/workflow-execute-additional-data.ts +++ b/packages/cli/src/workflow-execute-additional-data.ts @@ -14,7 +14,6 @@ import type { INode, INodeExecutionData, INodeParameters, - IRun, IRunExecutionData, IWorkflowBase, IWorkflowExecuteAdditionalData, @@ -29,6 +28,7 @@ import type { EnvProviderState, ExecuteWorkflowData, RelatedExecution, + NodeError, } from 'n8n-workflow'; import { ActiveExecutions } from '@/active-executions'; @@ -204,6 +204,8 @@ async function startExecution( */ await executionRepository.setRunning(executionId); + const startTime = Date.now(); + let data; try { await Container.get(PermissionChecker).check(workflowData.id, workflowData.nodes); @@ -239,7 +241,7 @@ async function startExecution( // If no timeout was given from the parent, then we use our timeout. subworkflowTimeout = Math.min( additionalData.executionTimeoutTimestamp || Number.MAX_SAFE_INTEGER, - Date.now() + workflowSettings.executionTimeout * 1000, + startTime + workflowSettings.executionTimeout * 1000, ); } @@ -257,20 +259,14 @@ async function startExecution( activeExecutions.attachWorkflowExecution(executionId, execution); data = await execution; } catch (error) { - const executionError = error ? (error as ExecutionError) : undefined; - const fullRunData: IRun = { - data: { - resultData: { - error: executionError, - runData: {}, - }, - }, - finished: false, - mode: 'integrated', - startedAt: new Date(), - stoppedAt: new Date(), - status: 'error', - }; + const executionError = error as ExecutionError; + const fullRunData = WorkflowHelpers.generateFailedExecutionFromError( + runData.executionMode, + executionError, + (error as NodeError).node, + startTime, + ); + // When failing, we might not have finished the execution // Therefore, database might not contain finished errors. // Force an update to db as there should be no harm doing this diff --git a/packages/cli/src/workflow-helpers.ts b/packages/cli/src/workflow-helpers.ts index 3f47b13c821c6..5ba8b7ef5bfca 100644 --- a/packages/cli/src/workflow-helpers.ts +++ b/packages/cli/src/workflow-helpers.ts @@ -5,11 +5,9 @@ import type { INodeCredentialsDetails, IRun, ITaskData, - NodeApiError, WorkflowExecuteMode, - WorkflowOperationError, - NodeOperationError, IWorkflowBase, + ExecutionError, } from 'n8n-workflow'; import { v4 as uuid } from 'uuid'; @@ -18,41 +16,20 @@ import { VariablesService } from '@/environments.ee/variables/variables.service. export function generateFailedExecutionFromError( mode: WorkflowExecuteMode, - error: NodeApiError | NodeOperationError | WorkflowOperationError, - node: INode, + error: ExecutionError, + node: INode | undefined, + startTime = Date.now(), ): IRun { - return { + const executionError = { + ...error, + message: error.message, + stack: error.stack, + }; + const returnData: IRun = { data: { - startData: { - destinationNode: node.name, - runNodeFilter: [node.name], - }, resultData: { - error, - runData: { - [node.name]: [ - { - startTime: 0, - executionTime: 0, - error, - source: [], - }, - ], - }, - lastNodeExecuted: node.name, - }, - executionData: { - contextData: {}, - metadata: {}, - nodeExecutionStack: [ - { - node, - data: {}, - source: null, - }, - ], - waitingExecution: {}, - waitingExecutionSource: {}, + error: executionError, + runData: {}, }, }, finished: false, @@ -61,6 +38,37 @@ export function generateFailedExecutionFromError( stoppedAt: new Date(), status: 'error', }; + + if (node) { + returnData.data.startData = { + destinationNode: node.name, + runNodeFilter: [node.name], + }; + returnData.data.resultData.lastNodeExecuted = node.name; + returnData.data.resultData.runData[node.name] = [ + { + startTime, + executionTime: 0, + executionStatus: 'error', + error: executionError, + source: [], + }, + ]; + returnData.data.executionData = { + contextData: {}, + metadata: {}, + waitingExecution: {}, + waitingExecutionSource: {}, + nodeExecutionStack: [ + { + node, + data: {}, + source: null, + }, + ], + }; + } + return returnData; } /** From 40d9c33b3a0a1f5bec7e0bf258773cb47289334a 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, 11 Mar 2025 11:42:53 +0100 Subject: [PATCH 3/5] address PR feedback --- .../workflow-execute-additional-data.test.ts | 8 ++- .../cli/src/__tests__/workflow-helper.test.ts | 43 ------------- .../cli/src/__tests__/workflow-runner.test.ts | 6 +- .../__tests__/execution.service.test.ts | 46 +++++++++++++- .../cli/src/executions/execution.service.ts | 58 ++++++++++++++++++ .../credentials-permission-checker.test.ts} | 6 +- .../subworkflow-policy-checker.test.ts | 2 +- .../credentials-permission-checker.ts} | 4 +- .../executions/pre-execution-checks/index.ts | 2 + .../subworkflow-policy-checker.ts} | 0 .../src/workflow-execute-additional-data.ts | 14 +++-- packages/cli/src/workflow-helpers.ts | 60 ------------------- packages/cli/src/workflow-runner.ts | 18 +++--- .../workflow-execution.service.test.ts | 1 + .../workflows/workflow-execution.service.ts | 7 ++- .../credentials-permission-checker.test.ts} | 21 ++++--- 16 files changed, 153 insertions(+), 143 deletions(-) delete mode 100644 packages/cli/src/__tests__/workflow-helper.test.ts rename packages/cli/src/{user-management/__tests__/permission-checker.test.ts => executions/pre-execution-checks/__tests__/credentials-permission-checker.test.ts} (95%) rename packages/cli/src/{subworkflows => executions/pre-execution-checks}/__tests__/subworkflow-policy-checker.test.ts (99%) rename packages/cli/src/{user-management/permission-checker.ts => executions/pre-execution-checks/credentials-permission-checker.ts} (97%) create mode 100644 packages/cli/src/executions/pre-execution-checks/index.ts rename packages/cli/src/{subworkflows/subworkflow-policy-checker.service.ts => executions/pre-execution-checks/subworkflow-policy-checker.ts} (100%) rename packages/cli/test/integration/{permission-checker.test.ts => executions/pre-execution-checks/credentials-permission-checker.test.ts} (90%) diff --git a/packages/cli/src/__tests__/workflow-execute-additional-data.test.ts b/packages/cli/src/__tests__/workflow-execute-additional-data.test.ts index 4c3af0e69646a..4b0154e11ccc9 100644 --- a/packages/cli/src/__tests__/workflow-execute-additional-data.test.ts +++ b/packages/cli/src/__tests__/workflow-execute-additional-data.test.ts @@ -17,12 +17,14 @@ import { ExecutionRepository } from '@/databases/repositories/execution.reposito import { WorkflowRepository } from '@/databases/repositories/workflow.repository'; import { VariablesService } from '@/environments.ee/variables/variables.service.ee'; import { EventService } from '@/events/event.service'; +import { + CredentialsPermissionChecker, + SubworkflowPolicyChecker, +} from '@/executions/pre-execution-checks'; import { ExternalHooks } from '@/external-hooks'; import { SecretsHelper } from '@/secrets-helpers.ee'; import { WorkflowStatisticsService } from '@/services/workflow-statistics.service'; -import { SubworkflowPolicyChecker } from '@/subworkflows/subworkflow-policy-checker.service'; import { Telemetry } from '@/telemetry'; -import { PermissionChecker } from '@/user-management/permission-checker'; import { executeWorkflow, getBase, getRunData } from '@/workflow-execute-additional-data'; import { mockInstance } from '@test/mocking'; @@ -91,7 +93,7 @@ describe('WorkflowExecuteAdditionalData', () => { mockInstance(Telemetry); const workflowRepository = mockInstance(WorkflowRepository); const activeExecutions = mockInstance(ActiveExecutions); - mockInstance(PermissionChecker); + mockInstance(CredentialsPermissionChecker); mockInstance(SubworkflowPolicyChecker); mockInstance(WorkflowStatisticsService); diff --git a/packages/cli/src/__tests__/workflow-helper.test.ts b/packages/cli/src/__tests__/workflow-helper.test.ts deleted file mode 100644 index 7537ab53f3214..0000000000000 --- a/packages/cli/src/__tests__/workflow-helper.test.ts +++ /dev/null @@ -1,43 +0,0 @@ -import { mock } from 'jest-mock-extended'; -import type { INode, WorkflowExecuteMode } from 'n8n-workflow'; -import { NodeOperationError } from 'n8n-workflow'; - -import { generateFailedExecutionFromError } from '@/workflow-helpers'; - -describe('generateFailedExecutionFromError', () => { - const mode: WorkflowExecuteMode = 'manual'; - const node = mock({ name: 'Test Node' }); - const error = new NodeOperationError(node, 'Test error message'); - - it('should generate a failed execution with error details', () => { - const startTime = Date.now(); - - const result = generateFailedExecutionFromError(mode, error, node, startTime); - - expect(result.mode).toBe(mode); - expect(result.status).toBe('error'); - expect(result.startedAt).toBeInstanceOf(Date); - expect(result.stoppedAt).toBeInstanceOf(Date); - expect(result.data.resultData.error?.message).toEqual(error.message); - - const taskData = result.data.resultData.runData[node.name][0]; - expect(taskData.error?.message).toEqual(error.message); - expect(taskData.startTime).toBe(startTime); - expect(taskData.executionStatus).toBe('error'); - expect(result.data.resultData.lastNodeExecuted).toBe(node.name); - expect(result.data.executionData?.nodeExecutionStack[0].node).toEqual(node); - }); - - it('should generate a failed execution without node details if node is undefined', () => { - const result = generateFailedExecutionFromError(mode, error, undefined); - - expect(result.mode).toBe(mode); - expect(result.status).toBe('error'); - expect(result.startedAt).toBeInstanceOf(Date); - expect(result.stoppedAt).toBeInstanceOf(Date); - expect(result.data.resultData.error?.message).toEqual(error.message); - expect(result.data.resultData.runData).toEqual({}); - expect(result.data.resultData.lastNodeExecuted).toBeUndefined(); - expect(result.data.executionData).toBeUndefined(); - }); -}); diff --git a/packages/cli/src/__tests__/workflow-runner.test.ts b/packages/cli/src/__tests__/workflow-runner.test.ts index 0ea0646276e11..8a666114991eb 100644 --- a/packages/cli/src/__tests__/workflow-runner.test.ts +++ b/packages/cli/src/__tests__/workflow-runner.test.ts @@ -21,8 +21,8 @@ import config from '@/config'; import type { ExecutionEntity } from '@/databases/entities/execution-entity'; import type { User } from '@/databases/entities/user'; import { ExecutionNotFoundError } from '@/errors/execution-not-found-error'; +import { CredentialsPermissionChecker } from '@/executions/pre-execution-checks'; import { Telemetry } from '@/telemetry'; -import { PermissionChecker } from '@/user-management/permission-checker'; import { WorkflowRunner } from '@/workflow-runner'; import { mockInstance } from '@test/mocking'; import { createExecution } from '@test-integration/db/executions'; @@ -131,7 +131,7 @@ describe('run', () => { const activeExecutions = Container.get(ActiveExecutions); jest.spyOn(activeExecutions, 'add').mockResolvedValue('1'); jest.spyOn(activeExecutions, 'attachWorkflowExecution').mockReturnValueOnce(); - const permissionChecker = Container.get(PermissionChecker); + const permissionChecker = Container.get(CredentialsPermissionChecker); jest.spyOn(permissionChecker, 'check').mockResolvedValueOnce(); jest.spyOn(WorkflowExecute.prototype, 'processRunExecutionData').mockReturnValueOnce( @@ -171,7 +171,7 @@ describe('run', () => { const activeExecutions = Container.get(ActiveExecutions); jest.spyOn(activeExecutions, 'add').mockResolvedValue('1'); jest.spyOn(activeExecutions, 'attachWorkflowExecution').mockReturnValueOnce(); - const permissionChecker = Container.get(PermissionChecker); + const permissionChecker = Container.get(CredentialsPermissionChecker); jest.spyOn(permissionChecker, 'check').mockResolvedValueOnce(); jest.spyOn(WorkflowExecute.prototype, 'processRunExecutionData').mockReturnValueOnce( diff --git a/packages/cli/src/executions/__tests__/execution.service.test.ts b/packages/cli/src/executions/__tests__/execution.service.test.ts index e425c1e588eeb..97d8729f46c27 100644 --- a/packages/cli/src/executions/__tests__/execution.service.test.ts +++ b/packages/cli/src/executions/__tests__/execution.service.test.ts @@ -1,5 +1,6 @@ import { mock } from 'jest-mock-extended'; -import { WorkflowOperationError } from 'n8n-workflow'; +import type { INode, WorkflowExecuteMode } from 'n8n-workflow'; +import { NodeOperationError, WorkflowOperationError } from 'n8n-workflow'; import type { ActiveExecutions } from '@/active-executions'; import type { ConcurrencyControlService } from '@/concurrency/concurrency-control.service'; @@ -277,4 +278,47 @@ describe('ExecutionService', () => { }); }); }); + + describe('generateFailedExecutionFromError', () => { + const mode: WorkflowExecuteMode = 'manual'; + const node = mock({ name: 'Test Node' }); + const error = new NodeOperationError(node, 'Test error message'); + + it('should generate a failed execution with error details', () => { + const startTime = Date.now(); + + const result = executionService.generateFailedExecutionFromError( + mode, + error, + node, + startTime, + ); + + expect(result.mode).toBe(mode); + expect(result.status).toBe('error'); + expect(result.startedAt).toBeInstanceOf(Date); + expect(result.stoppedAt).toBeInstanceOf(Date); + expect(result.data.resultData.error?.message).toBe(error.message); + + const taskData = result.data.resultData.runData[node.name][0]; + expect(taskData.error?.message).toEqual(error.message); + expect(taskData.startTime).toBe(startTime); + expect(taskData.executionStatus).toBe('error'); + expect(result.data.resultData.lastNodeExecuted).toBe(node.name); + expect(result.data.executionData?.nodeExecutionStack[0].node).toEqual(node); + }); + + it('should generate a failed execution without node details if node is undefined', () => { + const result = executionService.generateFailedExecutionFromError(mode, error, undefined); + + expect(result.mode).toBe(mode); + expect(result.status).toBe('error'); + expect(result.startedAt).toBeInstanceOf(Date); + expect(result.stoppedAt).toBeInstanceOf(Date); + expect(result.data.resultData.error?.message).toBe(error.message); + expect(result.data.resultData.runData).toEqual({}); + expect(result.data.resultData.lastNodeExecuted).toBeUndefined(); + expect(result.data.executionData).toBeUndefined(); + }); + }); }); diff --git a/packages/cli/src/executions/execution.service.ts b/packages/cli/src/executions/execution.service.ts index c4a8188f0ece5..aa0a970ef0e04 100644 --- a/packages/cli/src/executions/execution.service.ts +++ b/packages/cli/src/executions/execution.service.ts @@ -10,6 +10,7 @@ import type { IWorkflowBase, WorkflowExecuteMode, IWorkflowExecutionDataProcess, + IRun, } from 'n8n-workflow'; import { ExecutionStatusList, @@ -524,4 +525,61 @@ export class ExecutionService { await this.annotationTagMappingRepository.overwriteTags(annotation.id, updateData.tags); } } + + generateFailedExecutionFromError( + mode: WorkflowExecuteMode, + error: ExecutionError, + node: INode | undefined, + startTime = Date.now(), + ): IRun { + const executionError = { + ...error, + message: error.message, + stack: error.stack, + }; + const returnData: IRun = { + data: { + resultData: { + error: executionError, + runData: {}, + }, + }, + finished: false, + mode, + startedAt: new Date(), + stoppedAt: new Date(), + status: 'error', + }; + + if (node) { + returnData.data.startData = { + destinationNode: node.name, + runNodeFilter: [node.name], + }; + returnData.data.resultData.lastNodeExecuted = node.name; + returnData.data.resultData.runData[node.name] = [ + { + startTime, + executionTime: 0, + executionStatus: 'error', + error: executionError, + source: [], + }, + ]; + returnData.data.executionData = { + contextData: {}, + metadata: {}, + waitingExecution: {}, + waitingExecutionSource: {}, + nodeExecutionStack: [ + { + node, + data: {}, + source: null, + }, + ], + }; + } + return returnData; + } } diff --git a/packages/cli/src/user-management/__tests__/permission-checker.test.ts b/packages/cli/src/executions/pre-execution-checks/__tests__/credentials-permission-checker.test.ts similarity index 95% rename from packages/cli/src/user-management/__tests__/permission-checker.test.ts rename to packages/cli/src/executions/pre-execution-checks/__tests__/credentials-permission-checker.test.ts index 945e079328dc9..b957a3f5175ff 100644 --- a/packages/cli/src/user-management/__tests__/permission-checker.test.ts +++ b/packages/cli/src/executions/pre-execution-checks/__tests__/credentials-permission-checker.test.ts @@ -7,13 +7,13 @@ import type { SharedCredentialsRepository } from '@/databases/repositories/share import type { OwnershipService } from '@/services/ownership.service'; import type { ProjectService } from '@/services/project.service.ee'; -import { PermissionChecker } from '../permission-checker'; +import { CredentialsPermissionChecker } from '../credentials-permission-checker'; -describe('PermissionChecker', () => { +describe('CredentialsPermissionChecker', () => { const sharedCredentialsRepository = mock(); const ownershipService = mock(); const projectService = mock(); - const permissionChecker = new PermissionChecker( + const permissionChecker = new CredentialsPermissionChecker( sharedCredentialsRepository, ownershipService, projectService, diff --git a/packages/cli/src/subworkflows/__tests__/subworkflow-policy-checker.test.ts b/packages/cli/src/executions/pre-execution-checks/__tests__/subworkflow-policy-checker.test.ts similarity index 99% rename from packages/cli/src/subworkflows/__tests__/subworkflow-policy-checker.test.ts rename to packages/cli/src/executions/pre-execution-checks/__tests__/subworkflow-policy-checker.test.ts index a47524fc1bb84..35eab2ceca071 100644 --- a/packages/cli/src/subworkflows/__tests__/subworkflow-policy-checker.test.ts +++ b/packages/cli/src/executions/pre-execution-checks/__tests__/subworkflow-policy-checker.test.ts @@ -15,7 +15,7 @@ import { OwnershipService } from '@/services/ownership.service'; import type { UrlService } from '@/services/url.service'; import { mockInstance } from '@test/mocking'; -import { SubworkflowPolicyChecker } from '../subworkflow-policy-checker.service'; +import { SubworkflowPolicyChecker } from '../subworkflow-policy-checker'; describe('SubworkflowPolicyChecker', () => { const ownershipService = mockInstance(OwnershipService); diff --git a/packages/cli/src/user-management/permission-checker.ts b/packages/cli/src/executions/pre-execution-checks/credentials-permission-checker.ts similarity index 97% rename from packages/cli/src/user-management/permission-checker.ts rename to packages/cli/src/executions/pre-execution-checks/credentials-permission-checker.ts index cdf0911f806b7..3123ea19d2ef2 100644 --- a/packages/cli/src/user-management/permission-checker.ts +++ b/packages/cli/src/executions/pre-execution-checks/credentials-permission-checker.ts @@ -23,14 +23,14 @@ class InaccessibleCredentialError extends UserError { constructor( readonly node: INode, - private project: Project, + private readonly project: Project, ) { super(`Node "${node.name}" does not have access to the credential`); } } @Service() -export class PermissionChecker { +export class CredentialsPermissionChecker { constructor( private readonly sharedCredentialsRepository: SharedCredentialsRepository, private readonly ownershipService: OwnershipService, diff --git a/packages/cli/src/executions/pre-execution-checks/index.ts b/packages/cli/src/executions/pre-execution-checks/index.ts new file mode 100644 index 0000000000000..8b1165e24e0fb --- /dev/null +++ b/packages/cli/src/executions/pre-execution-checks/index.ts @@ -0,0 +1,2 @@ +export { CredentialsPermissionChecker } from './credentials-permission-checker'; +export { SubworkflowPolicyChecker } from './subworkflow-policy-checker'; diff --git a/packages/cli/src/subworkflows/subworkflow-policy-checker.service.ts b/packages/cli/src/executions/pre-execution-checks/subworkflow-policy-checker.ts similarity index 100% rename from packages/cli/src/subworkflows/subworkflow-policy-checker.service.ts rename to packages/cli/src/executions/pre-execution-checks/subworkflow-policy-checker.ts diff --git a/packages/cli/src/workflow-execute-additional-data.ts b/packages/cli/src/workflow-execute-additional-data.ts index 7d4c96a0f45b5..4d1df2e633d47 100644 --- a/packages/cli/src/workflow-execute-additional-data.ts +++ b/packages/cli/src/workflow-execute-additional-data.ts @@ -28,7 +28,6 @@ import type { EnvProviderState, ExecuteWorkflowData, RelatedExecution, - NodeError, } from 'n8n-workflow'; import { ActiveExecutions } from '@/active-executions'; @@ -38,14 +37,17 @@ import { WorkflowRepository } from '@/databases/repositories/workflow.repository import { EventService } from '@/events/event.service'; import type { AiEventMap, AiEventPayload } from '@/events/maps/ai.event-map'; import { getLifecycleHooksForSubExecutions } from '@/execution-lifecycle/execution-lifecycle-hooks'; +import { ExecutionService } from '@/executions/execution.service'; +import { + CredentialsPermissionChecker, + SubworkflowPolicyChecker, +} from '@/executions/pre-execution-checks'; import type { UpdateExecutionPayload } from '@/interfaces'; import { NodeTypes } from '@/node-types'; import { Push } from '@/push'; import { SecretsHelper } from '@/secrets-helpers.ee'; import { UrlService } from '@/services/url.service'; -import { SubworkflowPolicyChecker } from '@/subworkflows/subworkflow-policy-checker.service'; import { TaskRequester } from '@/task-runners/task-managers/task-requester'; -import { PermissionChecker } from '@/user-management/permission-checker'; import { findSubworkflowStart } from '@/utils'; import { objectToError } from '@/utils/object-to-error'; import * as WorkflowHelpers from '@/workflow-helpers'; @@ -208,7 +210,7 @@ async function startExecution( let data; try { - await Container.get(PermissionChecker).check(workflowData.id, workflowData.nodes); + await Container.get(CredentialsPermissionChecker).check(workflowData.id, workflowData.nodes); await Container.get(SubworkflowPolicyChecker).check( workflow, options.parentWorkflowId, @@ -260,10 +262,10 @@ async function startExecution( data = await execution; } catch (error) { const executionError = error as ExecutionError; - const fullRunData = WorkflowHelpers.generateFailedExecutionFromError( + const fullRunData = Container.get(ExecutionService).generateFailedExecutionFromError( runData.executionMode, executionError, - (error as NodeError).node, + 'node' in executionError ? executionError.node : undefined, startTime, ); diff --git a/packages/cli/src/workflow-helpers.ts b/packages/cli/src/workflow-helpers.ts index 5ba8b7ef5bfca..620664c78475f 100644 --- a/packages/cli/src/workflow-helpers.ts +++ b/packages/cli/src/workflow-helpers.ts @@ -1,76 +1,16 @@ import { Container } from '@n8n/di'; import type { IDataObject, - INode, INodeCredentialsDetails, IRun, ITaskData, - WorkflowExecuteMode, IWorkflowBase, - ExecutionError, } from 'n8n-workflow'; import { v4 as uuid } from 'uuid'; import { CredentialsRepository } from '@/databases/repositories/credentials.repository'; import { VariablesService } from '@/environments.ee/variables/variables.service.ee'; -export function generateFailedExecutionFromError( - mode: WorkflowExecuteMode, - error: ExecutionError, - node: INode | undefined, - startTime = Date.now(), -): IRun { - const executionError = { - ...error, - message: error.message, - stack: error.stack, - }; - const returnData: IRun = { - data: { - resultData: { - error: executionError, - runData: {}, - }, - }, - finished: false, - mode, - startedAt: new Date(), - stoppedAt: new Date(), - status: 'error', - }; - - if (node) { - returnData.data.startData = { - destinationNode: node.name, - runNodeFilter: [node.name], - }; - returnData.data.resultData.lastNodeExecuted = node.name; - returnData.data.resultData.runData[node.name] = [ - { - startTime, - executionTime: 0, - executionStatus: 'error', - error: executionError, - source: [], - }, - ]; - returnData.data.executionData = { - contextData: {}, - metadata: {}, - waitingExecution: {}, - waitingExecutionSource: {}, - nodeExecutionStack: [ - { - node, - data: {}, - source: null, - }, - ], - }; - } - return returnData; -} - /** * Returns the data of the last executed node */ diff --git a/packages/cli/src/workflow-runner.ts b/packages/cli/src/workflow-runner.ts index 1c51d6e442a5c..54e656c97c2af 100644 --- a/packages/cli/src/workflow-runner.ts +++ b/packages/cli/src/workflow-runner.ts @@ -21,22 +21,21 @@ import { ActiveExecutions } from '@/active-executions'; import config from '@/config'; import { ExecutionRepository } from '@/databases/repositories/execution.repository'; import { ExecutionNotFoundError } from '@/errors/execution-not-found-error'; +import { MaxStalledCountError } from '@/errors/max-stalled-count.error'; import { getLifecycleHooksForRegularMain, getLifecycleHooksForScalingWorker, getLifecycleHooksForScalingMain, } from '@/execution-lifecycle/execution-lifecycle-hooks'; +import { ExecutionService } from '@/executions/execution.service'; +import { CredentialsPermissionChecker } from '@/executions/pre-execution-checks'; import { ManualExecutionService } from '@/manual-execution.service'; import { NodeTypes } from '@/node-types'; import type { ScalingService } from '@/scaling/scaling.service'; import type { Job, JobData } from '@/scaling/scaling.types'; -import { PermissionChecker } from '@/user-management/permission-checker'; import * as WorkflowExecuteAdditionalData from '@/workflow-execute-additional-data'; -import { generateFailedExecutionFromError } from '@/workflow-helpers'; import { WorkflowStaticDataService } from '@/workflows/workflow-static-data.service'; -import { MaxStalledCountError } from './errors/max-stalled-count.error'; - @Service() export class WorkflowRunner { private scalingService: ScalingService; @@ -50,9 +49,10 @@ export class WorkflowRunner { private readonly executionRepository: ExecutionRepository, private readonly workflowStaticDataService: WorkflowStaticDataService, private readonly nodeTypes: NodeTypes, - private readonly permissionChecker: PermissionChecker, + private readonly credentialsPermissionChecker: CredentialsPermissionChecker, private readonly instanceSettings: InstanceSettings, private readonly manualExecutionService: ManualExecutionService, + private readonly executionService: ExecutionService, ) {} /** The process did error */ @@ -134,10 +134,14 @@ export class WorkflowRunner { const { id: workflowId, nodes } = data.workflowData; try { - await this.permissionChecker.check(workflowId, nodes); + await this.credentialsPermissionChecker.check(workflowId, nodes); } catch (error) { // Create a failed execution with the data for the node, save it and abort execution - const runData = generateFailedExecutionFromError(data.executionMode, error, error.node); + const runData = this.executionService.generateFailedExecutionFromError( + data.executionMode, + error, + error.node, + ); const lifecycleHooks = getLifecycleHooksForRegularMain(data, executionId); await lifecycleHooks.runHook('workflowExecuteBefore', [undefined, data.executionData]); await lifecycleHooks.runHook('workflowExecuteAfter', [runData]); diff --git a/packages/cli/src/workflows/__tests__/workflow-execution.service.test.ts b/packages/cli/src/workflows/__tests__/workflow-execution.service.test.ts index dfac6d7366d16..64b5f12a06158 100644 --- a/packages/cli/src/workflows/__tests__/workflow-execution.service.test.ts +++ b/packages/cli/src/workflows/__tests__/workflow-execution.service.test.ts @@ -65,6 +65,7 @@ describe('WorkflowExecutionService', () => { workflowRunner, mock(), mock(), + mock(), ); const additionalData = mock({}); diff --git a/packages/cli/src/workflows/workflow-execution.service.ts b/packages/cli/src/workflows/workflow-execution.service.ts index 8693c4c6435cd..c12babfc5d5ba 100644 --- a/packages/cli/src/workflows/workflow-execution.service.ts +++ b/packages/cli/src/workflows/workflow-execution.service.ts @@ -21,12 +21,12 @@ import type { Project } from '@/databases/entities/project'; import type { User } from '@/databases/entities/user'; import { ExecutionRepository } from '@/databases/repositories/execution.repository'; import { WorkflowRepository } from '@/databases/repositories/workflow.repository'; +import { ExecutionService } from '@/executions/execution.service'; +import { SubworkflowPolicyChecker } from '@/executions/pre-execution-checks'; import type { CreateExecutionPayload, IWorkflowErrorData } from '@/interfaces'; import { NodeTypes } from '@/node-types'; -import { SubworkflowPolicyChecker } from '@/subworkflows/subworkflow-policy-checker.service'; import { TestWebhooks } from '@/webhooks/test-webhooks'; import * as WorkflowExecuteAdditionalData from '@/workflow-execute-additional-data'; -import * as WorkflowHelpers from '@/workflow-helpers'; import { WorkflowRunner } from '@/workflow-runner'; import type { WorkflowRequest } from '@/workflows/workflow.request'; @@ -42,6 +42,7 @@ export class WorkflowExecutionService { private readonly workflowRunner: WorkflowRunner, private readonly globalConfig: GlobalConfig, private readonly subworkflowPolicyChecker: SubworkflowPolicyChecker, + private readonly executionService: ExecutionService, ) {} async runWorkflow( @@ -273,7 +274,7 @@ export class WorkflowExecutionService { ); // Create a fake execution and save it to DB. - const fakeExecution = WorkflowHelpers.generateFailedExecutionFromError( + const fakeExecution = this.executionService.generateFailedExecutionFromError( 'error', errorWorkflowPermissionError, initialNode, diff --git a/packages/cli/test/integration/permission-checker.test.ts b/packages/cli/test/integration/executions/pre-execution-checks/credentials-permission-checker.test.ts similarity index 90% rename from packages/cli/test/integration/permission-checker.test.ts rename to packages/cli/test/integration/executions/pre-execution-checks/credentials-permission-checker.test.ts index 40a31d423643d..3d050011cb173 100644 --- a/packages/cli/test/integration/permission-checker.test.ts +++ b/packages/cli/test/integration/executions/pre-execution-checks/credentials-permission-checker.test.ts @@ -9,20 +9,19 @@ import { ProjectRepository } from '@/databases/repositories/project.repository'; import { SharedCredentialsRepository } from '@/databases/repositories/shared-credentials.repository'; import { SharedWorkflowRepository } from '@/databases/repositories/shared-workflow.repository'; import { WorkflowRepository } from '@/databases/repositories/workflow.repository'; +import { CredentialsPermissionChecker } from '@/executions/pre-execution-checks'; import { LoadNodesAndCredentials } from '@/load-nodes-and-credentials'; import { NodeTypes } from '@/node-types'; import { OwnershipService } from '@/services/ownership.service'; -import { PermissionChecker } from '@/user-management/permission-checker'; +import { mockInstance } from '@test/mocking'; +import { affixRoleToSaveCredential } from '@test-integration/db/credentials'; +import { getPersonalProject } from '@test-integration/db/projects'; +import { createOwner, createUser } from '@test-integration/db/users'; +import { randomCredentialPayload as randomCred } from '@test-integration/random'; +import * as testDb from '@test-integration/test-db'; +import type { SaveCredentialFunction } from '@test-integration/types'; import { mockNodeTypesData } from '@test-integration/utils/node-types-data'; -import { affixRoleToSaveCredential } from './shared/db/credentials'; -import { getPersonalProject } from './shared/db/projects'; -import { createOwner, createUser } from './shared/db/users'; -import { randomCredentialPayload as randomCred } from './shared/random'; -import * as testDb from './shared/test-db'; -import type { SaveCredentialFunction } from './shared/types'; -import { mockInstance } from '../shared/mocking'; - const ownershipService = mockInstance(OwnershipService); const createWorkflow = async (nodes: INode[], workflowOwner?: User): Promise => { @@ -62,14 +61,14 @@ mockInstance(LoadNodesAndCredentials, { loadedNodes: mockNodeTypesData(['start', 'actionNetwork']), }); -let permissionChecker: PermissionChecker; +let permissionChecker: CredentialsPermissionChecker; beforeAll(async () => { await testDb.init(); saveCredential = affixRoleToSaveCredential('credential:owner'); - permissionChecker = Container.get(PermissionChecker); + permissionChecker = Container.get(CredentialsPermissionChecker); [owner, member] = await Promise.all([createOwner(), createUser()]); ownerPersonalProject = await Container.get(ProjectRepository).getPersonalProjectForUserOrFail( From 06681608f1ddaae3a6b584ceb90fbf0435121377 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, 11 Mar 2025 13:25:46 +0100 Subject: [PATCH 4/5] Update packages/cli/src/executions/__tests__/execution.service.test.ts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Iván Ovejero --- packages/cli/src/executions/__tests__/execution.service.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli/src/executions/__tests__/execution.service.test.ts b/packages/cli/src/executions/__tests__/execution.service.test.ts index 97d8729f46c27..11e373603c081 100644 --- a/packages/cli/src/executions/__tests__/execution.service.test.ts +++ b/packages/cli/src/executions/__tests__/execution.service.test.ts @@ -301,7 +301,7 @@ describe('ExecutionService', () => { expect(result.data.resultData.error?.message).toBe(error.message); const taskData = result.data.resultData.runData[node.name][0]; - expect(taskData.error?.message).toEqual(error.message); + expect(taskData.error?.message).toBe(error.message); expect(taskData.startTime).toBe(startTime); expect(taskData.executionStatus).toBe('error'); expect(result.data.resultData.lastNodeExecuted).toBe(node.name); From 9a9462b23dfb8fffc77970fa6b81b9f2495024df 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, 11 Mar 2025 15:25:09 +0100 Subject: [PATCH 5/5] move generateFailedExecutionFromError to ExecutionDataService --- .../__tests__/execution-data.service.test.ts | 47 ++++++++++++++ .../__tests__/execution.service.test.ts | 46 +------------- .../src/executions/execution-data.service.ts | 62 +++++++++++++++++++ .../cli/src/executions/execution.service.ts | 58 ----------------- .../src/workflow-execute-additional-data.ts | 4 +- packages/cli/src/workflow-runner.ts | 6 +- .../workflows/workflow-execution.service.ts | 6 +- 7 files changed, 118 insertions(+), 111 deletions(-) create mode 100644 packages/cli/src/executions/__tests__/execution-data.service.test.ts create mode 100644 packages/cli/src/executions/execution-data.service.ts diff --git a/packages/cli/src/executions/__tests__/execution-data.service.test.ts b/packages/cli/src/executions/__tests__/execution-data.service.test.ts new file mode 100644 index 0000000000000..d579279af0a61 --- /dev/null +++ b/packages/cli/src/executions/__tests__/execution-data.service.test.ts @@ -0,0 +1,47 @@ +import { mock } from 'jest-mock-extended'; +import { NodeOperationError } from 'n8n-workflow'; +import type { INode, WorkflowExecuteMode } from 'n8n-workflow'; + +import { ExecutionDataService } from '../execution-data.service'; + +describe('ExecutionDataService', () => { + const service = new ExecutionDataService(); + + describe('generateFailedExecutionFromError', () => { + const mode: WorkflowExecuteMode = 'manual'; + const node = mock({ name: 'Test Node' }); + const error = new NodeOperationError(node, 'Test error message'); + + it('should generate a failed execution with error details', () => { + const startTime = Date.now(); + + const result = service.generateFailedExecutionFromError(mode, error, node, startTime); + + expect(result.mode).toBe(mode); + expect(result.status).toBe('error'); + expect(result.startedAt).toBeInstanceOf(Date); + expect(result.stoppedAt).toBeInstanceOf(Date); + expect(result.data.resultData.error?.message).toBe(error.message); + + const taskData = result.data.resultData.runData[node.name][0]; + expect(taskData.error?.message).toBe(error.message); + expect(taskData.startTime).toBe(startTime); + expect(taskData.executionStatus).toBe('error'); + expect(result.data.resultData.lastNodeExecuted).toBe(node.name); + expect(result.data.executionData?.nodeExecutionStack[0].node).toEqual(node); + }); + + it('should generate a failed execution without node details if node is undefined', () => { + const result = service.generateFailedExecutionFromError(mode, error, undefined); + + expect(result.mode).toBe(mode); + expect(result.status).toBe('error'); + expect(result.startedAt).toBeInstanceOf(Date); + expect(result.stoppedAt).toBeInstanceOf(Date); + expect(result.data.resultData.error?.message).toBe(error.message); + expect(result.data.resultData.runData).toEqual({}); + expect(result.data.resultData.lastNodeExecuted).toBeUndefined(); + expect(result.data.executionData).toBeUndefined(); + }); + }); +}); diff --git a/packages/cli/src/executions/__tests__/execution.service.test.ts b/packages/cli/src/executions/__tests__/execution.service.test.ts index 11e373603c081..e425c1e588eeb 100644 --- a/packages/cli/src/executions/__tests__/execution.service.test.ts +++ b/packages/cli/src/executions/__tests__/execution.service.test.ts @@ -1,6 +1,5 @@ import { mock } from 'jest-mock-extended'; -import type { INode, WorkflowExecuteMode } from 'n8n-workflow'; -import { NodeOperationError, WorkflowOperationError } from 'n8n-workflow'; +import { WorkflowOperationError } from 'n8n-workflow'; import type { ActiveExecutions } from '@/active-executions'; import type { ConcurrencyControlService } from '@/concurrency/concurrency-control.service'; @@ -278,47 +277,4 @@ describe('ExecutionService', () => { }); }); }); - - describe('generateFailedExecutionFromError', () => { - const mode: WorkflowExecuteMode = 'manual'; - const node = mock({ name: 'Test Node' }); - const error = new NodeOperationError(node, 'Test error message'); - - it('should generate a failed execution with error details', () => { - const startTime = Date.now(); - - const result = executionService.generateFailedExecutionFromError( - mode, - error, - node, - startTime, - ); - - expect(result.mode).toBe(mode); - expect(result.status).toBe('error'); - expect(result.startedAt).toBeInstanceOf(Date); - expect(result.stoppedAt).toBeInstanceOf(Date); - expect(result.data.resultData.error?.message).toBe(error.message); - - const taskData = result.data.resultData.runData[node.name][0]; - expect(taskData.error?.message).toBe(error.message); - expect(taskData.startTime).toBe(startTime); - expect(taskData.executionStatus).toBe('error'); - expect(result.data.resultData.lastNodeExecuted).toBe(node.name); - expect(result.data.executionData?.nodeExecutionStack[0].node).toEqual(node); - }); - - it('should generate a failed execution without node details if node is undefined', () => { - const result = executionService.generateFailedExecutionFromError(mode, error, undefined); - - expect(result.mode).toBe(mode); - expect(result.status).toBe('error'); - expect(result.startedAt).toBeInstanceOf(Date); - expect(result.stoppedAt).toBeInstanceOf(Date); - expect(result.data.resultData.error?.message).toBe(error.message); - expect(result.data.resultData.runData).toEqual({}); - expect(result.data.resultData.lastNodeExecuted).toBeUndefined(); - expect(result.data.executionData).toBeUndefined(); - }); - }); }); diff --git a/packages/cli/src/executions/execution-data.service.ts b/packages/cli/src/executions/execution-data.service.ts new file mode 100644 index 0000000000000..9e23e39d1dc84 --- /dev/null +++ b/packages/cli/src/executions/execution-data.service.ts @@ -0,0 +1,62 @@ +import { Service } from '@n8n/di'; +import type { ExecutionError, INode, IRun, WorkflowExecuteMode } from 'n8n-workflow'; + +@Service() +export class ExecutionDataService { + generateFailedExecutionFromError( + mode: WorkflowExecuteMode, + error: ExecutionError, + node: INode | undefined, + startTime = Date.now(), + ): IRun { + const executionError = { + ...error, + message: error.message, + stack: error.stack, + }; + const returnData: IRun = { + data: { + resultData: { + error: executionError, + runData: {}, + }, + }, + finished: false, + mode, + startedAt: new Date(), + stoppedAt: new Date(), + status: 'error', + }; + + if (node) { + returnData.data.startData = { + destinationNode: node.name, + runNodeFilter: [node.name], + }; + returnData.data.resultData.lastNodeExecuted = node.name; + returnData.data.resultData.runData[node.name] = [ + { + startTime, + executionTime: 0, + executionStatus: 'error', + error: executionError, + source: [], + }, + ]; + returnData.data.executionData = { + contextData: {}, + metadata: {}, + waitingExecution: {}, + waitingExecutionSource: {}, + nodeExecutionStack: [ + { + node, + data: {}, + source: null, + }, + ], + }; + } + return returnData; + } +} diff --git a/packages/cli/src/executions/execution.service.ts b/packages/cli/src/executions/execution.service.ts index aa0a970ef0e04..c4a8188f0ece5 100644 --- a/packages/cli/src/executions/execution.service.ts +++ b/packages/cli/src/executions/execution.service.ts @@ -10,7 +10,6 @@ import type { IWorkflowBase, WorkflowExecuteMode, IWorkflowExecutionDataProcess, - IRun, } from 'n8n-workflow'; import { ExecutionStatusList, @@ -525,61 +524,4 @@ export class ExecutionService { await this.annotationTagMappingRepository.overwriteTags(annotation.id, updateData.tags); } } - - generateFailedExecutionFromError( - mode: WorkflowExecuteMode, - error: ExecutionError, - node: INode | undefined, - startTime = Date.now(), - ): IRun { - const executionError = { - ...error, - message: error.message, - stack: error.stack, - }; - const returnData: IRun = { - data: { - resultData: { - error: executionError, - runData: {}, - }, - }, - finished: false, - mode, - startedAt: new Date(), - stoppedAt: new Date(), - status: 'error', - }; - - if (node) { - returnData.data.startData = { - destinationNode: node.name, - runNodeFilter: [node.name], - }; - returnData.data.resultData.lastNodeExecuted = node.name; - returnData.data.resultData.runData[node.name] = [ - { - startTime, - executionTime: 0, - executionStatus: 'error', - error: executionError, - source: [], - }, - ]; - returnData.data.executionData = { - contextData: {}, - metadata: {}, - waitingExecution: {}, - waitingExecutionSource: {}, - nodeExecutionStack: [ - { - node, - data: {}, - source: null, - }, - ], - }; - } - return returnData; - } } diff --git a/packages/cli/src/workflow-execute-additional-data.ts b/packages/cli/src/workflow-execute-additional-data.ts index 4d1df2e633d47..0155f985ec1bd 100644 --- a/packages/cli/src/workflow-execute-additional-data.ts +++ b/packages/cli/src/workflow-execute-additional-data.ts @@ -37,7 +37,7 @@ import { WorkflowRepository } from '@/databases/repositories/workflow.repository import { EventService } from '@/events/event.service'; import type { AiEventMap, AiEventPayload } from '@/events/maps/ai.event-map'; import { getLifecycleHooksForSubExecutions } from '@/execution-lifecycle/execution-lifecycle-hooks'; -import { ExecutionService } from '@/executions/execution.service'; +import { ExecutionDataService } from '@/executions/execution-data.service'; import { CredentialsPermissionChecker, SubworkflowPolicyChecker, @@ -262,7 +262,7 @@ async function startExecution( data = await execution; } catch (error) { const executionError = error as ExecutionError; - const fullRunData = Container.get(ExecutionService).generateFailedExecutionFromError( + const fullRunData = Container.get(ExecutionDataService).generateFailedExecutionFromError( runData.executionMode, executionError, 'node' in executionError ? executionError.node : undefined, diff --git a/packages/cli/src/workflow-runner.ts b/packages/cli/src/workflow-runner.ts index 54e656c97c2af..bf6b51d9c293d 100644 --- a/packages/cli/src/workflow-runner.ts +++ b/packages/cli/src/workflow-runner.ts @@ -27,7 +27,7 @@ import { getLifecycleHooksForScalingWorker, getLifecycleHooksForScalingMain, } from '@/execution-lifecycle/execution-lifecycle-hooks'; -import { ExecutionService } from '@/executions/execution.service'; +import { ExecutionDataService } from '@/executions/execution-data.service'; import { CredentialsPermissionChecker } from '@/executions/pre-execution-checks'; import { ManualExecutionService } from '@/manual-execution.service'; import { NodeTypes } from '@/node-types'; @@ -52,7 +52,7 @@ export class WorkflowRunner { private readonly credentialsPermissionChecker: CredentialsPermissionChecker, private readonly instanceSettings: InstanceSettings, private readonly manualExecutionService: ManualExecutionService, - private readonly executionService: ExecutionService, + private readonly executionDataService: ExecutionDataService, ) {} /** The process did error */ @@ -137,7 +137,7 @@ export class WorkflowRunner { await this.credentialsPermissionChecker.check(workflowId, nodes); } catch (error) { // Create a failed execution with the data for the node, save it and abort execution - const runData = this.executionService.generateFailedExecutionFromError( + const runData = this.executionDataService.generateFailedExecutionFromError( data.executionMode, error, error.node, diff --git a/packages/cli/src/workflows/workflow-execution.service.ts b/packages/cli/src/workflows/workflow-execution.service.ts index c12babfc5d5ba..62cc6fb361e0c 100644 --- a/packages/cli/src/workflows/workflow-execution.service.ts +++ b/packages/cli/src/workflows/workflow-execution.service.ts @@ -21,7 +21,7 @@ import type { Project } from '@/databases/entities/project'; import type { User } from '@/databases/entities/user'; import { ExecutionRepository } from '@/databases/repositories/execution.repository'; import { WorkflowRepository } from '@/databases/repositories/workflow.repository'; -import { ExecutionService } from '@/executions/execution.service'; +import { ExecutionDataService } from '@/executions/execution-data.service'; import { SubworkflowPolicyChecker } from '@/executions/pre-execution-checks'; import type { CreateExecutionPayload, IWorkflowErrorData } from '@/interfaces'; import { NodeTypes } from '@/node-types'; @@ -42,7 +42,7 @@ export class WorkflowExecutionService { private readonly workflowRunner: WorkflowRunner, private readonly globalConfig: GlobalConfig, private readonly subworkflowPolicyChecker: SubworkflowPolicyChecker, - private readonly executionService: ExecutionService, + private readonly executionDataService: ExecutionDataService, ) {} async runWorkflow( @@ -274,7 +274,7 @@ export class WorkflowExecutionService { ); // Create a fake execution and save it to DB. - const fakeExecution = this.executionService.generateFailedExecutionFromError( + const fakeExecution = this.executionDataService.generateFailedExecutionFromError( 'error', errorWorkflowPermissionError, initialNode,