refactor(core): Add support for memoized evaluation on getters (no-changelog) (#12185)

Co-authored-by: Tomi Turtiainen <10324676+tomi@users.noreply.github.com>
This commit is contained in:
कारतोफ्फेलस्क्रिप्ट™ 2024-12-13 12:52:36 +01:00 committed by GitHub
parent 70b0d81604
commit 785549cbec
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 325 additions and 77 deletions

View file

@ -5,7 +5,6 @@ import { mkdir } from 'fs/promises';
import uniq from 'lodash/uniq';
import { InstanceSettings } from 'n8n-core';
import type { ICredentialType, INodeTypeBaseDescription } from 'n8n-workflow';
import fs from 'node:fs';
import path from 'path';
import { Container, Service } from 'typedi';
@ -83,7 +82,7 @@ export class FrontendService {
this.settings = {
inE2ETests,
isDocker: this.isDocker(),
isDocker: this.instanceSettings.isDocker,
databaseType: this.globalConfig.database.type,
previewMode: process.env.N8N_PREVIEW_MODE === 'true',
endpointForm: this.globalConfig.endpoints.form,
@ -392,20 +391,4 @@ export class FrontendService {
}
}
}
/**
* Whether this instance is running inside a Docker container.
*
* Based on: https://github.com/sindresorhus/is-docker
*/
private isDocker() {
try {
return (
fs.existsSync('/.dockerenv') ||
fs.readFileSync('/proc/self/cgroup', 'utf8').includes('docker')
);
} catch {
return false;
}
}
}

View file

@ -1,10 +1,11 @@
import { createHash, randomBytes } from 'crypto';
import { chmodSync, existsSync, mkdirSync, readFileSync, statSync, writeFileSync } from 'fs';
import { ApplicationError, jsonParse, ALPHABET, toResult } from 'n8n-workflow';
import { customAlphabet } from 'nanoid';
import { chmodSync, existsSync, mkdirSync, readFileSync, statSync, writeFileSync } from 'node:fs';
import path from 'path';
import { Service } from 'typedi';
import { Memoized } from './decorators';
import { InstanceSettingsConfig } from './InstanceSettingsConfig';
const nanoid = customAlphabet(ALPHABET, 16);
@ -133,6 +134,22 @@ export class InstanceSettings {
return this.settings.tunnelSubdomain;
}
/**
* Whether this instance is running inside a Docker container.
*
* Based on: https://github.com/sindresorhus/is-docker
*/
@Memoized
get isDocker() {
try {
return (
existsSync('/.dockerenv') || readFileSync('/proc/self/cgroup', 'utf8').includes('docker')
);
} catch {
return false;
}
}
update(newSettings: WritableSettings) {
this.save({ ...this.settings, ...newSettings });
}

View file

