Skip to content

Commit

Permalink
fix(core): Prevent workflow history saving error from happening (#7812)
Browse files Browse the repository at this point in the history 
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):
  • Loading branch information
@krynble
krynble authored Dec 13, 2023
1 parent b00b905 commit e5581ce
Show file tree
Hide file tree
Showing 6 changed files with 287 additions and 155 deletions.
5 changes: 4 additions & 1 deletion packages/cli/src/workflows/workflowHistory/workflowHistory.service.ee.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
21 changes: 5 additions & 16 deletions packages/cli/src/workflows/workflows.services.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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`,
Expand Down Expand Up @@ -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);
}

Expand Down
2 changes: 1 addition & 1 deletion packages/cli/test/unit/WorkflowHelpers.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ function generateCredentialEntity(credentialId: string) {
return credentialEntity;
}

function getWorkflow(options?: {
export function getWorkflow(options?: {
addNodeWithoutCreds?: boolean;
addNodeWithOneCred?: boolean;
addNodeWithTwoCreds?: boolean;
Expand Down
258 changes: 121 additions & 137 deletions packages/cli/test/unit/services/ownership.service.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>): 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);
Expand All @@ -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'],
});
});
});
});
Expand Down
Loading

0 comments on commit e5581ce

Please sign in to comment.