From 9dc491c3a5b3cc1b57619f358b7d3185f9035bd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Tue, 19 Dec 2023 17:32:02 +0100 Subject: [PATCH] refactor(core): Improve test-webhooks (no-changelog) (#8069) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove duplication, improve readability, and expand tests for `TestWebhooks.ts` - in anticipation for storing test webhooks in Redis. --------- Co-authored-by: कारतोफ्फेलस्क्रिप्ट™ --- packages/cli/src/ActiveWebhooks.ts | 205 ---------- packages/cli/src/ActiveWorkflowRunner.ts | 13 +- packages/cli/src/Interfaces.ts | 13 +- packages/cli/src/TestWebhooks.ts | 353 +++++++++++------- .../src/databases/entities/WebhookEntity.ts | 10 +- .../webhook-not-found.error.ts | 57 +++ .../src/errors/workflow-missing-id.error.ts | 8 + packages/cli/src/utils.ts | 28 -- .../cli/src/workflows/workflow.service.ts | 9 +- packages/cli/test/unit/TestWebhooks.test.ts | 154 ++++++++ packages/cli/test/unit/utils.test.ts | 24 +- packages/core/src/NodeExecuteFunctions.ts | 3 +- packages/workflow/src/Interfaces.ts | 2 +- packages/workflow/src/Workflow.ts | 23 +- 14 files changed, 486 insertions(+), 416 deletions(-) delete mode 100644 packages/cli/src/ActiveWebhooks.ts create mode 100644 packages/cli/src/errors/response-errors/webhook-not-found.error.ts create mode 100644 packages/cli/src/errors/workflow-missing-id.error.ts create mode 100644 packages/cli/test/unit/TestWebhooks.test.ts diff --git a/packages/cli/src/ActiveWebhooks.ts b/packages/cli/src/ActiveWebhooks.ts deleted file mode 100644 index 73eecd0379..0000000000 --- a/packages/cli/src/ActiveWebhooks.ts +++ /dev/null @@ -1,205 +0,0 @@ -import { Service } from 'typedi'; -import type { - IWebhookData, - IHttpRequestMethods, - Workflow, - WorkflowActivateMode, - WorkflowExecuteMode, -} from 'n8n-workflow'; -import { ApplicationError, WebhookPathTakenError } from 'n8n-workflow'; -import * as NodeExecuteFunctions from 'n8n-core'; - -@Service() -export class ActiveWebhooks { - private workflowWebhooks: { - [key: string]: IWebhookData[]; - } = {}; - - private webhookUrls: { - [key: string]: IWebhookData[]; - } = {}; - - testWebhooks = false; - - /** - * Adds a new webhook - * - */ - async add( - workflow: Workflow, - webhookData: IWebhookData, - mode: WorkflowExecuteMode, - activation: WorkflowActivateMode, - ): Promise { - if (workflow.id === undefined) { - throw new ApplicationError( - 'Webhooks can only be added for saved workflows as an ID is needed', - ); - } - if (webhookData.path.endsWith('/')) { - webhookData.path = webhookData.path.slice(0, -1); - } - - const webhookKey = this.getWebhookKey( - webhookData.httpMethod, - webhookData.path, - webhookData.webhookId, - ); - - // check that there is not a webhook already registered with that path/method - if (this.webhookUrls[webhookKey] && !webhookData.webhookId) { - throw new WebhookPathTakenError(webhookData.node); - } - - if (this.workflowWebhooks[webhookData.workflowId] === undefined) { - this.workflowWebhooks[webhookData.workflowId] = []; - } - - // Make the webhook available directly because sometimes to create it successfully - // it gets called - if (!this.webhookUrls[webhookKey]) { - this.webhookUrls[webhookKey] = []; - } - this.webhookUrls[webhookKey].push(webhookData); - - try { - await workflow.createWebhookIfNotExists( - webhookData, - NodeExecuteFunctions, - mode, - activation, - this.testWebhooks, - ); - } catch (error) { - // If there was a problem unregister the webhook again - if (this.webhookUrls[webhookKey].length <= 1) { - delete this.webhookUrls[webhookKey]; - } else { - this.webhookUrls[webhookKey] = this.webhookUrls[webhookKey].filter( - (webhook) => webhook.path !== webhookData.path, - ); - } - - throw error; - } - this.workflowWebhooks[webhookData.workflowId].push(webhookData); - } - - /** - * Returns webhookData if a webhook with matches is currently registered - * - * @param {(string | undefined)} webhookId - */ - get(httpMethod: IHttpRequestMethods, path: string, webhookId?: string): IWebhookData | undefined { - const webhookKey = this.getWebhookKey(httpMethod, path, webhookId); - if (this.webhookUrls[webhookKey] === undefined) { - return undefined; - } - - let webhook: IWebhookData | undefined; - let maxMatches = 0; - 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 staticElements = dynamicWebhook.path.split('/').filter((ele) => !ele.startsWith(':')); - const allStaticExist = staticElements.every((staticEle) => pathElementsSet.has(staticEle)); - - if (allStaticExist && staticElements.length > maxMatches) { - maxMatches = staticElements.length; - webhook = dynamicWebhook; - } - // handle routes with no static elements - else if (staticElements.length === 0 && !webhook) { - webhook = dynamicWebhook; - } - }); - - return webhook; - } - - /** - * Gets all request methods associated with a single webhook - */ - getWebhookMethods(path: string): IHttpRequestMethods[] { - return Object.keys(this.webhookUrls) - .filter((key) => key.includes(path)) - .map((key) => key.split('|')[0] as IHttpRequestMethods); - } - - /** - * Returns the ids of all the workflows which have active webhooks - * - */ - getWorkflowIds(): string[] { - return Object.keys(this.workflowWebhooks); - } - - /** - * Returns key to uniquely identify a webhook - * - * @param {(string | undefined)} webhookId - */ - getWebhookKey(httpMethod: IHttpRequestMethods, path: string, webhookId?: string): string { - if (webhookId) { - if (path.startsWith(webhookId)) { - const cutFromIndex = path.indexOf('/') + 1; - - path = path.slice(cutFromIndex); - } - return `${httpMethod}|${webhookId}|${path.split('/').length}`; - } - return `${httpMethod}|${path}`; - } - - /** - * Removes all webhooks of a workflow - * - */ - async removeWorkflow(workflow: Workflow): Promise { - const workflowId = workflow.id; - - if (this.workflowWebhooks[workflowId] === undefined) { - // If it did not exist then there is nothing to remove - return false; - } - - 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', - this.testWebhooks, - ); - - delete this.webhookUrls[ - this.getWebhookKey(webhookData.httpMethod, webhookData.path, webhookData.webhookId) - ]; - } - - // Remove also the workflow-webhook entry - delete this.workflowWebhooks[workflowId]; - - return true; - } - - /** - * Removes all the webhooks of the given workflows - */ - async removeAll(workflows: Workflow[]): Promise { - const removePromises = []; - - for (const workflow of workflows) { - removePromises.push(this.removeWorkflow(workflow)); - } - - await Promise.all(removePromises); - } -} diff --git a/packages/cli/src/ActiveWorkflowRunner.ts b/packages/cli/src/ActiveWorkflowRunner.ts index a12ea36c5b..5b4a15f40c 100644 --- a/packages/cli/src/ActiveWorkflowRunner.ts +++ b/packages/cli/src/ActiveWorkflowRunner.ts @@ -60,7 +60,7 @@ import { WorkflowRunner } from '@/WorkflowRunner'; import { ExternalHooks } from '@/ExternalHooks'; import { whereClause } from './UserManagement/UserManagementHelper'; import { WorkflowService } from './workflows/workflow.service'; -import { webhookNotFoundErrorMessage } from './utils'; +import { WebhookNotFoundError } from './errors/response-errors/webhook-not-found.error'; import { In } from 'typeorm'; import { WebhookService } from './services/webhook.service'; import { Logger } from './Logger'; @@ -71,9 +71,6 @@ import { ActivationErrorsService } from '@/ActivationErrors.service'; import type { Scope } from '@n8n/permissions'; import { NotFoundError } from './errors/response-errors/not-found.error'; -const WEBHOOK_PROD_UNREGISTERED_HINT = - "The workflow must be active for a production URL to run successfully. You can activate the workflow using the toggle in the top-right of the editor. Note that unlike test URL calls, production URL calls aren't shown on the canvas (only in the executions list)"; - @Service() export class ActiveWorkflowRunner implements IWebhookManager { activeWorkflows = new ActiveWorkflows(); @@ -256,10 +253,7 @@ export class ActiveWorkflowRunner implements IWebhookManager { const webhook = await this.webhookService.findWebhook(httpMethod, path); if (webhook === null) { - throw new NotFoundError( - webhookNotFoundErrorMessage(path, httpMethod), - WEBHOOK_PROD_UNREGISTERED_HINT, - ); + throw new WebhookNotFoundError({ path, httpMethod }, { hint: 'production' }); } return webhook; @@ -383,7 +377,6 @@ export class ActiveWorkflowRunner implements IWebhookManager { NodeExecuteFunctions, mode, activation, - false, ); } catch (error) { if (activation === 'init' && error.name === 'QueryFailedError') { @@ -455,7 +448,7 @@ export class ActiveWorkflowRunner implements IWebhookManager { const webhooks = WebhookHelpers.getWorkflowWebhooks(workflow, additionalData, undefined, true); for (const webhookData of webhooks) { - await workflow.deleteWebhook(webhookData, NodeExecuteFunctions, mode, 'update', false); + await workflow.deleteWebhook(webhookData, NodeExecuteFunctions, mode, 'update'); } await Container.get(WorkflowService).saveStaticData(workflow); diff --git a/packages/cli/src/Interfaces.ts b/packages/cli/src/Interfaces.ts index b57836d397..05231828e8 100644 --- a/packages/cli/src/Interfaces.ts +++ b/packages/cli/src/Interfaces.ts @@ -261,7 +261,10 @@ export interface IExternalHooksClass { export type WebhookCORSRequest = Request & { method: 'OPTIONS' }; -export type WebhookRequest = Request<{ path: string }> & { method: IHttpRequestMethods }; +export type WebhookRequest = Request<{ path: string }> & { + method: IHttpRequestMethods; + params: Record; +}; export type WaitingWebhookRequest = WebhookRequest & { params: WebhookRequest['path'] & { suffix?: string }; @@ -874,3 +877,11 @@ export abstract class SecretsProvider { } export type N8nInstanceType = 'main' | 'webhook' | 'worker'; + +export type RegisteredWebhook = { + sessionId?: string; + timeout: NodeJS.Timeout; + workflowEntity: IWorkflowDb; + workflow: Workflow; + destinationNode?: string; +}; diff --git a/packages/cli/src/TestWebhooks.ts b/packages/cli/src/TestWebhooks.ts index a889bfbd30..a4da7e7ad4 100644 --- a/packages/cli/src/TestWebhooks.ts +++ b/packages/cli/src/TestWebhooks.ts @@ -8,45 +8,38 @@ import { type Workflow, type WorkflowActivateMode, type WorkflowExecuteMode, - ApplicationError, + WebhookPathTakenError, } from 'n8n-workflow'; -import { ActiveWebhooks } from '@/ActiveWebhooks'; import type { IResponseCallbackData, IWebhookManager, IWorkflowDb, + RegisteredWebhook, WebhookAccessControlOptions, WebhookRequest, } from '@/Interfaces'; import { Push } from '@/push'; import { NodeTypes } from '@/NodeTypes'; import * as WebhookHelpers from '@/WebhookHelpers'; -import { webhookNotFoundErrorMessage } from './utils'; import { NotFoundError } from './errors/response-errors/not-found.error'; - -const WEBHOOK_TEST_UNREGISTERED_HINT = - "Click the 'Execute workflow' button on the canvas, then try again. (In test mode, the webhook only works for one call after you click this button)"; +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'; @Service() export class TestWebhooks implements IWebhookManager { - private testWebhookData: { - [key: string]: { - sessionId?: string; - timeout: NodeJS.Timeout; - workflowData: IWorkflowDb; - workflow: Workflow; - destinationNode?: string; - }; - } = {}; - constructor( - private readonly activeWebhooks: ActiveWebhooks, private readonly push: Push, private readonly nodeTypes: NodeTypes, - ) { - activeWebhooks.testWebhooks = true; - } + ) {} + + private registeredWebhooks: { [webhookKey: string]: RegisteredWebhook } = {}; + + private workflowWebhooks: { [workflowId: string]: IWebhookData[] } = {}; + + private webhookUrls: { [webhookUrl: string]: IWebhookData[] } = {}; /** * Executes a test-webhook and returns the data. It also makes sure that the @@ -58,69 +51,57 @@ export class TestWebhooks implements IWebhookManager { response: express.Response, ): Promise { const httpMethod = request.method; - let path = request.params.path; - // Reset request parameters + let path = request.params.path.endsWith('/') + ? request.params.path.slice(0, -1) + : request.params.path; + request.params = {} as WebhookRequest['params']; - // Remove trailing slash - if (path.endsWith('/')) { - path = path.slice(0, -1); - } + let webhook = this.getActiveWebhook(httpMethod, path); - const { activeWebhooks, push, testWebhookData } = this; + if (!webhook) { + // no static webhook, so check if dynamic + // e.g. `/webhook-test//user/:id/create` - let webhookData: IWebhookData | undefined = activeWebhooks.get(httpMethod, path); + const [webhookId, ...segments] = path.split('/'); - // check if path is dynamic - if (webhookData === undefined) { - const pathElements = path.split('/'); - const webhookId = pathElements.shift(); + webhook = this.getActiveWebhook(httpMethod, segments.join('/'), webhookId); - webhookData = activeWebhooks.get(httpMethod, pathElements.join('/'), webhookId); - if (webhookData === undefined) { - // The requested webhook is not registered - const methods = await this.getWebhookMethods(path); - throw new NotFoundError( - webhookNotFoundErrorMessage(path, httpMethod, methods), - WEBHOOK_TEST_UNREGISTERED_HINT, - ); - } + if (!webhook) + throw new WebhookNotFoundError({ + path, + httpMethod, + webhookMethods: await this.getWebhookMethods(path), + }); - path = webhookData.path; - // extracting params from path - path.split('/').forEach((ele, index) => { - if (ele.startsWith(':')) { - // write params to req.params - // @ts-ignore - request.params[ele.slice(1)] = pathElements[index]; + path = webhook.path; + + path.split('/').forEach((segment, index) => { + if (segment.startsWith(':')) { + request.params[segment.slice(1)] = segments[index]; } }); } - const { workflowId } = webhookData; - const webhookKey = `${activeWebhooks.getWebhookKey( - webhookData.httpMethod, - webhookData.path, - webhookData.webhookId, - )}|${workflowId}`; + const key = [ + this.toWebhookKey(webhook.httpMethod, webhook.path, webhook.webhookId), + webhook.workflowId, + ].join('|'); - // TODO: Clean that duplication up one day and improve code generally - if (testWebhookData[webhookKey] === undefined) { - // The requested webhook is not registered - const methods = await this.getWebhookMethods(path); - throw new NotFoundError( - webhookNotFoundErrorMessage(path, httpMethod, methods), - WEBHOOK_TEST_UNREGISTERED_HINT, - ); - } + if (!(key in this.registeredWebhooks)) + throw new WebhookNotFoundError({ + path, + httpMethod, + webhookMethods: await this.getWebhookMethods(path), + }); - const { destinationNode, sessionId, workflow, workflowData, timeout } = - testWebhookData[webhookKey]; + const { destinationNode, sessionId, workflow, workflowEntity, timeout } = + this.registeredWebhooks[key]; // Get the node which has the webhook defined to know where to start from and to // get additional data - const workflowStartNode = workflow.getNode(webhookData.node); + const workflowStartNode = workflow.getNode(webhook.node); if (workflowStartNode === null) { throw new NotFoundError('Could not find node to process webhook.'); } @@ -130,8 +111,8 @@ export class TestWebhooks implements IWebhookManager { const executionMode = 'manual'; const executionId = await WebhookHelpers.executeWebhook( workflow, - webhookData!, - workflowData, + webhook!, + workflowEntity, workflowStartNode, executionMode, sessionId, @@ -153,107 +134,101 @@ export class TestWebhooks implements IWebhookManager { // Inform editor-ui that webhook got received if (sessionId !== undefined) { - push.send('testWebhookReceived', { workflowId, executionId }, sessionId); + this.push.send( + 'testWebhookReceived', + { workflowId: webhook?.workflowId, executionId }, + sessionId, + ); } } catch {} // Delete webhook also if an error is thrown if (timeout) clearTimeout(timeout); - delete testWebhookData[webhookKey]; + delete this.registeredWebhooks[key]; - await activeWebhooks.removeWorkflow(workflow); + await this.deactivateWebhooksFor(workflow); }); } - async getWebhookMethods(path: string): Promise { - const webhookMethods = this.activeWebhooks.getWebhookMethods(path); - if (!webhookMethods.length) { - // The requested webhook is not registered - throw new NotFoundError(webhookNotFoundErrorMessage(path), WEBHOOK_TEST_UNREGISTERED_HINT); - } + async getWebhookMethods(path: string) { + const webhookMethods = Object.keys(this.webhookUrls) + .filter((key) => key.includes(path)) + .map((key) => key.split('|')[0] as IHttpRequestMethods); + + if (!webhookMethods.length) throw new WebhookNotFoundError({ path }); return webhookMethods; } async findAccessControlOptions(path: string, httpMethod: IHttpRequestMethods) { - const webhookKey = Object.keys(this.testWebhookData).find( + const webhookKey = Object.keys(this.registeredWebhooks).find( (key) => key.includes(path) && key.startsWith(httpMethod), ); + if (!webhookKey) return; - const { workflow } = this.testWebhookData[webhookKey]; + const { workflow } = this.registeredWebhooks[webhookKey]; const webhookNode = Object.values(workflow.nodes).find( ({ type, parameters, typeVersion }) => parameters?.path === path && (parameters?.httpMethod ?? 'GET') === httpMethod && 'webhook' in this.nodeTypes.getByNameAndVersion(type, typeVersion), ); + return webhookNode?.parameters?.options as WebhookAccessControlOptions; } - /** - * Checks if it has to wait for webhook data to execute the workflow. - * If yes it waits for it and resolves with the result of the workflow if not it simply resolves with undefined - */ - async needsWebhookData( - workflowData: IWorkflowDb, + async needsWebhook( + workflowEntity: IWorkflowDb, workflow: Workflow, additionalData: IWorkflowExecuteAdditionalData, - mode: WorkflowExecuteMode, - activation: WorkflowActivateMode, + executionMode: WorkflowExecuteMode, + activationMode: WorkflowActivateMode, sessionId?: string, destinationNode?: string, - ): Promise { + ) { + if (!workflow.id) throw new WorkflowMissingIdError(workflow); + const webhooks = WebhookHelpers.getWorkflowWebhooks( workflow, additionalData, destinationNode, true, ); - if (!webhooks.find((webhook) => webhook.webhookDescription.restartWebhook !== true)) { - // No webhooks found to start a workflow - return false; + + if (!webhooks.find((w) => w.webhookDescription.restartWebhook !== true)) { + return false; // no webhooks found to start a workflow } - if (workflow.id === undefined) { - throw new ApplicationError( - 'Webhooks can only be added for saved workflows as an ID is needed', - ); - } - - // Remove test-webhooks automatically if they do not get called (after 120 seconds) const timeout = setTimeout(() => { - this.cancelTestWebhook(workflowData.id); - }, 120000); + this.cancelTestWebhook(workflowEntity.id); + }, 2 * TIME.MINUTE); - const { activeWebhooks, testWebhookData } = this; + const activatedKeys: string[] = []; - let key: string; - const activatedKey: string[] = []; + for (const webhook of webhooks) { + const key = [ + this.toWebhookKey(webhook.httpMethod, webhook.path, webhook.webhookId), + workflowEntity.id, + ].join('|'); - for (const webhookData of webhooks) { - key = `${activeWebhooks.getWebhookKey( - webhookData.httpMethod, - webhookData.path, - webhookData.webhookId, - )}|${workflowData.id}`; + activatedKeys.push(key); - activatedKey.push(key); - - testWebhookData[key] = { + this.registeredWebhooks[key] = { sessionId, timeout, workflow, - workflowData, + workflowEntity, destinationNode, }; try { - await activeWebhooks.add(workflow, webhookData, mode, activation); + await this.activateWebhook(workflow, webhook, executionMode, activationMode); } catch (error) { - activatedKey.forEach((deleteKey) => delete testWebhookData[deleteKey]); + activatedKeys.forEach((ak) => delete this.registeredWebhooks[ak]); + + await this.deactivateWebhooksFor(workflow); - await activeWebhooks.removeWorkflow(workflow); throw error; } } @@ -261,38 +236,29 @@ export class TestWebhooks implements IWebhookManager { return true; } - /** - * Removes a test webhook of the workflow with the given id - * - */ - cancelTestWebhook(workflowId: string): boolean { + cancelTestWebhook(workflowId: string) { let foundWebhook = false; - const { activeWebhooks, push, testWebhookData } = this; - for (const webhookKey of Object.keys(testWebhookData)) { - const { sessionId, timeout, workflow, workflowData } = testWebhookData[webhookKey]; + for (const key of Object.keys(this.registeredWebhooks)) { + const { sessionId, timeout, workflow, workflowEntity } = this.registeredWebhooks[key]; - if (workflowData.id !== workflowId) { - continue; - } + if (workflowEntity.id !== workflowId) continue; clearTimeout(timeout); - // Inform editor-ui that webhook got received if (sessionId !== undefined) { try { - push.send('testWebhookDeleted', { workflowId }, sessionId); + this.push.send('testWebhookDeleted', { workflowId }, sessionId); } catch { // Could not inform editor, probably is not connected anymore. So simply go on. } } - // Remove the webhook - delete testWebhookData[webhookKey]; + delete this.registeredWebhooks[key]; if (!foundWebhook) { // As it removes all webhooks of the workflow execute only once - void activeWebhooks.removeWorkflow(workflow); + void this.deactivateWebhooksFor(workflow); } foundWebhook = true; @@ -300,4 +266,127 @@ export class TestWebhooks implements IWebhookManager { return foundWebhook; } + + async activateWebhook( + workflow: Workflow, + webhook: IWebhookData, + executionMode: WorkflowExecuteMode, + activationMode: WorkflowActivateMode, + ) { + if (!workflow.id) throw new WorkflowMissingIdError(workflow); + + if (webhook.path.endsWith('/')) { + 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); + + try { + await workflow.createWebhookIfNotExists( + webhook, + NodeExecuteFunctions, + executionMode, + activationMode, + ); + } catch (error) { + // If there was a problem unregister the webhook again + 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); + if (this.webhookUrls[webhookKey] === undefined) { + return undefined; + } + + let webhook: IWebhookData | undefined; + let maxMatches = 0; + 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 staticElements = dynamicWebhook.path.split('/').filter((ele) => !ele.startsWith(':')); + const allStaticExist = staticElements.every((staticEle) => pathElementsSet.has(staticEle)); + + if (allStaticExist && staticElements.length > maxMatches) { + maxMatches = staticElements.length; + 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) { + if (!webhookId) return `${httpMethod}|${path}`; + + if (path.startsWith(webhookId)) { + const cutFromIndex = path.indexOf('/') + 1; + + path = path.slice(cutFromIndex); + } + + return `${httpMethod}|${webhookId}|${path.split('/').length}`; + } + + async deactivateWebhooksFor(workflow: Workflow) { + const workflowId = workflow.id; + + if (this.workflowWebhooks[workflowId] === undefined) { + // If it did not exist then there is nothing to remove + return false; + } + + 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; + } } diff --git a/packages/cli/src/databases/entities/WebhookEntity.ts b/packages/cli/src/databases/entities/WebhookEntity.ts index 6527a7be30..89b863d427 100644 --- a/packages/cli/src/databases/entities/WebhookEntity.ts +++ b/packages/cli/src/databases/entities/WebhookEntity.ts @@ -23,10 +23,12 @@ export class WebhookEntity { pathLength?: number; /** - * Unique section of production webhook path, appended to `${instanceUrl}/webhook/`. - * - Example for static UUID webhook: `87dd035f-9606-47b7-b443-8b675fe25719` - * - Example for static user-defined webhook: `user/:id/posts` - * - Example for dynamic webhook: `7e0e2b2a-19ba-4a6c-b452-4b46c0e11749/user/:id/posts` + * Unique section of webhook path. + * + * - Static: `${uuid}` or `user/defined/path` + * - Dynamic: `${uuid}/user/:id/posts` + * + * Appended to `${instanceUrl}/webhook/` or `${instanceUrl}/test-webhook/`. */ private get uniquePath() { return this.webhookPath.includes(':') diff --git a/packages/cli/src/errors/response-errors/webhook-not-found.error.ts b/packages/cli/src/errors/response-errors/webhook-not-found.error.ts new file mode 100644 index 0000000000..617c119a1e --- /dev/null +++ b/packages/cli/src/errors/response-errors/webhook-not-found.error.ts @@ -0,0 +1,57 @@ +import { NotFoundError } from './not-found.error'; + +export const webhookNotFoundErrorMessage = ({ + path, + httpMethod, + webhookMethods, +}: { + path: string; + httpMethod?: string; + webhookMethods?: string[]; +}) => { + let webhookPath = path; + + if (httpMethod) { + webhookPath = `${httpMethod} ${webhookPath}`; + } + + if (webhookMethods?.length && httpMethod) { + let methods = ''; + + if (webhookMethods.length === 1) { + methods = webhookMethods[0]; + } else { + const lastMethod = webhookMethods.pop(); + + methods = `${webhookMethods.join(', ')} or ${lastMethod as string}`; + } + + return `This webhook is not registered for ${httpMethod} requests. Did you mean to make a ${methods} request?`; + } else { + return `The requested webhook "${webhookPath}" is not registered.`; + } +}; + +export class WebhookNotFoundError extends NotFoundError { + constructor( + { + path, + httpMethod, + webhookMethods, + }: { + path: string; + httpMethod?: string; + webhookMethods?: string[]; + }, + { hint }: { hint: 'default' | 'production' } = { hint: 'default' }, + ) { + const errorMsg = webhookNotFoundErrorMessage({ path, httpMethod, webhookMethods }); + + const hintMsg = + hint === 'default' + ? "Click the 'Execute workflow' button on the canvas, then try again. (In test mode, the webhook only works for one call after you click this button)" + : "The workflow must be active for a production URL to run successfully. You can activate the workflow using the toggle in the top-right of the editor. Note that unlike test URL calls, production URL calls aren't shown on the canvas (only in the executions list)"; + + super(errorMsg, hintMsg); + } +} diff --git a/packages/cli/src/errors/workflow-missing-id.error.ts b/packages/cli/src/errors/workflow-missing-id.error.ts new file mode 100644 index 0000000000..613d418884 --- /dev/null +++ b/packages/cli/src/errors/workflow-missing-id.error.ts @@ -0,0 +1,8 @@ +import type { Workflow } from 'n8n-workflow'; +import { ApplicationError } from 'n8n-workflow'; + +export class WorkflowMissingIdError extends ApplicationError { + constructor(workflow: Workflow) { + super('Detected ID-less worklfow', { extra: { workflow } }); + } +} diff --git a/packages/cli/src/utils.ts b/packages/cli/src/utils.ts index 5e4f38d011..36960f5dbc 100644 --- a/packages/cli/src/utils.ts +++ b/packages/cli/src/utils.ts @@ -60,34 +60,6 @@ export const separate = (array: T[], test: (element: T) => boolean) => { return [pass, fail]; }; -export const webhookNotFoundErrorMessage = ( - path: string, - httpMethod?: string, - webhookMethods?: string[], -) => { - let webhookPath = path; - - if (httpMethod) { - webhookPath = `${httpMethod} ${webhookPath}`; - } - - if (webhookMethods?.length && httpMethod) { - let methods = ''; - - if (webhookMethods.length === 1) { - methods = webhookMethods[0]; - } else { - const lastMethod = webhookMethods.pop(); - - methods = `${webhookMethods.join(', ')} or ${lastMethod as string}`; - } - - return `This webhook is not registered for ${httpMethod} requests. Did you mean to make a ${methods} request?`; - } else { - return `The requested webhook "${webhookPath}" is not registered.`; - } -}; - export const toError = (maybeError: unknown) => // eslint-disable-next-line @typescript-eslint/restrict-template-expressions maybeError instanceof Error ? maybeError : new Error(`${maybeError}`); diff --git a/packages/cli/src/workflows/workflow.service.ts b/packages/cli/src/workflows/workflow.service.ts index 7a37d7ba7a..d4854d123d 100644 --- a/packages/cli/src/workflows/workflow.service.ts +++ b/packages/cli/src/workflows/workflow.service.ts @@ -432,7 +432,7 @@ export class WorkflowService { const additionalData = await WorkflowExecuteAdditionalData.getBase(user.id); - const needsWebhook = await this.testWebhooks.needsWebhookData( + const needsWebhook = await this.testWebhooks.needsWebhook( workflowData, workflow, additionalData, @@ -441,11 +441,8 @@ export class WorkflowService { sessionId, destinationNode, ); - if (needsWebhook) { - return { - waitingForWebhook: true, - }; - } + + if (needsWebhook) return { waitingForWebhook: true }; } // For manual testing always set to not active diff --git a/packages/cli/test/unit/TestWebhooks.test.ts b/packages/cli/test/unit/TestWebhooks.test.ts new file mode 100644 index 0000000000..7eb321f52c --- /dev/null +++ b/packages/cli/test/unit/TestWebhooks.test.ts @@ -0,0 +1,154 @@ +import { mockInstance } from '../shared/mocking'; +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'; +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 express from 'express'; +import type { + IWebhookData, + IWorkflowExecuteAdditionalData, + Workflow, + WorkflowActivateMode, + WorkflowExecuteMode, +} from 'n8n-workflow'; + +describe('TestWebhooks', () => { + jest.useFakeTimers(); + + const push = mockInstance(Push); + const nodeTypes = mockInstance(NodeTypes); + + const testWebhooks = new TestWebhooks(push, nodeTypes); + + afterEach(() => { + jest.clearAllMocks(); + }); + + 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, + IWorkflowExecuteAdditionalData, + WorkflowExecuteMode, + WorkflowActivateMode, + ]; + + const workflow = { + id: workflowId, + createWebhookIfNotExists: () => {}, + deleteWebhook: () => {}, + } as unknown as Workflow; + + const args: NeedsWebhookArgs = [ + { id: workflowId } as unknown as IWorkflowDb, + workflow, + {} as unknown as IWorkflowExecuteAdditionalData, + 'manual', + 'manual', + ]; + + test('should register a webhook as active', 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); + + expect(needsWebhook).toBe(true); + expect(activateWebhookSpy).toHaveBeenCalledWith(workflow, webhook, 'manual', 'manual'); + }); + + test('should remove from active webhooks on failure to add', 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 needsWebhook = testWebhooks.needsWebhook(...args); + + await expect(needsWebhook).rejects.toThrowError(msg); + expect(deactivateSpy).toHaveBeenCalledWith(workflow); + }); + + test('should return false if no webhook to start workflow', async () => { + webhook.webhookDescription.restartWebhook = true; + jest.spyOn(WebhookHelpers, 'getWorkflowWebhooks').mockReturnValue([webhook]); + + const result = await testWebhooks.needsWebhook(...args); + + expect(result).toBe(false); + }); + }); + + 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 response = {} as express.Response; + const promise = testWebhooks.executeWebhook(request, response); + + await expect(promise).rejects.toThrowError(WebhookNotFoundError); + }); + + 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 + testWebhooks.registeredWebhooks[`${keyPart}|${workflowId}`] = { + sessionId: 'some-session-id', + timeout: setTimeout(() => {}, 0), + workflowEntity: {} as IWorkflowDb, + workflow: { + getNode: () => null, + } as unknown as Workflow, + }; + + const request = { params: { path } } as WebhookRequest; + const response = {} as express.Response; + const promise = testWebhooks.executeWebhook(request, response); + + await expect(promise).rejects.toThrowError(NotFoundError); + + // @ts-expect-error Private property + delete testWebhooks.registeredWebhooks[`${keyPart}|${workflowId}`]; + }); + }); +}); diff --git a/packages/cli/test/unit/utils.test.ts b/packages/cli/test/unit/utils.test.ts index cf6de8103b..92a89003d6 100644 --- a/packages/cli/test/unit/utils.test.ts +++ b/packages/cli/test/unit/utils.test.ts @@ -1,32 +1,44 @@ -import { webhookNotFoundErrorMessage } from '@/utils'; +import { webhookNotFoundErrorMessage } from '@/errors/response-errors/webhook-not-found.error'; describe('utils test webhookNotFoundErrorMessage ', () => { it('should return a message with path and method', () => { - const message = webhookNotFoundErrorMessage('webhook12345', 'GET'); + const message = webhookNotFoundErrorMessage({ path: 'webhook12345', httpMethod: 'GET' }); expect(message).toEqual('The requested webhook "GET webhook12345" is not registered.'); }); it('should return a message with path', () => { - const message = webhookNotFoundErrorMessage('webhook12345'); + const message = webhookNotFoundErrorMessage({ path: 'webhook12345' }); expect(message).toEqual('The requested webhook "webhook12345" is not registered.'); }); it('should return a message with method with tip', () => { - const message = webhookNotFoundErrorMessage('webhook12345', 'POST', ['GET', 'PUT']); + const message = webhookNotFoundErrorMessage({ + path: 'webhook12345', + httpMethod: 'POST', + webhookMethods: ['GET', 'PUT'], + }); expect(message).toEqual( 'This webhook is not registered for POST requests. Did you mean to make a GET or PUT request?', ); }); it('should return a message with method with tip', () => { - const message = webhookNotFoundErrorMessage('webhook12345', 'POST', ['PUT']); + const message = webhookNotFoundErrorMessage({ + path: 'webhook12345', + httpMethod: 'POST', + webhookMethods: ['PUT'], + }); expect(message).toEqual( 'This webhook is not registered for POST requests. Did you mean to make a PUT request?', ); }); it('should return a message with method with tip', () => { - const message = webhookNotFoundErrorMessage('webhook12345', 'POST', ['GET', 'PUT', 'DELETE']); + const message = webhookNotFoundErrorMessage({ + path: 'webhook12345', + httpMethod: 'POST', + webhookMethods: ['GET', 'PUT', 'DELETE'], + }); expect(message).toEqual( 'This webhook is not registered for POST requests. Did you mean to make a GET, PUT or DELETE request?', diff --git a/packages/core/src/NodeExecuteFunctions.ts b/packages/core/src/NodeExecuteFunctions.ts index 3fa6dd4e61..659dbcae8e 100644 --- a/packages/core/src/NodeExecuteFunctions.ts +++ b/packages/core/src/NodeExecuteFunctions.ts @@ -3768,7 +3768,6 @@ export function getExecuteHookFunctions( additionalData: IWorkflowExecuteAdditionalData, mode: WorkflowExecuteMode, activation: WorkflowActivateMode, - isTest?: boolean, webhookData?: IWebhookData, ): IHookFunctions { return ((workflow: Workflow, node: INode) => { @@ -3810,7 +3809,7 @@ export function getExecuteHookFunctions( additionalData, mode, getAdditionalKeys(additionalData, mode, null), - isTest, + webhookData?.isTest, ); }, getWebhookName(): string { diff --git a/packages/workflow/src/Interfaces.ts b/packages/workflow/src/Interfaces.ts index f3043f68b9..4b6fded659 100644 --- a/packages/workflow/src/Interfaces.ts +++ b/packages/workflow/src/Interfaces.ts @@ -450,7 +450,6 @@ export interface IGetExecuteHookFunctions { additionalData: IWorkflowExecuteAdditionalData, mode: WorkflowExecuteMode, activation: WorkflowActivateMode, - isTest?: boolean, webhookData?: IWebhookData, ): IHookFunctions; } @@ -1660,6 +1659,7 @@ export interface IWebhookData { workflowId: string; workflowExecuteAdditionalData: IWorkflowExecuteAdditionalData; webhookId?: string; + isTest?: boolean; } export interface IWebhookDescription { diff --git a/packages/workflow/src/Workflow.ts b/packages/workflow/src/Workflow.ts index 7531eb7491..931dd668b4 100644 --- a/packages/workflow/src/Workflow.ts +++ b/packages/workflow/src/Workflow.ts @@ -1009,7 +1009,6 @@ export class Workflow { nodeExecuteFunctions: INodeExecuteFunctions, mode: WorkflowExecuteMode, activation: WorkflowActivateMode, - isTest?: boolean, ): Promise { const webhookExists = await this.runWebhookMethod( 'checkExists', @@ -1017,18 +1016,10 @@ export class Workflow { nodeExecuteFunctions, mode, activation, - isTest, ); if (!webhookExists) { // If webhook does not exist yet create it - await this.runWebhookMethod( - 'create', - webhookData, - nodeExecuteFunctions, - mode, - activation, - isTest, - ); + await this.runWebhookMethod('create', webhookData, nodeExecuteFunctions, mode, activation); } } @@ -1037,16 +1028,8 @@ export class Workflow { nodeExecuteFunctions: INodeExecuteFunctions, mode: WorkflowExecuteMode, activation: WorkflowActivateMode, - isTest?: boolean, ) { - await this.runWebhookMethod( - 'delete', - webhookData, - nodeExecuteFunctions, - mode, - activation, - isTest, - ); + await this.runWebhookMethod('delete', webhookData, nodeExecuteFunctions, mode, activation); } private async runWebhookMethod( @@ -1055,7 +1038,6 @@ export class Workflow { nodeExecuteFunctions: INodeExecuteFunctions, mode: WorkflowExecuteMode, activation: WorkflowActivateMode, - isTest?: boolean, ): Promise { const node = this.getNode(webhookData.node); @@ -1072,7 +1054,6 @@ export class Workflow { webhookData.workflowExecuteAdditionalData, mode, activation, - isTest, webhookData, );