refactor(core): Simplify state in test webhooks (no-changelog) (#8155)

This PR simplifies state in test webhooks so that it can be cached
easily. Caching this state will allow us to start using Redis for manual
webhooks, to support manual webhooks to work in multi-main setup.

- [x] Convert `workflowWebhooks` to a getter - no need to optimize for
deactivation
- [x] Remove array from value in `TestWebhooks.webhookUrls`
- [x] Consolidate `webhookUrls` and `registeredWebhooks`

packages/cli/src/AbstractServer.ts

@@ -214,7 +214,7 @@ export abstract class AbstractServer {
 			// TODO UM: check if this needs validation with user management.
 			this.app.delete(
 				`/${this.restEndpoint}/test-webhook/:id`,
-				send(async (req) => testWebhooks.cancelTestWebhook(req.params.id)),
+				send(async (req) => testWebhooks.cancelWebhook(req.params.id)),
 			);
 		}
 

packages/cli/src/Interfaces.ts

@@ -23,6 +23,7 @@ import type {
 	INodeProperties,
 	IUserSettings,
 	IHttpRequestMethods,
+	IWebhookData,
 } from 'n8n-workflow';
 
 import type { ActiveWorkflowRunner } from '@/ActiveWorkflowRunner';
@@ -746,10 +747,11 @@ export abstract class SecretsProvider {
 
 export type N8nInstanceType = 'main' | 'webhook' | 'worker';
 
-export type RegisteredWebhook = {
+export type WebhookRegistration = {
 	sessionId?: string;
 	timeout: NodeJS.Timeout;
 	workflowEntity: IWorkflowDb;
 	workflow: Workflow;
 	destinationNode?: string;
+	webhook: IWebhookData;
 };

packages/cli/src/TestWebhooks.ts

@@ -15,7 +15,7 @@ import type {
 	IResponseCallbackData,
 	IWebhookManager,
 	IWorkflowDb,
-	RegisteredWebhook,
+	WebhookRegistration,
 	WebhookAccessControlOptions,
 	WebhookRequest,
 } from '@/Interfaces';
@@ -27,6 +27,7 @@ import { TIME } from './constants';
 import { WorkflowMissingIdError } from './errors/workflow-missing-id.error';
 import { WebhookNotFoundError } from './errors/response-errors/webhook-not-found.error';
 import * as NodeExecuteFunctions from 'n8n-core';
+import { removeTrailingSlash } from './utils';
 
 @Service()
 export class TestWebhooks implements IWebhookManager {
@@ -35,16 +36,22 @@ export class TestWebhooks implements IWebhookManager {
 		private readonly nodeTypes: NodeTypes,
 	) {}
 
-	private registeredWebhooks: { [webhookKey: string]: RegisteredWebhook } = {};
+	private registrations: { [webhookKey: string]: WebhookRegistration } = {};
 
-	private workflowWebhooks: { [workflowId: string]: IWebhookData[] } = {};
+	private get webhooksByWorkflow() {
+		const result: { [workflowId: string]: IWebhookData[] } = {};
 
-	private webhookUrls: { [webhookUrl: string]: IWebhookData[] } = {};
+		for (const registration of Object.values(this.registrations)) {
+			result[registration.webhook.workflowId] ||= [];
+			result[registration.webhook.workflowId].push(registration.webhook);
+		}
+
+		return result;
+	}
 
 	/**
-	 * Executes a test-webhook and returns the data. It also makes sure that the
+	 * Return a promise that resolves when the test webhook is called.
-	 * data gets additionally send to the UI. After the request got handled it
+	 * Also inform the FE of the result and remove the test webhook.
-	 * automatically remove the test-webhook.
 	 */
 	async executeWebhook(
 		request: WebhookRequest,
@@ -52,9 +59,7 @@ export class TestWebhooks implements IWebhookManager {
 	): Promise<IResponseCallbackData> {
 		const httpMethod = request.method;
 
-		let path = request.params.path.endsWith('/')
+		let path = removeTrailingSlash(request.params.path);
-			? request.params.path.slice(0, -1)
-			: request.params.path;
 
 		request.params = {} as WebhookRequest['params'];
 
@@ -84,12 +89,9 @@ export class TestWebhooks implements IWebhookManager {
 			});
 		}
 
-		const key = [
+		const key = this.toWebhookKey(webhook);
-			this.toWebhookKey(webhook.httpMethod, webhook.path, webhook.webhookId),
-			webhook.workflowId,
-		].join('|');
 
-		if (!(key in this.registeredWebhooks))
+		if (!this.registrations[key])
 			throw new WebhookNotFoundError({
 				path,
 				httpMethod,
@@ -97,11 +99,10 @@ export class TestWebhooks implements IWebhookManager {
 			});
 
 		const { destinationNode, sessionId, workflow, workflowEntity, timeout } =
-			this.registeredWebhooks[key];
+			this.registrations[key];
 
-		// Get the node which has the webhook defined to know where to start from and to
-		// get additional data
 		const workflowStartNode = workflow.getNode(webhook.node);
+
 		if (workflowStartNode === null) {
 			throw new NotFoundError('Could not find node to process webhook.');
 		}
@@ -144,14 +145,14 @@ export class TestWebhooks implements IWebhookManager {
 
 			// Delete webhook also if an error is thrown
 			if (timeout) clearTimeout(timeout);
-			delete this.registeredWebhooks[key];
+			delete this.registrations[key];
 
-			await this.deactivateWebhooksFor(workflow);
+			await this.deactivateWebhooks(workflow);
 		});
 	}
 
 	async getWebhookMethods(path: string) {
-		const webhookMethods = Object.keys(this.webhookUrls)
+		const webhookMethods = Object.keys(this.registrations)
 			.filter((key) => key.includes(path))
 			.map((key) => key.split('|')[0] as IHttpRequestMethods);
 
@@ -161,13 +162,13 @@ export class TestWebhooks implements IWebhookManager {
 	}
 
 	async findAccessControlOptions(path: string, httpMethod: IHttpRequestMethods) {
-		const webhookKey = Object.keys(this.registeredWebhooks).find(
+		const webhookKey = Object.keys(this.registrations).find(
 			(key) => key.includes(path) && key.startsWith(httpMethod),
 		);
 
 		if (!webhookKey) return;
 
-		const { workflow } = this.registeredWebhooks[webhookKey];
+		const { workflow } = this.registrations[webhookKey];
 		const webhookNode = Object.values(workflow.nodes).find(
 			({ type, parameters, typeVersion }) =>
 				parameters?.path === path &&
@@ -178,6 +179,10 @@ export class TestWebhooks implements IWebhookManager {
 		return webhookNode?.parameters?.options as WebhookAccessControlOptions;
 	}
 
+	/**
+	 * Return whether activating a workflow requires listening for webhook calls.
+	 * For every webhook call to listen for, also activate the webhook.
+	 */
 	async needsWebhook(
 		workflowEntity: IWorkflowDb,
 		workflow: Workflow,
@@ -196,38 +201,40 @@ export class TestWebhooks implements IWebhookManager {
 			true,
 		);
 
-		if (!webhooks.find((w) => w.webhookDescription.restartWebhook !== true)) {
+		if (!webhooks.some((w) => w.webhookDescription.restartWebhook !== true)) {
 			return false; // no webhooks found to start a workflow
 		}
 
-		const timeout = setTimeout(() => {
+		// 1+ webhook(s) required, so activate webhook(s)
-			this.cancelTestWebhook(workflowEntity.id);
+
-		}, 2 * TIME.MINUTE);
+		const timeout = setTimeout(() => this.cancelWebhook(workflow.id), 2 * TIME.MINUTE);
 
 		const activatedKeys: string[] = [];
 
 		for (const webhook of webhooks) {
-			const key = [
+			const key = this.toWebhookKey(webhook);
-				this.toWebhookKey(webhook.httpMethod, webhook.path, webhook.webhookId),
+
-				workflowEntity.id,
+			if (this.registrations[key] && !webhook.webhookId) {
-			].join('|');
+				throw new WebhookPathTakenError(webhook.node);
+			}
 
 			activatedKeys.push(key);
 
-			this.registeredWebhooks[key] = {
+			this.setRegistration({
 				sessionId,
 				timeout,
 				workflow,
 				workflowEntity,
 				destinationNode,
-			};
+				webhook,
+			});
 
 			try {
 				await this.activateWebhook(workflow, webhook, executionMode, activationMode);
 			} catch (error) {
-				activatedKeys.forEach((ak) => delete this.registeredWebhooks[ak]);
+				activatedKeys.forEach((k) => delete this.registrations[k]);
 
-				await this.deactivateWebhooksFor(workflow);
+				await this.deactivateWebhooks(workflow);
 
 				throw error;
 			}
@@ -236,11 +243,11 @@ export class TestWebhooks implements IWebhookManager {
 		return true;
 	}
 
-	cancelTestWebhook(workflowId: string) {
+	cancelWebhook(workflowId: string) {
 		let foundWebhook = false;
 
-		for (const key of Object.keys(this.registeredWebhooks)) {
+		for (const key of Object.keys(this.registrations)) {
-			const { sessionId, timeout, workflow, workflowEntity } = this.registeredWebhooks[key];
+			const { sessionId, timeout, workflow, workflowEntity } = this.registrations[key];
 
 			if (workflowEntity.id !== workflowId) continue;
 
@@ -254,11 +261,11 @@ export class TestWebhooks implements IWebhookManager {
 				}
 			}
 
-			delete this.registeredWebhooks[key];
+			delete this.registrations[key];
 
 			if (!foundWebhook) {
 				// As it removes all webhooks of the workflow execute only once
-				void this.deactivateWebhooksFor(workflow);
+				void this.deactivateWebhooks(workflow);
 			}
 
 			foundWebhook = true;
@@ -273,30 +280,12 @@ export class TestWebhooks implements IWebhookManager {
 		executionMode: WorkflowExecuteMode,
 		activationMode: WorkflowActivateMode,
 	) {
-		if (!workflow.id) throw new WorkflowMissingIdError(workflow);
+		webhook.path = removeTrailingSlash(webhook.path);
 
-		if (webhook.path.endsWith('/')) {
+		const key = this.toWebhookKey(webhook);
-			webhook.path = webhook.path.slice(0, -1);
-		}
 
-		const key = this.toWebhookKey(webhook.httpMethod, webhook.path, webhook.webhookId);
-
-		// check that there is not a webhook already registered with that path/method
-		if (this.webhookUrls[key] && !webhook.webhookId) {
-			throw new WebhookPathTakenError(webhook.node);
-		}
-
-		if (this.workflowWebhooks[webhook.workflowId] === undefined) {
-			this.workflowWebhooks[webhook.workflowId] = [];
-		}
-
-		// Make the webhook available directly because sometimes to create it successfully
-		// it gets called
-		if (!this.webhookUrls[key]) {
-			this.webhookUrls[key] = [];
-		}
 		webhook.isTest = true;
-		this.webhookUrls[key].push(webhook);
+		this.registrations[key].webhook = webhook;
 
 		try {
 			await workflow.createWebhookIfNotExists(
@@ -306,21 +295,15 @@ export class TestWebhooks implements IWebhookManager {
 				activationMode,
 			);
 		} catch (error) {
-			// If there was a problem unregister the webhook again
+			if (this.registrations[key]) delete this.registrations[key];
-			if (this.webhookUrls[key].length <= 1) {
-				delete this.webhookUrls[key];
-			} else {
-				this.webhookUrls[key] = this.webhookUrls[key].filter((w) => w.path !== w.path);
-			}
 
 			throw error;
 		}
-		this.workflowWebhooks[webhook.workflowId].push(webhook);
 	}
 
 	getActiveWebhook(httpMethod: IHttpRequestMethods, path: string, webhookId?: string) {
-		const webhookKey = this.toWebhookKey(httpMethod, path, webhookId);
+		const key = this.toWebhookKey({ httpMethod, path, webhookId });
-		if (this.webhookUrls[webhookKey] === undefined) {
+		if (this.registrations[key] === undefined) {
 			return undefined;
 		}
 
@@ -329,25 +312,29 @@ export class TestWebhooks implements IWebhookManager {
 		const pathElementsSet = new Set(path.split('/'));
 		// check if static elements match in path
 		// if more results have been returned choose the one with the most static-route matches
-		this.webhookUrls[webhookKey].forEach((dynamicWebhook) => {
+		const dynamicWebhook = this.registrations[key].webhook;
-			const staticElements = dynamicWebhook.path.split('/').filter((ele) => !ele.startsWith(':'));
-			const allStaticExist = staticElements.every((staticEle) => pathElementsSet.has(staticEle));
 
-			if (allStaticExist && staticElements.length > maxMatches) {
+		const staticElements = dynamicWebhook.path.split('/').filter((ele) => !ele.startsWith(':'));
-				maxMatches = staticElements.length;
+		const allStaticExist = staticElements.every((staticEle) => pathElementsSet.has(staticEle));
-				webhook = dynamicWebhook;
+
-			}
+		if (allStaticExist && staticElements.length > maxMatches) {
-			// handle routes with no static elements
+			maxMatches = staticElements.length;
-			else if (staticElements.length === 0 && !webhook) {
+			webhook = dynamicWebhook;
-				webhook = dynamicWebhook;
+		}
-			}
+		// handle routes with no static elements
-		});
+		else if (staticElements.length === 0 && !webhook) {
+			webhook = dynamicWebhook;
+		}
 
 		return webhook;
 	}
 
-	toWebhookKey(httpMethod: IHttpRequestMethods, path: string, webhookId?: string) {
+	private toWebhookKey(webhook: Pick<IWebhookData, 'webhookId' | 'httpMethod' | 'path'>) {
-		if (!webhookId) return `${httpMethod}|${path}`;
+		const { webhookId, httpMethod, path: webhookPath } = webhook;
+
+		if (!webhookId) return `${httpMethod}|${webhookPath}`;
+
+		let path = webhookPath;
 
 		if (path.startsWith(webhookId)) {
 			const cutFromIndex = path.indexOf('/') + 1;
@@ -358,35 +345,32 @@ export class TestWebhooks implements IWebhookManager {
 		return `${httpMethod}|${webhookId}|${path.split('/').length}`;
 	}
 
-	async deactivateWebhooksFor(workflow: Workflow) {
+	/**
-		const workflowId = workflow.id;
+	 * Deactivate all registered webhooks of a workflow.
+	 */
+	async deactivateWebhooks(workflow: Workflow) {
+		const webhooks = this.webhooksByWorkflow[workflow.id];
 
-		if (this.workflowWebhooks[workflowId] === undefined) {
+		if (!webhooks) return false; // nothing to deactivate
-			// If it did not exist then there is nothing to remove
+
-			return false;
+		for (const webhook of webhooks) {
+			await workflow.deleteWebhook(webhook, NodeExecuteFunctions, 'internal', 'update');
+
+			const key = this.toWebhookKey(webhook);
+
+			delete this.registrations[key];
 		}
 
-		const webhooks = this.workflowWebhooks[workflowId];
-
-		const mode = 'internal';
-
-		// Go through all the registered webhooks of the workflow and remove them
-
-		for (const webhookData of webhooks) {
-			await workflow.deleteWebhook(webhookData, NodeExecuteFunctions, mode, 'update');
-
-			const key = this.toWebhookKey(
-				webhookData.httpMethod,
-				webhookData.path,
-				webhookData.webhookId,
-			);
-
-			delete this.webhookUrls[key];
-		}
-
-		// Remove also the workflow-webhook entry
-		delete this.workflowWebhooks[workflowId];
-
 		return true;
 	}
+
+	clearRegistrations() {
+		this.registrations = {};
+	}
+
+	setRegistration(registration: WebhookRegistration) {
+		const key = this.toWebhookKey(registration.webhook);
+
+		this.registrations[key] = registration;
+	}
 }

packages/cli/src/utils.ts

@@ -73,3 +73,7 @@ export const isIntegerString = (value: string) => /^\d+$/.test(value);
 export function isObjectLiteral(item: unknown): item is { [key: string]: string } {
 	return typeof item === 'object' && item !== null && !Array.isArray(item);
 }
+
+export function removeTrailingSlash(path: string) {
+	return path.endsWith('/') ? path.slice(0, -1) : path;
+}

packages/cli/test/unit/TestWebhooks.test.ts

@@ -1,6 +1,4 @@
-import { mockInstance } from '../shared/mocking';
+import { mock } from 'jest-mock-extended';
-import { NodeTypes } from '@/NodeTypes';
-import { Push } from '@/push';
 import { TestWebhooks } from '@/TestWebhooks';
 import { WebhookNotFoundError } from '@/errors/response-errors/webhook-not-found.error';
 import { v4 as uuid } from 'uuid';
@@ -8,8 +6,7 @@ import { generateNanoId } from '@/databases/utils/generators';
 import { NotFoundError } from '@/errors/response-errors/not-found.error';
 import * as WebhookHelpers from '@/WebhookHelpers';
 
-import type { IWorkflowDb, WebhookRequest } from '@/Interfaces';
+import type { IWorkflowDb, WebhookRegistration, WebhookRequest } from '@/Interfaces';
-import type express from 'express';
 import type {
 	IWebhookData,
 	IWorkflowExecuteAdditionalData,
@@ -19,31 +16,27 @@ import type {
 } from 'n8n-workflow';
 
 describe('TestWebhooks', () => {
-	jest.useFakeTimers();
+	const testWebhooks = new TestWebhooks(mock(), mock());
 
-	const push = mockInstance(Push);
+	beforeAll(() => {
-	const nodeTypes = mockInstance(NodeTypes);
+		jest.useFakeTimers();
-
+	});
-	const testWebhooks = new TestWebhooks(push, nodeTypes);
 
 	afterEach(() => {
-		jest.clearAllMocks();
+		testWebhooks.clearRegistrations();
+	});
+
+	const httpMethod = 'GET';
+	const path = uuid();
+	const workflowId = generateNanoId();
+
+	const webhook = mock<IWebhookData>({
+		httpMethod,
+		path,
+		workflowId,
 	});
 
 	describe('needsWebhook()', () => {
-		const httpMethod = 'GET';
-		const path = uuid();
-		const workflowId = generateNanoId();
-
-		const webhook = {
-			httpMethod,
-			path,
-			workflowId,
-			webhookDescription: {},
-		} as IWebhookData;
-
-		const keyPart = [httpMethod, path].join('|');
-
 		type NeedsWebhookArgs = [
 			IWorkflowDb,
 			Workflow,
@@ -52,23 +45,18 @@ describe('TestWebhooks', () => {
 			WorkflowActivateMode,
 		];
 
-		const workflow = {
+		const workflow = mock<Workflow>({ id: workflowId });
-			id: workflowId,
-			createWebhookIfNotExists: () => {},
-			deleteWebhook: () => {},
-		} as unknown as Workflow;
 
 		const args: NeedsWebhookArgs = [
-			{ id: workflowId } as unknown as IWorkflowDb,
+			mock<IWorkflowDb>({ id: workflowId }),
 			workflow,
-			{} as unknown as IWorkflowExecuteAdditionalData,
+			mock<IWorkflowExecuteAdditionalData>(),
 			'manual',
 			'manual',
 		];
 
-		test('should register a webhook as active', async () => {
+		test('should return true and activate webhook if needed', async () => {
 			jest.spyOn(WebhookHelpers, 'getWorkflowWebhooks').mockReturnValue([webhook]);
-			jest.spyOn(testWebhooks, 'toWebhookKey').mockReturnValue(keyPart);
 			const activateWebhookSpy = jest.spyOn(testWebhooks, 'activateWebhook');
 
 			const needsWebhook = await testWebhooks.needsWebhook(...args);
@@ -77,18 +65,17 @@ describe('TestWebhooks', () => {
 			expect(activateWebhookSpy).toHaveBeenCalledWith(workflow, webhook, 'manual', 'manual');
 		});
 
-		test('should remove from active webhooks on failure to add', async () => {
+		test('should deactivate webhooks on failure to activate', async () => {
 			const msg = 'Failed to add webhook to active webhooks';
 
 			jest.spyOn(WebhookHelpers, 'getWorkflowWebhooks').mockReturnValue([webhook]);
-			jest.spyOn(testWebhooks, 'toWebhookKey').mockReturnValue(keyPart);
 			jest.spyOn(testWebhooks, 'activateWebhook').mockRejectedValue(new Error(msg));
-			const deactivateSpy = jest.spyOn(testWebhooks, 'deactivateWebhooksFor');
+			const deactivateWebhooksSpy = jest.spyOn(testWebhooks, 'deactivateWebhooks');
 
 			const needsWebhook = testWebhooks.needsWebhook(...args);
 
 			await expect(needsWebhook).rejects.toThrowError(msg);
-			expect(deactivateSpy).toHaveBeenCalledWith(workflow);
+			expect(deactivateWebhooksSpy).toHaveBeenCalledWith(workflow);
 		});
 
 		test('should return false if no webhook to start workflow', async () => {
@@ -102,26 +89,14 @@ describe('TestWebhooks', () => {
 	});
 
 	describe('executeWebhook()', () => {
-		const httpMethod = 'GET';
-		const path = uuid();
-		const workflowId = generateNanoId();
-
-		const webhook = {
-			httpMethod,
-			path,
-			workflowId,
-		} as IWebhookData;
-
-		const keyPart = [httpMethod, path].join('|');
-
 		test('should throw if webhook is not registered', async () => {
 			jest.spyOn(testWebhooks, 'getActiveWebhook').mockReturnValue(webhook);
 			jest.spyOn(testWebhooks, 'getWebhookMethods').mockResolvedValue([]);
-			jest.spyOn(testWebhooks, 'toWebhookKey').mockReturnValue(keyPart);
 
-			const request = { params: { path } } as WebhookRequest;
+			const promise = testWebhooks.executeWebhook(
-			const response = {} as express.Response;
+				mock<WebhookRequest>({ params: { path } }),
-			const promise = testWebhooks.executeWebhook(request, response);
+				mock(),
+			);
 
 			await expect(promise).rejects.toThrowError(WebhookNotFoundError);
 		});
@@ -129,26 +104,22 @@ describe('TestWebhooks', () => {
 		test('should throw if webhook node is registered but missing from workflow', async () => {
 			jest.spyOn(testWebhooks, 'getActiveWebhook').mockReturnValue(webhook);
 			jest.spyOn(testWebhooks, 'getWebhookMethods').mockResolvedValue([]);
-			jest.spyOn(testWebhooks, 'toWebhookKey').mockReturnValue(keyPart);
 
-			// @ts-expect-error Private property
+			const registration = mock<WebhookRegistration>({
-			testWebhooks.registeredWebhooks[`${keyPart}|${workflowId}`] = {
 				sessionId: 'some-session-id',
-				timeout: setTimeout(() => {}, 0),
+				timeout: mock<NodeJS.Timeout>(),
-				workflowEntity: {} as IWorkflowDb,
+				workflowEntity: mock<IWorkflowDb>({}),
-				workflow: {
+				workflow: mock<Workflow>(),
-					getNode: () => null,
+			});
-				} as unknown as Workflow,
-			};
 
-			const request = { params: { path } } as WebhookRequest;
+			testWebhooks.setRegistration(registration);
-			const response = {} as express.Response;
+
-			const promise = testWebhooks.executeWebhook(request, response);
+			const promise = testWebhooks.executeWebhook(
+				mock<WebhookRequest>({ params: { path } }),
+				mock(),
+			);
 
 			await expect(promise).rejects.toThrowError(NotFoundError);
-
-			// @ts-expect-error Private property
-			delete testWebhooks.registeredWebhooks[`${keyPart}|${workflowId}`];
 		});
 	});
 });