fix(core): Prevent workflow history saving error from happening (#7812)

When performing actions such as renaming a workflow or updating its settings, n8n errors with "Failed to save workflow version" in the console although the saving process was successful. We are now correctly checking whether `nodes` and `connections` exist and only then save a snapshot. Github issue / Community forum post (link here to close automatically):
2025-12-18 02:21:13 +00:00 · 2023-12-13 11:41:06 +00:00
parent b00b9057a4
commit e5581ce802
6 changed files with 290 additions and 158 deletions
--- a/packages/cli/test/unit/WorkflowHelpers.test.ts
+++ b/packages/cli/test/unit/WorkflowHelpers.test.ts
@@ -193,7 +193,7 @@ function generateCredentialEntity(credentialId: string) {
 	return credentialEntity;
 }

-function getWorkflow(options?: {
+export function getWorkflow(options?: {
 	addNodeWithoutCreds?: boolean;
 	addNodeWithOneCred?: boolean;
 	addNodeWithTwoCreds?: boolean;
--- a/packages/cli/test/unit/services/ownership.service.test.ts
+++ b/packages/cli/test/unit/services/ownership.service.test.ts
@@ -4,51 +4,18 @@ import { Role } from '@db/entities/Role';
 import { SharedWorkflow } from '@db/entities/SharedWorkflow';
 import { User } from '@db/entities/User';
 import { RoleService } from '@/services/role.service';
-import { CredentialsEntity } from '@db/entities/CredentialsEntity';
 import type { SharedCredentials } from '@db/entities/SharedCredentials';
 import { mockInstance } from '../../shared/mocking';
-import {
-	randomCredentialPayload,
-	randomEmail,
-	randomInteger,
-	randomName,
-} from '../../integration/shared/random';
 import { WorkflowEntity } from '@/databases/entities/WorkflowEntity';
 import { UserRepository } from '@/databases/repositories/user.repository';
 import { mock } from 'jest-mock-extended';
-
-const wfOwnerRole = () =>
-	Object.assign(new Role(), {
-		scope: 'workflow',
-		name: 'owner',
-		id: randomInteger(),
-	});
-
-const mockCredRole = (name: 'owner' | 'editor'): Role =>
-	Object.assign(new Role(), {
-		scope: 'credentials',
-		name,
-		id: randomInteger(),
-	});
-
-const mockInstanceOwnerRole = () =>
-	Object.assign(new Role(), {
-		scope: 'global',
-		name: 'owner',
-		id: randomInteger(),
-	});
-
-const mockCredential = (): CredentialsEntity =>
-	Object.assign(new CredentialsEntity(), randomCredentialPayload());
-
-const mockUser = (attributes?: Partial<User>): User =>
-	Object.assign(new User(), {
-		id: randomInteger(),
-		email: randomEmail(),
-		firstName: randomName(),
-		lastName: randomName(),
-		...attributes,
-	});
+import {
+	mockCredRole,
+	mockCredential,
+	mockUser,
+	mockInstanceOwnerRole,
+	wfOwnerRole,
+} from '../shared/mockObjects';

 describe('OwnershipService', () => {
 	const roleService = mockInstance(RoleService);
@@ -66,132 +33,149 @@ describe('OwnershipService', () => {
 		jest.clearAllMocks();
 	});

-	describe('getWorkflowOwner()', () => {
-		test('should retrieve a workflow owner', async () => {
-			roleService.findWorkflowOwnerRole.mockResolvedValueOnce(wfOwnerRole());
+	describe('OwnershipService', () => {
+		const roleService = mockInstance(RoleService);
+		const userRepository = mockInstance(UserRepository);
+		const sharedWorkflowRepository = mockInstance(SharedWorkflowRepository);

-			const mockOwner = new User();
-			const mockNonOwner = new User();
+		const ownershipService = new OwnershipService(
+			mock(),
+			userRepository,
+			roleService,
+			sharedWorkflowRepository,
+		);

-			const sharedWorkflow = Object.assign(new SharedWorkflow(), {
-				role: new Role(),
-				user: mockOwner,
+		beforeEach(() => {
+			jest.clearAllMocks();
+		});
+
+		describe('getWorkflowOwner()', () => {
+			test('should retrieve a workflow owner', async () => {
+				roleService.findWorkflowOwnerRole.mockResolvedValueOnce(wfOwnerRole());
+
+				const mockOwner = new User();
+				const mockNonOwner = new User();
+
+				const sharedWorkflow = Object.assign(new SharedWorkflow(), {
+					role: new Role(),
+					user: mockOwner,
+				});
+
+				sharedWorkflowRepository.findOneOrFail.mockResolvedValueOnce(sharedWorkflow);
+
+				const returnedOwner = await ownershipService.getWorkflowOwnerCached('some-workflow-id');
+
+				expect(returnedOwner).toBe(mockOwner);
+				expect(returnedOwner).not.toBe(mockNonOwner);
 			});

-			sharedWorkflowRepository.findOneOrFail.mockResolvedValueOnce(sharedWorkflow);
+			test('should throw if no workflow owner role found', async () => {
+				roleService.findWorkflowOwnerRole.mockRejectedValueOnce(new Error());

-			const returnedOwner = await ownershipService.getWorkflowOwnerCached('some-workflow-id');
-
-			expect(returnedOwner).toBe(mockOwner);
-			expect(returnedOwner).not.toBe(mockNonOwner);
-		});
-
-		test('should throw if no workflow owner role found', async () => {
-			roleService.findWorkflowOwnerRole.mockRejectedValueOnce(new Error());
-
-			await expect(ownershipService.getWorkflowOwnerCached('some-workflow-id')).rejects.toThrow();
-		});
-
-		test('should throw if no workflow owner found', async () => {
-			roleService.findWorkflowOwnerRole.mockResolvedValueOnce(wfOwnerRole());
-
-			sharedWorkflowRepository.findOneOrFail.mockRejectedValue(new Error());
-
-			await expect(ownershipService.getWorkflowOwnerCached('some-workflow-id')).rejects.toThrow();
-		});
-	});
-
-	describe('addOwnedByAndSharedWith()', () => {
-		test('should add `ownedBy` and `sharedWith` to credential', async () => {
-			const owner = mockUser();
-			const editor = mockUser();
-
-			const credential = mockCredential();
-
-			credential.shared = [
-				{ role: mockCredRole('owner'), user: owner },
-				{ role: mockCredRole('editor'), user: editor },
-			] as SharedCredentials[];
-
-			const { ownedBy, sharedWith } = ownershipService.addOwnedByAndSharedWith(credential);
-
-			expect(ownedBy).toStrictEqual({
-				id: owner.id,
-				email: owner.email,
-				firstName: owner.firstName,
-				lastName: owner.lastName,
+				await expect(ownershipService.getWorkflowOwnerCached('some-workflow-id')).rejects.toThrow();
 			});

-			expect(sharedWith).toStrictEqual([
-				{
-					id: editor.id,
-					email: editor.email,
-					firstName: editor.firstName,
-					lastName: editor.lastName,
-				},
-			]);
+			test('should throw if no workflow owner found', async () => {
+				roleService.findWorkflowOwnerRole.mockResolvedValueOnce(wfOwnerRole());
+
+				sharedWorkflowRepository.findOneOrFail.mockRejectedValue(new Error());
+
+				await expect(ownershipService.getWorkflowOwnerCached('some-workflow-id')).rejects.toThrow();
+			});
 		});

-		test('should add `ownedBy` and `sharedWith` to workflow', async () => {
-			const owner = mockUser();
-			const editor = mockUser();
+		describe('addOwnedByAndSharedWith()', () => {
+			test('should add `ownedBy` and `sharedWith` to credential', async () => {
+				const owner = mockUser();
+				const editor = mockUser();

-			const workflow = new WorkflowEntity();
+				const credential = mockCredential();

-			workflow.shared = [
-				{ role: mockCredRole('owner'), user: owner },
-				{ role: mockCredRole('editor'), user: editor },
-			] as SharedWorkflow[];
+				credential.shared = [
+					{ role: mockCredRole('owner'), user: owner },
+					{ role: mockCredRole('editor'), user: editor },
+				] as SharedCredentials[];

-			const { ownedBy, sharedWith } = ownershipService.addOwnedByAndSharedWith(workflow);
+				const { ownedBy, sharedWith } = ownershipService.addOwnedByAndSharedWith(credential);

-			expect(ownedBy).toStrictEqual({
-				id: owner.id,
-				email: owner.email,
-				firstName: owner.firstName,
-				lastName: owner.lastName,
+				expect(ownedBy).toStrictEqual({
+					id: owner.id,
+					email: owner.email,
+					firstName: owner.firstName,
+					lastName: owner.lastName,
+				});
+
+				expect(sharedWith).toStrictEqual([
+					{
+						id: editor.id,
+						email: editor.email,
+						firstName: editor.firstName,
+						lastName: editor.lastName,
+					},
+				]);
 			});

-			expect(sharedWith).toStrictEqual([
-				{
-					id: editor.id,
-					email: editor.email,
-					firstName: editor.firstName,
-					lastName: editor.lastName,
-				},
-			]);
-		});
+			test('should add `ownedBy` and `sharedWith` to workflow', async () => {
+				const owner = mockUser();
+				const editor = mockUser();

-		test('should produce an empty sharedWith if no sharee', async () => {
-			const owner = mockUser();
+				const workflow = new WorkflowEntity();

-			const credential = mockCredential();
+				workflow.shared = [
+					{ role: mockCredRole('owner'), user: owner },
+					{ role: mockCredRole('editor'), user: editor },
+				] as SharedWorkflow[];

-			credential.shared = [{ role: mockCredRole('owner'), user: owner }] as SharedCredentials[];
+				const { ownedBy, sharedWith } = ownershipService.addOwnedByAndSharedWith(workflow);

-			const { ownedBy, sharedWith } = ownershipService.addOwnedByAndSharedWith(credential);
+				expect(ownedBy).toStrictEqual({
+					id: owner.id,
+					email: owner.email,
+					firstName: owner.firstName,
+					lastName: owner.lastName,
+				});

-			expect(ownedBy).toStrictEqual({
-				id: owner.id,
-				email: owner.email,
-				firstName: owner.firstName,
-				lastName: owner.lastName,
+				expect(sharedWith).toStrictEqual([
+					{
+						id: editor.id,
+						email: editor.email,
+						firstName: editor.firstName,
+						lastName: editor.lastName,
+					},
+				]);
 			});

-			expect(sharedWith).toHaveLength(0);
+			test('should produce an empty sharedWith if no sharee', async () => {
+				const owner = mockUser();
+
+				const credential = mockCredential();
+
+				credential.shared = [{ role: mockCredRole('owner'), user: owner }] as SharedCredentials[];
+
+				const { ownedBy, sharedWith } = ownershipService.addOwnedByAndSharedWith(credential);
+
+				expect(ownedBy).toStrictEqual({
+					id: owner.id,
+					email: owner.email,
+					firstName: owner.firstName,
+					lastName: owner.lastName,
+				});
+
+				expect(sharedWith).toHaveLength(0);
+			});
 		});
-	});

-	describe('getInstanceOwner()', () => {
-		test('should find owner using global owner role ID', async () => {
-			const instanceOwnerRole = mockInstanceOwnerRole();
-			roleService.findGlobalOwnerRole.mockResolvedValue(instanceOwnerRole);
+		describe('getInstanceOwner()', () => {
+			test('should find owner using global owner role ID', async () => {
+				const instanceOwnerRole = mockInstanceOwnerRole();
+				roleService.findGlobalOwnerRole.mockResolvedValue(instanceOwnerRole);

-			await ownershipService.getInstanceOwner();
+				await ownershipService.getInstanceOwner();

-			expect(userRepository.findOneOrFail).toHaveBeenCalledWith({
-				where: { globalRoleId: instanceOwnerRole.id },
-				relations: ['globalRole'],
+				expect(userRepository.findOneOrFail).toHaveBeenCalledWith({
+					where: { globalRoleId: instanceOwnerRole.id },
+					relations: ['globalRole'],
+				});
 			});
 		});
 	});
--- a/packages/cli/test/unit/services/workflowHistory.service.ee.test.ts
+++ b/packages/cli/test/unit/services/workflowHistory.service.ee.test.ts
@@ -0,0 +1,114 @@
+import { User } from '@db/entities/User';
+import { WorkflowHistoryRepository } from '@db/repositories/workflowHistory.repository';
+import { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.repository';
+import { WorkflowHistoryService } from '@/workflows/workflowHistory/workflowHistory.service.ee';
+import { mockInstance } from '../../shared/mocking';
+import { Logger } from '@/Logger';
+import { getWorkflow } from '../WorkflowHelpers.test';
+import { mockClear } from 'jest-mock-extended';
+
+const workflowHistoryRepository = mockInstance(WorkflowHistoryRepository);
+const logger = mockInstance(Logger);
+const sharedWorkflowRepository = mockInstance(SharedWorkflowRepository);
+const workflowHistoryService = new WorkflowHistoryService(
+	logger,
+	workflowHistoryRepository,
+	sharedWorkflowRepository,
+);
+const testUser = Object.assign(new User(), {
+	id: '1234',
+	password: 'passwordHash',
+	mfaEnabled: false,
+	firstName: 'John',
+	lastName: 'Doe',
+});
+let isWorkflowHistoryEnabled = true;
+
+jest.mock('@/workflows/workflowHistory/workflowHistoryHelper.ee', () => {
+	return {
+		isWorkflowHistoryEnabled: jest.fn(() => isWorkflowHistoryEnabled),
+	};
+});
+
+describe('WorkflowHistoryService', () => {
+	beforeEach(() => {
+		mockClear(workflowHistoryRepository.insert);
+	});
+
+	describe('saveVersion', () => {
+		it('should save a new version when workflow history is enabled and nodes and connections are present', async () => {
+			// Arrange
+			isWorkflowHistoryEnabled = true;
+			const workflow = getWorkflow({ addNodeWithoutCreds: true });
+			const workflowId = '123';
+			workflow.connections = {};
+			workflow.id = workflowId;
+			workflow.versionId = '456';
+
+			// Act
+			await workflowHistoryService.saveVersion(testUser, workflow, workflowId);
+
+			// Assert
+			expect(workflowHistoryRepository.insert).toHaveBeenCalledWith({
+				authors: 'John Doe',
+				connections: {},
+				nodes: workflow.nodes,
+				versionId: workflow.versionId,
+				workflowId,
+			});
+		});
+
+		it('should not save a new version when workflow history is disabled', async () => {
+			// Arrange
+			isWorkflowHistoryEnabled = false;
+			const workflow = getWorkflow({ addNodeWithoutCreds: true });
+			const workflowId = '123';
+			workflow.connections = {};
+			workflow.id = workflowId;
+			workflow.versionId = '456';
+
+			// Act
+			await workflowHistoryService.saveVersion(testUser, workflow, workflowId);
+
+			// Assert
+			expect(workflowHistoryRepository.insert).not.toHaveBeenCalled();
+		});
+
+		it('should not save a new version when nodes or connections are missing', async () => {
+			// Arrange
+			isWorkflowHistoryEnabled = true;
+			const workflow = getWorkflow({ addNodeWithoutCreds: true });
+			const workflowId = '123';
+			workflow.id = workflowId;
+			workflow.versionId = '456';
+			// Nodes are set but connections is empty
+
+			// Act
+			await workflowHistoryService.saveVersion(testUser, workflow, workflowId);
+
+			// Assert
+			expect(workflowHistoryRepository.insert).not.toHaveBeenCalled();
+		});
+
+		it('should log an error when failed to save workflow history version', async () => {
+			// Arrange
+			isWorkflowHistoryEnabled = true;
+			const workflow = getWorkflow({ addNodeWithoutCreds: true });
+			const workflowId = '123';
+			workflow.connections = {};
+			workflow.id = workflowId;
+			workflow.versionId = '456';
+			workflowHistoryRepository.insert.mockRejectedValueOnce(new Error('Test error'));
+
+			// Act
+			await workflowHistoryService.saveVersion(testUser, workflow, workflowId);
+
+			// Assert
+			expect(workflowHistoryRepository.insert).toHaveBeenCalled();
+			expect(logger.error).toHaveBeenCalledWith(
+				'Failed to save workflow history version for workflow 123',
+				expect.any(Error),
+			);
+		});
+	});
+});
--- a/packages/cli/test/unit/shared/mockObjects.ts
+++ b/packages/cli/test/unit/shared/mockObjects.ts
@@ -0,0 +1,42 @@
+import { User } from '@db/entities/User';
+import { Role } from '@db/entities/Role';
+import { CredentialsEntity } from '@db/entities/CredentialsEntity';
+
+import {
+	randomCredentialPayload,
+	randomEmail,
+	randomInteger,
+	randomName,
+} from '../../integration/shared/random';
+
+export const wfOwnerRole = () =>
+	Object.assign(new Role(), {
+		scope: 'workflow',
+		name: 'owner',
+		id: randomInteger(),
+	});
+
+export const mockCredRole = (name: 'owner' | 'editor'): Role =>
+	Object.assign(new Role(), {
+		scope: 'credentials',
+		name,
+		id: randomInteger(),
+	});
+
+export const mockCredential = (): CredentialsEntity =>
+	Object.assign(new CredentialsEntity(), randomCredentialPayload());
+
+export const mockUser = (): User =>
+	Object.assign(new User(), {
+		id: randomInteger(),
+		email: randomEmail(),
+		firstName: randomName(),
+		lastName: randomName(),
+	});
+
+export const mockInstanceOwnerRole = () =>
+	Object.assign(new Role(), {
+		scope: 'global',
+		name: 'owner',
+		id: randomInteger(),
+	});