refactor(core): Move execution save settings into lifecycle function (no-changelog) (#7370)

Move the handling of execution save settings into a tested lifecycle
function as discussed with Omar

packages/cli/src/WorkflowExecuteAdditionalData.ts

@@ -63,6 +63,7 @@ import {
 	updateExistingExecution,
 } from './executionLifecycleHooks/shared/sharedHookFunctions';
 import { restoreBinaryDataId } from './executionLifecycleHooks/restoreBinaryDataId';
+import { toSaveSettings } from './executionLifecycleHooks/toSaveSettings';
 import { Logger } from './Logger';
 
 const ERROR_TRIGGER_TYPE = config.getEnv('nodes.errorTriggerType');
@@ -508,14 +509,9 @@ function hookFunctionsSave(parentProcessMode?: string): IWorkflowExecuteHooks {
 						}
 					}
 
-					const workflowSettings = this.workflowData.settings;
+					const saveSettings = toSaveSettings(this.workflowData.settings);
-					let saveManualExecutions = config.getEnv('executions.saveDataManualExecutions');
-					if (workflowSettings?.saveManualExecutions !== undefined) {
-						// Apply to workflow override
-						saveManualExecutions = workflowSettings.saveManualExecutions as boolean;
-					}
 
-					if (isManualMode && !saveManualExecutions && !fullRunData.waitTill) {
+					if (isManualMode && !saveSettings.manual && !fullRunData.waitTill) {
 						await Container.get(ExecutionRepository).hardDelete({
 							workflowId: this.workflowData.id as string,
 							executionId: this.executionId,
@@ -524,24 +520,12 @@ function hookFunctionsSave(parentProcessMode?: string): IWorkflowExecuteHooks {
 						return;
 					}
 
-					// Check config to know if execution should be saved or not
+					const executionStatus = determineFinalExecutionStatus(fullRunData);
-					let saveDataErrorExecution = config.getEnv('executions.saveDataOnError') as string;
+					const shouldNotSave =
-					let saveDataSuccessExecution = config.getEnv('executions.saveDataOnSuccess') as string;
+						(executionStatus === 'success' && !saveSettings.success) ||
-					if (this.workflowData.settings !== undefined) {
+						(executionStatus !== 'success' && !saveSettings.error);
-						saveDataErrorExecution =
-							(this.workflowData.settings.saveDataErrorExecution as string) ||
-							saveDataErrorExecution;
-						saveDataSuccessExecution =
-							(this.workflowData.settings.saveDataSuccessExecution as string) ||
-							saveDataSuccessExecution;
-					}
 
-					const workflowStatusFinal = determineFinalExecutionStatus(fullRunData);
+					if (shouldNotSave) {
-
-					if (
-						(workflowStatusFinal === 'success' && saveDataSuccessExecution === 'none') ||
-						(workflowStatusFinal !== 'success' && saveDataErrorExecution === 'none')
-					) {
 						if (!fullRunData.waitTill && !isManualMode) {
 							executeErrorWorkflow(
 								this.workflowData,
@@ -564,7 +548,7 @@ function hookFunctionsSave(parentProcessMode?: string): IWorkflowExecuteHooks {
 					const fullExecutionData = prepareExecutionDataForDbUpdate({
 						runData: fullRunData,
 						workflowData: this.workflowData,
-						workflowStatusFinal,
+						workflowStatusFinal: executionStatus,
 						retryOf: this.retryOf,
 					});
 
@@ -1135,23 +1119,14 @@ export function getWorkflowHooksWorkerMain(
 			fullRunData: IRun,
 			newStaticData: IDataObject,
 		): Promise<void> {
-			// Check config to know if execution should be saved or not
+			const executionStatus = determineFinalExecutionStatus(fullRunData);
-			let saveDataErrorExecution = config.getEnv('executions.saveDataOnError') as string;
+			const saveSettings = toSaveSettings(this.workflowData.settings);
-			let saveDataSuccessExecution = config.getEnv('executions.saveDataOnSuccess') as string;
-			if (this.workflowData.settings !== undefined) {
-				saveDataErrorExecution =
-					(this.workflowData.settings.saveDataErrorExecution as string) || saveDataErrorExecution;
-				saveDataSuccessExecution =
-					(this.workflowData.settings.saveDataSuccessExecution as string) ||
-					saveDataSuccessExecution;
-			}
 
-			const workflowStatusFinal = determineFinalExecutionStatus(fullRunData);
+			const shouldNotSave =
+				(executionStatus === 'success' && !saveSettings.success) ||
+				(executionStatus !== 'success' && !saveSettings.error);
 
-			if (
+			if (shouldNotSave) {
-				(workflowStatusFinal === 'success' && saveDataSuccessExecution === 'none') ||
-				(workflowStatusFinal !== 'success' && saveDataErrorExecution === 'none')
-			) {
 				await Container.get(ExecutionRepository).hardDelete({
 					workflowId: this.workflowData.id as string,
 					executionId: this.executionId,

packages/cli/src/config/schema.ts

@@ -119,7 +119,7 @@ export const schema = {
 				},
 				rejectUnauthorized: {
 					doc: 'If unauthorized SSL connections should be rejected',
-					format: 'Boolean',
+					format: Boolean,
 					default: true,
 					env: 'DB_POSTGRESDB_SSL_REJECT_UNAUTHORIZED',
 				},
@@ -215,7 +215,7 @@ export const schema = {
 		},
 		onboardingFlowDisabled: {
 			doc: 'Show onboarding flow in new workflow',
-			format: 'Boolean',
+			format: Boolean,
 			default: false,
 			env: 'N8N_ONBOARDING_FLOW_DISABLED',
 		},
@@ -288,7 +288,7 @@ export const schema = {
 		},
 		saveExecutionProgress: {
 			doc: 'Whether or not to save progress for each node executed',
-			format: 'Boolean',
+			format: Boolean,
 			default: false,
 			env: 'EXECUTIONS_DATA_SAVE_ON_PROGRESS',
 		},
@@ -300,7 +300,7 @@ export const schema = {
 		// in the editor.
 		saveDataManualExecutions: {
 			doc: 'Save data of executions when started manually via editor',
-			format: 'Boolean',
+			format: Boolean,
 			default: true,
 			env: 'EXECUTIONS_DATA_SAVE_MANUAL_EXECUTIONS',
 		},
@@ -312,7 +312,7 @@ export const schema = {
 		// a future version.
 		pruneData: {
 			doc: 'Delete data of past executions on a rolling basis',
-			format: 'Boolean',
+			format: Boolean,
 			default: true,
 			env: 'EXECUTIONS_DATA_PRUNE',
 		},
@@ -358,7 +358,7 @@ export const schema = {
 		health: {
 			active: {
 				doc: 'If health checks should be enabled',
-				format: 'Boolean',
+				format: Boolean,
 				default: false,
 				env: 'QUEUE_HEALTH_CHECK_ACTIVE',
 			},
@@ -420,7 +420,7 @@ export const schema = {
 					env: 'QUEUE_BULL_REDIS_CLUSTER_NODES',
 				},
 				tls: {
-					format: 'Boolean',
+					format: Boolean,
 					default: false,
 					env: 'QUEUE_BULL_REDIS_TLS',
 					doc: 'Enable TLS on Redis connections. Default: false',
@@ -558,7 +558,7 @@ export const schema = {
 		},
 		metrics: {
 			enable: {
-				format: 'Boolean',
+				format: Boolean,
 				default: false,
 				env: 'N8N_METRICS',
 				doc: 'Enable /metrics endpoint. Default: false',

packages/cli/src/executionLifecycleHooks/toSaveSettings.ts

@@ -0,0 +1,20 @@
+import config from '@/config';
+import type { IWorkflowSettings } from 'n8n-workflow';
+
+const DEFAULTS = {
+	ERROR: config.getEnv('executions.saveDataOnError'),
+	SUCCESS: config.getEnv('executions.saveDataOnSuccess'),
+	MANUAL: config.getEnv('executions.saveDataManualExecutions'),
+};
+
+/**
+ * Return whether a workflow execution is configured to be saved or not,
+ * for error executions, success executions, and manual executions.
+ */
+export function toSaveSettings(workflowSettings: IWorkflowSettings = {}) {
+	return {
+		error: workflowSettings.saveDataErrorExecution !== 'none' ?? DEFAULTS.ERROR !== 'none',
+		success: workflowSettings.saveDataSuccessExecution !== 'none' ?? DEFAULTS.SUCCESS !== 'none',
+		manual: workflowSettings?.saveManualExecutions ?? DEFAULTS.MANUAL,
+	};
+}

packages/cli/test/unit/execution.lifecycle.test.ts

@@ -1,6 +1,7 @@
 import { restoreBinaryDataId } from '@/executionLifecycleHooks/restoreBinaryDataId';
 import { BinaryDataService } from 'n8n-core';
 import { mockInstance } from '../integration/shared/utils/mocking';
+import { toSaveSettings } from '@/executionLifecycleHooks/toSaveSettings';
 import type { IRun } from 'n8n-workflow';
 import config from '@/config';
 
@@ -129,4 +130,58 @@ for (const mode of ['filesystem-v2', 's3'] as const) {
 			});
 		});
 	});
+
+	it('should do nothing on itemless case', async () => {
+		const executionId = '999';
+
+		const promise = restoreBinaryDataId(toIRun(), executionId);
+
+		await expect(promise).resolves.not.toThrow();
+
+		expect(binaryDataService.rename).not.toHaveBeenCalled();
+	});
 }
+
+describe('toSaveSettings()', () => {
+	afterEach(() => {
+		jest.restoreAllMocks();
+	});
+
+	it('should set `error` based on workflow settings', () => {
+		const saveSettings = toSaveSettings({ saveDataErrorExecution: 'all' });
+
+		expect(saveSettings.error).toBe(true);
+
+		const _saveSettings = toSaveSettings({ saveDataErrorExecution: 'none' });
+
+		expect(_saveSettings.error).toBe(false);
+	});
+
+	it('should set `success` based on workflow settings', () => {
+		const saveSettings = toSaveSettings({ saveDataSuccessExecution: 'all' });
+
+		expect(saveSettings.success).toBe(true);
+
+		const _saveSettings = toSaveSettings({ saveDataSuccessExecution: 'none' });
+
+		expect(_saveSettings.success).toBe(false);
+	});
+
+	it('should set `manual` based on workflow settings', () => {
+		const saveSettings = toSaveSettings({ saveManualExecutions: true });
+
+		expect(saveSettings.manual).toBe(true);
+
+		const _saveSettings = toSaveSettings({ saveManualExecutions: false });
+
+		expect(_saveSettings.manual).toBe(false);
+	});
+
+	it('should return defaults if no workflow settings', async () => {
+		const saveSettings = toSaveSettings();
+
+		expect(saveSettings.error).toBe(true);
+		expect(saveSettings.success).toBe(true);
+		expect(saveSettings.manual).toBe(true);
+	});
+});