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

RicardoE105 · 2024-12-26T20:22:36Z

Summary

Title self explanatory

I have no clue why we have the N8N_WORKFLOW_TAGS_DISABLED env variable at all. Not sure in which scenario we want to disable tags 🤔. Removing this env variable will simplify things as we need to constantly check this value. @netroy any idea why?

Review / Merge checklist

PR title and summary are descriptive. (conventions)
Docs updated or follow-up ticket created.
Tests included.
PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

codecov · 2024-12-26T21:20:50Z

Codecov Report

Attention: Patch coverage is 74.57627% with 15 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
packages/@n8n/api-types/src/dto/index.ts	0.00%	3 Missing ⚠️
packages/editor-ui/src/stores/tags.store.ts	0.00%	3 Missing ⚠️
...8n/api-types/src/dto/tag/create-tag-request.dto.ts	0.00%	2 Missing ⚠️
...8n/api-types/src/dto/tag/update-tag-request.dto.ts	0.00%	2 Missing ⚠️
packages/cli/src/workflows/workflow.service.ts	60.00%	0 Missing and 2 partials ⚠️
packages/@n8n/config/src/index.ts	50.00%	0 Missing and 1 partial ⚠️
packages/cli/src/controllers/tags.controller.ts	95.23%	0 Missing and 1 partial ⚠️
...ckages/cli/src/workflow-execute-additional-data.ts	0.00%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

RicardoE105 · 2024-12-26T22:10:26Z

packages/cli/src/controllers/tags.controller.ts

