Dynamic webhooks improvements (#1396)

*  remove trailing slash in routes

* 🔧 update logic to select dynamicWebhook

* 🐛 fix logic in static route matching
This commit is contained in:
Ben Hesseldieck 2021-02-09 09:14:40 +01:00 committed by GitHub
parent 7a3aaf8a24
commit 98fa529e51
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 61 additions and 43 deletions

View file

@ -120,6 +120,11 @@ export class ActiveWorkflowRunner {
// Reset request parameters // Reset request parameters
req.params = {}; 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 webhook = await Db.collections.Webhook?.findOne({ webhookPath: path, method: httpMethod }) as IWebhookDb;
let webhookId: string | undefined; 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); throw new ResponseHelper.ResponseError(`The requested webhook "${httpMethod} ${path}" is not registered.`, 404, 404);
} }
// set webhook to the first webhook result let maxMatches = 0;
// if more results have been returned choose the one with the most route-matches const pathElementsSet = new Set(pathElements);
webhook = dynamicWebhooks[0]; // check if static elements match in path
if (dynamicWebhooks.length > 1) { // if more results have been returned choose the one with the most static-route matches
let maxMatches = 0; dynamicWebhooks.forEach(dynamicWebhook => {
const pathElementsSet = new Set(pathElements); const staticElements = dynamicWebhook.webhookPath.split('/').filter(ele => !ele.startsWith(':'));
dynamicWebhooks.forEach(dynamicWebhook => { const allStaticExist = staticElements.every(staticEle => pathElementsSet.has(staticEle));
const intersection =
dynamicWebhook.webhookPath
.split('/')
.reduce((acc, element) => pathElementsSet.has(element) ? acc += 1 : acc, 0);
if (intersection > maxMatches) { if (allStaticExist && staticElements.length > maxMatches) {
maxMatches = intersection; maxMatches = staticElements.length;
webhook = dynamicWebhook; webhook = dynamicWebhook;
}
});
if (maxMatches === 0) {
throw new ResponseHelper.ResponseError(`The requested webhook "${httpMethod} ${path}" is not registered.`, 404, 404);
} }
// 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 // extracting params from path
webhook.webhookPath.split('/').forEach((ele, index) => { webhook!.webhookPath.split('/').forEach((ele, index) => {
if (ele.startsWith(':')) { if (ele.startsWith(':')) {
// write params to req.params // write params to req.params
req.params[ele.slice(1)] = pathElements[index]; req.params[ele.slice(1)] = pathElements[index];
@ -298,6 +302,9 @@ export class ActiveWorkflowRunner {
if (webhook.webhookPath.startsWith('/')) { if (webhook.webhookPath.startsWith('/')) {
webhook.webhookPath = webhook.webhookPath.slice(1); webhook.webhookPath = webhook.webhookPath.slice(1);
} }
if (webhook.webhookPath.endsWith('/')) {
webhook.webhookPath = webhook.webhookPath.slice(0, -1);
}
if ((path.startsWith(':') || path.includes('/:')) && node.webhookId) { if ((path.startsWith(':') || path.includes('/:')) && node.webhookId) {
webhook.webhookId = node.webhookId; webhook.webhookId = node.webhookId;

View file

@ -54,11 +54,16 @@ export class TestWebhooks {
* @memberof TestWebhooks * @memberof TestWebhooks
*/ */
async callTestWebhook(httpMethod: WebhookHttpMethod, path: string, request: express.Request, response: express.Response): Promise<IResponseCallbackData> { async callTestWebhook(httpMethod: WebhookHttpMethod, path: string, request: express.Request, response: express.Response): Promise<IResponseCallbackData> {
let webhookData: IWebhookData | undefined = this.activeWebhooks!.get(httpMethod, path);
// Reset request parameters // Reset request parameters
request.params = {}; 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 // check if path is dynamic
if (webhookData === undefined) { if (webhookData === undefined) {
const pathElements = path.split('/'); const pathElements = path.split('/');

View file

@ -34,6 +34,9 @@ export class ActiveWebhooks {
if (workflow.id === undefined) { if (workflow.id === undefined) {
throw new Error('Webhooks can only be added for saved workflows as an id is needed!'); 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); const webhookKey = this.getWebhookKey(webhookData.httpMethod, webhookData.path, webhookData.webhookId);
@ -89,27 +92,24 @@ export class ActiveWebhooks {
return undefined; return undefined;
} }
// set webhook to the first webhook result let webhook: IWebhookData | undefined;
// if more results have been returned choose the one with the most route-matches let maxMatches = 0;
let webhook = this.webhookUrls[webhookKey][0]; const pathElementsSet = new Set(path.split('/'));
if (this.webhookUrls[webhookKey].length > 1) { // check if static elements match in path
let maxMatches = 0; // if more results have been returned choose the one with the most static-route matches
const pathElementsSet = new Set(path.split('/')); this.webhookUrls[webhookKey].forEach(dynamicWebhook => {
this.webhookUrls[webhookKey].forEach(dynamicWebhook => { const staticElements = dynamicWebhook.path.split('/').filter(ele => !ele.startsWith(':'));
const intersection = const allStaticExist = staticElements.every(staticEle => pathElementsSet.has(staticEle));
dynamicWebhook.path
.split('/')
.reduce((acc, element) => pathElementsSet.has(element) ? acc += 1 : acc, 0);
if (intersection > maxMatches) { if (allStaticExist && staticElements.length > maxMatches) {
maxMatches = intersection; maxMatches = staticElements.length;
webhook = dynamicWebhook; webhook = dynamicWebhook;
}
});
if (maxMatches === 0) {
return undefined;
} }
} // handle routes with no static elements
else if (staticElements.length === 0 && !webhook) {
webhook = dynamicWebhook;
}
});
return webhook; return webhook;
} }

View file

@ -765,9 +765,12 @@ export function getNodeWebhooks(workflow: Workflow, node: INode, additionalData:
nodeWebhookPath = nodeWebhookPath.toString(); nodeWebhookPath = nodeWebhookPath.toString();
if (nodeWebhookPath.charAt(0) === '/') { if (nodeWebhookPath.startsWith('/')) {
nodeWebhookPath = nodeWebhookPath.slice(1); 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 isFullPath: boolean = workflow.expression.getSimpleParameterValue(node, webhookDescription['isFullPath'], 'internal', false) as boolean;
const path = getNodeWebhookPath(workflowId, node, nodeWebhookPath, isFullPath); const path = getNodeWebhookPath(workflowId, node, nodeWebhookPath, isFullPath);
@ -827,9 +830,12 @@ export function getNodeWebhooksBasic(workflow: Workflow, node: INode): IWebhookD
nodeWebhookPath = nodeWebhookPath.toString(); nodeWebhookPath = nodeWebhookPath.toString();
if (nodeWebhookPath.charAt(0) === '/') { if (nodeWebhookPath.startsWith('/')) {
nodeWebhookPath = nodeWebhookPath.slice(1); 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; const isFullPath: boolean = workflow.expression.getSimpleParameterValue(node, webhookDescription['isFullPath'], mode, false) as boolean;