@ -0,0 +1,153 @@
import { AssertionError, ok } from 'node:assert';
import { setFlagsFromString } from 'node:v8';
import { runInNewContext } from 'node:vm';
import { Memoized } from '../memoized';
describe('Memoized Decorator', () => {
class TestClass {
private computeCount = 0;
constructor(private readonly value: number = 42) {}
@Memoized
get expensiveComputation() {
this.computeCount++;
return this.value * 2;
}
getComputeCount() {
return this.computeCount;
}
}
it('should only compute the value once', () => {
const instance = new TestClass();
// First access should compute
expect(instance.expensiveComputation).toBe(84);
expect(instance.getComputeCount()).toBe(1);
// Second access should use cached value
expect(instance.expensiveComputation).toBe(84);
expect(instance.getComputeCount()).toBe(1);
// Third access should still use cached value
expect(instance.expensiveComputation).toBe(84);
expect(instance.getComputeCount()).toBe(1);
});
it('should cache values independently for different instances', () => {
const instance1 = new TestClass(10);
const instance2 = new TestClass(20);
expect(instance1.expensiveComputation).toBe(20);
expect(instance2.expensiveComputation).toBe(40);
expect(instance1.getComputeCount()).toBe(1);
expect(instance2.getComputeCount()).toBe(1);
});
it('should throw error when used on non-getter', () => {
expect(() => {
class InvalidClass {
// @ts-expect-error this code will fail at compile time and at runtime
@Memoized
public normalProperty = 42;
}
new InvalidClass();
}).toThrow(AssertionError);
});
it('should make cached value non-enumerable', () => {
const instance = new TestClass();
instance.expensiveComputation; // Access to trigger caching
const propertyNames = Object.keys(instance);
expect(propertyNames).not.toContain('expensiveComputation');
});
it('should not allow reconfiguring the cached value', () => {
const instance = new TestClass();
instance.expensiveComputation; // Access to trigger caching
expect(() => {
Object.defineProperty(instance, 'expensiveComputation', {
value: 999,
configurable: true,
});
}).toThrow();
});
it('should work when child class references memoized getter in parent class', () => {
class ParentClass {
protected computeCount = 0;
@Memoized
get parentValue() {
this.computeCount++;
return 42;
}
getComputeCount() {
return this.computeCount;
}
}
class ChildClass extends ParentClass {
get childValue() {
return this.parentValue * 2;
}
}
const child = new ChildClass();
expect(child.childValue).toBe(84);
expect(child.getComputeCount()).toBe(1);
expect(child.childValue).toBe(84);
expect(child.getComputeCount()).toBe(1);
});
it('should have correct property descriptor after memoization', () => {
const instance = new TestClass();
// Before accessing (original getter descriptor)
const beforeDescriptor = Object.getOwnPropertyDescriptor(
TestClass.prototype,
'expensiveComputation',
);
expect(beforeDescriptor?.configurable).toBe(true);
expect(beforeDescriptor?.enumerable).toBe(false);
expect(typeof beforeDescriptor?.get).toBe('function');
expect(beforeDescriptor?.set).toBeUndefined();
// After accessing (memoized value descriptor)
instance.expensiveComputation; // Trigger memoization
const afterDescriptor = Object.getOwnPropertyDescriptor(instance, 'expensiveComputation');
expect(afterDescriptor?.configurable).toBe(false);
expect(afterDescriptor?.enumerable).toBe(false);
expect(afterDescriptor?.writable).toBe(false);
expect(afterDescriptor?.value).toBe(84);
expect(afterDescriptor?.get).toBeUndefined();
});
it('should not prevent garbage collection of instances', async () => {
setFlagsFromString('--expose_gc');
const gc = runInNewContext('gc') as unknown as () => void;
let instance: TestClass | undefined = new TestClass();
const weakRef = new WeakRef(instance);
instance.expensiveComputation;
// Remove the strong reference
instance = undefined;
// Wait for garbage collection, forcing it if needed
await new Promise((resolve) => setTimeout(resolve, 10));
gc();
const ref = weakRef.deref();
ok(!ref, 'GC did not collect the instance ref');
});
});

View file

@ -0,0 +1 @@
export { Memoized } from './memoized';

View file

@ -0,0 +1,41 @@
import assert from 'node:assert';
/**
* A decorator that implements memoization for class property getters.
*
* The decorated getter will only be executed once and its value cached for subsequent access
*
* @example
* class Example {
* @Memoized
* get computedValue() {
* // This will only run once and the result will be cached
* return heavyComputation();
* }
* }
*
* @throws If decorator is used on something other than a getter
*/
export function Memoized<T = unknown>(
target: object,
propertyKey: string | symbol,
descriptor?: TypedPropertyDescriptor<T>,
): TypedPropertyDescriptor<T> {
const originalGetter = descriptor?.get;
assert(originalGetter, '@Memoized can only be used on getters');
// Replace the original getter for the first call
descriptor.get = function (this: typeof target.constructor): T {
const value = originalGetter.call(this);
// Add a property on the class instance to stop reading from the getter on class prototype
Object.defineProperty(this, propertyKey, {
value,
configurable: false,
enumerable: false,
writable: false,
});
return value;
};
return descriptor;
}