-		if (this.config.getEnv('workflowTagsDisabled'))
-			throw new BadRequestError('Workflow tags are disabled');
+	workflowsEnabledMiddleware(_req: Request, res: Response, next: NextFunction) {
+		if (this.config.getEnv('workflowTagsDisabled')) {


Throwing an exception from a middleware returns 500 with an empty response instead of the BadRequest. Probably a bug, due to the error not handled correctly, so for now use the response object to return.

RicardoE105 · 2024-12-27T02:24:28Z

packages/@n8n/config/src/configs/tags.config.ts

+	*/
+
+	@Env('N8N_WORKFLOW_TAGS_DISABLED')
+	workflowTagsDisabled: boolean = false;


Maybe we call the property disabled so we can do tags.disabled. Or move this to the workflow configuration instead? 🤔

yeah. let's do that.

netroy

couple of minor comments, but looks good so far. 🙇🏽

netroy · 2024-12-27T14:27:53Z

packages/cli/src/controllers/tags.controller.ts

@@ -1,54 +1,72 @@
+import { UpdateTagRequestDto, CreateTagRequestDto } from '@n8n/api-types';
+import { RetrieveTagQueryDto } from '@n8n/api-types/src/dto/tag/retrieve-tag-query.dto';


there is no /src/ in released code. This will break.

ahh wondering why the import added the source 🤔

netroy · 2024-12-27T14:28:03Z

packages/@n8n/config/src/configs/tags.config.ts

+	*/
+
+	@Env('N8N_WORKFLOW_TAGS_DISABLED')
+	workflowTagsDisabled: boolean = false;


yeah. let's do that.

netroy · 2024-12-27T14:29:18Z

packages/@n8n/api-types/src/dto/tag/base-tag-request.dto.ts

+import { z } from 'zod';
+import { Z } from 'zod-class';
+
+export class BaseTagRequestDto extends Z.class({


I'm wondering if we really need to create two separate DTOs here. Can we instead have a CreateOrDeleteTagRequestDto or UpsertTagRequestDto ?

netroy · 2024-12-27T14:31:32Z

packages/cli/src/controllers/tags.controller.ts

-		if (this.config.getEnv('workflowTagsDisabled'))
-			throw new BadRequestError('Workflow tags are disabled');
+	workflowsEnabledMiddleware(_req: Request, res: Response, next: NextFunction) {
+		if (this.globalConfig.tags.workflowTagsDisabled) {


I've been wondering about this code. The logic to return 400 comes from a long time ago, and I don't know if we ever even discussed this.
Perhaps a better solution here would be delete this middleware, and load this controller conditionally, like we do for MFAController.

netroy · 2024-12-27T14:32:22Z

packages/cli/src/controllers/tags.controller.ts

 		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) {


once we have migrated all controllers, we can remove these implicit parameters 🤞🏽

netroy · 2024-12-27T14:32:59Z

packages/cli/src/databases/repositories/workflow.repository.ts

@@ -132,7 +131,7 @@ export class WorkflowRepository extends Repository<WorkflowEntity> {

 		const relations: string[] = [];

-		const areTagsEnabled = !config.getEnv('workflowTagsDisabled');
+		const areTagsEnabled = !Container.get(GlobalConfig).tags.workflowTagsDisabled;


Suggested change

const areTagsEnabled = !Container.get(GlobalConfig).tags.workflowTagsDisabled;

const areTagsEnabled = !this.GlobalConfig.tags.workflowTagsDisabled;

netroy · 2024-12-27T14:33:10Z

packages/cli/src/databases/repositories/workflow.repository.ts

@@ -10,9 +10,8 @@ import {
 	type FindManyOptions,
 	type FindOptionsRelations,
 } from '@n8n/typeorm';
-import { Service } from 'typedi';
+import Container, { Service } from 'typedi';


Suggested change

import Container, { Service } from 'typedi';

import { Service } from 'typedi';

netroy · 2024-12-27T14:34:50Z

packages/cli/src/workflows/workflow.service.ts

@@ -202,15 +203,17 @@ export class WorkflowService {
 			]),
 		);

-		if (tagIds && !config.getEnv('workflowTagsDisabled')) {
+		const tagsDisabled = Container.get(GlobalConfig).tags.workflowTagsDisabled;


we should add GlobalConfig to the constructor, and replace this with

Suggested change

const tagsDisabled = Container.get(GlobalConfig).tags.workflowTagsDisabled;

const tagsDisabled = this.globalConfig.tags.workflowTagsDisabled;

netroy · 2024-12-27T14:35:37Z

packages/cli/test/integration/security-audit/instance-risk-reporter.test.ts

@@ -240,6 +240,8 @@ test('should not report outdated instance when up to date', async () => {

 test('should report security settings', async () => {
 	Container.get(GlobalConfig).diagnostics.enabled = true;
+	Container.get(GlobalConfig).publicApi.disabled = true;


this seems like an unrelated change. what's the context here?

netroy · 2024-12-27T14:37:05Z

packages/@n8n/api-types/src/dto/tag/retrieve-tag-query.dto.ts

+const StringBooleanEnum = z.enum(['true', 'false']).optional();
+
+export class RetrieveTagQueryDto extends Z.class({
+	withUsageCount: StringBooleanEnum,


we should update this to use booleanFromString after #12220 is merged.

RicardoE105 added 2 commits December 26, 2024 15:22

refactor(core): Update tag endpoints to use DTOs

2d8fe46

fix tests

90d46f6

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Dec 26, 2024

RicardoE105 added 2 commits December 26, 2024 15:53

Apply decorators in proper order

ee01f6e

fix typing issue

5b1363a

add tests

41c9eb2

RicardoE105 requested a review from netroy December 26, 2024 22:09

RicardoE105 commented Dec 26, 2024

View reviewed changes

RicardoE105 added 2 commits December 26, 2024 17:37

remove unused library

90b9257

add N8N_WORKFLOW_TAGS_DISABLED to global config

6da1e63

RicardoE105 changed the title ~~refactor(core): Update tag endpoints to use DTOs~~ refactor(core): Update tag endpoints to use DTOs and injectable config Dec 26, 2024

RicardoE105 requested review from netroy and removed request for netroy December 26, 2024 22:39

RicardoE105 added 4 commits December 26, 2024 17:44

fix config.test.ts

0d04484

fix tests

86e1be4

fix test

286895e

refactor test

e740135

RicardoE105 commented Dec 27, 2024

View reviewed changes

netroy requested changes Dec 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

RicardoE105 commented Dec 26, 2024 •

edited

Loading

codecov bot commented Dec 26, 2024 •

edited

Loading

RicardoE105 Dec 26, 2024 •

edited

Loading

RicardoE105 Dec 27, 2024

netroy Dec 27, 2024

netroy left a comment

netroy Dec 27, 2024

RicardoE105 Dec 27, 2024

RicardoE105 Dec 27, 2024

netroy Dec 27, 2024

netroy Dec 27, 2024

netroy Dec 27, 2024

netroy Dec 27, 2024

netroy Dec 27, 2024

netroy Dec 27, 2024

netroy Dec 27, 2024

netroy Dec 27, 2024

netroy Dec 27, 2024

		@@ -1,54 +1,72 @@
		import { UpdateTagRequestDto, CreateTagRequestDto } from '@n8n/api-types';
		import { RetrieveTagQueryDto } from '@n8n/api-types/src/dto/tag/retrieve-tag-query.dto';

	const areTagsEnabled = !Container.get(GlobalConfig).tags.workflowTagsDisabled;
	const areTagsEnabled = !this.GlobalConfig.tags.workflowTagsDisabled;

	import Container, { Service } from 'typedi';
	import { Service } from 'typedi';

	const tagsDisabled = Container.get(GlobalConfig).tags.workflowTagsDisabled;
	const tagsDisabled = this.globalConfig.tags.workflowTagsDisabled;

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

Are you sure you want to change the base?

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

Conversation

RicardoE105 commented Dec 26, 2024 • edited Loading

Summary

Review / Merge checklist

codecov bot commented Dec 26, 2024 • edited Loading

Codecov Report

RicardoE105 Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RicardoE105 commented Dec 26, 2024 •

edited

Loading

codecov bot commented Dec 26, 2024 •

edited

Loading

RicardoE105 Dec 26, 2024 •

edited

Loading