From 6c558bbef51977fcd145a23220be1dd16fd68325 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Mon, 26 Aug 2024 18:20:22 +0200 Subject: [PATCH 1/6] fix(core): Make boolean config value parsing backward-compatible --- packages/@n8n/config/src/decorators.ts | 14 ++++++++----- packages/@n8n/config/test/config.test.ts | 25 ++++++++++++++++++------ 2 files changed, 28 insertions(+), 11 deletions(-) diff --git a/packages/@n8n/config/src/decorators.ts b/packages/@n8n/config/src/decorators.ts index c5549a674fcc5..783d6aae9fdbe 100644 --- a/packages/@n8n/config/src/decorators.ts +++ b/packages/@n8n/config/src/decorators.ts @@ -34,18 +34,22 @@ export const Config: ClassDecorator = (ConfigClass: Class) => { value = readFileSync(filePath, 'utf8'); } + if (value === undefined) continue; + if (type === Number) { value = Number(value); if (isNaN(value as number)) { - // TODO: add a warning + console.warn(`Invalid number value for ${envName}: ${value}`); value = undefined; } } else if (type === Boolean) { - if (value !== 'true' && value !== 'false') { - // TODO: add a warning - value = undefined; + if (['true', 'TRUE', '1'].includes(value as string)) { + value = true; + } else if (['false', 'FALSE', '0'].includes(value as string)) { + value = false; } else { - value = value === 'true'; + console.warn(`Invalid boolean value for ${envName}: ${value}`); + value = undefined; } } else if (type === Object) { // eslint-disable-next-line n8n-local-rules/no-plain-errors diff --git a/packages/@n8n/config/test/config.test.ts b/packages/@n8n/config/test/config.test.ts index eb7c077a0a08f..301b99ca567ba 100644 --- a/packages/@n8n/config/test/config.test.ts +++ b/packages/@n8n/config/test/config.test.ts @@ -17,6 +17,10 @@ describe('GlobalConfig', () => { process.env = originalEnv; }); + // deepCopy for diff to show plain objects + // eslint-disable-next-line n8n-local-rules/no-json-parse-json-stringify + const deepCopy = (obj: T): T => JSON.parse(JSON.stringify(obj)); + const defaultConfig: GlobalConfig = { path: '/', host: 'localhost', @@ -218,10 +222,6 @@ describe('GlobalConfig', () => { process.env = {}; const config = Container.get(GlobalConfig); - // deepCopy for diff to show plain objects - // eslint-disable-next-line n8n-local-rules/no-json-parse-json-stringify - const deepCopy = (obj: T): T => JSON.parse(JSON.stringify(obj)); - expect(deepCopy(config)).toEqual(defaultConfig); expect(mockFs.readFileSync).not.toHaveBeenCalled(); }); @@ -233,9 +233,11 @@ describe('GlobalConfig', () => { DB_TABLE_PREFIX: 'test_', NODES_INCLUDE: '["n8n-nodes-base.hackerNews"]', DB_LOGGING_MAX_EXECUTION_TIME: '0', + N8N_METRICS: 'TRUE', + N8N_TEMPLATES_ENABLED: '0', }; const config = Container.get(GlobalConfig); - expect(config).toEqual({ + expect(deepCopy(config)).toEqual({ ...defaultConfig, database: { logging: defaultConfig.database.logging, @@ -249,10 +251,21 @@ describe('GlobalConfig', () => { tablePrefix: 'test_', type: 'sqlite', }, + endpoints: { + ...defaultConfig.endpoints, + metrics: { + ...defaultConfig.endpoints.metrics, + enable: true, + }, + }, nodes: { ...defaultConfig.nodes, include: ['n8n-nodes-base.hackerNews'], }, + templates: { + ...defaultConfig.templates, + enabled: false, + }, }); expect(mockFs.readFileSync).not.toHaveBeenCalled(); }); @@ -265,7 +278,7 @@ describe('GlobalConfig', () => { mockFs.readFileSync.calledWith(passwordFile, 'utf8').mockReturnValueOnce('password-from-file'); const config = Container.get(GlobalConfig); - expect(config).toEqual({ + expect(deepCopy(config)).toEqual({ ...defaultConfig, database: { ...defaultConfig.database, From 8769ee4745ebe7868aa8a4f4ba374ff9d713c021 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Mon, 26 Aug 2024 19:00:14 +0200 Subject: [PATCH 2/6] improve type-safety, and add additional tests --- packages/@n8n/config/src/decorators.ts | 62 +++++++++++--------- packages/@n8n/config/test/decorators.test.ts | 19 ++++++ 2 files changed, 52 insertions(+), 29 deletions(-) create mode 100644 packages/@n8n/config/test/decorators.test.ts diff --git a/packages/@n8n/config/src/decorators.ts b/packages/@n8n/config/src/decorators.ts index 783d6aae9fdbe..4f7e30b97f92b 100644 --- a/packages/@n8n/config/src/decorators.ts +++ b/packages/@n8n/config/src/decorators.ts @@ -6,13 +6,25 @@ import { Container, Service } from 'typedi'; type Class = Function; type Constructable = new (rawValue: string) => T; type PropertyKey = string | symbol; +type PropertyType = typeof Number | typeof Boolean | typeof String | Class; interface PropertyMetadata { - type: unknown; + type: PropertyType; envName?: string; } const globalMetadata = new Map>(); +const readEnv = (envName: string) => { + const value = process.env[envName]; + if (value) return value; + + // Read the value from a file, if "_FILE" environment variable is defined + const filePath = process.env[`${envName}_FILE`]; + if (filePath) return readFileSync(filePath, 'utf8'); + + return undefined; +}; + export const Config: ClassDecorator = (ConfigClass: Class) => { const factory = function () { const config = new (ConfigClass as new () => Record)(); @@ -26,42 +38,28 @@ export const Config: ClassDecorator = (ConfigClass: Class) => { if (typeof type === 'function' && globalMetadata.has(type)) { config[key] = Container.get(type); } else if (envName) { - let value: unknown = process.env[envName]; - - // Read the value from a file, if "_FILE" environment variable is defined - const filePath = process.env[`${envName}_FILE`]; - if (filePath) { - value = readFileSync(filePath, 'utf8'); - } - + const value = readEnv(envName); if (value === undefined) continue; if (type === Number) { - value = Number(value); - if (isNaN(value as number)) { + const parsed = Number(value); + if (isNaN(parsed)) { console.warn(`Invalid number value for ${envName}: ${value}`); - value = undefined; + } else { + config[key] = parsed; } } else if (type === Boolean) { - if (['true', 'TRUE', '1'].includes(value as string)) { - value = true; - } else if (['false', 'FALSE', '0'].includes(value as string)) { - value = false; + if (['true', 'TRUE', '1'].includes(value)) { + config[key] = true; + } else if (['false', 'FALSE', '0'].includes(value)) { + config[key] = false; } else { console.warn(`Invalid boolean value for ${envName}: ${value}`); - value = undefined; } - } else if (type === Object) { - // eslint-disable-next-line n8n-local-rules/no-plain-errors - throw new Error( - `Invalid decorator metadata on key "${key as string}" on ${ConfigClass.name}\n Please use explicit typing on all config fields`, - ); - } else if (type !== String && type !== Object) { - value = new (type as Constructable)(value as string); - } - - if (value !== undefined) { + } else if (type === String) { config[key] = value; + } else { + config[key] = new (type as Constructable)(value); } } } @@ -74,7 +72,7 @@ export const Config: ClassDecorator = (ConfigClass: Class) => { export const Nested: PropertyDecorator = (target: object, key: PropertyKey) => { const ConfigClass = target.constructor; const classMetadata = globalMetadata.get(ConfigClass) ?? new Map(); - const type = Reflect.getMetadata('design:type', target, key) as unknown; + const type = Reflect.getMetadata('design:type', target, key) as PropertyType; classMetadata.set(key, { type }); globalMetadata.set(ConfigClass, classMetadata); }; @@ -85,7 +83,13 @@ export const Env = const ConfigClass = target.constructor; const classMetadata = globalMetadata.get(ConfigClass) ?? new Map(); - const type = Reflect.getMetadata('design:type', target, key) as unknown; + const type = Reflect.getMetadata('design:type', target, key) as PropertyType; + if (type === Object) { + // eslint-disable-next-line n8n-local-rules/no-plain-errors + throw new Error( + `Invalid decorator metadata on key "${key as string}" on ${ConfigClass.name}\n Please use explicit typing on all config fields`, + ); + } classMetadata.set(key, { type, envName }); globalMetadata.set(ConfigClass, classMetadata); }; diff --git a/packages/@n8n/config/test/decorators.test.ts b/packages/@n8n/config/test/decorators.test.ts new file mode 100644 index 0000000000000..4cb81eaa53831 --- /dev/null +++ b/packages/@n8n/config/test/decorators.test.ts @@ -0,0 +1,19 @@ +import { Container } from 'typedi'; +import { Config, Env } from '../src/decorators'; + +describe('decorators', () => { + beforeEach(() => { + Container.reset(); + }); + + it('should throw when explicit typing is missing', () => { + expect(() => { + @Config + class InvalidConfig { + @Env('STRING_VALUE') + value = 'string'; + } + Container.get(InvalidConfig); + }).toThrowError('Invalid decorator metadata on key "value" on InvalidConfig'); + }); +}); From 709a4345b4d2708f473a8630826c9ebab3b0235a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Tue, 27 Aug 2024 13:20:35 +0200 Subject: [PATCH 3/6] use primitive types --- packages/@n8n/config/src/decorators.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@n8n/config/src/decorators.ts b/packages/@n8n/config/src/decorators.ts index 4f7e30b97f92b..44a2e747c097a 100644 --- a/packages/@n8n/config/src/decorators.ts +++ b/packages/@n8n/config/src/decorators.ts @@ -6,7 +6,7 @@ import { Container, Service } from 'typedi'; type Class = Function; type Constructable = new (rawValue: string) => T; type PropertyKey = string | symbol; -type PropertyType = typeof Number | typeof Boolean | typeof String | Class; +type PropertyType = number | boolean | string | Class; interface PropertyMetadata { type: PropertyType; envName?: string; From 09bfd3be0162fc28f50d103cedaffdb9e97de7cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Tue, 27 Aug 2024 13:22:35 +0200 Subject: [PATCH 4/6] make boolean configs case-insensitive --- packages/@n8n/config/src/decorators.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@n8n/config/src/decorators.ts b/packages/@n8n/config/src/decorators.ts index 44a2e747c097a..a5743ff22e878 100644 --- a/packages/@n8n/config/src/decorators.ts +++ b/packages/@n8n/config/src/decorators.ts @@ -49,9 +49,9 @@ export const Config: ClassDecorator = (ConfigClass: Class) => { config[key] = parsed; } } else if (type === Boolean) { - if (['true', 'TRUE', '1'].includes(value)) { + if (['true', '1'].includes(value.toLowerCase())) { config[key] = true; - } else if (['false', 'FALSE', '0'].includes(value)) { + } else if (['false', '0'].includes(value.toLowerCase())) { config[key] = false; } else { console.warn(`Invalid boolean value for ${envName}: ${value}`); From 691a8d99c631db10b364f011dc33c93c4290e36c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Tue, 27 Aug 2024 13:23:39 +0200 Subject: [PATCH 5/6] assert the full error message in tests --- packages/@n8n/config/test/decorators.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/@n8n/config/test/decorators.test.ts b/packages/@n8n/config/test/decorators.test.ts index 4cb81eaa53831..8267399a03dbb 100644 --- a/packages/@n8n/config/test/decorators.test.ts +++ b/packages/@n8n/config/test/decorators.test.ts @@ -14,6 +14,8 @@ describe('decorators', () => { value = 'string'; } Container.get(InvalidConfig); - }).toThrowError('Invalid decorator metadata on key "value" on InvalidConfig'); + }).toThrowError( + 'Invalid decorator metadata on key "value" on InvalidConfig\n Please use explicit typing on all config fields', + ); }); }); From a8eceecfeb6a2d38bfe86a114b945f3cfe8f19d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Tue, 27 Aug 2024 13:27:45 +0200 Subject: [PATCH 6/6] backward compatible string configs when the value is an empty string --- packages/@n8n/config/src/decorators.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/@n8n/config/src/decorators.ts b/packages/@n8n/config/src/decorators.ts index a5743ff22e878..cafdf3fcd4ff9 100644 --- a/packages/@n8n/config/src/decorators.ts +++ b/packages/@n8n/config/src/decorators.ts @@ -15,8 +15,7 @@ interface PropertyMetadata { const globalMetadata = new Map>(); const readEnv = (envName: string) => { - const value = process.env[envName]; - if (value) return value; + if (envName in process.env) return process.env[envName]; // Read the value from a file, if "_FILE" environment variable is defined const filePath = process.env[`${envName}_FILE`];