From 29ae2396c99d54d8f3db71e6370516f0dc354d00 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 21 Feb 2025 10:25:53 +0100 Subject: [PATCH] feat: Enable partial exections v2 by default (#13344) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: कारतोफ्फेलस्क्रिप्ट™ --- cypress/e2e/19-execution.cy.ts | 6 +- cypress/e2e/40-manual-partial-execution.cy.ts | 8 +- .../@n8n/api-types/src/frontend-settings.ts | 1 - .../src/configs/partial-executions.config.ts | 6 +- packages/@n8n/config/test/config.test.ts | 3 +- .../__tests__/deprecation.service.test.ts | 81 +++++++++++++++---- .../src/deprecation/deprecation.service.ts | 38 +++++---- packages/editor-ui/src/__tests__/defaults.ts | 1 - .../src/stores/settings.store.test.ts | 22 ++--- .../editor-ui/src/stores/settings.store.ts | 7 +- .../src/stores/workflows.store.test.ts | 34 +++----- 11 files changed, 115 insertions(+), 92 deletions(-) diff --git a/cypress/e2e/19-execution.cy.ts b/cypress/e2e/19-execution.cy.ts index 7563cc4827..23172f8fda 100644 --- a/cypress/e2e/19-execution.cy.ts +++ b/cypress/e2e/19-execution.cy.ts @@ -489,7 +489,11 @@ describe('Execution', () => { cy.wait('@workflowRun').then((interception) => { expect(interception.request.body).to.have.property('runData').that.is.an('object'); - const expectedKeys = ['When clicking ‘Test workflow’', 'fetch 5 random users']; + const expectedKeys = [ + 'When clicking ‘Test workflow’', + 'fetch 5 random users', + 'do something with them', + ]; const { runData } = interception.request.body as Record; expect(Object.keys(runData)).to.have.lengthOf(expectedKeys.length); diff --git a/cypress/e2e/40-manual-partial-execution.cy.ts b/cypress/e2e/40-manual-partial-execution.cy.ts index 2eb129475f..28a1539252 100644 --- a/cypress/e2e/40-manual-partial-execution.cy.ts +++ b/cypress/e2e/40-manual-partial-execution.cy.ts @@ -4,7 +4,7 @@ const canvas = new WorkflowPage(); const ndv = new NDV(); describe('Manual partial execution', () => { - it('should execute parent nodes with no run data only once', () => { + it('should not execute parent nodes with no run data', () => { canvas.actions.visit(); cy.fixture('manual-partial-execution.json').then((data) => { @@ -22,8 +22,8 @@ describe('Manual partial execution', () => { canvas.actions.openNode('Webhook1'); - ndv.getters.nodeRunSuccessIndicator().should('exist'); - ndv.getters.nodeRunTooltipIndicator().should('exist'); - ndv.getters.outputRunSelector().should('not.exist'); // single run + ndv.getters.nodeRunSuccessIndicator().should('not.exist'); + ndv.getters.nodeRunTooltipIndicator().should('not.exist'); + ndv.getters.outputRunSelector().should('not.exist'); }); }); diff --git a/packages/@n8n/api-types/src/frontend-settings.ts b/packages/@n8n/api-types/src/frontend-settings.ts index e4a5acd7f3..e6c133e126 100644 --- a/packages/@n8n/api-types/src/frontend-settings.ts +++ b/packages/@n8n/api-types/src/frontend-settings.ts @@ -181,6 +181,5 @@ export interface FrontendSettings { easyAIWorkflowOnboarded: boolean; partialExecution: { version: 1 | 2; - enforce: boolean; }; } diff --git a/packages/@n8n/config/src/configs/partial-executions.config.ts b/packages/@n8n/config/src/configs/partial-executions.config.ts index 7937f451d3..2cef62e390 100644 --- a/packages/@n8n/config/src/configs/partial-executions.config.ts +++ b/packages/@n8n/config/src/configs/partial-executions.config.ts @@ -4,9 +4,5 @@ import { Config, Env } from '../decorators'; export class PartialExecutionsConfig { /** Partial execution logic version to use by default. */ @Env('N8N_PARTIAL_EXECUTION_VERSION_DEFAULT') - version: 1 | 2 = 1; - - /** Set this to true to enforce using the default version. Users cannot use the other version then by setting a local storage key. */ - @Env('N8N_PARTIAL_EXECUTION_ENFORCE_VERSION') - enforce: boolean = false; + version: 1 | 2 = 2; } diff --git a/packages/@n8n/config/test/config.test.ts b/packages/@n8n/config/test/config.test.ts index 36f39e1ce7..735cb4bfcf 100644 --- a/packages/@n8n/config/test/config.test.ts +++ b/packages/@n8n/config/test/config.test.ts @@ -305,8 +305,7 @@ describe('GlobalConfig', () => { disabled: false, }, partialExecutions: { - version: 1, - enforce: false, + version: 2, }, }; diff --git a/packages/cli/src/deprecation/__tests__/deprecation.service.test.ts b/packages/cli/src/deprecation/__tests__/deprecation.service.test.ts index 591a0348e3..d9d00f6893 100644 --- a/packages/cli/src/deprecation/__tests__/deprecation.service.test.ts +++ b/packages/cli/src/deprecation/__tests__/deprecation.service.test.ts @@ -1,15 +1,69 @@ -import { mockLogger } from '@test/mocking'; +import { captor, mock } from 'jest-mock-extended'; +import type { Logger } from 'n8n-core'; import { DeprecationService } from '../deprecation.service'; describe('DeprecationService', () => { - const toTest = (envVar: string, value: string, mustWarn: boolean) => { - process.env[envVar] = value; - const deprecationService = new DeprecationService(mockLogger()); + const logger = mock(); + const deprecationService = new DeprecationService(logger); - deprecationService.warn(); + beforeEach(() => { + // Ignore environment variables coming in from the environment when running + // this test suite. + process.env = {}; - expect(deprecationService.mustWarn(envVar)).toBe(mustWarn); + jest.resetAllMocks(); + }); + + describe('N8N_PARTIAL_EXECUTION_VERSION_DEFAULT', () => { + test('supports multiple warnings for the same environment variable', () => { + // ARRANGE + process.env.N8N_PARTIAL_EXECUTION_VERSION_DEFAULT = '1'; + const dataCaptor = captor(); + + // ACT + deprecationService.warn(); + + // ASSERT + expect(logger.warn).toHaveBeenCalledTimes(1); + expect(logger.warn).toHaveBeenCalledWith(dataCaptor); + expect(dataCaptor.value.split('\n')).toEqual( + expect.arrayContaining([ + ' - N8N_PARTIAL_EXECUTION_VERSION_DEFAULT -> Version 1 of partial executions is deprecated and will be removed as early as v1.85.0', + ' - N8N_PARTIAL_EXECUTION_VERSION_DEFAULT -> This environment variable is internal and should not be set.', + ]), + ); + }); + }); + + const toTest = (envVar: string, value: string | undefined, mustWarn: boolean) => { + const originalEnv = process.env[envVar]; + try { + // ARRANGE + if (value) { + process.env[envVar] = value; + } else { + delete process.env[envVar]; + } + + // ACT + deprecationService.warn(); + + // ASSERT + if (mustWarn) { + expect(logger.warn).toHaveBeenCalledTimes(1); + expect(logger.warn.mock.lastCall?.[0]).toMatch(envVar); + } else { + expect(logger.warn.mock.lastCall?.[0] ?? '').not.toMatch(envVar); + } + } finally { + // CLEANUP + if (originalEnv) { + process.env[envVar] = originalEnv; + } else { + delete process.env[envVar]; + } + } }; test.each([ @@ -18,7 +72,10 @@ describe('DeprecationService', () => { ['EXECUTIONS_DATA_PRUNE_TIMEOUT', '1', true], ['N8N_CONFIG_FILES', '1', true], ['N8N_SKIP_WEBHOOK_DEREGISTRATION_SHUTDOWN', '1', true], - ])('should detect when %s is in use', (envVar, value, mustWarn) => { + ['N8N_PARTIAL_EXECUTION_VERSION_DEFAULT', '1', true], + ['N8N_PARTIAL_EXECUTION_VERSION_DEFAULT', '2', true], + ['N8N_PARTIAL_EXECUTION_VERSION_DEFAULT', undefined, false], + ])('should detect when %s is `%s`', (envVar, value, mustWarn) => { toTest(envVar, value, mustWarn); }); @@ -48,15 +105,7 @@ describe('DeprecationService', () => { ['true', false], [undefined /* warnIfMissing */, true], ])('should handle value: %s', (value, mustWarn) => { - if (value === undefined) { - delete process.env[envVar]; - } else { - process.env[envVar] = value; - } - - const deprecationService = new DeprecationService(mockLogger()); - deprecationService.warn(); - expect(deprecationService.mustWarn(envVar)).toBe(mustWarn); + toTest(envVar, value, mustWarn); }); }); }); diff --git a/packages/cli/src/deprecation/deprecation.service.ts b/packages/cli/src/deprecation/deprecation.service.ts index e70cfcd257..a367331815 100644 --- a/packages/cli/src/deprecation/deprecation.service.ts +++ b/packages/cli/src/deprecation/deprecation.service.ts @@ -1,6 +1,5 @@ import { Service } from '@n8n/di'; import { Logger } from 'n8n-core'; -import { ApplicationError } from 'n8n-workflow'; type EnvVarName = string; @@ -49,32 +48,41 @@ export class DeprecationService { checkValue: (value?: string) => value?.toLowerCase() !== 'true' && value !== '1', warnIfMissing: true, }, + { + envVar: 'N8N_PARTIAL_EXECUTION_VERSION_DEFAULT', + checkValue: (value: string) => value === '1', + message: + 'Version 1 of partial executions is deprecated and will be removed as early as v1.85.0', + }, + { + envVar: 'N8N_PARTIAL_EXECUTION_VERSION_DEFAULT', + message: 'This environment variable is internal and should not be set.', + }, ]; /** Runtime state of deprecation-related env vars. */ - private readonly state: Record = {}; + private readonly state: Map = new Map(); constructor(private readonly logger: Logger) {} warn() { this.deprecations.forEach((d) => { const envValue = process.env[d.envVar]; - this.state[d.envVar] = { + this.state.set(d, { mustWarn: (d.warnIfMissing !== undefined && envValue === undefined) || (d.checkValue ? d.checkValue(envValue) : envValue !== undefined), - }; + }); }); - const mustWarn = Object.entries(this.state) - .filter(([, d]) => d.mustWarn) - .map(([envVar]) => { - const deprecation = this.deprecations.find((d) => d.envVar === envVar); - if (!deprecation) { - throw new ApplicationError(`Deprecation not found for env var: ${envVar}`); - } - return deprecation; - }); + const mustWarn: Deprecation[] = []; + for (const [deprecation, metadata] of this.state.entries()) { + if (!metadata.mustWarn) { + continue; + } + + mustWarn.push(deprecation); + } if (mustWarn.length === 0) return; @@ -87,8 +95,4 @@ export class DeprecationService { this.logger.warn(`\n${header}:\n${deprecations}`); } - - mustWarn(envVar: string) { - return this.state[envVar]?.mustWarn ?? false; - } } diff --git a/packages/editor-ui/src/__tests__/defaults.ts b/packages/editor-ui/src/__tests__/defaults.ts index 818aaab3a8..43970ce786 100644 --- a/packages/editor-ui/src/__tests__/defaults.ts +++ b/packages/editor-ui/src/__tests__/defaults.ts @@ -138,7 +138,6 @@ export const defaultSettings: FrontendSettings = { easyAIWorkflowOnboarded: false, partialExecution: { version: 1, - enforce: false, }, folders: { enabled: false, diff --git a/packages/editor-ui/src/stores/settings.store.test.ts b/packages/editor-ui/src/stores/settings.store.test.ts index 4d7d00c76c..5b7f3ce3fe 100644 --- a/packages/editor-ui/src/stores/settings.store.test.ts +++ b/packages/editor-ui/src/stores/settings.store.test.ts @@ -117,44 +117,32 @@ describe('settings.store', () => { { name: 'pick the default', default: 1 as const, - enforce: false, userVersion: -1, result: 1, }, { - name: "pick the user' choice", - default: 1 as const, - enforce: false, - userVersion: 2, + name: 'pick the default', + default: 2 as const, + userVersion: -1, result: 2, }, { - name: 'enforce the default', + name: "pick the user's choice", default: 1 as const, - enforce: true, userVersion: 2, - result: 1, - }, - { - name: 'enforce the default', - default: 2 as const, - enforce: true, - userVersion: 1, result: 2, }, { name: 'handle values that used to be allowed in local storage', default: 1 as const, - enforce: false, userVersion: 0, result: 1, }, - ])('%name', async ({ default: defaultVersion, userVersion, enforce, result }) => { + ])('%name', async ({ default: defaultVersion, userVersion, result }) => { const settingsStore = useSettingsStore(); settingsStore.settings.partialExecution = { version: defaultVersion, - enforce, }; vi.mocked(useLocalStorage).mockReturnValueOnce(ref(userVersion)); diff --git a/packages/editor-ui/src/stores/settings.store.ts b/packages/editor-ui/src/stores/settings.store.ts index b64a3d32fa..dc0c6463a8 100644 --- a/packages/editor-ui/src/stores/settings.store.ts +++ b/packages/editor-ui/src/stores/settings.store.ts @@ -103,16 +103,11 @@ export const useSettingsStore = defineStore(STORES.SETTINGS, () => { const partialExecutionVersion = computed(() => { const defaultVersion = settings.value.partialExecution?.version ?? 1; - const enforceVersion = settings.value.partialExecution?.enforce ?? false; // -1 means we pick the defaultVersion // 1 is the old flow // 2 is the new flow const userVersion = useLocalStorage('PartialExecution.version', -1).value; - const version = enforceVersion - ? defaultVersion - : userVersion === -1 - ? defaultVersion - : userVersion; + const version = userVersion === -1 ? defaultVersion : userVersion; // For backwards compatibility, e.g. if the user has 0 in their local // storage, which used to be allowed, but not anymore. diff --git a/packages/editor-ui/src/stores/workflows.store.test.ts b/packages/editor-ui/src/stores/workflows.store.test.ts index 59201c67d3..6ae267f3ec 100644 --- a/packages/editor-ui/src/stores/workflows.store.test.ts +++ b/packages/editor-ui/src/stores/workflows.store.test.ts @@ -682,31 +682,21 @@ describe('useWorkflowsStore', () => { }); test.each([ - // enforce true cases - the version is always the defaultVersion - [-1, 1, true, 1], // enforce true, use default (1) - [0, 1, true, 1], // enforce true, use default (1) - [1, 1, true, 1], // enforce true, use default (1) - [2, 1, true, 1], // enforce true, use default (1) - [-1, 2, true, 2], // enforce true, use default (2) - [0, 2, true, 2], // enforce true, use default (2) - [1, 2, true, 2], // enforce true, use default (2) - [2, 2, true, 2], // enforce true, use default (2) - - // enforce false cases - check userVersion behavior - [-1, 1, false, 1], // userVersion -1, use default (1) - [0, 1, false, 1], // userVersion 0, invalid, use default (1) - [1, 1, false, 1], // userVersion 1, valid, use userVersion (1) - [2, 1, false, 2], // userVersion 2, valid, use userVersion (2) - [-1, 2, false, 2], // userVersion -1, use default (2) - [0, 2, false, 1], // userVersion 0, invalid, use default (2) - [1, 2, false, 1], // userVersion 1, valid, use userVersion (1) - [2, 2, false, 2], // userVersion 2, valid, use userVersion (2) - ] as Array<[number, 1 | 2, boolean, number]>)( + // check userVersion behavior + [-1, 1, 1], // userVersion -1, use default (1) + [0, 1, 1], // userVersion 0, invalid, use default (1) + [1, 1, 1], // userVersion 1, valid, use userVersion (1) + [2, 1, 2], // userVersion 2, valid, use userVersion (2) + [-1, 2, 2], // userVersion -1, use default (2) + [0, 2, 1], // userVersion 0, invalid, use default (2) + [1, 2, 1], // userVersion 1, valid, use userVersion (1) + [2, 2, 2], // userVersion 2, valid, use userVersion (2) + ] as Array<[number, 1 | 2, number]>)( 'when { userVersion:%s, defaultVersion:%s, enforced:%s } run workflow should use partial execution version %s', - async (userVersion, defaultVersion, enforce, expectedVersion) => { + async (userVersion, defaultVersion, expectedVersion) => { vi.mocked(useLocalStorage).mockReturnValueOnce(ref(userVersion)); settingsStore.settings = { - partialExecution: { version: defaultVersion, enforce }, + partialExecution: { version: defaultVersion }, } as FrontendSettings; const workflowData = { id: '1', nodes: [], connections: {} };