View file

@ -1,5 +1,6 @@
import * as NodeExecuteFunctions from './NodeExecuteFunctions';
export * from './decorators';
export * from './errors';
export * from './ActiveWorkflows';
export * from './BinaryData/BinaryData.service';

View file

@ -1,16 +1,18 @@
import fs from 'fs';
import { mock } from 'jest-mock-extended';
jest.mock('node:fs', () => mock<typeof fs>());
import * as fs from 'node:fs';
import { InstanceSettings } from '../src/InstanceSettings';
import { InstanceSettingsConfig } from '../src/InstanceSettingsConfig';
import { InstanceSettings } from '@/InstanceSettings';
import { InstanceSettingsConfig } from '@/InstanceSettingsConfig';
describe('InstanceSettings', () => {
process.env.N8N_USER_FOLDER = '/test';
const userFolder = '/test';
process.env.N8N_USER_FOLDER = userFolder;
const settingsFile = `${userFolder}/.n8n/config`;
const existSpy = jest.spyOn(fs, 'existsSync');
const statSpy = jest.spyOn(fs, 'statSync');
const chmodSpy = jest.spyOn(fs, 'chmodSync');
const mockFs = mock(fs);
const createSettingsInstance = (opts?: Partial<InstanceSettingsConfig>) =>
const createInstanceSettings = (opts?: Partial<InstanceSettingsConfig>) =>
new InstanceSettings({
...new InstanceSettingsConfig(),
...opts,
@ -18,18 +20,17 @@ describe('InstanceSettings', () => {
beforeEach(() => {
jest.resetAllMocks();
statSpy.mockReturnValue({ mode: 0o600 } as fs.Stats);
mockFs.statSync.mockReturnValue({ mode: 0o600 } as fs.Stats);
});
describe('If the settings file exists', () => {
const readSpy = jest.spyOn(fs, 'readFileSync');
beforeEach(() => {
existSpy.mockReturnValue(true);
mockFs.existsSync.mockReturnValue(true);
});
it('should load settings from the file', () => {
readSpy.mockReturnValue(JSON.stringify({ encryptionKey: 'test_key' }));
const settings = createSettingsInstance();
mockFs.readFileSync.mockReturnValue(JSON.stringify({ encryptionKey: 'test_key' }));
const settings = createInstanceSettings();
expect(settings.encryptionKey).toEqual('test_key');
expect(settings.instanceId).toEqual(
'6ce26c63596f0cc4323563c529acfca0cccb0e57f6533d79a60a42c9ff862ae7',
@ -37,71 +38,69 @@ describe('InstanceSettings', () => {
});
it('should throw error if settings file is not valid JSON', () => {
readSpy.mockReturnValue('{"encryptionKey":"test_key"');
expect(() => createSettingsInstance()).toThrowError();
mockFs.readFileSync.mockReturnValue('{"encryptionKey":"test_key"');
expect(() => createInstanceSettings()).toThrowError();
});
it('should throw if the env and file keys do not match', () => {
readSpy.mockReturnValue(JSON.stringify({ encryptionKey: 'key_1' }));
mockFs.readFileSync.mockReturnValue(JSON.stringify({ encryptionKey: 'key_1' }));
process.env.N8N_ENCRYPTION_KEY = 'key_2';
expect(() => createSettingsInstance()).toThrowError();
expect(() => createInstanceSettings()).toThrowError();
});
it('should check if the settings file has the correct permissions', () => {
process.env.N8N_ENCRYPTION_KEY = 'test_key';
readSpy.mockReturnValueOnce(JSON.stringify({ encryptionKey: 'test_key' }));
statSpy.mockReturnValueOnce({ mode: 0o600 } as fs.Stats);
const settings = createSettingsInstance();
mockFs.readFileSync.mockReturnValueOnce(JSON.stringify({ encryptionKey: 'test_key' }));
mockFs.statSync.mockReturnValueOnce({ mode: 0o600 } as fs.Stats);
const settings = createInstanceSettings();
expect(settings.encryptionKey).toEqual('test_key');
expect(settings.instanceId).toEqual(
'6ce26c63596f0cc4323563c529acfca0cccb0e57f6533d79a60a42c9ff862ae7',
);
expect(statSpy).toHaveBeenCalledWith('/test/.n8n/config');
expect(mockFs.statSync).toHaveBeenCalledWith('/test/.n8n/config');
});
it('should check the permissions but not fix them if settings file has incorrect permissions by default', () => {
readSpy.mockReturnValueOnce(JSON.stringify({ encryptionKey: 'test_key' }));
statSpy.mockReturnValueOnce({ mode: 0o644 } as fs.Stats);
createSettingsInstance();
expect(statSpy).toHaveBeenCalledWith('/test/.n8n/config');
expect(chmodSpy).not.toHaveBeenCalled();
mockFs.readFileSync.mockReturnValueOnce(JSON.stringify({ encryptionKey: 'test_key' }));
mockFs.statSync.mockReturnValueOnce({ mode: 0o644 } as fs.Stats);
createInstanceSettings();
expect(mockFs.statSync).toHaveBeenCalledWith('/test/.n8n/config');
expect(mockFs.chmodSync).not.toHaveBeenCalled();
});
it("should not check the permissions if 'N8N_ENFORCE_SETTINGS_FILE_PERMISSIONS' is false", () => {
process.env.N8N_ENFORCE_SETTINGS_FILE_PERMISSIONS = 'false';
readSpy.mockReturnValueOnce(JSON.stringify({ encryptionKey: 'test_key' }));
createSettingsInstance();
expect(statSpy).not.toHaveBeenCalled();
expect(chmodSpy).not.toHaveBeenCalled();
mockFs.readFileSync.mockReturnValueOnce(JSON.stringify({ encryptionKey: 'test_key' }));
createInstanceSettings();
expect(mockFs.statSync).not.toHaveBeenCalled();
expect(mockFs.chmodSync).not.toHaveBeenCalled();
});
it("should fix the permissions of the settings file if 'N8N_ENFORCE_SETTINGS_FILE_PERMISSIONS' is true", () => {
process.env.N8N_ENFORCE_SETTINGS_FILE_PERMISSIONS = 'true';
readSpy.mockReturnValueOnce(JSON.stringify({ encryptionKey: 'test_key' }));
statSpy.mockReturnValueOnce({ mode: 0o644 } as fs.Stats);
createSettingsInstance({
mockFs.readFileSync.mockReturnValueOnce(JSON.stringify({ encryptionKey: 'test_key' }));
mockFs.statSync.mockReturnValueOnce({ mode: 0o644 } as fs.Stats);
createInstanceSettings({
enforceSettingsFilePermissions: true,
});
expect(statSpy).toHaveBeenCalledWith('/test/.n8n/config');
expect(chmodSpy).toHaveBeenCalledWith('/test/.n8n/config', 0o600);
expect(mockFs.statSync).toHaveBeenCalledWith('/test/.n8n/config');
expect(mockFs.chmodSync).toHaveBeenCalledWith('/test/.n8n/config', 0o600);
});
});
describe('If the settings file does not exist', () => {
const mkdirSpy = jest.spyOn(fs, 'mkdirSync');
const writeFileSpy = jest.spyOn(fs, 'writeFileSync');
beforeEach(() => {
existSpy.mockReturnValue(false);
mkdirSpy.mockReturnValue('');
writeFileSpy.mockReturnValue();
mockFs.existsSync.mockReturnValue(false);
mockFs.mkdirSync.mockReturnValue('');
mockFs.writeFileSync.mockReturnValue();
});
it('should create a new settings file without explicit permissions if N8N_ENFORCE_SETTINGS_FILE_PERMISSIONS is not set', () => {
process.env.N8N_ENCRYPTION_KEY = 'key_2';
const settings = createSettingsInstance();
const settings = createInstanceSettings();
expect(settings.encryptionKey).not.toEqual('test_key');
expect(mkdirSpy).toHaveBeenCalledWith('/test/.n8n', { recursive: true });
expect(writeFileSpy).toHaveBeenCalledWith(
expect(mockFs.mkdirSync).toHaveBeenCalledWith('/test/.n8n', { recursive: true });
expect(mockFs.writeFileSync).toHaveBeenCalledWith(
'/test/.n8n/config',
expect.stringContaining('"encryptionKey":'),
{
@ -114,10 +113,10 @@ describe('InstanceSettings', () => {
it('should create a new settings file without explicit permissions if N8N_ENFORCE_SETTINGS_FILE_PERMISSIONS=false', () => {
process.env.N8N_ENFORCE_SETTINGS_FILE_PERMISSIONS = 'false';
process.env.N8N_ENCRYPTION_KEY = 'key_2';
const settings = createSettingsInstance();
const settings = createInstanceSettings();
expect(settings.encryptionKey).not.toEqual('test_key');
expect(mkdirSpy).toHaveBeenCalledWith('/test/.n8n', { recursive: true });
expect(writeFileSpy).toHaveBeenCalledWith(
expect(mockFs.mkdirSync).toHaveBeenCalledWith('/test/.n8n', { recursive: true });
expect(mockFs.writeFileSync).toHaveBeenCalledWith(
'/test/.n8n/config',
expect.stringContaining('"encryptionKey":'),
{
@ -130,12 +129,12 @@ describe('InstanceSettings', () => {
it('should create a new settings file with explicit permissions if N8N_ENFORCE_SETTINGS_FILE_PERMISSIONS=true', () => {
process.env.N8N_ENFORCE_SETTINGS_FILE_PERMISSIONS = 'true';
process.env.N8N_ENCRYPTION_KEY = 'key_2';
const settings = createSettingsInstance({
const settings = createInstanceSettings({
enforceSettingsFilePermissions: true,
});
expect(settings.encryptionKey).not.toEqual('test_key');
expect(mkdirSpy).toHaveBeenCalledWith('/test/.n8n', { recursive: true });
expect(writeFileSpy).toHaveBeenCalledWith(
expect(mockFs.mkdirSync).toHaveBeenCalledWith('/test/.n8n', { recursive: true });
expect(mockFs.writeFileSync).toHaveBeenCalledWith(
'/test/.n8n/config',
expect.stringContaining('"encryptionKey":'),
{
@ -147,14 +146,14 @@ describe('InstanceSettings', () => {
it('should pick up the encryption key from env var N8N_ENCRYPTION_KEY', () => {
process.env.N8N_ENCRYPTION_KEY = 'env_key';
const settings = createSettingsInstance();
const settings = createInstanceSettings();
expect(settings.encryptionKey).toEqual('env_key');
expect(settings.instanceId).toEqual(
'2c70e12b7a0646f92279f427c7b38e7334d8e5389cff167a1dc30e73f826b683',
);
expect(settings.encryptionKey).not.toEqual('test_key');
expect(mkdirSpy).toHaveBeenCalledWith('/test/.n8n', { recursive: true });
expect(writeFileSpy).toHaveBeenCalledWith(
expect(mockFs.mkdirSync).toHaveBeenCalledWith('/test/.n8n', { recursive: true });
expect(mockFs.writeFileSync).toHaveBeenCalledWith(
'/test/.n8n/config',
expect.stringContaining('"encryptionKey":'),
{
@ -167,10 +166,10 @@ describe('InstanceSettings', () => {
it("should not set the permissions of the settings file if 'N8N_IGNORE_SETTINGS_FILE_PERMISSIONS' is true", () => {
process.env.N8N_ENCRYPTION_KEY = 'key_2';
process.env.N8N_IGNORE_SETTINGS_FILE_PERMISSIONS = 'true';
const settings = createSettingsInstance();
const settings = createInstanceSettings();
expect(settings.encryptionKey).not.toEqual('test_key');
expect(mkdirSpy).toHaveBeenCalledWith('/test/.n8n', { recursive: true });
expect(writeFileSpy).toHaveBeenCalledWith(
expect(mockFs.mkdirSync).toHaveBeenCalledWith('/test/.n8n', { recursive: true });
expect(mockFs.writeFileSync).toHaveBeenCalledWith(
'/test/.n8n/config',
expect.stringContaining('"encryptionKey":'),
{
@ -185,14 +184,67 @@ describe('InstanceSettings', () => {
it('should generate a `hostId`', () => {
const encryptionKey = 'test_key';
process.env.N8N_ENCRYPTION_KEY = encryptionKey;
jest.spyOn(fs, 'existsSync').mockReturnValueOnce(true);
jest.spyOn(fs, 'readFileSync').mockReturnValueOnce(JSON.stringify({ encryptionKey }));
mockFs.existsSync.mockReturnValueOnce(true);
mockFs.readFileSync.mockReturnValueOnce(JSON.stringify({ encryptionKey }));
const settings = createSettingsInstance();
const settings = createInstanceSettings();
const [instanceType, nanoid] = settings.hostId.split('-');
expect(instanceType).toEqual('main');
expect(nanoid).toHaveLength(16); // e.g. sDX6ZPc0bozv66zM
});
});
describe('isDocker', () => {
let settings: InstanceSettings;
beforeEach(() => {
mockFs.existsSync.calledWith(settingsFile).mockReturnValue(true);
mockFs.readFileSync
.calledWith(settingsFile)
.mockReturnValue(JSON.stringify({ encryptionKey: 'test_key' }));
settings = new InstanceSettings(mock());
});
it('should return true if /.dockerenv exists', () => {
mockFs.existsSync.calledWith('/.dockerenv').mockReturnValueOnce(true);
expect(settings.isDocker).toBe(true);
expect(mockFs.existsSync).toHaveBeenCalledWith('/.dockerenv');
expect(mockFs.readFileSync).not.toHaveBeenCalledWith('/proc/self/cgroup', 'utf8');
});
it('should return true if /proc/self/cgroup contains docker', () => {
mockFs.existsSync.calledWith('/.dockerenv').mockReturnValueOnce(false);
mockFs.readFileSync
.calledWith('/proc/self/cgroup', 'utf8')
.mockReturnValueOnce('docker cgroup');
expect(settings.isDocker).toBe(true);
expect(mockFs.existsSync).toHaveBeenCalledWith('/.dockerenv');
expect(mockFs.readFileSync).toHaveBeenCalledWith('/proc/self/cgroup', 'utf8');
});
it('should return false if no docker indicators are found', () => {
mockFs.existsSync.calledWith('/.dockerenv').mockReturnValueOnce(false);
mockFs.readFileSync.calledWith('/proc/self/cgroup', 'utf8').mockReturnValueOnce('');
expect(settings.isDocker).toBe(false);
});
it('should return false if checking for docker throws an error', () => {
mockFs.existsSync.calledWith('/.dockerenv').mockImplementationOnce(() => {
throw new Error('Access denied');
});
expect(settings.isDocker).toBe(false);
});
it('should cache the result of isDocker check', () => {
mockFs.existsSync.calledWith('/.dockerenv').mockReturnValueOnce(true);
expect(settings.isDocker).toBe(true);
mockFs.existsSync.mockClear();
expect(settings.isDocker).toBe(true);
expect(mockFs.existsSync).not.toHaveBeenCalled();
});
});
});