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):

packages/cli/src/workflows/workflowHistory/workflowHistory.service.ee.ts

@@ -66,7 +66,10 @@ export class WorkflowHistoryService {
 	}
 
 	async saveVersion(user: User, workflow: WorkflowEntity, workflowId: string) {
-		if (isWorkflowHistoryEnabled()) {
+		// On some update scenarios, `nodes` and `connections` are missing, such as when
+		// changing workflow settings or renaming. In these cases, we don't want to save
+		// a new version
+		if (isWorkflowHistoryEnabled() && workflow.nodes && workflow.connections) {
 			try {
 				await this.workflowHistoryRepository.insert({
 					authors: user.firstName + ' ' + user.lastName,

packages/cli/src/workflows/workflows.services.ts

@@ -4,6 +4,7 @@ import { NodeApiError, ErrorReporterProxy as ErrorReporter, Workflow } from 'n8n
 import type { FindManyOptions, FindOptionsSelect, FindOptionsWhere, UpdateResult } from 'typeorm';
 import { In, Like } from 'typeorm';
 import pick from 'lodash/pick';
+import omit from 'lodash/omit';
 import { v4 as uuid } from 'uuid';
 import { ActiveWorkflowRunner } from '@/ActiveWorkflowRunner';
 import * as WorkflowHelpers from '@/WorkflowHelpers';
@@ -230,21 +231,9 @@ export class WorkflowsService {
 			);
 		}
 
-		let onlyActiveUpdate = false;
+		if (Object.keys(omit(workflow, ['id', 'versionId', 'active'])).length > 0) {
-
+			// Update the workflow's version when changing properties such as
-		if (
+			// `name`, `pinData`, `nodes`, `connections`, `settings` or `tags`
-			(Object.keys(workflow).length === 3 &&
-				workflow.id !== undefined &&
-				workflow.versionId !== undefined &&
-				workflow.active !== undefined) ||
-			(Object.keys(workflow).length === 2 &&
-				workflow.versionId !== undefined &&
-				workflow.active !== undefined)
-		) {
-			// we're just updating the active status of the workflow, don't update the versionId
-			onlyActiveUpdate = true;
-		} else {
-			// Update the workflow's version
 			workflow.versionId = uuid();
 			logger.verbose(
 				`Updating versionId for workflow ${workflowId} for user ${user.id} after saving`,
@@ -320,7 +309,7 @@ export class WorkflowsService {
 			);
 		}
 
-		if (!onlyActiveUpdate && workflow.versionId !== shared.workflow.versionId) {
+		if (workflow.versionId !== shared.workflow.versionId) {
 			await Container.get(WorkflowHistoryService).saveVersion(user, workflow, workflowId);
 		}
 

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;

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';
-
+import {
-const wfOwnerRole = () =>
+	mockCredRole,
-	Object.assign(new Role(), {
+	mockCredential,
-		scope: 'workflow',
+	mockUser,
-		name: 'owner',
+	mockInstanceOwnerRole,
-		id: randomInteger(),
+	wfOwnerRole,
-	});
+} from '../shared/mockObjects';
-
-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,
-	});
 
 describe('OwnershipService', () => {
 	const roleService = mockInstance(RoleService);
@@ -66,132 +33,149 @@ describe('OwnershipService', () => {
 		jest.clearAllMocks();
 	});
 
-	describe('getWorkflowOwner()', () => {
+	describe('OwnershipService', () => {
-		test('should retrieve a workflow owner', async () => {
+		const roleService = mockInstance(RoleService);
-			roleService.findWorkflowOwnerRole.mockResolvedValueOnce(wfOwnerRole());
+		const userRepository = mockInstance(UserRepository);
+		const sharedWorkflowRepository = mockInstance(SharedWorkflowRepository);
 
-			const mockOwner = new User();
+		const ownershipService = new OwnershipService(
-			const mockNonOwner = new User();
+			mock(),
+			userRepository,
+			roleService,
+			sharedWorkflowRepository,
+		);
 
-			const sharedWorkflow = Object.assign(new SharedWorkflow(), {
+		beforeEach(() => {
-				role: new Role(),
+			jest.clearAllMocks();
-				user: mockOwner,
+		});
+
+		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');
+				await expect(ownershipService.getWorkflowOwnerCached('some-workflow-id')).rejects.toThrow();
-
-			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,
 			});
 
-			expect(sharedWith).toStrictEqual([
+			test('should throw if no workflow owner found', async () => {
-				{
+				roleService.findWorkflowOwnerRole.mockResolvedValueOnce(wfOwnerRole());
-					id: editor.id,
+
-					email: editor.email,
+				sharedWorkflowRepository.findOneOrFail.mockRejectedValue(new Error());
-					firstName: editor.firstName,
+
-					lastName: editor.lastName,
+				await expect(ownershipService.getWorkflowOwnerCached('some-workflow-id')).rejects.toThrow();
-				},
+			});
-			]);
 		});
 
-		test('should add `ownedBy` and `sharedWith` to workflow', async () => {
+		describe('addOwnedByAndSharedWith()', () => {
-			const owner = mockUser();
+			test('should add `ownedBy` and `sharedWith` to credential', async () => {
-			const editor = mockUser();
+				const owner = mockUser();
+				const editor = mockUser();
 
-			const workflow = new WorkflowEntity();
+				const credential = mockCredential();
 
-			workflow.shared = [
+				credential.shared = [
-				{ role: mockCredRole('owner'), user: owner },
+					{ role: mockCredRole('owner'), user: owner },
-				{ role: mockCredRole('editor'), user: editor },
+					{ role: mockCredRole('editor'), user: editor },
-			] as SharedWorkflow[];
+				] as SharedCredentials[];
 
-			const { ownedBy, sharedWith } = ownershipService.addOwnedByAndSharedWith(workflow);
+				const { ownedBy, sharedWith } = ownershipService.addOwnedByAndSharedWith(credential);
 
-			expect(ownedBy).toStrictEqual({
+				expect(ownedBy).toStrictEqual({
-				id: owner.id,
+					id: owner.id,
-				email: owner.email,
+					email: owner.email,
-				firstName: owner.firstName,
+					firstName: owner.firstName,
-				lastName: owner.lastName,
+					lastName: owner.lastName,
+				});
+
+				expect(sharedWith).toStrictEqual([
+					{
+						id: editor.id,
+						email: editor.email,
+						firstName: editor.firstName,
+						lastName: editor.lastName,
+					},
+				]);
 			});
 
-			expect(sharedWith).toStrictEqual([
+			test('should add `ownedBy` and `sharedWith` to workflow', async () => {
-				{
+				const owner = mockUser();
-					id: editor.id,
+				const editor = mockUser();
-					email: editor.email,
-					firstName: editor.firstName,
-					lastName: editor.lastName,
-				},
-			]);
-		});
 
-		test('should produce an empty sharedWith if no sharee', async () => {
+				const workflow = new WorkflowEntity();
-			const owner = mockUser();
 
-			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({
+				expect(sharedWith).toStrictEqual([
-				id: owner.id,
+					{
-				email: owner.email,
+						id: editor.id,
-				firstName: owner.firstName,
+						email: editor.email,
-				lastName: owner.lastName,
+						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()', () => {
+		describe('getInstanceOwner()', () => {
-		test('should find owner using global owner role ID', async () => {
+			test('should find owner using global owner role ID', async () => {
-			const instanceOwnerRole = mockInstanceOwnerRole();
+				const instanceOwnerRole = mockInstanceOwnerRole();
-			roleService.findGlobalOwnerRole.mockResolvedValue(instanceOwnerRole);
+				roleService.findGlobalOwnerRole.mockResolvedValue(instanceOwnerRole);
 
-			await ownershipService.getInstanceOwner();
+				await ownershipService.getInstanceOwner();
 
-			expect(userRepository.findOneOrFail).toHaveBeenCalledWith({
+				expect(userRepository.findOneOrFail).toHaveBeenCalledWith({
-				where: { globalRoleId: instanceOwnerRole.id },
+					where: { globalRoleId: instanceOwnerRole.id },
-				relations: ['globalRole'],
+					relations: ['globalRole'],
+				});
 			});
 		});
 	});

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),
+			);
+		});
+	});
+});

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(),
+	});