From 0bbea438e1906a0773cdbb46e273b4c01fea359b Mon Sep 17 00:00:00 2001 From: Theodore Nastos <15264814+theonastos@users.noreply.github.com> Date: Wed, 7 Oct 2026 13:30:44 +0000 Subject: [PATCH] fix(core): Show the policy block before node issues on execute and publish (#40290) --- .../cli/src/__tests__/workflow-runner.test.ts | 33 ++----- packages/cli/src/scaling/job-processor.ts | 46 ++-------- packages/cli/src/workflow-runner.ts | 6 -- ...process-process-run-execution-data.test.ts | 85 ++++++++++++------- .../src/execution-engine/workflow-execute.ts | 8 +- .../WorkflowHeaderDraftPublishActions.test.ts | 29 +++++++ .../WorkflowHeaderDraftPublishActions.vue | 46 ++++------ 7 files changed, 122 insertions(+), 131 deletions(-) diff --git a/packages/cli/src/__tests__/workflow-runner.test.ts b/packages/cli/src/__tests__/workflow-runner.test.ts index b1612a0ae6c..017027dcf3a 100644 --- a/packages/cli/src/__tests__/workflow-runner.test.ts +++ b/packages/cli/src/__tests__/workflow-runner.test.ts @@ -15,7 +15,7 @@ import { createExecution } from '@test-integration/db/executions'; import { createUser } from '@test-integration/db/users'; import { setupTestServer } from '@test-integration/utils'; import type { Response } from 'express'; -import { DirectedGraph, StorageConfig, WorkflowExecute, WorkflowHasIssuesError } from 'n8n-core'; +import { DirectedGraph, StorageConfig, WorkflowExecute } from 'n8n-core'; import * as core from 'n8n-core'; import { type IExecuteData, @@ -1108,8 +1108,9 @@ describe('run', () => { }); }); - describe('workflow issues pre-flight failure', () => { - function arrangeFailingRunDeps(error: Error) { + describe('startup failure', () => { + it('rejects on startup errors', async () => { + const error = new Error('boom'); const activeExecutions = Container.get(ActiveExecutions); vi.spyOn(activeExecutions, 'add').mockResolvedValue('1'); vi.spyOn(Container.get(CredentialsPermissionChecker), 'check').mockResolvedValueOnce(); @@ -1119,7 +1120,7 @@ describe('run', () => { vi.spyOn(WorkflowExecuteAdditionalData, 'getBase').mockResolvedValue( mock(), ); - + const processError = vi.spyOn(runner, 'processError').mockResolvedValueOnce(); const data = mock({ workflowData: { nodes: [], id: 'workflow-id', settings: undefined, staticData: {} }, executionData: createRunExecutionData({}), @@ -1128,34 +1129,10 @@ describe('run', () => { destinationNode: undefined, userId: 'mock-user-id', }); - return { data }; - } - - it('surfaces a WorkflowHasIssuesError as a failed run instead of rejecting', async () => { - const error = new WorkflowHasIssuesError( - { node1: { parameters: { field: ['is missing'] } } }, - {}, - ); - const { data } = arrangeFailingRunDeps(error); - const failExecution = vi.spyOn(runner, 'failExecution').mockResolvedValueOnce(); - const processError = vi.spyOn(runner, 'processError').mockResolvedValueOnce(); - - await expect(runner.run(data)).resolves.toBe('1'); - - expect(failExecution).toHaveBeenCalledWith(data, '1', error); - expect(processError).not.toHaveBeenCalled(); - }); - - it('still rejects on other startup errors', async () => { - const error = new Error('boom'); - const { data } = arrangeFailingRunDeps(error); - const failExecution = vi.spyOn(runner, 'failExecution').mockResolvedValueOnce(); - const processError = vi.spyOn(runner, 'processError').mockResolvedValueOnce(); await expect(runner.run(data)).rejects.toThrowError(error); expect(processError).toHaveBeenCalled(); - expect(failExecution).not.toHaveBeenCalled(); }); }); }); diff --git a/packages/cli/src/scaling/job-processor.ts b/packages/cli/src/scaling/job-processor.ts index 7909291eebe..89c8c3a9286 100644 --- a/packages/cli/src/scaling/job-processor.ts +++ b/packages/cli/src/scaling/job-processor.ts @@ -6,13 +6,7 @@ import { ExecutionsConfig } from '@n8n/config'; import { MAX_INTEGER_32BITS_SIGNED } from '@n8n/constants'; import { ExecutionRepository, WorkflowRepository } from '@n8n/db'; import { Service } from '@n8n/di'; -import { - WorkflowHasIssuesError, - InstanceSettings, - WorkflowExecute, - SupplyDataContext, - StructuredToolkit, -} from 'n8n-core'; +import { InstanceSettings, WorkflowExecute, SupplyDataContext, StructuredToolkit } from 'n8n-core'; import { ManualExecutionCancelledError, NodeConnectionTypes, @@ -20,7 +14,6 @@ import { TimeoutExecutionCancelledError, Workflow, UnexpectedError, - createRunExecutionData, runDataAttemptedDynamicCredentials, runDataUsedDynamicCredentials, } from 'n8n-workflow'; @@ -341,35 +334,14 @@ export class JobProcessor { agentRequest: manualData?.agentRequest, }; - try { - workflowRun = this.manualExecutionService.runManually( - data, - workflow, - additionalData, - executionId, - resultData.pinData, - execution.storedAt, - ); - } catch (error) { - if (error instanceof WorkflowHasIssuesError) { - // execution did not even start, but we call `workflowExecuteAfter` to notify main - - const now = new Date(); - const runData: IRun = { - mode: 'manual', - status: 'error', - finished: false, - startedAt: now, - stoppedAt: now, - data: createRunExecutionData({ resultData: { error, runData: {} } }), - storedAt: execution.storedAt, - }; - - await lifecycleHooks.runHook('workflowExecuteAfter', [runData]); - return { success: false }; - } - throw error; - } + workflowRun = this.manualExecutionService.runManually( + data, + workflow, + additionalData, + executionId, + resultData.pinData, + execution.storedAt, + ); } const runningJob: RunningJob = { diff --git a/packages/cli/src/workflow-runner.ts b/packages/cli/src/workflow-runner.ts index 5c9cd798285..2a5ec1aebea 100644 --- a/packages/cli/src/workflow-runner.ts +++ b/packages/cli/src/workflow-runner.ts @@ -17,7 +17,6 @@ import { InstanceSettings, StorageConfig, WorkflowExecute, - WorkflowHasIssuesError, } from 'n8n-core'; import type { ExecutionError, @@ -720,11 +719,6 @@ export class WorkflowRunner { ), ); } catch (error) { - if (error instanceof WorkflowHasIssuesError) { - await this.failExecution(data, executionId, error); - return; - } - await this.processError( error, new Date(), diff --git a/packages/core/src/execution-engine/__tests__/workflow-execute-process-process-run-execution-data.test.ts b/packages/core/src/execution-engine/__tests__/workflow-execute-process-process-run-execution-data.test.ts index 1412967ebe2..b9fdcd95227 100644 --- a/packages/core/src/execution-engine/__tests__/workflow-execute-process-process-run-execution-data.test.ts +++ b/packages/core/src/execution-engine/__tests__/workflow-execute-process-process-run-execution-data.test.ts @@ -9,12 +9,7 @@ import type { INodeType, IRunExecutionData, } from 'n8n-workflow'; -import { - BaseError, - NodeConnectionTypes, - UnexpectedError, - createRunExecutionData, -} from 'n8n-workflow'; +import { NodeConnectionTypes, UnexpectedError, createRunExecutionData } from 'n8n-workflow'; import { mock } from 'vitest-mock-extended'; import { NodeTypes } from '@test/helpers'; @@ -53,6 +48,8 @@ describe('processRunExecutionData', () => { runHook.mockResolvedValue(undefined); }); + const hookNames = () => (runHook.mock.calls as Array<[string]>).map(([name]) => name); + test('throws if execution-data is missing', () => { // ARRANGE const node = createNodeData({ name: 'passThrough', type: types.passThrough }); @@ -137,7 +134,7 @@ describe('processRunExecutionData', () => { .addNodes(node) .toWorkflow({ name: '', active: false, nodeTypes, settings: { executionOrder: 'v1' } }); - const runBlocked = async (executionData: IRunExecutionData) => { + const runBlocked = async (executionData: IRunExecutionData, workflowToRun = workflow) => { runHook.mockImplementation(async (hookName: string) => { if (hookName === 'workflowExecuteBefore') throw blocked; }); @@ -146,9 +143,9 @@ describe('processRunExecutionData', () => { additionalData, executionMode, executionData, - ).processRunExecutionData(workflow); + ).processRunExecutionData(workflowToRun); - return { result, hooks: (runHook.mock.calls as Array<[string]>).map(([name]) => name) }; + return { result, hooks: hookNames() }; }; test('aborts the execution and keeps the error it was given', async () => { @@ -174,6 +171,28 @@ describe('processRunExecutionData', () => { expect(result.data.resultData.error?.message).toBe(blocked.message); expect(hooks).toEqual(['workflowExecuteBefore', 'workflowExecuteAfter']); }); + + test('is reported ahead of the workflow issues', async () => { + const nodeWithIssues = createNodeData({ + name: 'node', + type: types.testNodeWithRequiredProperty, + }); + const { result } = await runBlocked( + createRunExecutionData({ + startData: { startNodes: [{ name: nodeWithIssues.name, sourceData: null }] }, + executionData: { + nodeExecutionStack: [ + { data: { main: [[{ json: { foo: 1 } }]] }, node: nodeWithIssues, source: null }, + ], + }, + }), + new DirectedGraph() + .addNodes(nodeWithIssues) + .toWorkflow({ name: '', active: false, nodeTypes, settings: { executionOrder: 'v1' } }), + ); + + expect(result.data.resultData.error?.message).toBe(blocked.message); + }); }); test('agent node emits nodeExecuteBefore only once when resuming after tool execution', async () => { @@ -387,35 +406,36 @@ describe('processRunExecutionData', () => { }); describe('workflow issues', () => { - test('throws if workflow contains nodes with missing required properties', () => { - // ARRANGE - const node = createNodeData({ name: 'node', type: types.testNodeWithRequiredProperty }); - const workflow = new DirectedGraph() - .addNodes(node) - .toWorkflow({ name: '', active: false, nodeTypes, settings: { executionOrder: 'v1' } }); + const node = createNodeData({ name: 'node', type: types.testNodeWithRequiredProperty }); + const workflow = new DirectedGraph() + .addNodes(node) + .toWorkflow({ name: '', active: false, nodeTypes, settings: { executionOrder: 'v1' } }); - const taskDataConnection = { main: [[{ json: { foo: 1 } }]] }; + test('fails the run if workflow contains nodes with missing required properties', async () => { + // ARRANGE const executionData = createRunExecutionData({ startData: { startNodes: [{ name: node.name, sourceData: null }] }, executionData: { - nodeExecutionStack: [{ data: taskDataConnection, node, source: null }], + nodeExecutionStack: [{ data: { main: [[{ json: { foo: 1 } }]] }, node, source: null }], }, }); - const workflowExecute = new WorkflowExecute(additionalData, executionMode, executionData); + // ACT + const result = await new WorkflowExecute( + additionalData, + executionMode, + executionData, + ).processRunExecutionData(workflow); - // ACT & ASSERT - // The function returns a Promise, but throws synchronously, so we can't await it. - // eslint-disable-next-line @typescript-eslint/promise-function-async - const execution = () => workflowExecute.processRunExecutionData(workflow); - - expect(execution).toThrow(BaseError); - expect(execution).toThrow( + // ASSERT + expect(result.data.resultData.error?.name).toBe('WorkflowHasIssuesError'); + expect(result.data.resultData.error?.message).toMatch( /^The 'node' node has issues:\n- Parameter "Required Text" is required\.$/, ); + expect(hookNames()).toEqual(['workflowExecuteBefore', 'workflowExecuteAfter']); }); - test('does not complain about nodes with issue past the destination node', () => { + test('does not complain about nodes with issue past the destination node', async () => { // ARRANGE const node1 = createNodeData({ name: 'node1', type: types.passThrough }); const node2 = createNodeData({ name: 'node2', type: types.testNodeWithRequiredProperty }); @@ -435,12 +455,15 @@ describe('processRunExecutionData', () => { }, }); - const workflowExecute = new WorkflowExecute(additionalData, executionMode, executionData); + // ACT + const result = await new WorkflowExecute( + additionalData, + executionMode, + executionData, + ).processRunExecutionData(workflow); - // ACT & ASSERT - // The function returns a Promise, but throws synchronously, so we can't await it. - // eslint-disable-next-line @typescript-eslint/promise-function-async - expect(() => workflowExecute.processRunExecutionData(workflow)).not.toThrowError(); + // ASSERT + expect(result.data.resultData.error).toBeUndefined(); }); }); diff --git a/packages/core/src/execution-engine/workflow-execute.ts b/packages/core/src/execution-engine/workflow-execute.ts index 962835e51b8..e6fb30899e0 100644 --- a/packages/core/src/execution-engine/workflow-execute.ts +++ b/packages/core/src/execution-engine/workflow-execute.ts @@ -2260,7 +2260,12 @@ export class WorkflowExecute { processRunExecutionData(workflow: Workflow): PCancelable { Logger.debug('Workflow execution started', { workflowId: workflow.id }); const { startedAt, hooks } = this.setupExecution(); - this.checkForWorkflowIssues(workflow); + assertExecutionDataExists( + this.runExecutionData.executionData, + workflow, + this.additionalData, + this.mode, + ); this.handleWaitingState(workflow); // Variables which hold temporary data for each node-execution @@ -2279,6 +2284,7 @@ export class WorkflowExecute { // eslint-disable-next-line complexity const returnPromise = (async () => { await this.initializeExecution(workflow, hooks); + this.checkForWorkflowIssues(workflow); executionLoop: while (this.isExecutionStackNotEmpty()) { if (this.shouldStopExecuting()) { diff --git a/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.test.ts b/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.test.ts index 913f118d790..7d556e691f7 100644 --- a/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.test.ts +++ b/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.test.ts @@ -25,6 +25,7 @@ import { createWorkflowDocumentId, } from '@/app/stores/workflowDocument.store'; import { useNodeTypesStore } from '@/app/stores/nodeTypes.store'; +import { mockRestrictedNodeTypes } from '@n8n/frontend-module-type-availability-policies/__tests__/mocks'; import { nodeViewEventBus } from '@/app/event-bus'; import { useReviewRequiredStore } from '@/features/workflow-reviews/reviewRequired.store'; import { useWorkflowReviewStatusStore } from '@/features/workflow-reviews/reviewStatus.store'; @@ -899,6 +900,34 @@ describe('WorkflowHeaderDraftPublishActions', () => { expect(getByTestId('workflow-open-publish-modal-button')).toBeDisabled(); }); + it('should name the restricted node before its parameter issues', () => { + const restrictedNodeWithIssues: INodeUi = { + id: 'http-1', + name: 'HTTP Request', + type: 'n8n-nodes-base.httpRequest', + typeVersion: 4.2, + position: [200, 0], + parameters: {}, + disabled: false, + issues: { parameters: { url: ['Parameter "URL" is required.'] } }, + }; + workflowDocumentStore.setNodes([triggerNode, restrictedNodeWithIssues]); + workflowDocumentStore.setConnections({ + [triggerNode.name]: { + main: [[{ node: restrictedNodeWithIssues.name, type: 'main', index: 0 }]], + }, + }); + mockRestrictedNodeTypes({ [restrictedNodeWithIssues.type]: 'instance' }); + + const { getByTestId, getByText, queryByText } = renderComponent(); + + expect(getByTestId('workflow-open-publish-modal-button')).toBeDisabled(); + expect( + getByText(/An administrator blocked 'HTTP Request' on this instance/), + ).toBeInTheDocument(); + expect(queryByText(/node has issues/)).not.toBeInTheDocument(); + }); + it('should show publish button enabled when there are unpublished changes (versionId mismatch)', () => { workflowDocumentStore.setNodes([triggerNode]); diff --git a/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue b/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue index 4480f1084c8..35c25f21b4f 100644 --- a/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue +++ b/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.vue @@ -38,6 +38,10 @@ import { ProjectTypes } from '@/features/collaboration/projects/projects.types'; import { useProjectsStore } from '@/features/collaboration/projects/projects.store'; import { useWorkflowActivate } from '@/app/composables/useWorkflowActivate'; import { useToast } from '@n8n/composables/useToast'; +import { + describeNodeTypeRestriction, + getNodeTypeRestriction, +} from '@n8n/frontend-module-type-availability-policies'; import { createEventBus } from '@n8n/utils/event-bus'; import type { WorkflowVersionFormModalEventBusEvents } from '@/features/workflows/workflowHistory/components/WorkflowVersionFormModal.vue'; import { useWorkflowHistoryStore } from '@/features/workflows/workflowHistory/workflowHistory.store'; @@ -180,16 +184,20 @@ const hasNodeIssues = computed(() => workflowDocumentStore.value.hasPublishBlock const isWorkflowPublishable = computed(() => containsTrigger.value && !hasNodeIssues.value); -/** Why publishing is blocked, or '' when it is not. Same copy as the Publish tooltip. */ +/** Why publishing is blocked, or '' when it is not. Also the Publish tooltip. */ const publishBlockedReason = computed(() => { if (isWorkflowPublishable.value) return ''; + if (!containsTrigger.value) return i18n.baseText('workflows.publishModal.noTriggerMessage'); - return !containsTrigger.value - ? i18n.baseText('workflows.publishModal.noTriggerMessage') - : i18n.baseText('workflowActivator.showMessage.activeChangedNodesIssuesExistTrue.title', { - interpolate: { count: nodesWithValidationIssues.value.length }, - adjustToNumber: nodesWithValidationIssues.value.length, - }); + for (const node of nodesWithValidationIssues.value) { + const restriction = getNodeTypeRestriction(node.type); + if (restriction) return describeNodeTypeRestriction(node.name, restriction.scope, 'replace'); + } + + return i18n.baseText('workflowActivator.showMessage.activeChangedNodesIssuesExistTrue.title', { + interpolate: { count: nodesWithValidationIssues.value.length }, + adjustToNumber: nodesWithValidationIssues.value.length, + }); }); type WorkflowPublishState = @@ -457,14 +465,7 @@ const publishButtonConfig = computed(() => { loading: false, showIndicator: false, indicatorClass: '', - tooltip: !containsTrigger.value - ? i18n.baseText('workflows.publishModal.noTriggerMessage') - : hasNodeIssues.value - ? i18n.baseText('workflowActivator.showMessage.activeChangedNodesIssuesExistTrue.title', { - interpolate: { count: nodesWithValidationIssues.value.length }, - adjustToNumber: nodesWithValidationIssues.value.length, - }) - : '', + tooltip: publishBlockedReason.value, showVersionInfo: false, }; } @@ -477,12 +478,7 @@ const publishButtonConfig = computed(() => { loading: false, showIndicator: false, indicatorClass: '', - tooltip: !containsTrigger.value - ? i18n.baseText('workflows.publishModal.noTriggerMessage') - : i18n.baseText('workflowActivator.showMessage.activeChangedNodesIssuesExistTrue.title', { - interpolate: { count: nodesWithValidationIssues.value.length }, - adjustToNumber: nodesWithValidationIssues.value.length, - }), + tooltip: publishBlockedReason.value, showVersionInfo: false, }, 'not-published-eligible': { @@ -518,13 +514,7 @@ const publishButtonConfig = computed(() => { loading: false, showIndicator: true, indicatorClass: 'error', - tooltip: i18n.baseText( - 'workflowActivator.showMessage.activeChangedNodesIssuesExistTrue.title', - { - interpolate: { count: nodesWithValidationIssues.value.length }, - adjustToNumber: nodesWithValidationIssues.value.length, - }, - ), + tooltip: publishBlockedReason.value, showVersionInfo: true, }, 'published-invalid-trigger': {