From 98fa529e518a3c155de7217b2a8860a8a8850b80 Mon Sep 17 00:00:00 2001 From: Ben Hesseldieck <1849459+BHesseldieck@users.noreply.github.com> Date: Tue, 9 Feb 2021 09:14:40 +0100 Subject: [PATCH] :zap: Dynamic webhooks improvements (#1396) * :zap: remove trailing slash in routes * :wrench: update logic to select dynamicWebhook * :bug: fix logic in static route matching --- packages/cli/src/ActiveWorkflowRunner.ts | 47 ++++++++++++++---------- packages/cli/src/TestWebhooks.ts | 9 ++++- packages/core/src/ActiveWebhooks.ts | 38 +++++++++---------- packages/workflow/src/NodeHelpers.ts | 10 ++++- 4 files changed, 61 insertions(+), 43 deletions(-) diff --git a/packages/cli/src/ActiveWorkflowRunner.ts b/packages/cli/src/ActiveWorkflowRunner.ts index 1f35f0e198..3583007680 100644 --- a/packages/cli/src/ActiveWorkflowRunner.ts +++ b/packages/cli/src/ActiveWorkflowRunner.ts @@ -120,6 +120,11 @@ export class ActiveWorkflowRunner { // Reset request parameters req.params = {}; + // Remove trailing slash + if (path.endsWith('/')) { + path = path.slice(0, -1); + } + let webhook = await Db.collections.Webhook?.findOne({ webhookPath: path, method: httpMethod }) as IWebhookDb; let webhookId: string | undefined; @@ -134,31 +139,30 @@ export class ActiveWorkflowRunner { throw new ResponseHelper.ResponseError(`The requested webhook "${httpMethod} ${path}" is not registered.`, 404, 404); } - // set webhook to the first webhook result - // if more results have been returned choose the one with the most route-matches - webhook = dynamicWebhooks[0]; - if (dynamicWebhooks.length > 1) { - let maxMatches = 0; - const pathElementsSet = new Set(pathElements); - dynamicWebhooks.forEach(dynamicWebhook => { - const intersection = - dynamicWebhook.webhookPath - .split('/') - .reduce((acc, element) => pathElementsSet.has(element) ? acc += 1 : acc, 0); + let maxMatches = 0; + const pathElementsSet = new Set(pathElements); + // check if static elements match in path + // if more results have been returned choose the one with the most static-route matches + dynamicWebhooks.forEach(dynamicWebhook => { + const staticElements = dynamicWebhook.webhookPath.split('/').filter(ele => !ele.startsWith(':')); + const allStaticExist = staticElements.every(staticEle => pathElementsSet.has(staticEle)); - if (intersection > maxMatches) { - maxMatches = intersection; - webhook = dynamicWebhook; - } - }); - if (maxMatches === 0) { - throw new ResponseHelper.ResponseError(`The requested webhook "${httpMethod} ${path}" is not registered.`, 404, 404); + if (allStaticExist && staticElements.length > maxMatches) { + maxMatches = staticElements.length; + webhook = dynamicWebhook; } + // handle routes with no static elements + else if (staticElements.length === 0 && !webhook) { + webhook = dynamicWebhook; + } + }); + if (webhook === undefined) { + throw new ResponseHelper.ResponseError(`The requested webhook "${httpMethod} ${path}" is not registered.`, 404, 404); } - path = webhook.webhookPath; + path = webhook!.webhookPath; // extracting params from path - webhook.webhookPath.split('/').forEach((ele, index) => { + webhook!.webhookPath.split('/').forEach((ele, index) => { if (ele.startsWith(':')) { // write params to req.params req.params[ele.slice(1)] = pathElements[index]; @@ -298,6 +302,9 @@ export class ActiveWorkflowRunner { if (webhook.webhookPath.startsWith('/')) { webhook.webhookPath = webhook.webhookPath.slice(1); } + if (webhook.webhookPath.endsWith('/')) { + webhook.webhookPath = webhook.webhookPath.slice(0, -1); + } if ((path.startsWith(':') || path.includes('/:')) && node.webhookId) { webhook.webhookId = node.webhookId; diff --git a/packages/cli/src/TestWebhooks.ts b/packages/cli/src/TestWebhooks.ts index 580de9c34c..e5caef9d93 100644 --- a/packages/cli/src/TestWebhooks.ts +++ b/packages/cli/src/TestWebhooks.ts @@ -54,11 +54,16 @@ export class TestWebhooks { * @memberof TestWebhooks */ async callTestWebhook(httpMethod: WebhookHttpMethod, path: string, request: express.Request, response: express.Response): Promise { - let webhookData: IWebhookData | undefined = this.activeWebhooks!.get(httpMethod, path); - // Reset request parameters request.params = {}; + // Remove trailing slash + if (path.endsWith('/')) { + path = path.slice(0, -1); + } + + let webhookData: IWebhookData | undefined = this.activeWebhooks!.get(httpMethod, path); + // check if path is dynamic if (webhookData === undefined) { const pathElements = path.split('/'); diff --git a/packages/core/src/ActiveWebhooks.ts b/packages/core/src/ActiveWebhooks.ts index d521043cc7..86a1c0f690 100644 --- a/packages/core/src/ActiveWebhooks.ts +++ b/packages/core/src/ActiveWebhooks.ts @@ -34,6 +34,9 @@ export class ActiveWebhooks { if (workflow.id === undefined) { throw new Error('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); @@ -89,27 +92,24 @@ export class ActiveWebhooks { return undefined; } - // set webhook to the first webhook result - // if more results have been returned choose the one with the most route-matches - let webhook = this.webhookUrls[webhookKey][0]; - if (this.webhookUrls[webhookKey].length > 1) { - let maxMatches = 0; - const pathElementsSet = new Set(path.split('/')); - this.webhookUrls[webhookKey].forEach(dynamicWebhook => { - const intersection = - dynamicWebhook.path - .split('/') - .reduce((acc, element) => pathElementsSet.has(element) ? acc += 1 : acc, 0); + 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 (intersection > maxMatches) { - maxMatches = intersection; - webhook = dynamicWebhook; - } - }); - if (maxMatches === 0) { - return undefined; + 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; } diff --git a/packages/workflow/src/NodeHelpers.ts b/packages/workflow/src/NodeHelpers.ts index ef87a7313f..52f3e2668a 100644 --- a/packages/workflow/src/NodeHelpers.ts +++ b/packages/workflow/src/NodeHelpers.ts @@ -765,9 +765,12 @@ export function getNodeWebhooks(workflow: Workflow, node: INode, additionalData: nodeWebhookPath = nodeWebhookPath.toString(); - if (nodeWebhookPath.charAt(0) === '/') { + if (nodeWebhookPath.startsWith('/')) { nodeWebhookPath = nodeWebhookPath.slice(1); } + if (nodeWebhookPath.endsWith('/')) { + nodeWebhookPath = nodeWebhookPath.slice(0, -1); + } const isFullPath: boolean = workflow.expression.getSimpleParameterValue(node, webhookDescription['isFullPath'], 'internal', false) as boolean; const path = getNodeWebhookPath(workflowId, node, nodeWebhookPath, isFullPath); @@ -827,9 +830,12 @@ export function getNodeWebhooksBasic(workflow: Workflow, node: INode): IWebhookD nodeWebhookPath = nodeWebhookPath.toString(); - if (nodeWebhookPath.charAt(0) === '/') { + if (nodeWebhookPath.startsWith('/')) { nodeWebhookPath = nodeWebhookPath.slice(1); } + if (nodeWebhookPath.endsWith('/')) { + nodeWebhookPath = nodeWebhookPath.slice(0, -1); + } const isFullPath: boolean = workflow.expression.getSimpleParameterValue(node, webhookDescription['isFullPath'], mode, false) as boolean;