From 70b410f4b00dd599fcd4249aa105098aa262da66 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:51:26 +0200 Subject: [PATCH] fix(core): Make boolean config value parsing backward-compatible (#10560) --- packages/@n8n/config/src/decorators.ts | 63 +++++++++++--------- packages/@n8n/config/test/config.test.ts | 25 ++++++-- packages/@n8n/config/test/decorators.test.ts | 21 +++++++ 3 files changed, 75 insertions(+), 34 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 c5549a674fcc5..cafdf3fcd4ff9 100644 --- a/packages/@n8n/config/src/decorators.ts +++ b/packages/@n8n/config/src/decorators.ts @@ -6,13 +6,24 @@ import { Container, Service } from 'typedi'; type Class = Function; type Constructable = new (rawValue: string) => T; type PropertyKey = string | symbol; +type PropertyType = number | boolean | string | Class; interface PropertyMetadata { - type: unknown; + type: PropertyType; envName?: string; } const globalMetadata = new Map>(); +const readEnv = (envName: string) => { + 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`]; + 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,38 +37,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)) { - // TODO: add a warning - value = undefined; + const parsed = Number(value); + if (isNaN(parsed)) { + console.warn(`Invalid number value for ${envName}: ${value}`); + } else { + config[key] = parsed; } } else if (type === Boolean) { - if (value !== 'true' && value !== 'false') { - // TODO: add a warning - value = undefined; + if (['true', '1'].includes(value.toLowerCase())) { + config[key] = true; + } else if (['false', '0'].includes(value.toLowerCase())) { + config[key] = false; } else { - value = value === 'true'; + console.warn(`Invalid boolean value for ${envName}: ${value}`); } - } 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); } } } @@ -70,7 +71,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); }; @@ -81,7 +82,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/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, diff --git a/packages/@n8n/config/test/decorators.test.ts b/packages/@n8n/config/test/decorators.test.ts new file mode 100644 index 0000000000000..8267399a03dbb --- /dev/null +++ b/packages/@n8n/config/test/decorators.test.ts @@ -0,0 +1,21 @@ +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\n Please use explicit typing on all config fields', + ); + }); +});