Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): Adjust starter node priority for manual executions with pinned activators #8386

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 41 additions & 23 deletions packages/cli/src/workflows/workflowExecution.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export class WorkflowExecutionService {
user: User,
sessionId?: string,
) {
const pinnedTrigger = this.findPinnedTrigger(workflowData, startNodes, pinData);
const pinnedTrigger = this.selectPinnedActivatorStarter(workflowData, startNodes, pinData);

// If webhooks nodes exist and are active we have to wait for till we receive a call
if (
Expand Down Expand Up @@ -243,44 +243,62 @@ export class WorkflowExecutionService {
}

/**
* Find the pinned trigger to execute the workflow from, if any.
* Select the pinned activator node to use as starter for a manual execution.
*
* - In a full execution, select the _first_ pinned trigger.
* - In a partial execution,
* - select the _first_ pinned trigger that leads to the executed node,
* - else select the executed pinned trigger.
* In a full manual execution, select the pinned activator that was first added
* to the workflow, prioritizing `n8n-nodes-base.webhook` over other activators.
*
* In a partial manual execution, if the executed node has parent nodes among the
* pinned activators, select the pinned activator that was first added to the workflow,
* prioritizing `n8n-nodes-base.webhook` over other activators. If the executed node
* has no upstream nodes and is itself is a pinned activator, select it.
*/
private findPinnedTrigger(workflow: IWorkflowDb, startNodes?: string[], pinData?: IPinData) {
selectPinnedActivatorStarter(workflow: IWorkflowDb, startNodes?: string[], pinData?: IPinData) {
if (!pinData || !startNodes) return null;

const isTrigger = (nodeTypeName: string) =>
['trigger', 'webhook'].some((suffix) => nodeTypeName.toLowerCase().includes(suffix));
const allPinnedActivators = this.findAllPinnedActivators(workflow, pinData);

const [firstPinnedActivator] = allPinnedActivators;

const pinnedTriggers = workflow.nodes.filter(
(node) => !node.disabled && pinData[node.name] && isTrigger(node.type),
);
// full manual execution

if (pinnedTriggers.length === 0) return null;
if (startNodes?.length === 0) return firstPinnedActivator ?? null;

if (startNodes?.length === 0) return pinnedTriggers[0]; // full execution
// partial manual execution

const [startNodeName] = startNodes;
/**
* If the partial manual execution has 2+ start nodes, we search only the zeroth
* start node's parents for a pinned activator. If we had 2+ start nodes without
* a common ancestor and so if we end up finding multiple pinned activators, we
* would still need to return one to comply with existing usage.
*/
const [firstStartNodeName] = startNodes;

const parentNames = new Workflow({
const parentNodeNames = new Workflow({
nodes: workflow.nodes,
connections: workflow.connections,
active: workflow.active,
nodeTypes: this.nodeTypes,
}).getParentNodes(startNodeName);
}).getParentNodes(firstStartNodeName);

let checkNodeName = '';
if (parentNodeNames.length > 0) {
const parentNodeName = parentNodeNames.find((p) => p === firstPinnedActivator.name);

if (parentNames.length === 0) {
checkNodeName = startNodeName;
} else {
checkNodeName = parentNames.find((pn) => pn === pinnedTriggers[0].name) as string;
return allPinnedActivators.find((pa) => pa.name === parentNodeName) ?? null;
}

return pinnedTriggers.find((pt) => pt.name === checkNodeName) ?? null; // partial execution
return allPinnedActivators.find((pa) => pa.name === firstStartNodeName) ?? null;
}

private findAllPinnedActivators(workflow: IWorkflowDb, pinData?: IPinData) {
return workflow.nodes
.filter(
(node) =>
!node.disabled &&
pinData?.[node.name] &&
['trigger', 'webhook'].some((suffix) => node.type.toLowerCase().endsWith(suffix)) &&
node.type !== 'n8n-nodes-base.respondToWebhook',
)
.sort((a) => (a.type.endsWith('webhook') ? -1 : 1));
}
}
137 changes: 137 additions & 0 deletions packages/cli/test/unit/workflow-execution.service.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
import type { INode } from 'n8n-workflow';
import { WorkflowExecutionService } from '@/workflows/workflowExecution.service';
import type { IWorkflowDb } from '@/Interfaces';
import { mock } from 'jest-mock-extended';

const webhookNode: INode = {
name: 'Webhook',
type: 'n8n-nodes-base.webhook',
id: '111f1db0-e7be-44c5-9ce9-3e35362490f0',
parameters: {},
typeVersion: 1,
position: [0, 0],
webhookId: 'de0f8dcb-7b64-4f22-b66d-d8f74d6aefb7',
};

const secondWebhookNode = {
...webhookNode,
name: 'Webhook 2',
id: '222f1db0-e7be-44c5-9ce9-3e35362490f1',
};

const executeWorkflowTriggerNode: INode = {
name: 'Execute Workflow Trigger',
type: 'n8n-nodes-base.executeWorkflowTrigger',
id: '78d63bca-bb6c-4568-948f-8ed9aacb1fe9',
parameters: {},
typeVersion: 1,
position: [0, 0],
};

const respondToWebhookNode: INode = {
name: 'Respond to Webhook',
type: 'n8n-nodes-base.respondToWebhook',
id: '66d63bca-bb6c-4568-948f-8ed9aacb1fe9',
parameters: {},
typeVersion: 1,
position: [0, 0],
};

const hackerNewsNode: INode = {
name: 'Hacker News',
type: 'n8n-nodes-base.hackerNews',
id: '55d63bca-bb6c-4568-948f-8ed9aacb1fe9',
parameters: {},
typeVersion: 1,
position: [0, 0],
};

describe('WorkflowExecutionService', () => {
let workflowExecutionService: WorkflowExecutionService;

beforeAll(() => {
workflowExecutionService = new WorkflowExecutionService(
mock(),
mock(),
mock(),
mock(),
mock(),
mock(),
);
});

describe('selectPinnedActivatorStarter()', () => {
const workflow = mock<IWorkflowDb>({
nodes: [],
});

const pinData = {
[webhookNode.name]: [{ json: { key: 'value' } }],
[executeWorkflowTriggerNode.name]: [{ json: { key: 'value' } }],
};

afterEach(() => {
workflow.nodes = [];
});

it('should return `null` if no pindata', () => {
const node = workflowExecutionService.selectPinnedActivatorStarter(workflow, []);

expect(node).toBeNull();
});

it('should return `null` if no starter nodes', () => {
const node = workflowExecutionService.selectPinnedActivatorStarter(workflow);

expect(node).toBeNull();
});

it('should select webhook node if only choice', () => {
workflow.nodes.push(webhookNode);

const node = workflowExecutionService.selectPinnedActivatorStarter(workflow, [], pinData);

expect(node).toEqual(webhookNode);
});

it('should return `null` if no choice', () => {
workflow.nodes.push(hackerNewsNode);

const node = workflowExecutionService.selectPinnedActivatorStarter(workflow, [], pinData);

expect(node).toBeNull();
});

it('should return ignore Respond to Webhook', () => {
workflow.nodes.push(respondToWebhookNode);

const node = workflowExecutionService.selectPinnedActivatorStarter(workflow, [], pinData);

expect(node).toBeNull();
});

it('should select execute workflow trigger if only choice', () => {
workflow.nodes.push(executeWorkflowTriggerNode);

const node = workflowExecutionService.selectPinnedActivatorStarter(workflow, [], pinData);

expect(node).toEqual(executeWorkflowTriggerNode);
});

it('should favor webhook node over execute workflow trigger', () => {
workflow.nodes.push(webhookNode, executeWorkflowTriggerNode);

const node = workflowExecutionService.selectPinnedActivatorStarter(workflow, [], pinData);

expect(node).toEqual(webhookNode);
});

it('should favor first webhook node over second webhook node', () => {
workflow.nodes.push(webhookNode, secondWebhookNode);

const node = workflowExecutionService.selectPinnedActivatorStarter(workflow, [], pinData);

expect(node).toEqual(webhookNode);
});
});
});
Loading