fix(core): Fix workflow activation with history and workflow history for EE (no-changelog) (#7508)

Github issue / Community forum post (link here to close automatically):
This commit is contained in:
Val 2023-10-25 11:07:11 +01:00 committed by GitHub
parent 671c95760b
commit 93cfabbeac
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 361 additions and 34 deletions

View file

@ -27,7 +27,6 @@ import {
import { WorkflowsService } from '@/workflows/workflows.services';
import { InternalHooks } from '@/InternalHooks';
import { RoleService } from '@/services/role.service';
import { isWorkflowHistoryLicensed } from '@/workflows/workflowHistory/workflowHistoryHelper.ee';
import { WorkflowHistoryService } from '@/workflows/workflowHistory/workflowHistory.service.ee';
export = {
@ -47,13 +46,11 @@ export = {
const createdWorkflow = await createWorkflow(workflow, req.user, role);
if (isWorkflowHistoryLicensed()) {
await Container.get(WorkflowHistoryService).saveVersion(
req.user,
createdWorkflow,
createdWorkflow.id,
);
}
await Container.get(ExternalHooks).run('workflow.afterCreate', [createdWorkflow]);
void Container.get(InternalHooks).onWorkflowCreated(req.user, createdWorkflow, true);
@ -202,7 +199,7 @@ export = {
const updatedWorkflow = await getWorkflowById(sharedWorkflow.workflowId);
if (isWorkflowHistoryLicensed() && updatedWorkflow) {
if (updatedWorkflow) {
await Container.get(WorkflowHistoryService).saveVersion(
req.user,
updatedWorkflow,

View file

@ -6,6 +6,7 @@ import { SharedWorkflowRepository } from '@/databases/repositories';
import { WorkflowHistoryRepository } from '@db/repositories/workflowHistory.repository';
import { Service } from 'typedi';
import { isWorkflowHistoryEnabled } from './workflowHistoryHelper.ee';
import { getLogger } from '@/Logger';
export class SharedWorkflowNotFoundError extends Error {}
export class HistoryVersionNotFoundError extends Error {}
@ -66,6 +67,7 @@ export class WorkflowHistoryService {
async saveVersion(user: User, workflow: WorkflowEntity, workflowId: string) {
if (isWorkflowHistoryEnabled()) {
try {
await this.workflowHistoryRepository.insert({
authors: user.firstName + ' ' + user.lastName,
connections: workflow.connections,
@ -73,6 +75,12 @@ export class WorkflowHistoryService {
versionId: workflow.versionId,
workflowId,
});
} catch (e) {
getLogger().error(
`Failed to save workflow history version for workflow ${workflowId}`,
e as Error,
);
}
}
}
}

View file

@ -22,6 +22,7 @@ import { RoleService } from '@/services/role.service';
import * as utils from '@/utils';
import { listQueryMiddleware } from '@/middlewares';
import { TagService } from '@/services/tag.service';
import { WorkflowHistoryService } from './workflowHistory/workflowHistory.service.ee';
// eslint-disable-next-line @typescript-eslint/naming-convention
export const EEWorkflowController = express.Router();
@ -186,6 +187,12 @@ EEWorkflowController.post(
);
}
await Container.get(WorkflowHistoryService).saveVersion(
req.user,
savedWorkflow,
savedWorkflow.id,
);
if (tagIds && !config.getEnv('workflowTagsDisabled') && savedWorkflow.tags) {
savedWorkflow.tags = Container.get(TagService).sortByRequestOrder(savedWorkflow.tags, {
requestOrder: tagIds,

View file

@ -26,7 +26,6 @@ import { RoleService } from '@/services/role.service';
import * as utils from '@/utils';
import { listQueryMiddleware } from '@/middlewares';
import { TagService } from '@/services/tag.service';
import { isWorkflowHistoryLicensed } from './workflowHistory/workflowHistoryHelper.ee';
import { WorkflowHistoryService } from './workflowHistory/workflowHistory.service.ee';
export const workflowsController = express.Router();
@ -101,13 +100,11 @@ workflowsController.post(
throw new ResponseHelper.InternalServerError('Failed to save workflow');
}
if (isWorkflowHistoryLicensed()) {
await Container.get(WorkflowHistoryService).saveVersion(
req.user,
savedWorkflow,
savedWorkflow.id,
);
}
if (tagIds && !config.getEnv('workflowTagsDisabled') && savedWorkflow.tags) {
savedWorkflow.tags = Container.get(TagService).sortByRequestOrder(savedWorkflow.tags, {

View file

@ -33,7 +33,6 @@ import { WorkflowRepository } from '@/databases/repositories';
import { RoleService } from '@/services/role.service';
import { OwnershipService } from '@/services/ownership.service';
import { isStringArray, isWorkflowIdValid } from '@/utils';
import { isWorkflowHistoryLicensed } from './workflowHistory/workflowHistoryHelper.ee';
import { WorkflowHistoryService } from './workflowHistory/workflowHistory.service.ee';
import { BinaryDataService } from 'n8n-core';
@ -222,13 +221,19 @@ export class WorkflowsService {
);
}
let onlyActiveUpdate = false;
if (
Object.keys(workflow).length === 3 &&
(Object.keys(workflow).length === 3 &&
workflow.id !== undefined &&
workflow.versionId !== undefined &&
workflow.active !== 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();
@ -301,7 +306,7 @@ export class WorkflowsService {
);
}
if (isWorkflowHistoryLicensed() && workflow.versionId !== shared.workflow.versionId) {
if (!onlyActiveUpdate && workflow.versionId !== shared.workflow.versionId) {
await Container.get(WorkflowHistoryService).saveVersion(user, workflow, workflowId);
}

View file

@ -447,7 +447,7 @@ export async function createManyWorkflows(
* @param user user to assign the workflow to
*/
export async function createWorkflow(attributes: Partial<WorkflowEntity> = {}, user?: User) {
const { active, name, nodes, connections } = attributes;
const { active, name, nodes, connections, versionId } = attributes;
const workflowEntity = Db.collections.Workflow.create({
active: active ?? false,
@ -463,6 +463,7 @@ export async function createWorkflow(attributes: Partial<WorkflowEntity> = {}, u
},
],
connections: connections ?? {},
versionId: versionId ?? uuid(),
...attributes,
});

View file

@ -12,6 +12,10 @@ import { createWorkflow, getGlobalMemberRole, getGlobalOwnerRole } from './share
import type { SaveCredentialFunction } from './shared/types';
import { makeWorkflow } from './shared/utils/';
import { randomCredentialPayload } from './shared/random';
import { License } from '@/License';
import { WorkflowHistoryRepository } from '@/databases/repositories';
import Container from 'typedi';
import { ActiveWorkflowRunner } from '@/ActiveWorkflowRunner';
let owner: User;
let member: User;
@ -21,6 +25,12 @@ let authMemberAgent: SuperAgentTest;
let authAnotherMemberAgent: SuperAgentTest;
let saveCredential: SaveCredentialFunction;
const licenseLike = utils.mockInstance(License, {
isWorkflowHistoryLicensed: jest.fn().mockReturnValue(false),
isWithinUsersLimit: jest.fn().mockReturnValue(true),
});
const activeWorkflowRunnerLike = utils.mockInstance(ActiveWorkflowRunner);
const sharingSpy = jest.spyOn(UserManagementHelpers, 'isSharingEnabled').mockReturnValue(true);
const testServer = utils.setupTestServer({
endpointGroups: ['workflows'],
@ -46,7 +56,11 @@ beforeAll(async () => {
});
beforeEach(async () => {
await testDb.truncate(['Workflow', 'SharedWorkflow']);
activeWorkflowRunnerLike.add.mockReset();
activeWorkflowRunnerLike.remove.mockReset();
await testDb.truncate(['Workflow', 'SharedWorkflow', WorkflowHistoryRepository]);
licenseLike.isWorkflowHistoryLicensed.mockReturnValue(false);
});
describe('router should switch based on flag', () => {
@ -399,6 +413,96 @@ describe('POST /workflows', () => {
const response = await authAnotherMemberAgent.post('/workflows').send(workflow);
expect(response.statusCode).toBe(200);
});
test('Should create workflow history version when licensed', async () => {
licenseLike.isWorkflowHistoryLicensed.mockReturnValue(true);
const payload = {
name: 'testing',
nodes: [
{
id: 'uuid-1234',
parameters: {},
name: 'Start',
type: 'n8n-nodes-base.start',
typeVersion: 1,
position: [240, 300],
},
],
connections: {},
staticData: null,
settings: {
saveExecutionProgress: true,
saveManualExecutions: true,
saveDataErrorExecution: 'all',
saveDataSuccessExecution: 'all',
executionTimeout: 3600,
timezone: 'America/New_York',
},
active: false,
};
const response = await authOwnerAgent.post('/workflows').send(payload);
expect(response.statusCode).toBe(200);
const {
data: { id },
} = response.body;
expect(id).toBeDefined();
expect(
await Container.get(WorkflowHistoryRepository).count({ where: { workflowId: id } }),
).toBe(1);
const historyVersion = await Container.get(WorkflowHistoryRepository).findOne({
where: {
workflowId: id,
},
});
expect(historyVersion).not.toBeNull();
expect(historyVersion!.connections).toEqual(payload.connections);
expect(historyVersion!.nodes).toEqual(payload.nodes);
});
test('Should not create workflow history version when not licensed', async () => {
licenseLike.isWorkflowHistoryLicensed.mockReturnValue(false);
const payload = {
name: 'testing',
nodes: [
{
id: 'uuid-1234',
parameters: {},
name: 'Start',
type: 'n8n-nodes-base.start',
typeVersion: 1,
position: [240, 300],
},
],
connections: {},
staticData: null,
settings: {
saveExecutionProgress: true,
saveManualExecutions: true,
saveDataErrorExecution: 'all',
saveDataSuccessExecution: 'all',
executionTimeout: 3600,
timezone: 'America/New_York',
},
active: false,
};
const response = await authOwnerAgent.post('/workflows').send(payload);
expect(response.statusCode).toBe(200);
const {
data: { id },
} = response.body;
expect(id).toBeDefined();
expect(
await Container.get(WorkflowHistoryRepository).count({ where: { workflowId: id } }),
).toBe(0);
});
});
describe('PATCH /workflows/:id - validate credential permissions to user', () => {
@ -831,3 +935,160 @@ describe('getSharedWorkflowIds', () => {
expect(sharedWorkflowIds).toContain(workflow3.id);
});
});
describe('PATCH /workflows/:id - workflow history', () => {
test('Should create workflow history version when licensed', async () => {
licenseLike.isWorkflowHistoryLicensed.mockReturnValue(true);
const workflow = await testDb.createWorkflow({}, owner);
const payload = {
name: 'name updated',
versionId: workflow.versionId,
nodes: [
{
id: 'uuid-1234',
parameters: {},
name: 'Start',
type: 'n8n-nodes-base.start',
typeVersion: 1,
position: [240, 300],
},
{
id: 'uuid-1234',
parameters: {},
name: 'Cron',
type: 'n8n-nodes-base.cron',
typeVersion: 1,
position: [400, 300],
},
],
connections: {},
staticData: '{"id":1}',
settings: {
saveExecutionProgress: false,
saveManualExecutions: false,
saveDataErrorExecution: 'all',
saveDataSuccessExecution: 'all',
executionTimeout: 3600,
timezone: 'America/New_York',
},
};
const response = await authOwnerAgent.patch(`/workflows/${workflow.id}`).send(payload);
const {
data: { id },
} = response.body;
expect(response.statusCode).toBe(200);
expect(id).toBe(workflow.id);
expect(
await Container.get(WorkflowHistoryRepository).count({ where: { workflowId: id } }),
).toBe(1);
const historyVersion = await Container.get(WorkflowHistoryRepository).findOne({
where: {
workflowId: id,
},
});
expect(historyVersion).not.toBeNull();
expect(historyVersion!.connections).toEqual(payload.connections);
expect(historyVersion!.nodes).toEqual(payload.nodes);
});
test('Should not create workflow history version when not licensed', async () => {
licenseLike.isWorkflowHistoryLicensed.mockReturnValue(false);
const workflow = await testDb.createWorkflow({}, owner);
const payload = {
name: 'name updated',
versionId: workflow.versionId,
nodes: [
{
id: 'uuid-1234',
parameters: {},
name: 'Start',
type: 'n8n-nodes-base.start',
typeVersion: 1,
position: [240, 300],
},
{
id: 'uuid-1234',
parameters: {},
name: 'Cron',
type: 'n8n-nodes-base.cron',
typeVersion: 1,
position: [400, 300],
},
],
connections: {},
staticData: '{"id":1}',
settings: {
saveExecutionProgress: false,
saveManualExecutions: false,
saveDataErrorExecution: 'all',
saveDataSuccessExecution: 'all',
executionTimeout: 3600,
timezone: 'America/New_York',
},
};
const response = await authOwnerAgent.patch(`/workflows/${workflow.id}`).send(payload);
const {
data: { id },
} = response.body;
expect(response.statusCode).toBe(200);
expect(id).toBe(workflow.id);
expect(
await Container.get(WorkflowHistoryRepository).count({ where: { workflowId: id } }),
).toBe(0);
});
});
describe('PATCH /workflows/:id - activate workflow', () => {
test('should activate workflow without changing version ID', async () => {
licenseLike.isWorkflowHistoryLicensed.mockReturnValue(false);
const workflow = await testDb.createWorkflow({}, owner);
const payload = {
versionId: workflow.versionId,
active: true,
};
const response = await authOwnerAgent.patch(`/workflows/${workflow.id}`).send(payload);
expect(response.statusCode).toBe(200);
expect(activeWorkflowRunnerLike.add).toBeCalled();
const {
data: { id, versionId, active },
} = response.body;
expect(id).toBe(workflow.id);
expect(versionId).toBe(workflow.versionId);
expect(active).toBe(true);
});
test('should deactivate workflow without changing version ID', async () => {
licenseLike.isWorkflowHistoryLicensed.mockReturnValue(false);
const workflow = await testDb.createWorkflow({ active: true }, owner);
const payload = {
versionId: workflow.versionId,
active: false,
};
const response = await authOwnerAgent.patch(`/workflows/${workflow.id}`).send(payload);
expect(response.statusCode).toBe(200);
expect(activeWorkflowRunnerLike.add).not.toBeCalled();
expect(activeWorkflowRunnerLike.remove).toBeCalled();
const {
data: { id, versionId, active },
} = response.body;
expect(id).toBe(workflow.id);
expect(versionId).toBe(workflow.versionId);
expect(active).toBe(false);
});
});

View file

@ -13,6 +13,7 @@ import Container from 'typedi';
import type { ListQuery } from '@/requests';
import { License } from '@/License';
import { WorkflowHistoryRepository } from '@/databases/repositories';
import { ActiveWorkflowRunner } from '@/ActiveWorkflowRunner';
let owner: User;
let authOwnerAgent: SuperAgentTest;
@ -27,12 +28,15 @@ const licenseLike = utils.mockInstance(License, {
isWithinUsersLimit: jest.fn().mockReturnValue(true),
});
const activeWorkflowRunnerLike = utils.mockInstance(ActiveWorkflowRunner);
beforeAll(async () => {
owner = await testDb.createOwner();
authOwnerAgent = testServer.authAgentFor(owner);
});
beforeEach(async () => {
jest.resetAllMocks();
await testDb.truncate(['Workflow', 'SharedWorkflow', 'Tag', WorkflowHistoryRepository]);
licenseLike.isWorkflowHistoryLicensed.mockReturnValue(false);
});
@ -207,7 +211,7 @@ describe('GET /workflows', () => {
createdAt: any(String),
updatedAt: any(String),
tags: [{ id: any(String), name: 'A' }],
versionId: null,
versionId: any(String),
ownedBy: { id: owner.id },
}),
objectContaining({
@ -217,7 +221,7 @@ describe('GET /workflows', () => {
createdAt: any(String),
updatedAt: any(String),
tags: [],
versionId: null,
versionId: any(String),
ownedBy: { id: owner.id },
}),
]),
@ -423,6 +427,7 @@ describe('PATCH /workflows/:id', () => {
const workflow = await testDb.createWorkflow({}, owner);
const payload = {
name: 'name updated',
versionId: workflow.versionId,
nodes: [
{
id: 'uuid-1234',
@ -480,6 +485,7 @@ describe('PATCH /workflows/:id', () => {
const workflow = await testDb.createWorkflow({}, owner);
const payload = {
name: 'name updated',
versionId: workflow.versionId,
nodes: [
{
id: 'uuid-1234',
@ -523,4 +529,49 @@ describe('PATCH /workflows/:id', () => {
await Container.get(WorkflowHistoryRepository).count({ where: { workflowId: id } }),
).toBe(0);
});
test('should activate workflow without changing version ID', async () => {
licenseLike.isWorkflowHistoryLicensed.mockReturnValue(false);
const workflow = await testDb.createWorkflow({}, owner);
const payload = {
versionId: workflow.versionId,
active: true,
};
const response = await authOwnerAgent.patch(`/workflows/${workflow.id}`).send(payload);
expect(response.statusCode).toBe(200);
expect(activeWorkflowRunnerLike.add).toBeCalled();
const {
data: { id, versionId, active },
} = response.body;
expect(id).toBe(workflow.id);
expect(versionId).toBe(workflow.versionId);
expect(active).toBe(true);
});
test('should deactivate workflow without changing version ID', async () => {
licenseLike.isWorkflowHistoryLicensed.mockReturnValue(false);
const workflow = await testDb.createWorkflow({ active: true }, owner);
const payload = {
versionId: workflow.versionId,
active: false,
};
const response = await authOwnerAgent.patch(`/workflows/${workflow.id}`).send(payload);
expect(response.statusCode).toBe(200);
expect(activeWorkflowRunnerLike.add).not.toBeCalled();
expect(activeWorkflowRunnerLike.remove).toBeCalled();
const {
data: { id, versionId, active },
} = response.body;
expect(id).toBe(workflow.id);
expect(versionId).toBe(workflow.versionId);
expect(active).toBe(false);
});
});