Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(core): Prevent workflow history saving error from happening #7812

Merged
merged 4 commits into from
Dec 13, 2023
Merged

fix(core): Prevent workflow history saving error from happening #7812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
578a4a9
fix(core): Prevent workflow history saving error from happening
krynble Nov 17, 2023
c1abcf9
chore: merge master
krynble Dec 12, 2023
0b38029
fix: Missing brackets
krynble Dec 13, 2023
646d48c
refactor: Lint issue
krynble Dec 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/cli/src/workflows/workflowHistory/workflowHistory.service.ee.ts
View file
Open in desktop
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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for stuff like this we should really add different set of methods on the repository, and add strong typing on the payload that gets passed in. That way not only we wouldn't need vague checks with comments like these, but also the method names are going to be self explanatory. a good example is markAsCrashed in ExecutionRepository

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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading