diff --git a/packages/cli/src/workflows/workflowHistory/workflowHistory.service.ee.ts b/packages/cli/src/workflows/workflowHistory/workflowHistory.service.ee.ts index e5243551b2098..2b5852f23b95a 100644 --- a/packages/cli/src/workflows/workflowHistory/workflowHistory.service.ee.ts +++ b/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, diff --git a/packages/cli/src/workflows/workflows.services.ts b/packages/cli/src/workflows/workflows.services.ts index ea6d6704a22c0..9dc153d62e483 100644 --- a/packages/cli/src/workflows/workflows.services.ts +++ b/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); } diff --git a/packages/cli/test/unit/WorkflowHelpers.test.ts b/packages/cli/test/unit/WorkflowHelpers.test.ts index dde5cdcd60850..54f8b6e912ce7 100644 --- 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; diff --git a/packages/cli/test/unit/services/ownership.service.test.ts b/packages/cli/test/unit/services/ownership.service.test.ts index 983ca278ab588..20be942c2f128 100644 --- 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 => - 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()); - - const mockOwner = new User(); - const mockNonOwner = new User(); - - const sharedWorkflow = Object.assign(new SharedWorkflow(), { - role: new Role(), - user: mockOwner, - }); - - sharedWorkflowRepository.findOneOrFail.mockResolvedValueOnce(sharedWorkflow); + describe('OwnershipService', () => { + const roleService = mockInstance(RoleService); + const userRepository = mockInstance(UserRepository); + const sharedWorkflowRepository = mockInstance(SharedWorkflowRepository); - const returnedOwner = await ownershipService.getWorkflowOwnerCached('some-workflow-id'); + const ownershipService = new OwnershipService( + mock(), + userRepository, + roleService, + sharedWorkflowRepository, + ); - expect(returnedOwner).toBe(mockOwner); - expect(returnedOwner).not.toBe(mockNonOwner); + beforeEach(() => { + jest.clearAllMocks(); }); - test('should throw if no workflow owner role found', async () => { - roleService.findWorkflowOwnerRole.mockRejectedValueOnce(new Error()); + describe('getWorkflowOwner()', () => { + test('should retrieve a workflow owner', async () => { + roleService.findWorkflowOwnerRole.mockResolvedValueOnce(wfOwnerRole()); - await expect(ownershipService.getWorkflowOwnerCached('some-workflow-id')).rejects.toThrow(); - }); - - test('should throw if no workflow owner found', async () => { - roleService.findWorkflowOwnerRole.mockResolvedValueOnce(wfOwnerRole()); + const mockOwner = new User(); + const mockNonOwner = new User(); - sharedWorkflowRepository.findOneOrFail.mockRejectedValue(new Error()); + const sharedWorkflow = Object.assign(new SharedWorkflow(), { + role: new Role(), + user: mockOwner, + }); - await expect(ownershipService.getWorkflowOwnerCached('some-workflow-id')).rejects.toThrow(); - }); - }); + sharedWorkflowRepository.findOneOrFail.mockResolvedValueOnce(sharedWorkflow); - describe('addOwnedByAndSharedWith()', () => { - test('should add `ownedBy` and `sharedWith` to credential', async () => { - const owner = mockUser(); - const editor = mockUser(); + const returnedOwner = await ownershipService.getWorkflowOwnerCached('some-workflow-id'); - const credential = mockCredential(); - - credential.shared = [ - { role: mockCredRole('owner'), user: owner }, - { role: mockCredRole('editor'), user: editor }, - ] as SharedCredentials[]; + expect(returnedOwner).toBe(mockOwner); + expect(returnedOwner).not.toBe(mockNonOwner); + }); - const { ownedBy, sharedWith } = ownershipService.addOwnedByAndSharedWith(credential); + test('should throw if no workflow owner role found', async () => { + roleService.findWorkflowOwnerRole.mockRejectedValueOnce(new Error()); - 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 add `ownedBy` and `sharedWith` to workflow', async () => { - const owner = mockUser(); - const editor = mockUser(); + test('should throw if no workflow owner found', async () => { + roleService.findWorkflowOwnerRole.mockResolvedValueOnce(wfOwnerRole()); - const workflow = new WorkflowEntity(); + sharedWorkflowRepository.findOneOrFail.mockRejectedValue(new Error()); - workflow.shared = [ - { role: mockCredRole('owner'), user: owner }, - { role: mockCredRole('editor'), user: editor }, - ] as SharedWorkflow[]; + await expect(ownershipService.getWorkflowOwnerCached('some-workflow-id')).rejects.toThrow(); + }); + }); - const { ownedBy, sharedWith } = ownershipService.addOwnedByAndSharedWith(workflow); + 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([ + { + id: editor.id, + email: editor.email, + firstName: editor.firstName, + lastName: editor.lastName, + }, + ]); + }); - expect(ownedBy).toStrictEqual({ - id: owner.id, - email: owner.email, - firstName: owner.firstName, - lastName: owner.lastName, + test('should add `ownedBy` and `sharedWith` to workflow', async () => { + const owner = mockUser(); + const editor = mockUser(); + + const workflow = new WorkflowEntity(); + + workflow.shared = [ + { role: mockCredRole('owner'), user: owner }, + { role: mockCredRole('editor'), user: editor }, + ] as SharedWorkflow[]; + + const { ownedBy, sharedWith } = ownershipService.addOwnedByAndSharedWith(workflow); + + 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 produce an empty sharedWith if no sharee', async () => { + const owner = mockUser(); - test('should produce an empty sharedWith if no sharee', async () => { - const owner = mockUser(); + const credential = mockCredential(); - const credential = mockCredential(); + credential.shared = [{ role: mockCredRole('owner'), user: owner }] as SharedCredentials[]; - credential.shared = [{ role: mockCredRole('owner'), user: owner }] as SharedCredentials[]; + const { ownedBy, sharedWith } = ownershipService.addOwnedByAndSharedWith(credential); - 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).toHaveLength(0); }); - - 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'], + }); }); }); }); diff --git a/packages/cli/test/unit/services/workflowHistory.service.ee.test.ts b/packages/cli/test/unit/services/workflowHistory.service.ee.test.ts new file mode 100644 index 0000000000000..7a623188e6a83 --- /dev/null +++ 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), + ); + }); + }); +}); diff --git a/packages/cli/test/unit/shared/mockObjects.ts b/packages/cli/test/unit/shared/mockObjects.ts new file mode 100644 index 0000000000000..dee4d97150266 --- /dev/null +++ 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(), + });