From 5032bf0e346dccf7cade17a1518b3031118af5e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Tue, 9 Jan 2024 16:02:32 +0100 Subject: [PATCH] fix(core): Fix test webhook deregistration (#8247) --- packages/cli/src/TestWebhooks.ts | 33 ++++++-- .../cli/src/workflows/workflow.service.ts | 1 + packages/cli/test/unit/TestWebhooks.test.ts | 78 ++++++++++--------- packages/workflow/src/Interfaces.ts | 2 + 4 files changed, 72 insertions(+), 42 deletions(-) diff --git a/packages/cli/src/TestWebhooks.ts b/packages/cli/src/TestWebhooks.ts index cffff29faa..5dd1ce3ac7 100644 --- a/packages/cli/src/TestWebhooks.ts +++ b/packages/cli/src/TestWebhooks.ts @@ -24,6 +24,7 @@ import { WebhookNotFoundError } from '@/errors/response-errors/webhook-not-found import * as NodeExecuteFunctions from 'n8n-core'; import { removeTrailingSlash } from './utils'; import { TestWebhookRegistrationsService } from '@/services/test-webhook-registrations.service'; +import * as WorkflowExecuteAdditionalData from '@/WorkflowExecuteAdditionalData'; @Service() export class TestWebhooks implements IWebhookManager { @@ -185,6 +186,7 @@ export class TestWebhooks implements IWebhookManager { * For every webhook call to listen for, also activate the webhook. */ async needsWebhook( + userId: string, workflowEntity: IWorkflowDb, additionalData: IWorkflowExecuteAdditionalData, sessionId?: string, @@ -219,21 +221,23 @@ export class TestWebhooks implements IWebhookManager { webhook.isTest = true; /** - * Remove additional data from webhook because: - * - * - It is not needed for the test webhook to be executed. - * - It contains circular refs that cannot be cached. + * Additional data cannot be cached because of circular refs. + * Hence store the `userId` and recreate additional data when needed. */ - const { workflowExecuteAdditionalData: _, ...rest } = webhook; + const { workflowExecuteAdditionalData: _, ...cacheableWebhook } = webhook; + + cacheableWebhook.userId = userId; try { await workflow.createWebhookIfNotExists(webhook, NodeExecuteFunctions, 'manual', 'manual'); + cacheableWebhook.staticData = workflow.staticData; + await this.registrations.register({ sessionId, workflowEntity, destinationNode, - webhook: rest as IWebhookData, + webhook: cacheableWebhook as IWebhookData, }); this.timeouts[key] = timeout; @@ -341,6 +345,14 @@ export class TestWebhooks implements IWebhookManager { if (!webhooks) return; // nothing to deactivate for (const webhook of webhooks) { + const { userId, staticData } = webhook; + + if (userId) { + webhook.workflowExecuteAdditionalData = await WorkflowExecuteAdditionalData.getBase(userId); + } + + if (staticData) workflow.staticData = staticData; + await workflow.deleteWebhook(webhook, NodeExecuteFunctions, 'internal', 'update'); await this.registrations.deregister(webhook); @@ -350,7 +362,7 @@ export class TestWebhooks implements IWebhookManager { /** * Convert a `WorkflowEntity` from `typeorm` to a `Workflow` from `n8n-workflow`. */ - private toWorkflow(workflowEntity: IWorkflowDb) { + toWorkflow(workflowEntity: IWorkflowDb) { return new Workflow({ id: workflowEntity.id, name: workflowEntity.name, @@ -358,7 +370,14 @@ export class TestWebhooks implements IWebhookManager { connections: workflowEntity.connections, active: false, nodeTypes: this.nodeTypes, + + /** + * `staticData` in the original workflow entity has production webhook IDs. + * Since we are creating here a temporary workflow only for a test webhook, + * `staticData` from the original workflow entity should not be transferred. + */ staticData: undefined, + settings: workflowEntity.settings, }); } diff --git a/packages/cli/src/workflows/workflow.service.ts b/packages/cli/src/workflows/workflow.service.ts index 5a6a321d7f..253eac6414 100644 --- a/packages/cli/src/workflows/workflow.service.ts +++ b/packages/cli/src/workflows/workflow.service.ts @@ -317,6 +317,7 @@ export class WorkflowService { const additionalData = await WorkflowExecuteAdditionalData.getBase(user.id); const needsWebhook = await this.testWebhooks.needsWebhook( + user.id, workflowData, additionalData, sessionId, diff --git a/packages/cli/test/unit/TestWebhooks.test.ts b/packages/cli/test/unit/TestWebhooks.test.ts index e90917d15e..ecb3fe8044 100644 --- a/packages/cli/test/unit/TestWebhooks.test.ts +++ b/packages/cli/test/unit/TestWebhooks.test.ts @@ -5,53 +5,49 @@ 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 * as express from 'express'; import type { IWorkflowDb, WebhookRequest } from '@/Interfaces'; -import type { - IWebhookData, - IWorkflowExecuteAdditionalData, - Workflow, - WorkflowActivateMode, - WorkflowExecuteMode, -} from 'n8n-workflow'; +import type { IWebhookData, IWorkflowExecuteAdditionalData } from 'n8n-workflow'; import type { TestWebhookRegistrationsService, TestWebhookRegistration, } from '@/services/test-webhook-registrations.service'; -describe('TestWebhooks', () => { - const registrations = mock(); - const testWebhooks = new TestWebhooks(mock(), mock(), registrations); +import * as AdditionalData from '@/WorkflowExecuteAdditionalData'; +jest.mock('@/WorkflowExecuteAdditionalData'); + +const mockedAdditionalData = AdditionalData as jest.Mocked; + +const workflowEntity = mock({ id: generateNanoId(), nodes: [] }); + +const httpMethod = 'GET'; +const path = uuid(); +const userId = '04ab4baf-85df-478f-917b-d303934a97de'; + +const webhook = mock({ + httpMethod, + path, + workflowId: workflowEntity.id, + userId, +}); + +const registrations = mock(); + +let testWebhooks: TestWebhooks; + +describe('TestWebhooks', () => { beforeAll(() => { + testWebhooks = new TestWebhooks(mock(), mock(), registrations); jest.useFakeTimers(); }); - const httpMethod = 'GET'; - const path = uuid(); - const workflowId = generateNanoId(); - - const webhook = mock({ - httpMethod, - path, - workflowId, - }); - describe('needsWebhook()', () => { - type NeedsWebhookArgs = [ - IWorkflowDb, - IWorkflowExecuteAdditionalData, - WorkflowExecuteMode, - WorkflowActivateMode, - ]; - - const workflow = mock({ id: workflowId }); - - const args: NeedsWebhookArgs = [ - mock({ id: workflowId, nodes: [] }), + const args: Parameters = [ + userId, + workflowEntity, mock(), - 'manual', - 'manual', ]; test('if webhook is needed, should return true and activate webhook', async () => { @@ -103,17 +99,29 @@ describe('TestWebhooks', () => { const registration = mock({ sessionId: 'some-session-id', - workflowEntity: mock({}), + workflowEntity, }); await registrations.register(registration); const promise = testWebhooks.executeWebhook( mock({ params: { path } }), - mock(), + mock(), ); await expect(promise).rejects.toThrowError(NotFoundError); }); }); + + describe('deactivateWebhooks()', () => { + test('should add additional data to workflow', async () => { + registrations.getAllRegistrations.mockResolvedValue([{ workflowEntity, webhook }]); + + const workflow = testWebhooks.toWorkflow(workflowEntity); + + await testWebhooks.deactivateWebhooks(workflow); + + expect(mockedAdditionalData.getBase).toHaveBeenCalledWith(userId); + }); + }); }); diff --git a/packages/workflow/src/Interfaces.ts b/packages/workflow/src/Interfaces.ts index 542d55ce1d..a4e9040e84 100644 --- a/packages/workflow/src/Interfaces.ts +++ b/packages/workflow/src/Interfaces.ts @@ -1674,6 +1674,8 @@ export interface IWebhookData { workflowExecuteAdditionalData: IWorkflowExecuteAdditionalData; webhookId?: string; isTest?: boolean; + userId?: string; + staticData?: Workflow['staticData']; } export interface IWebhookDescription {