diff --git a/packages/cli/src/workflows/workflow.service.ts b/packages/cli/src/workflows/workflow.service.ts index f6caed84c73..dd40c054c01 100644 --- a/packages/cli/src/workflows/workflow.service.ts +++ b/packages/cli/src/workflows/workflow.service.ts @@ -6,6 +6,7 @@ import type { ListQueryDb, WorkflowFolderUnionFull, WorkflowHistoryUpdate, + WorkflowHistory, } from '@n8n/db'; import { SharedWorkflow, @@ -48,6 +49,12 @@ import { WorkflowFinderService } from './workflow-finder.service'; import { WorkflowHistoryService } from './workflow-history/workflow-history.service'; import { WorkflowSharingService } from './workflow-sharing.service'; +type RollbackPayload = { + active: boolean; + activeVersionId: string | null; + activeVersion: WorkflowHistory | null; +} & Partial>; + @Service() export class WorkflowService { constructor( @@ -447,7 +454,14 @@ export class WorkflowService { workflowId, updatedWorkflow, wasActive ? 'update' : 'activate', - workflow.versionId, + // If workflow could not be activated, set it again to inactive + // and revert the versionId and activeVersionId change so UI remains consistent + { + versionId: workflow.versionId, + active: false, + activeVersionId: null, + activeVersion: null, + }, ); } } @@ -457,45 +471,35 @@ export class WorkflowService { /** * Private helper to add a workflow to the active workflow manager - * @param originalVersionId - Optional versionId to roll back to if activation fails + * @param rollBackOptions - Optional rollback options */ private async _addToActiveWorkflowManager( user: User, workflowId: string, workflow: WorkflowEntity, mode: 'activate' | 'update', - originalVersionId?: string, + rollbackPayload: RollbackPayload, ): Promise { try { await this.externalHooks.run('workflow.activate', [workflow]); await this.activeWorkflowManager.add(workflowId, mode); } catch (error) { - // If workflow could not be activated, set it again to inactive - // and revert the versionId and activeVersionId change so UI remains consistent - const rollbackPayload: QueryDeepPartialEntity = { - active: false, - activeVersion: null, - }; - - // Roll back versionId if provided (used in update flow) - if (originalVersionId !== undefined) { - rollbackPayload.versionId = originalVersionId; - } - await this.workflowRepository.update(workflowId, rollbackPayload); // Also set it in the returned data - workflow.active = false; - workflow.activeVersionId = null; - workflow.activeVersion = null; + workflow.active = rollbackPayload.active; + workflow.activeVersionId = rollbackPayload.activeVersionId; + workflow.activeVersion = rollbackPayload.activeVersion; - // Emit deactivation event since activation failed - this.eventService.emit('workflow-deactivated', { - user, - workflowId, - workflow, - publicApi: false, - }); + if (!workflow.activeVersionId) { + // Emit deactivation event since activation failed + this.eventService.emit('workflow-deactivated', { + user, + workflowId, + workflow, + publicApi: false, + }); + } let message; if (error instanceof NodeApiError) message = error.description; @@ -554,6 +558,7 @@ export class WorkflowService { const updatedWorkflow = await this.workflowRepository.findOne({ where: { id: workflowId }, + relations: ['activeVersion'], }); if (!updatedWorkflow) { @@ -567,7 +572,11 @@ export class WorkflowService { publicApi: false, }); - await this._addToActiveWorkflowManager(user, workflowId, updatedWorkflow, 'activate'); + await this._addToActiveWorkflowManager(user, workflowId, updatedWorkflow, 'activate', { + active: workflow.active, + activeVersionId: workflow.activeVersionId, + activeVersion: workflow.activeVersion, + }); return updatedWorkflow; } diff --git a/packages/cli/src/workflows/workflows.controller.ts b/packages/cli/src/workflows/workflows.controller.ts index 6604de7b8e3..37341cbe914 100644 --- a/packages/cli/src/workflows/workflows.controller.ts +++ b/packages/cli/src/workflows/workflows.controller.ts @@ -343,7 +343,11 @@ export class WorkflowsController { workflowId, req.user, ['workflow:read'], - { includeTags: !this.globalConfig.tags.disabled, includeParentFolder: true }, + { + includeTags: !this.globalConfig.tags.disabled, + includeParentFolder: true, + includeActiveVersion: true, + }, ); if (!workflow) { @@ -371,7 +375,11 @@ export class WorkflowsController { workflowId, req.user, ['workflow:read'], - { includeTags: !this.globalConfig.tags.disabled, includeParentFolder: true }, + { + includeTags: !this.globalConfig.tags.disabled, + includeParentFolder: true, + includeActiveVersion: true, + }, ); if (!workflow) { diff --git a/packages/cli/test/integration/workflows/workflows.controller.test.ts b/packages/cli/test/integration/workflows/workflows.controller.test.ts index 8f2060682ca..6b4f7e53655 100644 --- a/packages/cli/test/integration/workflows/workflows.controller.test.ts +++ b/packages/cli/test/integration/workflows/workflows.controller.test.ts @@ -14,7 +14,7 @@ import { mockInstance, } from '@n8n/backend-test-utils'; import { GlobalConfig } from '@n8n/config'; -import type { User, ListQueryDb, WorkflowFolderUnionFull, Role } from '@n8n/db'; +import type { User, ListQueryDb, WorkflowFolderUnionFull, Role, WorkflowHistory } from '@n8n/db'; import { ProjectRepository, WorkflowHistoryRepository, @@ -28,6 +28,7 @@ import { PROJECT_ROOT, type INode, type IPinData, type IWorkflowBase } from 'n8n import { v4 as uuid } from 'uuid'; import { ActiveWorkflowManager } from '@/active-workflow-manager'; +import { EventService } from '@/events/event.service'; import { License } from '@/license'; import { ProjectService } from '@/services/project.service.ee'; import { EnterpriseWorkflowService } from '@/workflows/workflow.service.ee'; @@ -37,6 +38,7 @@ import { saveCredential } from '../shared/db/credentials'; import { createCustomRoleWithScopeSlugs, cleanupRolesAndScopes } from '../shared/db/roles'; import { assignTagToWorkflow, createTag } from '../shared/db/tags'; import { createManyUsers, createMember, createOwner } from '../shared/db/users'; +import { createWorkflowHistoryItem } from '../shared/db/workflow-history'; import type { SuperAgentTest } from '../shared/types'; import * as utils from '../shared/utils/'; import { makeWorkflow, MOCK_PINDATA } from '../shared/utils/'; @@ -61,6 +63,9 @@ const { objectContaining, arrayContaining, any } = expect; const activeWorkflowManagerLike = mockInstance(ActiveWorkflowManager); let projectRepository: ProjectRepository; +let workflowRepository: WorkflowRepository; +let workflowHistoryRepository: WorkflowHistoryRepository; +let eventService: EventService; let globalConfig: GlobalConfig; let folderListMissingRole: Role; @@ -77,6 +82,9 @@ beforeEach(async () => { ]); await cleanupRolesAndScopes(); projectRepository = Container.get(ProjectRepository); + workflowRepository = Container.get(WorkflowRepository); + workflowHistoryRepository = Container.get(WorkflowHistoryRepository); + eventService = Container.get(EventService); globalConfig = Container.get(GlobalConfig); owner = await createOwner(); authOwnerAgent = testServer.authAgentFor(owner); @@ -279,11 +287,11 @@ describe('POST /workflows', () => { expect(active).toBe(true); // Verify in database - const workflow = await Container.get(WorkflowRepository).findOneBy({ id }); + const workflow = await workflowRepository.findOneBy({ id }); expect(workflow?.activeVersionId).toBe(versionId); // Verify history was created - const historyVersion = await Container.get(WorkflowHistoryRepository).findOne({ + const historyVersion = await workflowHistoryRepository.findOne({ where: { workflowId: id, versionId, @@ -582,6 +590,20 @@ describe('GET /workflows/:workflowId', () => { }); }); + test('should return active version', async () => { + const workflow = await createActiveWorkflow({}, owner); + + const response = await authOwnerAgent.get(`/workflows/${workflow.id}`).expect(200); + + const { data: responseData } = response.body as { data: { activeVersion: WorkflowHistory } }; + const { activeVersion } = responseData; + + expect(activeVersion).toMatchObject({ + versionId: workflow.activeVersionId, + workflowId: workflow.id, + }); + }); + test('should return parent folder', async () => { const personalProject = await projectRepository.getPersonalProjectForUserOrFail(owner.id); @@ -953,7 +975,7 @@ describe('GET /workflows', () => { test('should filter workflows by projectId', async () => { const workflow = await createWorkflow({ name: 'First' }, owner); - const pp = await Container.get(ProjectRepository).getPersonalProjectForUserOrFail(owner.id); + const pp = await projectRepository.getPersonalProjectForUserOrFail(owner.id); const response1 = await authOwnerAgent .get('/workflows') @@ -975,7 +997,7 @@ describe('GET /workflows', () => { const workflow = await createWorkflow({ name: 'First' }, owner); const workflow2 = await createWorkflow({ name: 'Second' }, member); await shareWorkflowWithUsers(workflow2, [owner]); - const pp = await Container.get(ProjectRepository).getPersonalProjectForUserOrFail(owner.id); + const pp = await projectRepository.getPersonalProjectForUserOrFail(owner.id); const response1 = await authOwnerAgent .get('/workflows') @@ -1000,7 +1022,7 @@ describe('GET /workflows', () => { const workflow = await createWorkflow({ name: 'First' }, member); const workflow2 = await createWorkflow({ name: 'Second' }, owner); await shareWorkflowWithUsers(workflow2, [member]); - const pp = await Container.get(ProjectRepository).getPersonalProjectForUserOrFail(member.id); + const pp = await projectRepository.getPersonalProjectForUserOrFail(member.id); const response1 = await authMemberAgent .get('/workflows') @@ -1022,7 +1044,7 @@ describe('GET /workflows', () => { }); test('should filter workflows by parentFolderId', async () => { - const pp = await Container.get(ProjectRepository).getPersonalProjectForUserOrFail(owner.id); + const pp = await projectRepository.getPersonalProjectForUserOrFail(owner.id); const folder1 = await createFolder(pp, { name: 'Folder 1' }); @@ -2010,7 +2032,7 @@ describe('GET /workflows?includeFolders=true', () => { test('should filter workflows by projectId', async () => { const workflow = await createWorkflow({ name: 'First' }, owner); - const pp = await Container.get(ProjectRepository).getPersonalProjectForUserOrFail(owner.id); + const pp = await projectRepository.getPersonalProjectForUserOrFail(owner.id); const folder = await createFolder(pp, { name: 'First Folder', @@ -2034,7 +2056,7 @@ describe('GET /workflows?includeFolders=true', () => { }); test('should filter workflows by parentFolderId and its descendants when filtering by query', async () => { - const pp = await Container.get(ProjectRepository).getPersonalProjectForUserOrFail(owner.id); + const pp = await projectRepository.getPersonalProjectForUserOrFail(owner.id); await createFolder(pp, { name: 'Root Folder 1', @@ -2448,10 +2470,8 @@ describe('PATCH /workflows/:workflowId', () => { expect(response.statusCode).toBe(200); expect(id).toBe(workflow.id); - expect( - await Container.get(WorkflowHistoryRepository).count({ where: { workflowId: id } }), - ).toBe(2); - const historyVersion = await Container.get(WorkflowHistoryRepository).findOne({ + expect(await workflowHistoryRepository.count({ where: { workflowId: id } })).toBe(2); + const historyVersion = await workflowHistoryRepository.findOne({ where: { workflowId: id, versionId: updatedVersionId, @@ -2548,7 +2568,7 @@ describe('PATCH /workflows/:workflowId', () => { expect(activeVersionId).toBe(workflow.versionId); // Verify activeVersion is set - const updatedWorkflow = await Container.get(WorkflowRepository).findOne({ + const updatedWorkflow = await workflowRepository.findOne({ where: { id: workflow.id }, relations: ['activeVersion'], }); @@ -2583,7 +2603,7 @@ describe('PATCH /workflows/:workflowId', () => { expect(activeVersionId).toBeNull(); // Verify activeVersion is cleared - const updatedWorkflow = await Container.get(WorkflowRepository).findOne({ + const updatedWorkflow = await workflowRepository.findOne({ where: { id: workflow.id }, }); @@ -2596,7 +2616,7 @@ describe('PATCH /workflows/:workflowId', () => { await setActiveVersion(workflow.id, workflow.versionId); // Verify initial state - const initialWorkflow = await Container.get(WorkflowRepository).findOne({ + const initialWorkflow = await workflowRepository.findOne({ where: { id: workflow.id }, relations: ['activeVersion'], }); @@ -2632,7 +2652,7 @@ describe('PATCH /workflows/:workflowId', () => { expect(newVersionId).not.toBe(workflow.versionId); // Verify activeVersion points to the new version - const updatedWorkflow = await Container.get(WorkflowRepository).findOne({ + const updatedWorkflow = await workflowRepository.findOne({ where: { id: workflow.id }, relations: ['activeVersion'], }); @@ -2676,7 +2696,7 @@ describe('PATCH /workflows/:workflowId', () => { expect(response.statusCode).toBe(200); - const updatedWorkflow = await Container.get(WorkflowRepository).findOneOrFail({ + const updatedWorkflow = await workflowRepository.findOneOrFail({ where: { id: workflow.id }, relations: ['parentFolder'], }); @@ -2695,7 +2715,7 @@ describe('PATCH /workflows/:workflowId', () => { expect(response.statusCode).toBe(200); - const updatedWorkflow = await Container.get(WorkflowRepository).findOneOrFail({ + const updatedWorkflow = await workflowRepository.findOneOrFail({ where: { id: workflow.id }, relations: ['parentFolder'], }); @@ -2777,7 +2797,6 @@ describe('PATCH /workflows/:workflowId', () => { expect(response.statusCode).toBe(200); - const workflowRepository = Container.get(WorkflowRepository); const updatedWorkflow = await workflowRepository.findOne({ where: { id: workflow.id }, }); @@ -2797,7 +2816,6 @@ describe('PATCH /workflows/:workflowId', () => { expect(response.statusCode).toBe(200); - const workflowRepository = Container.get(WorkflowRepository); const updatedWorkflow = await workflowRepository.findOne({ where: { id: workflow.id }, }); @@ -2859,6 +2877,18 @@ describe('POST /workflows/:workflowId/activate', () => { const { data } = response.body; expect(data.id).toBe(workflow.id); expect(data.activeVersionId).toBe(workflow.versionId); + expect(data.activeVersion.versionId).toBe(workflow.versionId); + }); + + test('should send activated event', async () => { + const workflow = await createWorkflowWithHistory({}, owner); + + const emitSpy = jest.spyOn(eventService, 'emit'); + await authOwnerAgent + .post(`/workflows/${workflow.id}/activate`) + .send({ versionId: workflow.versionId }); + + expect(emitSpy).toHaveBeenCalledWith('workflow-activated', expect.anything()); }); test('should return 400 when versionId is missing', async () => { @@ -2974,6 +3004,35 @@ describe('POST /workflows/:workflowId/activate', () => { expect(historyVersion?.name).toBe(newVersionName); expect(historyVersion?.description).toBe(newDescription); }); + + test('should preserve current active version when activation fails', async () => { + const workflow = await createActiveWorkflow({}, owner); + + const newVersionId = uuid(); + await createWorkflowHistoryItem(workflow.id, { versionId: newVersionId }); + + const emitSpy = jest.spyOn(eventService, 'emit'); + + activeWorkflowManagerLike.add.mockRejectedValueOnce(new Error('Validation failed')); + + const response = await authOwnerAgent + .post(`/workflows/${workflow.id}/activate`) + .send({ versionId: newVersionId }); + + expect(response.statusCode).toBe(400); + expect(response.body.message).toBe('Validation failed'); + + const updatedWorkflow = await workflowRepository.findOne({ + where: { id: workflow.id }, + relations: ['activeVersion'], + }); + + expect(updatedWorkflow?.active).toBe(true); + expect(updatedWorkflow?.activeVersionId).toBe(workflow.versionId); + expect(updatedWorkflow?.activeVersion?.versionId).toBe(workflow.versionId); + + expect(emitSpy).not.toHaveBeenCalledWith('workflow-deactivated', expect.anything()); + }); }); describe('POST /workflows/:workflowId/deactivate', () => { @@ -2991,6 +3050,15 @@ describe('POST /workflows/:workflowId/deactivate', () => { expect(data.activeVersionId).toBeNull(); }); + test('should send deactivated event', async () => { + const workflow = await createActiveWorkflow({}, owner); + + const emitSpy = jest.spyOn(eventService, 'emit'); + await authOwnerAgent.post(`/workflows/${workflow.id}/deactivate`); + + expect(emitSpy).toHaveBeenCalledWith('workflow-deactivated', expect.anything()); + }); + test('should handle deactivating already inactive workflow', async () => { const workflow = await createWorkflow({}, owner);