From 4471c0f066f0416e8eccd9d367755f649c6afb72 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: Thu, 30 Jan 2025 14:05:53 +0100 Subject: [PATCH] calculate saving settings only once for an execution --- .../execution-lifecycle-hooks.test.ts | 17 +++++---- .../__tests__/save-execution-progress.test.ts | 35 ++++--------------- .../execution-lifecycle-hooks.ts | 29 +++++++-------- .../save-execution-progress.ts | 11 +++--- .../execution-lifecycle/to-save-settings.ts | 9 ++++- 5 files changed, 43 insertions(+), 58 deletions(-) diff --git a/packages/cli/src/execution-lifecycle/__tests__/execution-lifecycle-hooks.test.ts b/packages/cli/src/execution-lifecycle/__tests__/execution-lifecycle-hooks.test.ts index fc04fbaa9e..a65463d42a 100644 --- a/packages/cli/src/execution-lifecycle/__tests__/execution-lifecycle-hooks.test.ts +++ b/packages/cli/src/execution-lifecycle/__tests__/execution-lifecycle-hooks.test.ts @@ -185,16 +185,11 @@ describe('Execution Lifecycle Hooks', () => { }; describe('getWorkflowHooksMain', () => { + const createHooks = () => + getWorkflowHooksMain({ executionMode, workflowData, pushRef, retryOf }, executionId); + beforeEach(() => { - hooks = getWorkflowHooksMain( - { - executionMode, - workflowData, - pushRef, - retryOf, - }, - executionId, - ); + hooks = createHooks(); }); workflowEventTests(); @@ -245,6 +240,7 @@ describe('Execution Lifecycle Hooks', () => { it('should save execution progress when enabled', async () => { workflowData.settings = { saveExecutionProgress: true }; + hooks = createHooks(); await hooks.executeHookFunctions('nodeExecuteAfter', [ nodeName, @@ -260,6 +256,7 @@ describe('Execution Lifecycle Hooks', () => { it('should not save execution progress when disabled', async () => { workflowData.settings = { saveExecutionProgress: false }; + hooks = createHooks(); await hooks.executeHookFunctions('nodeExecuteAfter', [ nodeName, @@ -391,6 +388,7 @@ describe('Execution Lifecycle Hooks', () => { it('should soft delete manual executions when manual saving is disabled', async () => { hooks.workflowData.settings = { saveManualExecutions: false }; + hooks = createHooks(); await hooks.executeHookFunctions('workflowExecuteAfter', [successfulRun, {}]); @@ -399,6 +397,7 @@ describe('Execution Lifecycle Hooks', () => { it('should not soft delete manual executions with waitTill', async () => { hooks.workflowData.settings = { saveManualExecutions: false }; + hooks = createHooks(); await hooks.executeHookFunctions('workflowExecuteAfter', [waitingRun, {}]); diff --git a/packages/cli/src/execution-lifecycle/__tests__/save-execution-progress.test.ts b/packages/cli/src/execution-lifecycle/__tests__/save-execution-progress.test.ts index 863006d9e7..75d52f3ac4 100644 --- a/packages/cli/src/execution-lifecycle/__tests__/save-execution-progress.test.ts +++ b/packages/cli/src/execution-lifecycle/__tests__/save-execution-progress.test.ts @@ -1,13 +1,12 @@ import { ErrorReporter } from 'n8n-core'; import { Logger } from 'n8n-core'; -import type { IRunExecutionData, ITaskData, IWorkflowBase } from 'n8n-workflow'; +import type { IRunExecutionData, ITaskData } from 'n8n-workflow'; import { ExecutionRepository } from '@/databases/repositories/execution.repository'; import type { IExecutionResponse } from '@/interfaces'; import { mockInstance } from '@test/mocking'; import { saveExecutionProgress } from '../save-execution-progress'; -import * as fnModule from '../to-save-settings'; mockInstance(Logger); const errorReporter = mockInstance(ErrorReporter); @@ -17,8 +16,8 @@ afterEach(() => { jest.clearAllMocks(); }); -const commonArgs: [IWorkflowBase, string, string, ITaskData, IRunExecutionData, string] = [ - {} as IWorkflowBase, +const commonArgs: [string, string, string, ITaskData, IRunExecutionData, string] = [ + 'some-workflow-id', 'some-execution-id', 'My Node', {} as ITaskData, @@ -29,40 +28,25 @@ const commonArgs: [IWorkflowBase, string, string, ITaskData, IRunExecutionData, const commonSettings = { error: true, success: true, manual: true }; test('should ignore if save settings say so', async () => { - jest.spyOn(fnModule, 'toSaveSettings').mockReturnValue({ - ...commonSettings, - progress: false, - }); - - await saveExecutionProgress(...commonArgs); + await saveExecutionProgress({ ...commonSettings, progress: false }, ...commonArgs); expect(executionRepository.updateExistingExecution).not.toHaveBeenCalled(); }); test('should ignore on leftover async call', async () => { - jest.spyOn(fnModule, 'toSaveSettings').mockReturnValue({ - ...commonSettings, - progress: true, - }); - executionRepository.findSingleExecution.mockResolvedValue({ finished: true, } as IExecutionResponse); - await saveExecutionProgress(...commonArgs); + await saveExecutionProgress({ ...commonSettings, progress: true }, ...commonArgs); expect(executionRepository.updateExistingExecution).not.toHaveBeenCalled(); }); test('should update execution when saving progress is enabled', async () => { - jest.spyOn(fnModule, 'toSaveSettings').mockReturnValue({ - ...commonSettings, - progress: true, - }); - executionRepository.findSingleExecution.mockResolvedValue({} as IExecutionResponse); - await saveExecutionProgress(...commonArgs); + await saveExecutionProgress({ ...commonSettings, progress: true }, ...commonArgs); expect(executionRepository.updateExistingExecution).toHaveBeenCalledWith('some-execution-id', { data: { @@ -82,18 +66,13 @@ test('should update execution when saving progress is enabled', async () => { }); test('should report error on failure', async () => { - jest.spyOn(fnModule, 'toSaveSettings').mockReturnValue({ - ...commonSettings, - progress: true, - }); - const error = new Error('Something went wrong'); executionRepository.findSingleExecution.mockImplementation(() => { throw error; }); - await saveExecutionProgress(...commonArgs); + await saveExecutionProgress({ ...commonSettings, progress: true }, ...commonArgs); expect(executionRepository.updateExistingExecution).not.toHaveBeenCalled(); expect(errorReporter.error).toHaveBeenCalledWith(error); diff --git a/packages/cli/src/execution-lifecycle/execution-lifecycle-hooks.ts b/packages/cli/src/execution-lifecycle/execution-lifecycle-hooks.ts index 341146cf50..5350591fc6 100644 --- a/packages/cli/src/execution-lifecycle/execution-lifecycle-hooks.ts +++ b/packages/cli/src/execution-lifecycle/execution-lifecycle-hooks.ts @@ -32,7 +32,7 @@ import { prepareExecutionDataForDbUpdate, updateExistingExecution, } from './shared/shared-hook-functions'; -import { toSaveSettings } from './to-save-settings'; +import { type ExecutionSavingSettings, toSaveSettings } from './to-save-settings'; function mergeHookFunctions(...hookFunctions: IWorkflowExecuteHooks[]): IWorkflowExecuteHooks { const result: IWorkflowExecuteHooks = { @@ -212,7 +212,7 @@ function hookFunctionsExternalHooks(): IWorkflowExecuteHooks { }; } -function hookFunctionsSaveProgress(): IWorkflowExecuteHooks { +function hookFunctionsSaveProgress(saveSettings: ExecutionSavingSettings): IWorkflowExecuteHooks { return { nodeExecuteAfter: [ async function ( @@ -222,7 +222,8 @@ function hookFunctionsSaveProgress(): IWorkflowExecuteHooks { executionData: IRunExecutionData, ): Promise { await saveExecutionProgress( - this.workflowData, + saveSettings, + this.workflowData.id, this.executionId, nodeName, data, @@ -248,7 +249,7 @@ function hookFunctionsFinalizeExecutionStatus(): IWorkflowExecuteHooks { /** * Returns hook functions to save workflow execution and call error workflow */ -function hookFunctionsSave(): IWorkflowExecuteHooks { +function hookFunctionsSave(saveSettings: ExecutionSavingSettings): IWorkflowExecuteHooks { const logger = Container.get(Logger); const errorReporter = Container.get(ErrorReporter); const executionRepository = Container.get(ExecutionRepository); @@ -288,8 +289,6 @@ function hookFunctionsSave(): IWorkflowExecuteHooks { } } - const saveSettings = toSaveSettings(this.workflowData.settings); - if (isManualMode && !saveSettings.manual && !fullRunData.waitTill) { /** * When manual executions are not being saved, we only soft-delete @@ -472,12 +471,13 @@ export function getWorkflowHooksIntegrated( workflowData: IWorkflowBase, userId?: string, ): WorkflowHooks { + const saveSettings = toSaveSettings(workflowData.settings); const hookFunctions = mergeHookFunctions( hookFunctionsWorkflowEvents(userId), hookFunctionsNodeEvents(), hookFunctionsFinalizeExecutionStatus(), - hookFunctionsSave(), - hookFunctionsSaveProgress(), + hookFunctionsSave(saveSettings), + hookFunctionsSaveProgress(saveSettings), hookFunctionsExternalHooks(), ); return new WorkflowHooks(hookFunctions, mode, executionId, workflowData); @@ -492,11 +492,12 @@ export function getWorkflowHooksWorkerExecuter( workflowData: IWorkflowBase, optionalParameters: IWorkflowHooksOptionalParameters = {}, ): WorkflowHooks { + const saveSettings = toSaveSettings(workflowData.settings); const toMerge = [ hookFunctionsNodeEvents(), hookFunctionsFinalizeExecutionStatus(), hookFunctionsSaveWorker(), - hookFunctionsSaveProgress(), + hookFunctionsSaveProgress(saveSettings), hookFunctionsExternalHooks(), ]; @@ -517,10 +518,11 @@ export function getWorkflowHooksWorkerMain( workflowData: IWorkflowBase, optionalParameters: IWorkflowHooksOptionalParameters = {}, ): WorkflowHooks { + const saveSettings = toSaveSettings(workflowData.settings); const executionRepository = Container.get(ExecutionRepository); const hookFunctions = mergeHookFunctions( hookFunctionsWorkflowEvents(), - hookFunctionsSaveProgress(), + hookFunctionsSaveProgress(saveSettings), hookFunctionsExternalHooks(), hookFunctionsFinalizeExecutionStatus(), { @@ -529,8 +531,6 @@ export function getWorkflowHooksWorkerMain( // Don't delete executions before they are finished if (!fullRunData.finished) return; - const saveSettings = toSaveSettings(this.workflowData.settings); - const isManualMode = this.mode === 'manual'; if (isManualMode && !saveSettings.manual && !fullRunData.waitTill) { @@ -579,13 +579,14 @@ export function getWorkflowHooksMain( data: IWorkflowExecutionDataProcess, executionId: string, ): WorkflowHooks { + const saveSettings = toSaveSettings(data.workflowData.settings); const hookFunctions = mergeHookFunctions( hookFunctionsWorkflowEvents(), hookFunctionsNodeEvents(), hookFunctionsFinalizeExecutionStatus(), - hookFunctionsSave(), + hookFunctionsSave(saveSettings), hookFunctionsPush(), - hookFunctionsSaveProgress(), + hookFunctionsSaveProgress(saveSettings), hookFunctionsExternalHooks(), ); return new WorkflowHooks(hookFunctions, data.executionMode, executionId, data.workflowData, { diff --git a/packages/cli/src/execution-lifecycle/save-execution-progress.ts b/packages/cli/src/execution-lifecycle/save-execution-progress.ts index 8ca33c7095..fe12cae262 100644 --- a/packages/cli/src/execution-lifecycle/save-execution-progress.ts +++ b/packages/cli/src/execution-lifecycle/save-execution-progress.ts @@ -1,21 +1,20 @@ import { Container } from '@n8n/di'; import { ErrorReporter, Logger } from 'n8n-core'; -import type { IRunExecutionData, ITaskData, IWorkflowBase } from 'n8n-workflow'; +import type { IRunExecutionData, ITaskData } from 'n8n-workflow'; import { ExecutionRepository } from '@/databases/repositories/execution.repository'; -import { toSaveSettings } from './to-save-settings'; +import { type ExecutionSavingSettings } from './to-save-settings'; export async function saveExecutionProgress( - workflowData: IWorkflowBase, + saveSettings: ExecutionSavingSettings, + workflowId: string, executionId: string, nodeName: string, data: ITaskData, executionData: IRunExecutionData, pushRef?: string, ) { - const saveSettings = toSaveSettings(workflowData.settings); - if (!saveSettings.progress) return; const logger = Container.get(Logger); @@ -97,7 +96,7 @@ export async function saveExecutionProgress( ...error, executionId, pushRef, - workflowId: workflowData.id, + workflowId, }, ); } diff --git a/packages/cli/src/execution-lifecycle/to-save-settings.ts b/packages/cli/src/execution-lifecycle/to-save-settings.ts index a7af8f3ddc..9d86311b90 100644 --- a/packages/cli/src/execution-lifecycle/to-save-settings.ts +++ b/packages/cli/src/execution-lifecycle/to-save-settings.ts @@ -2,6 +2,13 @@ import type { IWorkflowSettings } from 'n8n-workflow'; import config from '@/config'; +export type ExecutionSavingSettings = { + error: boolean | 'all' | 'none'; + success: boolean | 'all' | 'none'; + manual: boolean; + progress: boolean; +}; + /** * Return whether a workflow execution is configured to be saved or not: * @@ -10,7 +17,7 @@ import config from '@/config'; * - `manual`: Whether to save successful or failed manual executions. * - `progress`: Whether to save execution progress, i.e. after each node's execution. */ -export function toSaveSettings(workflowSettings: IWorkflowSettings = {}) { +export function toSaveSettings(workflowSettings: IWorkflowSettings = {}): ExecutionSavingSettings { const DEFAULTS = { ERROR: config.getEnv('executions.saveDataOnError'), SUCCESS: config.getEnv('executions.saveDataOnSuccess'),