Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(core): Move webhook DB access to repository (no-changelog) #6706

Merged
merged 2 commits into from
Jul 25, 2023
Merged

refactor(core): Move webhook DB access to repository (no-changelog) #6706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f3377ed
refactor(core): Move webhook DB access to repository (no-changelog)
ivov Jul 20, 2023
634c88a
make sure `DataSource` is initialized before it's dependencies
netroy Jul 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/cli/src/AbstractServer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,6 @@ export abstract class AbstractServer {
this.endpointWebhook = config.getEnv('endpoints.webhook');
this.endpointWebhookTest = config.getEnv('endpoints.webhookTest');
this.endpointWebhookWaiting = config.getEnv('endpoints.webhookWaiting');

this.externalHooks = Container.get(ExternalHooks);
this.activeWorkflowRunner = Container.get(ActiveWorkflowRunner);
}

private async setupErrorHandlers() {
Expand Down Expand Up @@ -438,6 +435,9 @@ export abstract class AbstractServer {

await new Promise<void>((resolve) => this.server.listen(PORT, ADDRESS, () => resolve()));

this.externalHooks = Container.get(ExternalHooks);
this.activeWorkflowRunner = Container.get(ActiveWorkflowRunner);

await this.setupHealthCheck();

console.log(`n8n ready on ${ADDRESS}, port ${PORT}`);
Expand Down
14 changes: 8 additions & 6 deletions packages/cli/src/ActiveWorkflowRunner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ import { WorkflowsService } from './workflows/workflows.services';
import { STARTING_NODES } from './constants';
import { webhookNotFoundErrorMessage } from './utils';
import { In } from 'typeorm';
import { WebhookRepository } from '@db/repositories';

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)";
Expand All @@ -87,6 +88,7 @@ export class ActiveWorkflowRunner {
private activeExecutions: ActiveExecutions,
private externalHooks: ExternalHooks,
private nodeTypes: NodeTypes,
private webhookRepository: WebhookRepository,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dependency injection here is failing at start time

) {}

// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
Expand All @@ -110,7 +112,7 @@ export class ActiveWorkflowRunner {
// This is not officially supported but there is no reason
// it should not work.
// Clear up active workflow table
await Db.collections.Webhook.clear();
await this.webhookRepository.clear();
}

if (workflowsData.length !== 0) {
Expand Down Expand Up @@ -201,7 +203,7 @@ export class ActiveWorkflowRunner {
path = path.slice(0, -1);
}

let webhook = await Db.collections.Webhook.findOneBy({
let webhook = await this.webhookRepository.findOneBy({
webhookPath: path,
method: httpMethod,
});
Expand All @@ -212,7 +214,7 @@ export class ActiveWorkflowRunner {
// check if a dynamic webhook path exists
const pathElements = path.split('/');
webhookId = pathElements.shift();
const dynamicWebhooks = await Db.collections.Webhook.findBy({
const dynamicWebhooks = await this.webhookRepository.findBy({
webhookId,
method: httpMethod,
pathLength: pathElements.length,
Expand Down Expand Up @@ -333,7 +335,7 @@ export class ActiveWorkflowRunner {
* Gets all request methods associated with a single webhook
*/
async getWebhookMethods(path: string): Promise<string[]> {
const webhooks = await Db.collections.Webhook.find({
const webhooks = await this.webhookRepository.find({
select: ['method'],
where: { webhookPath: path },
});
Expand Down Expand Up @@ -442,7 +444,7 @@ export class ActiveWorkflowRunner {
try {
// eslint-disable-next-line no-await-in-loop
// TODO: this should happen in a transaction, that way we don't need to manually remove this in `catch`
await Db.collections.Webhook.insert(webhook);
await this.webhookRepository.insert(webhook);
const webhookExists = await workflow.runWebhookMethod(
'checkExists',
webhookData,
Expand Down Expand Up @@ -552,7 +554,7 @@ export class ActiveWorkflowRunner {

await WorkflowHelpers.saveStaticData(workflow);

await Db.collections.Webhook.delete({
await this.webhookRepository.delete({
workflowId: workflowData.id,
});
}
Expand Down
6 changes: 3 additions & 3 deletions packages/cli/src/WorkflowRunnerProcess.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,9 @@ class WorkflowRunnerProcess {

this.startedAt = new Date();

// Init db since we need to read the license.
await Db.init();

const userSettings = await UserSettings.prepareUserSettings();

const loadNodesAndCredentials = Container.get(LoadNodesAndCredentials);
Expand All @@ -117,9 +120,6 @@ class WorkflowRunnerProcess {
const externalHooks = Container.get(ExternalHooks);
await externalHooks.init();

// Init db since we need to read the license.
await Db.init();

const instanceId = userSettings.instanceId ?? '';
await Container.get(PostHogClient).init(instanceId);
await Container.get(InternalHooks).init(instanceId);
Expand Down
7 changes: 5 additions & 2 deletions packages/cli/test/unit/ActiveWorkflowRunner.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import { mockInstance } from '../integration/shared/utils/';
import { Push } from '@/push';
import { ActiveExecutions } from '@/ActiveExecutions';
import { NodeTypes } from '@/NodeTypes';
import type { WebhookRepository } from '@/databases/repositories';

/**
* TODO:
Expand Down Expand Up @@ -139,6 +140,7 @@ const workflowExecuteAdditionalDataExecuteErrorWorkflowSpy = jest.spyOn(
describe('ActiveWorkflowRunner', () => {
let externalHooks: ExternalHooks;
let activeWorkflowRunner: ActiveWorkflowRunner;
let webhookRepository = mock<WebhookRepository>();

beforeAll(async () => {
LoggerProxy.init(getLogger());
Expand All @@ -160,6 +162,7 @@ describe('ActiveWorkflowRunner', () => {
new ActiveExecutions(),
externalHooks,
Container.get(NodeTypes),
webhookRepository,
);
});

Expand All @@ -174,7 +177,7 @@ describe('ActiveWorkflowRunner', () => {
await activeWorkflowRunner.init();
expect(await activeWorkflowRunner.getActiveWorkflows()).toHaveLength(0);
expect(mocked(Db.collections.Workflow.find)).toHaveBeenCalled();
expect(mocked(Db.collections.Webhook.clear)).toHaveBeenCalled();
expect(mocked(webhookRepository.clear)).toHaveBeenCalled();
expect(externalHooks.run).toHaveBeenCalledTimes(1);
});

Expand All @@ -185,7 +188,7 @@ describe('ActiveWorkflowRunner', () => {
databaseActiveWorkflowsCount,
);
expect(mocked(Db.collections.Workflow.find)).toHaveBeenCalled();
expect(mocked(Db.collections.Webhook.clear)).toHaveBeenCalled();
expect(mocked(webhookRepository.clear)).toHaveBeenCalled();
expect(externalHooks.run).toHaveBeenCalled();
});

Expand Down
Loading