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.

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

refactor(core): Update tag endpoints to use DTOs and injectable config #12380

Merged
merged 22 commits into from
Jan 9, 2025
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
4 changes: 4 additions & 0 deletions packages/@n8n/api-types/src/dto/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,7 @@ export { UserUpdateRequestDto } from './user/user-update-request.dto';
export { CommunityRegisteredRequestDto } from './license/community-registered-request.dto';

export { VariableListRequestDto } from './variables/variables-list-request.dto';

export { CreateTagRequestDto } from './tag/create-tag-request.dto';
export { UpdateTagRequestDto } from './tag/update-tag-request.dto';
export { RetrieveTagQueryDto } from './tag/retrieve-tag-query.dto';
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { BaseTagRequestDto } from '../base-tag-request.dto';

describe('BaseTagRequestDto', () => {
describe('Valid requests', () => {
test.each([
{
name: 'valid name',
request: {
name: 'tag-name',
},
},
])('should validate $name', ({ request }) => {
const result = BaseTagRequestDto.safeParse(request);
expect(result.success).toBe(true);
});
});

describe('Invalid requests', () => {
test.each([
{
name: 'empty tag name',
request: {
name: '',
},
expectedErrorPath: ['name'],
},
])('should fail validation for $name', ({ request, expectedErrorPath }) => {
const result = BaseTagRequestDto.safeParse(request);

expect(result.success).toBe(false);

if (expectedErrorPath) {
expect(result.error?.issues[0].path).toEqual(expectedErrorPath);
}
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import { RetrieveTagQueryDto } from '../retrieve-tag-query.dto';

describe('RetrieveTagQueryDto', () => {
describe('Valid requests', () => {
test.each([
{
name: 'with "true"',
request: {
withUsageCount: 'true',
},
},
{
name: 'with "false"',
request: {
withUsageCount: 'false',
},
},
])('should pass validation for withUsageCount $name', ({ request }) => {
const result = RetrieveTagQueryDto.safeParse(request);
expect(result.success).toBe(true);
});
});

describe('Invalid requests', () => {
test.each([
{
name: 'with number',
request: {
withUsageCount: 1,
},
expectedErrorPath: ['withUsageCount'],
},
{
name: 'with boolean (true) ',
request: {
withUsageCount: true,
},
expectedErrorPath: ['withUsageCount'],
},
{
name: 'with boolean (false)',
request: {
withUsageCount: false,
},
expectedErrorPath: ['withUsageCount'],
},
{
name: 'with invalid string',
request: {
withUsageCount: 'invalid',
},
expectedErrorPath: ['withUsageCount'],
},
])('should fail validation for withUsageCount $name', ({ request, expectedErrorPath }) => {
const result = RetrieveTagQueryDto.safeParse(request);

expect(result.success).toBe(false);

if (expectedErrorPath) {
expect(result.error?.issues[0].path).toEqual(expectedErrorPath);
}
});
});
});
6 changes: 6 additions & 0 deletions packages/@n8n/api-types/src/dto/tag/base-tag-request.dto.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { z } from 'zod';
import { Z } from 'zod-class';

export class BaseTagRequestDto extends Z.class({
RicardoE105 marked this conversation as resolved.
Show resolved Hide resolved
name: z.string().trim().min(1),
}) {}
3 changes: 3 additions & 0 deletions packages/@n8n/api-types/src/dto/tag/create-tag-request.dto.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import { BaseTagRequestDto } from './base-tag-request.dto';

export class CreateTagRequestDto extends BaseTagRequestDto.extend({}) {}
8 changes: 8 additions & 0 deletions packages/@n8n/api-types/src/dto/tag/retrieve-tag-query.dto.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { z } from 'zod';
import { Z } from 'zod-class';

const StringBooleanEnum = z.enum(['true', 'false']).optional();

export class RetrieveTagQueryDto extends Z.class({
withUsageCount: StringBooleanEnum,
RicardoE105 marked this conversation as resolved.
Show resolved Hide resolved
}) {}
3 changes: 3 additions & 0 deletions packages/@n8n/api-types/src/dto/tag/update-tag-request.dto.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import { BaseTagRequestDto } from './base-tag-request.dto';

export class UpdateTagRequestDto extends BaseTagRequestDto.extend({}) {}
50 changes: 34 additions & 16 deletions packages/cli/src/controllers/tags.controller.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,22 @@
import { UpdateTagRequestDto, CreateTagRequestDto } from '@n8n/api-types';
import { RetrieveTagQueryDto } from '@n8n/api-types/src/dto/tag/retrieve-tag-query.dto';
RicardoE105 marked this conversation as resolved.
Show resolved Hide resolved
import { Request, Response, NextFunction } from 'express';

import config from '@/config';
import { Delete, Get, Middleware, Patch, Post, RestController, GlobalScope } from '@/decorators';
import {
Delete,
Get,
Middleware,
Patch,
Post,
RestController,
GlobalScope,
Body,
Param,
Query,
} from '@/decorators';
import { BadRequestError } from '@/errors/response-errors/bad-request.error';
import { TagsRequest } from '@/requests';
import { AuthenticatedRequest } from '@/requests';
import { TagService } from '@/services/tag.service';

@RestController('/tags')
Expand All @@ -14,41 +27,46 @@ export class TagsController {

// TODO: move this into a new decorator `@IfEnabled('workflowTagsDisabled')`
@Middleware()
workflowsEnabledMiddleware(_req: Request, _res: Response, next: NextFunction) {
if (this.config.getEnv('workflowTagsDisabled'))
throw new BadRequestError('Workflow tags are disabled');
workflowsEnabledMiddleware(_req: Request, res: Response, next: NextFunction) {
if (this.config.getEnv('workflowTagsDisabled')) {
RicardoE105 marked this conversation as resolved.
Show resolved Hide resolved
res.status(400).json({ message: 'Workflow tags are disabled' });
return;
}
next();
}

@Get('/')
@GlobalScope('tag:list')
async getAll(req: TagsRequest.GetAll) {
return await this.tagService.getAll({ withUsageCount: req.query.withUsageCount === 'true' });
async getAll(_req: AuthenticatedRequest, _res: Response, @Query query: RetrieveTagQueryDto) {
RicardoE105 marked this conversation as resolved.
Show resolved Hide resolved
return await this.tagService.getAll({ withUsageCount: query.withUsageCount === 'true' });
}

@Post('/')
@GlobalScope('tag:create')
async createTag(req: TagsRequest.Create) {
const tag = this.tagService.toEntity({ name: req.body.name });
async createTag(_req: AuthenticatedRequest, _res: Response, @Body payload: CreateTagRequestDto) {
const { name } = payload;
const tag = this.tagService.toEntity({ name });

return await this.tagService.save(tag, 'create');
}

@Patch('/:id(\\w+)')
@GlobalScope('tag:update')
async updateTag(req: TagsRequest.Update) {
const newTag = this.tagService.toEntity({ id: req.params.id, name: req.body.name.trim() });
async updateTag(
_req: AuthenticatedRequest,
_res: Response,
@Param('id') tagId: string,
@Body payload: UpdateTagRequestDto,
) {
const newTag = this.tagService.toEntity({ id: tagId, name: payload.name.trim() });

return await this.tagService.save(newTag, 'update');
}

@Delete('/:id(\\w+)')
@GlobalScope('tag:delete')
async deleteTag(req: TagsRequest.Delete) {
const { id } = req.params;

await this.tagService.delete(id);

async deleteTag(_req: AuthenticatedRequest, _res: Response, @Param('id') tagId: string) {
await this.tagService.delete(tagId);
return true;
}
}
11 changes: 0 additions & 11 deletions packages/cli/src/requests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -308,17 +308,6 @@ export declare namespace DynamicNodeParametersRequest {
}>;
}

// ----------------------------------
// /tags
// ----------------------------------

export declare namespace TagsRequest {
type GetAll = AuthenticatedRequest<{}, {}, {}, { withUsageCount: string }>;
type Create = AuthenticatedRequest<{}, {}, { name: string }>;
type Update = AuthenticatedRequest<{ id: string }, {}, { name: string }>;
type Delete = AuthenticatedRequest<{ id: string }>;
}

// ----------------------------------
// /annotation-tags
// ----------------------------------
Expand Down
64 changes: 63 additions & 1 deletion packages/cli/test/integration/tags.api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { createUserShell } from './shared/db/users';
import * as testDb from './shared/test-db';
import type { SuperAgentTest } from './shared/types';
import * as utils from './shared/utils/';
import config from '@/config';

let authOwnerAgent: SuperAgentTest;
const testServer = utils.setupTestServer({ endpointGroups: ['tags'] });
Expand All @@ -22,8 +23,8 @@ beforeEach(async () => {
describe('POST /tags', () => {
test('should create tag', async () => {
const resp = await authOwnerAgent.post('/tags').send({ name: 'test' });
expect(resp.statusCode).toBe(200);

expect(resp.statusCode).toBe(200);
const dbTag = await Container.get(TagRepository).findBy({ name: 'test' });
expect(dbTag.length === 1);
});
Expand All @@ -38,4 +39,65 @@ describe('POST /tags', () => {
const dbTag = await Container.get(TagRepository).findBy({ name: 'test' });
expect(dbTag.length).toBe(1);
});

test('should delete tag', async () => {
const newTag = Container.get(TagRepository).create({ name: 'test' });
await Container.get(TagRepository).save(newTag);

const resp = await authOwnerAgent.delete(`/tags/${newTag.id}`);
expect(resp.status).toBe(200);

const dbTag = await Container.get(TagRepository).findBy({ name: 'test' });
expect(dbTag.length).toBe(0);
});

test('should update tag name', async () => {
const newTag = Container.get(TagRepository).create({ name: 'test' });
await Container.get(TagRepository).save(newTag);

const resp = await authOwnerAgent.patch(`/tags/${newTag.id}`).send({ name: 'updated' });
expect(resp.status).toBe(200);

const dbTag = await Container.get(TagRepository).findBy({ name: 'updated' });
expect(dbTag.length).toBe(1);
});

test('should retrieve all tags', async () => {
const newTag = Container.get(TagRepository).create({ name: 'test' });
const savedTag = await Container.get(TagRepository).save(newTag);

const resp = await authOwnerAgent.get('/tags');
expect(resp.status).toBe(200);

expect(resp.body.data.length).toBe(1);
expect(resp.body.data[0]).toMatchObject({
id: savedTag.id,
name: savedTag.name,
createdAt: savedTag.createdAt.toISOString(),
updatedAt: savedTag.updatedAt.toISOString(),
});
});

test('should retrieve all tags with with usage count', async () => {
const newTag = Container.get(TagRepository).create({ name: 'test' });
const savedTag = await Container.get(TagRepository).save(newTag);

const resp = await authOwnerAgent.get('/tags').query({ withUsageCount: 'true' });
expect(resp.status).toBe(200);

expect(resp.body.data.length).toBe(1);
expect(resp.body.data[0]).toMatchObject({
id: savedTag.id,
name: savedTag.name,
createdAt: savedTag.createdAt.toISOString(),
updatedAt: savedTag.updatedAt.toISOString(),
usageCount: 0,
});
});

test('should throw error if workflowTagsDisabled is true', async () => {
config.set('workflowTagsDisabled', true);
const resp = await authOwnerAgent.post('/tags').send({ name: 'new tag' });
expect(resp.status).toBe(400);
});
});
9 changes: 5 additions & 4 deletions packages/editor-ui/src/api/tags.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
import type { IRestApiContext, ITag } from '@/Interface';
import { makeRestApiRequest } from '@/utils/apiUtils';
import type { CreateTagRequestDto, RetrieveTagQueryDto, UpdateTagRequestDto } from '@n8n/api-types';

type TagsApiEndpoint = '/tags' | '/annotation-tags';

export interface ITagsApi {
getTags: (context: IRestApiContext, withUsageCount?: boolean) => Promise<ITag[]>;
createTag: (context: IRestApiContext, params: { name: string }) => Promise<ITag>;
updateTag: (context: IRestApiContext, id: string, params: { name: string }) => Promise<ITag>;
getTags: (context: IRestApiContext, params: RetrieveTagQueryDto) => Promise<ITag[]>;
createTag: (context: IRestApiContext, params: CreateTagRequestDto) => Promise<ITag>;
updateTag: (context: IRestApiContext, id: string, params: UpdateTagRequestDto) => Promise<ITag>;
deleteTag: (context: IRestApiContext, id: string) => Promise<boolean>;
}

export function createTagsApi(endpoint: TagsApiEndpoint): ITagsApi {
return {
getTags: async (context: IRestApiContext, withUsageCount = false): Promise<ITag[]> => {
getTags: async (context: IRestApiContext, { withUsageCount = 'false' }): Promise<ITag[]> => {
return await makeRestApiRequest(context, 'GET', endpoint, { withUsageCount });
},
createTag: async (context: IRestApiContext, params: { name: string }): Promise<ITag> => {
Expand Down
7 changes: 3 additions & 4 deletions packages/editor-ui/src/stores/tags.store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,9 @@ const createTagsStore = (id: STORES.TAGS | STORES.ANNOTATION_TAGS) => {
}

loading.value = true;
const retrievedTags = await tagsApi.getTags(
rootStore.restApiContext,
Boolean(withUsageCount),
);
const retrievedTags = await tagsApi.getTags(rootStore.restApiContext, {
withUsageCount: withUsageCount ? 'true' : 'false',
});
setAllTags(retrievedTags);
loading.value = false;
return retrievedTags;
Expand Down
Loading