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(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
+		if (Object.keys(omit(workflow, ['id', 'versionId', 'active'])).length > 0) {
+			// Update the workflow's version when changing properties such as
+			// `name`, `pinData`, `nodes`, `connections`, `settings` or `tags`
 			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,50 +4,33 @@ 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 {
+	mockCredRole,
+	mockCredential,
+	mockUser,
+	mockInstanceOwnerRole,
+	wfOwnerRole,
+} from '../shared/mockObjects';
 
-const wfOwnerRole = () =>
-	Object.assign(new Role(), {
-		scope: 'workflow',
-		name: 'owner',
-		id: randomInteger(),
-	});
+describe('OwnershipService', () => {
+	const roleService = mockInstance(RoleService);
+	const userRepository = mockInstance(UserRepository);
+	const sharedWorkflowRepository = mockInstance(SharedWorkflowRepository);
 
-const mockCredRole = (name: 'owner' | 'editor'): Role =>
-	Object.assign(new Role(), {
-		scope: 'credentials',
-		name,
-		id: randomInteger(),
-	});
+	const ownershipService = new OwnershipService(
+		mock(),
+		userRepository,
+		roleService,
+		sharedWorkflowRepository,
+	);
 
-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,
+	beforeEach(() => {
+		jest.clearAllMocks();
 	});
 
 	describe('OwnershipService', () => {
@@ -196,3 +179,4 @@ describe('OwnershipService', () => {
 			});
 		});
 	});
+});

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