mohiit1502/Automata
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

refactor: Consolidate WorkflowService.getMany() (no-changelog) (#6892)

Browse Source 
In scope:

- Consolidate `WorkflowService.getMany()`.
- Support non-entity field `ownedBy` for `select`.
- Support `tags` for `filter`.
- Move `addOwnerId` to `OwnershipService`.
- Remove unneeded check for `filter.id`.
- Simplify DTO validation for `filter` and `select`.
- Expand tests for `GET /workflows`.

Workflow list query DTOs:

```
filter → name, active, tags
select → id, name, active, tags, createdAt, updatedAt, versionId, ownedBy
```

Out of scope:

- Migrate `shared_workflow.roleId` and `shared_credential.roleId` to
string IDs.
- Refactor `WorkflowHelpers.getSharedWorkflowIds()`.
This commit is contained in:
Iván Ovejero
2023-08-22 13:19:37 +02:00
committed by GitHub
parent f32e993227
commit 2cfa6d344e
20 changed files with 686 additions and 318 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
packages/cli/test/integration/shared/testDb.ts
View File

@@ -35,7 +35,7 @@ import type { ExecutionData } from '@db/entities/ExecutionData';
import { generateNanoId } from '@db/utils/generators';
import { RoleService } from '@/services/role.service';
import { VariablesService } from '@/environments/variables/variables.service';
import { TagRepository } from '@/databases/repositories';
import { TagRepository, WorkflowTagMappingRepository } from '@/databases/repositories';
import { separate } from '@/utils';
export type TestDBType = 'postgres' | 'mysql';
@@ -119,6 +119,7 @@ export async function truncate(collections: CollectionName[]) {
if (tag) {
await Container.get(TagRepository).delete({});
await Container.get(WorkflowTagMappingRepository).delete({});
}
for (const collection of rest) {
@@ -389,14 +390,24 @@ export async function createWaitingExecution(workflow: WorkflowEntity) {
// Tags
// ----------------------------------
export async function createTag(attributes: Partial<TagEntity> = {}) {
export async function createTag(attributes: Partial<TagEntity> = {}, workflow?: WorkflowEntity) {
const { name } = attributes;
return Container.get(TagRepository).save({
const tag = await Container.get(TagRepository).save({
id: generateNanoId(),
name: name ?? randomName(),
...attributes,
});
if (workflow) {
const mappingRepository = Container.get(WorkflowTagMappingRepository);
const mapping = mappingRepository.create({ tagId: tag.id, workflowId: workflow.id });
await mappingRepository.save(mapping);
}
return tag;
}
// ----------------------------------

54
packages/cli/test/integration/workflows.controller.ee.test.ts
View File

@@ -136,60 +136,6 @@ describe('PUT /workflows/:id', () => {
});
});
describe('GET /workflows', () => {
test('should return workflows without nodes, sharing and credential usage details', async () => {
const tag = await testDb.createTag({ name: 'test' });
const savedCredential = await saveCredential(randomCredentialPayload(), { user: owner });
const workflow = await createWorkflow(
{
nodes: [
{
id: uuid(),
name: 'Action Network',
type: 'n8n-nodes-base.actionNetwork',
parameters: {},
typeVersion: 1,
position: [0, 0],
credentials: {
actionNetworkApi: {
id: savedCredential.id,
name: savedCredential.name,
},
},
},
],
tags: [tag],
},
owner,
);
await testDb.shareWorkflowWithUsers(workflow, [member]);
const response = await authOwnerAgent.get('/workflows');
const [fetchedWorkflow] = response.body.data;
expect(response.statusCode).toBe(200);
expect(fetchedWorkflow.ownedBy).toMatchObject({
id: owner.id,
});
expect(fetchedWorkflow.sharedWith).not.toBeDefined();
expect(fetchedWorkflow.usedCredentials).not.toBeDefined();
expect(fetchedWorkflow.nodes).not.toBeDefined();
expect(fetchedWorkflow.tags).toEqual(
expect.arrayContaining([
expect.objectContaining({
id: expect.any(String),
name: expect.any(String),
}),
]),
);
});
});
describe('GET /workflows/new', () => {
[true, false].forEach((sharingEnabled) => {
test(`should return an auto-incremented name, even when sharing is ${

275
packages/cli/test/integration/workflows.controller.test.ts
View File

@@ -1,24 +1,32 @@
import type { SuperAgentTest } from 'supertest';
import type { IPinData } from 'n8n-workflow';
import type { INode, IPinData } from 'n8n-workflow';
import * as UserManagementHelpers from '@/UserManagement/UserManagementHelper';
import * as utils from './shared/utils/';
import * as testDb from './shared/testDb';
import { makeWorkflow, MOCK_PINDATA } from './shared/utils/';
import type { User } from '@/databases/entities/User';
import { randomCredentialPayload } from './shared/random';
import { v4 as uuid } from 'uuid';
import { RoleService } from '@/services/role.service';
import Container from 'typedi';
import type { ListQuery } from '@/requests';
let owner: User;
let authOwnerAgent: SuperAgentTest;
jest.spyOn(UserManagementHelpers, 'isSharingEnabled').mockReturnValue(false);
const testServer = utils.setupTestServer({ endpointGroups: ['workflows'] });
const { objectContaining, arrayContaining, any } = expect;
beforeAll(async () => {
const globalOwnerRole = await testDb.getGlobalOwnerRole();
const ownerShell = await testDb.createUserShell(globalOwnerRole);
authOwnerAgent = testServer.authAgentFor(ownerShell);
owner = await testDb.createOwner();
authOwnerAgent = testServer.authAgentFor(owner);
});
beforeEach(async () => {
await testDb.truncate(['Workflow', 'SharedWorkflow']);
await testDb.truncate(['Workflow', 'SharedWorkflow', 'Tag']);
});
describe('POST /workflows', () => {
@@ -53,3 +61,260 @@ describe('GET /workflows/:id', () => {
expect(pinData).toMatchObject(MOCK_PINDATA);
});
});
describe('GET /workflows', () => {
test('should return zero workflows if none exist', async () => {
const response = await authOwnerAgent.get('/workflows').expect(200);
expect(response.body).toEqual({ count: 0, data: [] });
});
test('should return workflows', async () => {
const credential = await testDb.saveCredential(randomCredentialPayload(), {
user: owner,
role: await Container.get(RoleService).findCredentialOwnerRole(),
});
const nodes: INode[] = [
{
id: uuid(),
name: 'Action Network',
type: 'n8n-nodes-base.actionNetwork',
parameters: {},
typeVersion: 1,
position: [0, 0],
credentials: {
actionNetworkApi: {
id: credential.id,
name: credential.name,
},
},
},
];
const tag = await testDb.createTag({ name: 'A' });
await testDb.createWorkflow({ name: 'First', nodes, tags: [tag] }, owner);
await testDb.createWorkflow({ name: 'Second' }, owner);
const response = await authOwnerAgent.get('/workflows').expect(200);
expect(response.body).toEqual({
count: 2,
data: arrayContaining([
objectContaining({
id: any(String),
name: 'First',
active: any(Boolean),
createdAt: any(String),
updatedAt: any(String),
tags: [{ id: any(String), name: 'A' }],
versionId: null,
ownedBy: { id: owner.id },
}),
objectContaining({
id: any(String),
name: 'Second',
active: any(Boolean),
createdAt: any(String),
updatedAt: any(String),
tags: [],
versionId: null,
ownedBy: { id: owner.id },
}),
]),
});
const found = response.body.data.find(
(w: ListQuery.Workflow.WithOwnership) => w.name === 'First',
);
expect(found.nodes).toBeUndefined();
expect(found.sharedWith).toBeUndefined();
expect(found.usedCredentials).toBeUndefined();
});
describe('filter', () => {
test('should filter workflows by field: name', async () => {
await testDb.createWorkflow({ name: 'First' }, owner);
await testDb.createWorkflow({ name: 'Second' }, owner);
const response = await authOwnerAgent
.get('/workflows')
.query('filter={"name":"First"}')
.expect(200);
expect(response.body).toEqual({
count: 1,
data: [objectContaining({ name: 'First' })],
});
});
test('should filter workflows by field: active', async () => {
await testDb.createWorkflow({ active: true }, owner);
await testDb.createWorkflow({ active: false }, owner);
const response = await authOwnerAgent
.get('/workflows')
.query('filter={ "active": true }')
.expect(200);
expect(response.body).toEqual({
count: 1,
data: [objectContaining({ active: true })],
});
});
test('should filter workflows by field: tags', async () => {
const workflow = await testDb.createWorkflow({ name: 'First' }, owner);
await testDb.createTag({ name: 'A' }, workflow);
await testDb.createTag({ name: 'B' }, workflow);
const response = await authOwnerAgent
.get('/workflows')
.query('filter={ "tags": ["A"] }')
.expect(200);
expect(response.body).toEqual({
count: 1,
data: [objectContaining({ name: 'First', tags: [{ id: any(String), name: 'A' }] })],
});
});
});
describe('select', () => {
test('should select workflow field: name', async () => {
await testDb.createWorkflow({ name: 'First' }, owner);
await testDb.createWorkflow({ name: 'Second' }, owner);
const response = await authOwnerAgent.get('/workflows').query('select=["name"]').expect(200);
expect(response.body).toEqual({
count: 2,
data: arrayContaining([
{ id: any(String), name: 'First' },
{ id: any(String), name: 'Second' },
]),
});
});
test('should select workflow field: active', async () => {
await testDb.createWorkflow({ active: true }, owner);
await testDb.createWorkflow({ active: false }, owner);
const response = await authOwnerAgent
.get('/workflows')
.query('select=["active"]')
.expect(200);
expect(response.body).toEqual({
count: 2,
data: arrayContaining([
{ id: any(String), active: true },
{ id: any(String), active: false },
]),
});
});
test('should select workflow field: tags', async () => {
const firstWorkflow = await testDb.createWorkflow({ name: 'First' }, owner);
const secondWorkflow = await testDb.createWorkflow({ name: 'Second' }, owner);
await testDb.createTag({ name: 'A' }, firstWorkflow);
await testDb.createTag({ name: 'B' }, secondWorkflow);
const response = await authOwnerAgent.get('/workflows').query('select=["tags"]').expect(200);
expect(response.body).toEqual({
count: 2,
data: arrayContaining([
objectContaining({ id: any(String), tags: [{ id: any(String), name: 'A' }] }),
objectContaining({ id: any(String), tags: [{ id: any(String), name: 'B' }] }),
]),
});
});
test('should select workflow fields: createdAt and updatedAt', async () => {
const firstWorkflowCreatedAt = '2023-08-08T09:31:25.000Z';
const firstWorkflowUpdatedAt = '2023-08-08T09:31:40.000Z';
const secondWorkflowCreatedAt = '2023-07-07T09:31:25.000Z';
const secondWorkflowUpdatedAt = '2023-07-07T09:31:40.000Z';
await testDb.createWorkflow(
{
createdAt: new Date(firstWorkflowCreatedAt),
updatedAt: new Date(firstWorkflowUpdatedAt),
},
owner,
);
await testDb.createWorkflow(
{
createdAt: new Date(secondWorkflowCreatedAt),
updatedAt: new Date(secondWorkflowUpdatedAt),
},
owner,
);
const response = await authOwnerAgent
.get('/workflows')
.query('select=["createdAt", "updatedAt"]')
.expect(200);
expect(response.body).toEqual({
count: 2,
data: arrayContaining([
objectContaining({
id: any(String),
createdAt: firstWorkflowCreatedAt,
updatedAt: firstWorkflowUpdatedAt,
}),
objectContaining({
id: any(String),
createdAt: secondWorkflowCreatedAt,
updatedAt: secondWorkflowUpdatedAt,
}),
]),
});
});
test('should select workflow field: versionId', async () => {
const firstWorkflowVersionId = 'e95ccdde-2b4e-4fd0-8834-220a2b5b4353';
const secondWorkflowVersionId = 'd099b8dc-b1d8-4b2d-9b02-26f32c0ee785';
await testDb.createWorkflow({ versionId: firstWorkflowVersionId }, owner);
await testDb.createWorkflow({ versionId: secondWorkflowVersionId }, owner);
const response = await authOwnerAgent
.get('/workflows')
.query('select=["versionId"]')
.expect(200);
expect(response.body).toEqual({
count: 2,
data: arrayContaining([
{ id: any(String), versionId: firstWorkflowVersionId },
{ id: any(String), versionId: secondWorkflowVersionId },
]),
});
});
test('should select workflow field: ownedBy', async () => {
await testDb.createWorkflow({}, owner);
await testDb.createWorkflow({}, owner);
const response = await authOwnerAgent
.get('/workflows')
.query('select=["ownedBy"]')
.expect(200);
expect(response.body).toEqual({
count: 2,
data: arrayContaining([
{ id: any(String), ownedBy: { id: owner.id } },
{ id: any(String), ownedBy: { id: owner.id } },
]),
});
});
});
});

147
packages/cli/test/unit/middleware/listQuery.test.ts
View File

@@ -1,52 +1,89 @@
import { filterListQueryMiddleware } from '@/middlewares/listQuery/filter';
import { LoggerProxy } from 'n8n-workflow';
import { getLogger } from '@/Logger';
import type { Request, Response, NextFunction } from 'express';
import type { ListQueryRequest } from '@/requests';
import { selectListQueryMiddleware } from '@/middlewares/listQuery/select';
import { paginationListQueryMiddleware } from '@/middlewares/listQuery/pagination';
import * as ResponseHelper from '@/ResponseHelper';
import type { ListQuery } from '@/requests';
import type { Response, NextFunction } from 'express';
describe('List query middleware', () => {
let mockReq: Partial<ListQueryRequest>;
let mockRes: Partial<Response>;
let mockReq: ListQuery.Request;
let mockRes: Response;
let nextFn: NextFunction = jest.fn();
let args: [Request, Response, NextFunction];
let args: [ListQuery.Request, Response, NextFunction];
let sendErrorResponse: jest.SpyInstance;
beforeEach(() => {
LoggerProxy.init(getLogger());
mockReq = { baseUrl: '/rest/workflows' };
args = [mockReq as Request, mockRes as Response, nextFn];
jest.restoreAllMocks();
LoggerProxy.init(getLogger());
mockReq = { baseUrl: '/rest/workflows' } as ListQuery.Request;
mockRes = { status: () => ({ json: jest.fn() }) } as unknown as Response;
args = [mockReq, mockRes, nextFn];
sendErrorResponse = jest.spyOn(ResponseHelper, 'sendErrorResponse');
});
describe('Query filter', () => {
test('should parse valid filter', () => {
test('should not set filter on request if none sent', async () => {
mockReq.query = {};
await filterListQueryMiddleware(...args);
expect(mockReq.listQueryOptions).toBeUndefined();
expect(nextFn).toBeCalledTimes(1);
});
test('should parse valid filter', async () => {
mockReq.query = { filter: '{ "name": "My Workflow" }' };
filterListQueryMiddleware(...args);
await filterListQueryMiddleware(...args);
expect(mockReq.listQueryOptions).toEqual({ filter: { name: 'My Workflow' } });
expect(nextFn).toBeCalledTimes(1);
});
test('should ignore invalid filter', () => {
test('should ignore invalid filter', async () => {
mockReq.query = { filter: '{ "name": "My Workflow", "foo": "bar" }' };
filterListQueryMiddleware(...args);
await filterListQueryMiddleware(...args);
expect(mockReq.listQueryOptions).toEqual({ filter: { name: 'My Workflow' } });
expect(nextFn).toBeCalledTimes(1);
});
test('should throw on invalid JSON', () => {
test('should throw on invalid JSON', async () => {
mockReq.query = { filter: '{ "name" : "My Workflow"' };
const call = () => filterListQueryMiddleware(...args);
expect(call).toThrowError('Failed to parse filter JSON');
await filterListQueryMiddleware(...args);
expect(sendErrorResponse).toHaveBeenCalledTimes(1);
});
test('should throw on valid filter with invalid type', async () => {
mockReq.query = { filter: '{ "name" : 123 }' };
await filterListQueryMiddleware(...args);
expect(sendErrorResponse).toHaveBeenCalledTimes(1);
});
});
describe('Query select', () => {
test('should not set select on request if none sent', async () => {
mockReq.query = {};
await filterListQueryMiddleware(...args);
expect(mockReq.listQueryOptions).toBeUndefined();
expect(nextFn).toBeCalledTimes(1);
});
test('should parse valid select', () => {
mockReq.query = { select: '["name", "id"]' };
selectListQueryMiddleware(...args);
expect(mockReq.listQueryOptions).toEqual({ select: { name: true, id: true } });
@@ -55,6 +92,7 @@ describe('List query middleware', () => {
test('ignore invalid select', () => {
mockReq.query = { select: '["name", "foo"]' };
selectListQueryMiddleware(...args);
expect(mockReq.listQueryOptions).toEqual({ select: { name: true } });
@@ -63,20 +101,31 @@ describe('List query middleware', () => {
test('throw on invalid JSON', () => {
mockReq.query = { select: '["name"' };
const call = () => selectListQueryMiddleware(...args);
expect(call).toThrowError('Failed to parse select JSON');
selectListQueryMiddleware(...args);
expect(sendErrorResponse).toHaveBeenCalledTimes(1);
});
test('throw on non-string-array JSON for select', () => {
mockReq.query = { select: '"name"' };
const call = () => selectListQueryMiddleware(...args);
expect(call).toThrowError('Parsed select is not a string array');
selectListQueryMiddleware(...args);
expect(sendErrorResponse).toHaveBeenCalledTimes(1);
});
});
describe('Query pagination', () => {
test('should not set pagination options on request if none sent', async () => {
mockReq.query = {};
await filterListQueryMiddleware(...args);
expect(mockReq.listQueryOptions).toBeUndefined();
expect(nextFn).toBeCalledTimes(1);
});
test('should parse valid pagination', () => {
mockReq.query = { skip: '1', take: '2' };
paginationListQueryMiddleware(...args);
@@ -103,6 +152,7 @@ describe('List query middleware', () => {
test('should cap take at 50', () => {
mockReq.query = { take: '51' };
paginationListQueryMiddleware(...args);
expect(mockReq.listQueryOptions).toEqual({ skip: 0, take: 50 });
@@ -111,9 +161,64 @@ describe('List query middleware', () => {
test('should throw on non-numeric-integer take', () => {
mockReq.query = { take: '3.2' };
const call = () => paginationListQueryMiddleware(...args);
expect(call).toThrowError('Parameter take or skip is not an integer string');
paginationListQueryMiddleware(...args);
expect(sendErrorResponse).toHaveBeenCalledTimes(1);
});
test('should throw on non-numeric-integer skip', () => {
mockReq.query = { take: '3', skip: '3.2' };
paginationListQueryMiddleware(...args);
expect(sendErrorResponse).toHaveBeenCalledTimes(1);
});
});
describe('Combinations', () => {
test('should combine filter with select', async () => {
mockReq.query = { filter: '{ "name": "My Workflow" }', select: '["name", "id"]' };
await filterListQueryMiddleware(...args);
selectListQueryMiddleware(...args);
expect(mockReq.listQueryOptions).toEqual({
select: { name: true, id: true },
filter: { name: 'My Workflow' },
});
expect(nextFn).toBeCalledTimes(2);
});
test('should combine filter with pagination options', async () => {
mockReq.query = { filter: '{ "name": "My Workflow" }', skip: '1', take: '2' };
await filterListQueryMiddleware(...args);
paginationListQueryMiddleware(...args);
expect(mockReq.listQueryOptions).toEqual({
filter: { name: 'My Workflow' },
skip: 1,
take: 2,
});
expect(nextFn).toBeCalledTimes(2);
});
test('should combine select with pagination options', async () => {
mockReq.query = { select: '["name", "id"]', skip: '1', take: '2' };
selectListQueryMiddleware(...args);
paginationListQueryMiddleware(...args);
expect(mockReq.listQueryOptions).toEqual({
select: { name: true, id: true },
skip: 1,
take: 2,
});
expect(nextFn).toBeCalledTimes(2);
});
});
});