fix(core): Make boolean config value parsing backward-compatible (#10560

)
n8n-io · Aug 27, 2024 · 70b410f · 70b410f
1 parent 4e15007
commit 70b410f
Show file tree

Hide file tree

Showing 3 changed files with 75 additions and 34 deletions.
diff --git 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<T = unknown> = 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<Class, Map<PropertyKey, PropertyMetadata>>();
 
+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<PropertyKey, unknown>)();
@@ -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<PropertyKey, PropertyMetadata>();
-	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<PropertyKey, PropertyMetadata>();
-		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
@@ -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 = <T>(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 = <T>(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
@@ -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',
+		);
+	});
+});