From eac7c75a8b01ac3be7185b3eb776b87cc9ffc532 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Fri, 12 Jul 2024 10:20:15 +0200 Subject: [PATCH 01/17] fix typos on "recurrence" --- .../nodes/Schedule/GenericFunctions.ts | 32 +++++++-------- .../nodes/Schedule/ScheduleTrigger.node.ts | 40 +++++++++---------- .../nodes/Schedule/SchedulerInterface.ts | 2 +- 3 files changed, 37 insertions(+), 37 deletions(-) diff --git a/packages/nodes-base/nodes/Schedule/GenericFunctions.ts b/packages/nodes-base/nodes/Schedule/GenericFunctions.ts index 97cec572411e7..4f41e15148c51 100644 --- a/packages/nodes-base/nodes/Schedule/GenericFunctions.ts +++ b/packages/nodes-base/nodes/Schedule/GenericFunctions.ts @@ -1,22 +1,22 @@ import type { IDataObject } from 'n8n-workflow'; import moment from 'moment-timezone'; -import type { IRecurencyRule } from './SchedulerInterface'; +import type { IRecurrenceRule } from './SchedulerInterface'; -export function recurencyCheck( - recurrency: IRecurencyRule, - recurrencyRules: number[], +export function recurrenceCheck( + recurrence: IRecurrenceRule, + recurrenceRules: number[], timezone: string, ): boolean { - const recurrencyRuleIndex = recurrency.index; - const intervalSize = recurrency.intervalSize; - const typeInterval = recurrency.typeInterval; + const recurrenceRuleIndex = recurrence.index; + const intervalSize = recurrence.intervalSize; + const typeInterval = recurrence.typeInterval; const lastExecution = - recurrencyRuleIndex !== undefined ? recurrencyRules[recurrencyRuleIndex] : undefined; + recurrenceRuleIndex !== undefined ? recurrenceRules[recurrenceRuleIndex] : undefined; if ( intervalSize && - recurrencyRuleIndex !== undefined && + recurrenceRuleIndex !== undefined && (typeInterval === 'weeks' || typeInterval === 'undefined') ) { if ( @@ -24,31 +24,31 @@ export function recurencyCheck( moment.tz(timezone).week() === (intervalSize + lastExecution) % 52 || // not first time, but minimum interval has passed moment.tz(timezone).week() === lastExecution // Trigger on multiple days in the same week ) { - recurrencyRules[recurrencyRuleIndex] = moment.tz(timezone).week(); + recurrenceRules[recurrenceRuleIndex] = moment.tz(timezone).week(); return true; } - } else if (intervalSize && recurrencyRuleIndex !== undefined && typeInterval === 'days') { + } else if (intervalSize && recurrenceRuleIndex !== undefined && typeInterval === 'days') { if ( lastExecution === undefined || moment.tz(timezone).dayOfYear() === (intervalSize + lastExecution) % 365 ) { - recurrencyRules[recurrencyRuleIndex] = moment.tz(timezone).dayOfYear(); + recurrenceRules[recurrenceRuleIndex] = moment.tz(timezone).dayOfYear(); return true; } - } else if (intervalSize && recurrencyRuleIndex !== undefined && typeInterval === 'hours') { + } else if (intervalSize && recurrenceRuleIndex !== undefined && typeInterval === 'hours') { if ( lastExecution === undefined || moment.tz(timezone).hour() === (intervalSize + lastExecution) % 24 ) { - recurrencyRules[recurrencyRuleIndex] = moment.tz(timezone).hour(); + recurrenceRules[recurrenceRuleIndex] = moment.tz(timezone).hour(); return true; } - } else if (intervalSize && recurrencyRuleIndex !== undefined && typeInterval === 'months') { + } else if (intervalSize && recurrenceRuleIndex !== undefined && typeInterval === 'months') { if ( lastExecution === undefined || moment.tz(timezone).month() === (intervalSize + lastExecution) % 12 ) { - recurrencyRules[recurrencyRuleIndex] = moment.tz(timezone).month(); + recurrenceRules[recurrenceRuleIndex] = moment.tz(timezone).month(); return true; } } else { diff --git a/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts b/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts index 09627e8d4b37d..bb18dced47a11 100644 --- a/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts +++ b/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts @@ -9,8 +9,8 @@ import { NodeOperationError } from 'n8n-workflow'; import { CronJob } from 'cron'; import moment from 'moment-timezone'; -import type { IRecurencyRule } from './SchedulerInterface'; -import { addFallbackValue, convertToUnixFormat, recurencyCheck } from './GenericFunctions'; +import type { IRecurrenceRule } from './SchedulerInterface'; +import { addFallbackValue, convertToUnixFormat, recurrenceCheck } from './GenericFunctions'; export class ScheduleTrigger implements INodeType { description: INodeTypeDescription = { @@ -419,13 +419,13 @@ export class ScheduleTrigger implements INodeType { const cronJobs: CronJob[] = []; const intervalArr: NodeJS.Timeout[] = []; const staticData = this.getWorkflowStaticData('node') as { - recurrencyRules: number[]; + recurrenceRules: number[]; }; - if (!staticData.recurrencyRules) { - staticData.recurrencyRules = []; + if (!staticData.recurrenceRules) { + staticData.recurrenceRules = []; } const fallbackToZero = addFallbackValue(nodeVersion >= 1.2, '0'); - const executeTrigger = async (recurency: IRecurencyRule) => { + const executeTrigger = async (recurrence: IRecurrenceRule) => { const resultData = { timestamp: moment.tz(timezone).toISOString(true), 'Readable date': moment.tz(timezone).format('MMMM Do YYYY, h:mm:ss a'), @@ -440,10 +440,10 @@ export class ScheduleTrigger implements INodeType { Timezone: moment.tz(timezone).format('z Z'), }; - if (!recurency.activated) { + if (!recurrence.activated) { this.emit([this.helpers.returnJsonArray([resultData])]); } else { - if (recurencyCheck(recurency, staticData.recurrencyRules, timezone)) { + if (recurrenceCheck(recurrence, staticData.recurrenceRules, timezone)) { this.emit([this.helpers.returnJsonArray([resultData])]); } } @@ -460,7 +460,7 @@ export class ScheduleTrigger implements INodeType { try { const cronJob = new CronJob( cronExpression, - async () => await executeTrigger({ activated: false } as IRecurencyRule), + async () => await executeTrigger({ activated: false } as IRecurrenceRule), undefined, true, timezone, @@ -477,7 +477,7 @@ export class ScheduleTrigger implements INodeType { const seconds = interval[i].secondsInterval as number; intervalValue *= seconds; const intervalObj = setInterval( - async () => await executeTrigger({ activated: false } as IRecurencyRule), + async () => await executeTrigger({ activated: false } as IRecurrenceRule), intervalValue, ) as NodeJS.Timeout; intervalArr.push(intervalObj); @@ -487,7 +487,7 @@ export class ScheduleTrigger implements INodeType { const minutes = interval[i].minutesInterval as number; intervalValue *= 60 * minutes; const intervalObj = setInterval( - async () => await executeTrigger({ activated: false } as IRecurencyRule), + async () => await executeTrigger({ activated: false } as IRecurrenceRule), intervalValue, ) as NodeJS.Timeout; intervalArr.push(intervalObj); @@ -502,7 +502,7 @@ export class ScheduleTrigger implements INodeType { if (hour === 1) { const cronJob = new CronJob( cronExpression, - async () => await executeTrigger({ activated: false } as IRecurencyRule), + async () => await executeTrigger({ activated: false } as IRecurrenceRule), undefined, true, timezone, @@ -517,7 +517,7 @@ export class ScheduleTrigger implements INodeType { index: i, intervalSize: hour, typeInterval: 'hours', - } as IRecurencyRule), + } as IRecurrenceRule), undefined, true, timezone, @@ -535,7 +535,7 @@ export class ScheduleTrigger implements INodeType { if (day === 1) { const cronJob = new CronJob( cronExpression, - async () => await executeTrigger({ activated: false } as IRecurencyRule), + async () => await executeTrigger({ activated: false } as IRecurrenceRule), undefined, true, timezone, @@ -550,7 +550,7 @@ export class ScheduleTrigger implements INodeType { index: i, intervalSize: day, typeInterval: 'days', - } as IRecurencyRule), + } as IRecurrenceRule), undefined, true, timezone, @@ -570,7 +570,7 @@ export class ScheduleTrigger implements INodeType { if (week === 1) { const cronJob = new CronJob( cronExpression, - async () => await executeTrigger({ activated: false } as IRecurencyRule), + async () => await executeTrigger({ activated: false } as IRecurrenceRule), undefined, true, timezone, @@ -585,7 +585,7 @@ export class ScheduleTrigger implements INodeType { index: i, intervalSize: week, typeInterval: 'weeks', - } as IRecurencyRule), + } as IRecurrenceRule), undefined, true, timezone, @@ -604,7 +604,7 @@ export class ScheduleTrigger implements INodeType { if (month === 1) { const cronJob = new CronJob( cronExpression, - async () => await executeTrigger({ activated: false } as IRecurencyRule), + async () => await executeTrigger({ activated: false } as IRecurrenceRule), undefined, true, timezone, @@ -619,7 +619,7 @@ export class ScheduleTrigger implements INodeType { index: i, intervalSize: month, typeInterval: 'months', - } as IRecurencyRule), + } as IRecurrenceRule), undefined, true, timezone, @@ -639,7 +639,7 @@ export class ScheduleTrigger implements INodeType { } async function manualTriggerFunction() { - void executeTrigger({ activated: false } as IRecurencyRule); + void executeTrigger({ activated: false } as IRecurrenceRule); } return { diff --git a/packages/nodes-base/nodes/Schedule/SchedulerInterface.ts b/packages/nodes-base/nodes/Schedule/SchedulerInterface.ts index b2aec0d748585..9367eab02dd09 100644 --- a/packages/nodes-base/nodes/Schedule/SchedulerInterface.ts +++ b/packages/nodes-base/nodes/Schedule/SchedulerInterface.ts @@ -1,4 +1,4 @@ -export interface IRecurencyRule { +export interface IRecurrenceRule { activated: boolean; index?: number; intervalSize?: number; From e387cec2122abb748aa7ed18461073d1a5224992 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Fri, 12 Jul 2024 11:05:10 +0200 Subject: [PATCH 02/17] refactor(core): Centralize CronJob management --- packages/core/package.json | 3 +- packages/core/src/ActiveWorkflows.ts | 65 ++--- packages/core/src/Interfaces.ts | 2 - packages/core/src/NodeExecuteFunctions.ts | 22 +- packages/core/src/ScheduledTaskManager.ts | 33 +++ .../core/test/ScheduledTaskManager.test.ts | 54 ++++ packages/nodes-base/nodes/Cron/Cron.node.ts | 24 +- .../nodes/Schedule/GenericFunctions.ts | 97 +++---- .../nodes/Schedule/ScheduleTrigger.node.ts | 261 +++++------------- .../nodes/Schedule/SchedulerInterface.ts | 57 +++- .../Schedule/test/GenericFunctions.test.ts | 115 ++++++++ packages/nodes-base/package.json | 2 - packages/workflow/src/Cron.ts | 4 +- packages/workflow/src/Interfaces.ts | 16 +- packages/workflow/src/Workflow.ts | 5 + pnpm-lock.yaml | 35 +-- 16 files changed, 424 insertions(+), 371 deletions(-) create mode 100644 packages/core/src/ScheduledTaskManager.ts create mode 100644 packages/core/test/ScheduledTaskManager.test.ts create mode 100644 packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts diff --git a/packages/core/package.json b/packages/core/package.json index 7378e983d8d9b..4df4c190022ff 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -28,7 +28,6 @@ "devDependencies": { "@types/aws4": "^1.5.1", "@types/concat-stream": "^2.0.0", - "@types/cron": "~1.7.1", "@types/express": "^4.17.21", "@types/lodash": "^4.14.195", "@types/mime-types": "^2.1.0", @@ -40,7 +39,7 @@ "aws4": "1.11.0", "axios": "1.6.7", "concat-stream": "2.0.0", - "cron": "1.7.2", + "cron": "3.1.7", "fast-glob": "3.2.12", "file-type": "16.5.4", "form-data": "4.0.0", diff --git a/packages/core/src/ActiveWorkflows.ts b/packages/core/src/ActiveWorkflows.ts index bc2b3a489595e..bfc631962690b 100644 --- a/packages/core/src/ActiveWorkflows.ts +++ b/packages/core/src/ActiveWorkflows.ts @@ -1,11 +1,9 @@ import { Service } from 'typedi'; -import { CronJob } from 'cron'; import type { IGetExecutePollFunctions, IGetExecuteTriggerFunctions, INode, - IPollResponse, ITriggerResponse, IWorkflowExecuteAdditionalData, TriggerTime, @@ -23,10 +21,13 @@ import { WorkflowDeactivationError, } from 'n8n-workflow'; +import { ScheduledTaskManager } from './ScheduledTaskManager'; import type { IWorkflowData } from './Interfaces'; @Service() export class ActiveWorkflows { + constructor(private readonly scheduledTaskManager: ScheduledTaskManager) {} + private activeWorkflows: { [workflowId: string]: IWorkflowData } = {}; /** @@ -102,20 +103,15 @@ export class ActiveWorkflows { if (pollingNodes.length === 0) return; - this.activeWorkflows[workflowId].pollResponses = []; - for (const pollNode of pollingNodes) { try { - // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion - this.activeWorkflows[workflowId].pollResponses!.push( - await this.activatePolling( - pollNode, - workflow, - additionalData, - getPollFunctions, - mode, - activation, - ), + await this.activatePolling( + pollNode, + workflow, + additionalData, + getPollFunctions, + mode, + activation, ); } catch (e) { const error = e instanceof Error ? e : new Error(`${e}`); @@ -138,7 +134,7 @@ export class ActiveWorkflows { getPollFunctions: IGetExecutePollFunctions, mode: WorkflowExecuteMode, activation: WorkflowActivateMode, - ): Promise { + ): Promise { const pollFunctions = getPollFunctions(workflow, node, additionalData, mode, activation); const pollTimes = pollFunctions.getNodeParameter('pollTimes') as unknown as { @@ -161,7 +157,7 @@ export class ActiveWorkflows { pollFunctions.__emit(pollResponse); } } catch (error) { - // If the poll function failes in the first activation + // If the poll function fails in the first activation // throw the error back so we let the user know there is // an issue with the trigger. if (testingTrigger) { @@ -174,11 +170,6 @@ export class ActiveWorkflows { // Execute the trigger directly to be able to know if it works await executeTrigger(true); - const timezone = pollFunctions.getTimezone(); - - // Start the cron-jobs - const cronJobs: CronJob[] = []; - for (const cronTime of cronTimes) { const cronTimeParts = cronTime.split(' '); if (cronTimeParts.length > 0 && cronTimeParts[0].includes('*')) { @@ -187,19 +178,8 @@ export class ActiveWorkflows { ); } - cronJobs.push(new CronJob(cronTime, executeTrigger, undefined, true, timezone)); - } - - // Stop the cron-jobs - async function closeFunction() { - for (const cronJob of cronJobs) { - cronJob.stop(); - } + this.scheduledTaskManager.registerCron(workflow, cronTime, executeTrigger); } - - return { - closeFunction, - }; } /** @@ -211,14 +191,11 @@ export class ActiveWorkflows { return false; } - const w = this.activeWorkflows[workflowId]; + this.scheduledTaskManager.deregisterCrons(workflowId); + const w = this.activeWorkflows[workflowId]; for (const r of w.triggerResponses ?? []) { - await this.close(r, workflowId, 'trigger'); - } - - for (const r of w.pollResponses ?? []) { - await this.close(r, workflowId, 'poller'); + await this.closeTrigger(r, workflowId); } delete this.activeWorkflows[workflowId]; @@ -232,11 +209,7 @@ export class ActiveWorkflows { } } - private async close( - response: ITriggerResponse | IPollResponse, - workflowId: string, - target: 'trigger' | 'poller', - ) { + private async closeTrigger(response: ITriggerResponse, workflowId: string) { if (!response.closeFunction) return; try { @@ -246,14 +219,14 @@ export class ActiveWorkflows { Logger.error( `There was a problem calling "closeFunction" on "${e.node.name}" in workflow "${workflowId}"`, ); - ErrorReporter.error(e, { extra: { target, workflowId } }); + ErrorReporter.error(e, { extra: { workflowId } }); return; } const error = e instanceof Error ? e : new Error(`${e}`); throw new WorkflowDeactivationError( - `Failed to deactivate ${target} of workflow ID "${workflowId}": "${error.message}"`, + `Failed to deactivate trigger of workflow ID "${workflowId}": "${error.message}"`, { cause: error, workflowId }, ); } diff --git a/packages/core/src/Interfaces.ts b/packages/core/src/Interfaces.ts index 66162ae171126..2963e46185e1f 100644 --- a/packages/core/src/Interfaces.ts +++ b/packages/core/src/Interfaces.ts @@ -1,5 +1,4 @@ import type { - IPollResponse, ITriggerResponse, IWorkflowSettings as IWorkflowSettingsWorkflow, ValidationResult, @@ -18,7 +17,6 @@ export interface IWorkflowSettings extends IWorkflowSettingsWorkflow { } export interface IWorkflowData { - pollResponses?: IPollResponse[]; triggerResponses?: ITriggerResponse[]; } diff --git a/packages/core/src/NodeExecuteFunctions.ts b/packages/core/src/NodeExecuteFunctions.ts index d5e0653bb6998..3e0b1dfb3bef3 100644 --- a/packages/core/src/NodeExecuteFunctions.ts +++ b/packages/core/src/NodeExecuteFunctions.ts @@ -102,6 +102,7 @@ import type { INodeParameters, EnsureTypeOptions, SSHTunnelFunctions, + SchedulingFunctions, } from 'n8n-workflow'; import { ExpressionError, @@ -114,7 +115,6 @@ import { createDeferredPromise, deepCopy, fileTypeFromMimeType, - getGlobalState, isObjectEmpty, isResourceMapperValue, validateFieldType, @@ -157,6 +157,7 @@ import Container from 'typedi'; import type { BinaryData } from './BinaryData/types'; import merge from 'lodash/merge'; import { InstanceSettings } from './InstanceSettings'; +import { ScheduledTaskManager } from './ScheduledTaskManager'; import { SSHClientsManager } from './SSHClientsManager'; import { binaryToBuffer } from './BinaryData/utils'; @@ -2585,13 +2586,6 @@ export function getNodeWebhookUrl( return NodeHelpers.getNodeWebhookUrl(baseUrl, workflow.id, node, path.toString(), isFullPath); } -/** - * Returns the timezone for the workflow - */ -export function getTimezone(workflow: Workflow): string { - return workflow.settings.timezone ?? getGlobalState().defaultTimezone; -} - /** * Returns the full webhook description of the webhook with the given name * @@ -2957,7 +2951,7 @@ const getCommonWorkflowFunctions = ( getRestApiUrl: () => additionalData.restApiUrl, getInstanceBaseUrl: () => additionalData.instanceBaseUrl, getInstanceId: () => Container.get(InstanceSettings).instanceId, - getTimezone: () => getTimezone(workflow), + getTimezone: () => workflow.timezone, getCredentialsProperties: (type: string) => additionalData.credentialsHelper.getCredentialsProperties(type), prepareOutputData: async (outputData) => [outputData], @@ -3286,6 +3280,14 @@ const getSSHTunnelFunctions = (): SSHTunnelFunctions => ({ await Container.get(SSHClientsManager).getClient(credentials), }); +const getSchedulingFunctions = (workflow: Workflow): SchedulingFunctions => { + const scheduledTaskManager = Container.get(ScheduledTaskManager); + return { + registerCron: (cronExpression, onTick) => + scheduledTaskManager.registerCron(workflow, cronExpression, onTick), + }; +}; + const getAllowedPaths = () => { const restrictFileAccessTo = process.env[RESTRICT_FILE_ACCESS_TO]; if (!restrictFileAccessTo) { @@ -3489,6 +3491,7 @@ export function getExecutePollFunctions( createDeferredPromise, ...getRequestHelperFunctions(workflow, node, additionalData), ...getBinaryHelperFunctions(additionalData, workflow.id), + ...getSchedulingFunctions(workflow), returnJsonArray, }, }; @@ -3553,6 +3556,7 @@ export function getExecuteTriggerFunctions( ...getSSHTunnelFunctions(), ...getRequestHelperFunctions(workflow, node, additionalData), ...getBinaryHelperFunctions(additionalData, workflow.id), + ...getSchedulingFunctions(workflow), returnJsonArray, }, }; diff --git a/packages/core/src/ScheduledTaskManager.ts b/packages/core/src/ScheduledTaskManager.ts new file mode 100644 index 0000000000000..68f105f5ddcf2 --- /dev/null +++ b/packages/core/src/ScheduledTaskManager.ts @@ -0,0 +1,33 @@ +import { Service } from 'typedi'; +import { CronJob } from 'cron'; +import type { CronExpression, Workflow } from 'n8n-workflow'; + +@Service() +export class ScheduledTaskManager { + readonly cronJobs = new Map(); + + registerCron(workflow: Workflow, cronExpression: CronExpression, onTick: () => void) { + const cronJob = new CronJob( + cronExpression as string, + onTick, + undefined, + true, + workflow.timezone, + ); + if (!this.cronJobs.has(workflow.id)) this.cronJobs.set(workflow.id, []); + this.cronJobs.get(workflow.id)!.push(cronJob); + } + + deregisterCrons(workflowId: string) { + const cronJobs = this.cronJobs.get(workflowId) ?? []; + for (const cronJob of cronJobs) { + cronJob.stop(); + } + } + + deregisterAllCrons() { + for (const workflowId of Object.keys(this.cronJobs)) { + this.deregisterCrons(workflowId); + } + } +} diff --git a/packages/core/test/ScheduledTaskManager.test.ts b/packages/core/test/ScheduledTaskManager.test.ts new file mode 100644 index 0000000000000..df7fb9b77e555 --- /dev/null +++ b/packages/core/test/ScheduledTaskManager.test.ts @@ -0,0 +1,54 @@ +import type { Workflow } from 'n8n-workflow'; +import { mock } from 'jest-mock-extended'; + +import { ScheduledTaskManager } from '@/ScheduledTaskManager'; + +describe('ScheduledTaskManager', () => { + const workflow = mock({ timezone: 'GMT' }); + const everyMinute = '0 * * * * *'; + const onTick = jest.fn(); + + let scheduledTaskManager: ScheduledTaskManager; + + beforeEach(() => { + jest.clearAllMocks(); + jest.useFakeTimers(); + scheduledTaskManager = new ScheduledTaskManager(); + }); + + it('should throw when workflow timezone is invalid', () => { + expect(() => + scheduledTaskManager.registerCron( + mock({ timezone: 'somewhere' }), + everyMinute, + onTick, + ), + ).toThrow('Invalid timezone.'); + }); + + it('should throw when cron expression is invalid', () => { + expect(() => + //@ts-expect-error invalid cron expression is a type-error + scheduledTaskManager.registerCron(workflow, 'invalid-cron-expression', onTick), + ).toThrow(); + }); + + it('should register valid CronJobs', async () => { + scheduledTaskManager.registerCron(workflow, everyMinute, onTick); + + expect(onTick).not.toHaveBeenCalled(); + jest.advanceTimersByTime(10 * 60 * 1000); // 10 minutes + expect(onTick).toHaveBeenCalledTimes(10); + }); + + it('should deregister CronJobs for a workflow', async () => { + scheduledTaskManager.registerCron(workflow, everyMinute, onTick); + scheduledTaskManager.registerCron(workflow, everyMinute, onTick); + scheduledTaskManager.registerCron(workflow, everyMinute, onTick); + scheduledTaskManager.deregisterCrons(workflow.id); + + expect(onTick).not.toHaveBeenCalled(); + jest.advanceTimersByTime(10 * 60 * 1000); // 10 minutes + expect(onTick).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/nodes-base/nodes/Cron/Cron.node.ts b/packages/nodes-base/nodes/Cron/Cron.node.ts index 7430e6c07a556..62f0c595f3ef1 100644 --- a/packages/nodes-base/nodes/Cron/Cron.node.ts +++ b/packages/nodes-base/nodes/Cron/Cron.node.ts @@ -7,8 +7,6 @@ import type { } from 'n8n-workflow'; import { NodeHelpers, toCronExpression } from 'n8n-workflow'; -import { CronJob } from 'cron'; - export class Cron implements INodeType { description: INodeTypeDescription = { displayName: 'Cron', @@ -66,27 +64,11 @@ export class Cron implements INodeType { this.emit([this.helpers.returnJsonArray([{}])]); }; - const timezone = this.getTimezone(); - - // Start the cron-jobs - const cronJobs = cronTimes.map( - (cronTime) => new CronJob(cronTime, executeTrigger, undefined, true, timezone), - ); - - // Stop the cron-jobs - async function closeFunction() { - for (const cronJob of cronJobs) { - cronJob.stop(); - } - } - - async function manualTriggerFunction() { - executeTrigger(); - } + // Register the cron-jobs + cronTimes.forEach((cronTime) => this.helpers.registerCron(cronTime, executeTrigger)); return { - closeFunction, - manualTriggerFunction, + manualTriggerFunction: async () => executeTrigger(), }; } } diff --git a/packages/nodes-base/nodes/Schedule/GenericFunctions.ts b/packages/nodes-base/nodes/Schedule/GenericFunctions.ts index 4f41e15148c51..892ce5fbc77f1 100644 --- a/packages/nodes-base/nodes/Schedule/GenericFunctions.ts +++ b/packages/nodes-base/nodes/Schedule/GenericFunctions.ts @@ -1,54 +1,47 @@ -import type { IDataObject } from 'n8n-workflow'; import moment from 'moment-timezone'; -import type { IRecurrenceRule } from './SchedulerInterface'; +import { type CronExpression, randomInt } from 'n8n-workflow'; +import type { IRecurrenceRule, ScheduleInterval } from './SchedulerInterface'; export function recurrenceCheck( - recurrence: IRecurrenceRule, + recurrence: IRecurrenceRule & { activated: true }, recurrenceRules: number[], timezone: string, ): boolean { + const momentTz = moment.tz(timezone); const recurrenceRuleIndex = recurrence.index; const intervalSize = recurrence.intervalSize; const typeInterval = recurrence.typeInterval; + if (!intervalSize) return false; const lastExecution = recurrenceRuleIndex !== undefined ? recurrenceRules[recurrenceRuleIndex] : undefined; - if ( - intervalSize && - recurrenceRuleIndex !== undefined && - (typeInterval === 'weeks' || typeInterval === 'undefined') - ) { + if (recurrenceRuleIndex !== undefined && typeInterval === 'weeks') { + const week = momentTz.week(); if ( lastExecution === undefined || // First time executing this rule - moment.tz(timezone).week() === (intervalSize + lastExecution) % 52 || // not first time, but minimum interval has passed - moment.tz(timezone).week() === lastExecution // Trigger on multiple days in the same week + week === (intervalSize + lastExecution) % 52 || // not first time, but minimum interval has passed + week === lastExecution // Trigger on multiple days in the same week ) { - recurrenceRules[recurrenceRuleIndex] = moment.tz(timezone).week(); + recurrenceRules[recurrenceRuleIndex] = week; return true; } - } else if (intervalSize && recurrenceRuleIndex !== undefined && typeInterval === 'days') { - if ( - lastExecution === undefined || - moment.tz(timezone).dayOfYear() === (intervalSize + lastExecution) % 365 - ) { - recurrenceRules[recurrenceRuleIndex] = moment.tz(timezone).dayOfYear(); + } else if (recurrenceRuleIndex !== undefined && typeInterval === 'days') { + const dayOfYear = momentTz.dayOfYear(); + if (lastExecution === undefined || dayOfYear === (intervalSize + lastExecution) % 365) { + recurrenceRules[recurrenceRuleIndex] = dayOfYear; return true; } - } else if (intervalSize && recurrenceRuleIndex !== undefined && typeInterval === 'hours') { - if ( - lastExecution === undefined || - moment.tz(timezone).hour() === (intervalSize + lastExecution) % 24 - ) { - recurrenceRules[recurrenceRuleIndex] = moment.tz(timezone).hour(); + } else if (recurrenceRuleIndex !== undefined && typeInterval === 'hours') { + const hour = momentTz.hour(); + if (lastExecution === undefined || hour === (intervalSize + lastExecution) % 24) { + recurrenceRules[recurrenceRuleIndex] = hour; return true; } - } else if (intervalSize && recurrenceRuleIndex !== undefined && typeInterval === 'months') { - if ( - lastExecution === undefined || - moment.tz(timezone).month() === (intervalSize + lastExecution) % 12 - ) { - recurrenceRules[recurrenceRuleIndex] = moment.tz(timezone).month(); + } else if (recurrenceRuleIndex !== undefined && typeInterval === 'months') { + const month = momentTz.month(); + if (lastExecution === undefined || month === (intervalSize + lastExecution) % 12) { + recurrenceRules[recurrenceRuleIndex] = month; return true; } } else { @@ -57,37 +50,23 @@ export function recurrenceCheck( return false; } -export function convertMonthToUnix(expression: string): string { - if (!isNaN(parseInt(expression)) || expression.includes('-') || expression.includes(',')) { - let matches = expression.match(/([0-9])+/g) as string[]; - if (matches) { - matches = matches.map((match) => - parseInt(match) !== 0 ? String(parseInt(match) - 1) : match, - ); - } - expression = matches?.join(expression.includes('-') ? '-' : ',') || ''; - } - return expression; -} +export const toCronExpression = (interval: ScheduleInterval): CronExpression => { + if (interval.field === 'cronExpression') return interval.expression; + if (interval.field === 'seconds') return `*/${interval.secondsInterval} * * * * *`; + if (interval.field === 'minutes') return `* */${interval.minutesInterval} * * * *`; -export function convertToUnixFormat(interval: IDataObject) { - const expression = (interval.expression as string).split(' '); - if (expression.length === 5) { - expression[3] = convertMonthToUnix(expression[3]); - expression[4] = expression[4].replace('7', '0'); - } else if (expression.length === 6) { - expression[4] = convertMonthToUnix(expression[4]); - expression[5] = expression[5].replace('7', '0'); - } - interval.expression = expression.join(' '); -} + const minute = interval.triggerAtMinute ?? randomInt(0, 60); + if (interval.field === 'hours') return `* ${minute} */${interval.hoursInterval} * * *`; -export const addFallbackValue = (enabled: boolean, fallback: T) => { - if (enabled) { - return (value: T) => { - if (!value) return fallback; - return value; - }; + // Since Cron does not support `*/` for days or weeks, all following expressions trigger more often, but are then filtered by `recurrenceCheck` + const hour = interval.triggerAtHour ?? randomInt(0, 24); + if (interval.field === 'days') return `* ${minute} ${hour} * * *`; + if (interval.field === 'weeks') { + const days = interval.triggerAtDay; + const daysOfWeek = days.length === 0 ? '*' : days.join(','); + return `* ${minute} ${hour} * * ${daysOfWeek}`; } - return (value: T) => value; + + const dayOfMonth = interval.triggerAtDayOfMonth ?? randomInt(0, 31); + return `* ${minute} ${hour} ${dayOfMonth} */${interval.monthsInterval} *`; }; diff --git a/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts b/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts index bb18dced47a11..c8eaa1092b21b 100644 --- a/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts +++ b/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts @@ -1,16 +1,14 @@ import type { ITriggerFunctions, - IDataObject, INodeType, INodeTypeDescription, ITriggerResponse, } from 'n8n-workflow'; import { NodeOperationError } from 'n8n-workflow'; -import { CronJob } from 'cron'; import moment from 'moment-timezone'; -import type { IRecurrenceRule } from './SchedulerInterface'; -import { addFallbackValue, convertToUnixFormat, recurrenceCheck } from './GenericFunctions'; +import type { IRecurrenceRule, Rule } from './SchedulerInterface'; +import { recurrenceCheck, toCronExpression } from './GenericFunctions'; export class ScheduleTrigger implements INodeType { description: INodeTypeDescription = { @@ -412,32 +410,29 @@ export class ScheduleTrigger implements INodeType { }; async trigger(this: ITriggerFunctions): Promise { - const rule = this.getNodeParameter('rule', []) as IDataObject; - const interval = rule.interval as IDataObject[]; + const { interval: intervals } = this.getNodeParameter('rule', []) as Rule; const timezone = this.getTimezone(); - const nodeVersion = this.getNode().typeVersion; - const cronJobs: CronJob[] = []; - const intervalArr: NodeJS.Timeout[] = []; const staticData = this.getWorkflowStaticData('node') as { recurrenceRules: number[]; }; if (!staticData.recurrenceRules) { staticData.recurrenceRules = []; } - const fallbackToZero = addFallbackValue(nodeVersion >= 1.2, '0'); + const executeTrigger = async (recurrence: IRecurrenceRule) => { + const momentTz = moment.tz(timezone); const resultData = { - timestamp: moment.tz(timezone).toISOString(true), - 'Readable date': moment.tz(timezone).format('MMMM Do YYYY, h:mm:ss a'), - 'Readable time': moment.tz(timezone).format('h:mm:ss a'), - 'Day of week': moment.tz(timezone).format('dddd'), - Year: moment.tz(timezone).format('YYYY'), - Month: moment.tz(timezone).format('MMMM'), - 'Day of month': moment.tz(timezone).format('DD'), - Hour: moment.tz(timezone).format('HH'), - Minute: moment.tz(timezone).format('mm'), - Second: moment.tz(timezone).format('ss'), - Timezone: moment.tz(timezone).format('z Z'), + timestamp: momentTz.toISOString(true), + 'Readable date': momentTz.format('MMMM Do YYYY, h:mm:ss a'), + 'Readable time': momentTz.format('h:mm:ss a'), + 'Day of week': momentTz.format('dddd'), + Year: momentTz.format('YYYY'), + Month: momentTz.format('MMMM'), + 'Day of month': momentTz.format('DD'), + Hour: momentTz.format('HH'), + Minute: momentTz.format('mm'), + Second: momentTz.format('ss'), + Timezone: momentTz.format('z Z'), }; if (!recurrence.activated) { @@ -449,201 +444,77 @@ export class ScheduleTrigger implements INodeType { } }; - for (let i = 0; i < interval.length; i++) { - let intervalValue = 1000; - if (interval[i].field === 'cronExpression') { - if (nodeVersion > 1) { - // ! Remove this part if we use a cron library that follows unix cron expression - convertToUnixFormat(interval[i]); - } - const cronExpression = interval[i].expression as string; - try { - const cronJob = new CronJob( - cronExpression, - async () => await executeTrigger({ activated: false } as IRecurrenceRule), - undefined, - true, - timezone, - ); - cronJobs.push(cronJob); - } catch (error) { - throw new NodeOperationError(this.getNode(), 'Invalid cron expression', { - description: 'More information on how to build them at https://crontab.guru/', - }); - } - } - - if (interval[i].field === 'seconds') { - const seconds = interval[i].secondsInterval as number; - intervalValue *= seconds; - const intervalObj = setInterval( - async () => await executeTrigger({ activated: false } as IRecurrenceRule), - intervalValue, - ) as NodeJS.Timeout; - intervalArr.push(intervalObj); - } + for (let i = 0; i < intervals.length; i++) { + const interval = intervals[i]; + const cronExpression = toCronExpression(interval); + let recurrence: IRecurrenceRule = { activated: false }; - if (interval[i].field === 'minutes') { - const minutes = interval[i].minutesInterval as number; - intervalValue *= 60 * minutes; - const intervalObj = setInterval( - async () => await executeTrigger({ activated: false } as IRecurrenceRule), - intervalValue, - ) as NodeJS.Timeout; - intervalArr.push(intervalObj); + if (interval.field === 'hours') { + const hour = interval.hoursInterval; + if (hour !== 1) { + recurrence = { + activated: true, + index: i, + intervalSize: hour, + typeInterval: 'hours', + }; + } } - if (interval[i].field === 'hours') { - const hour = interval[i].hoursInterval as number; - const minute = fallbackToZero(interval[i].triggerAtMinute?.toString() as string); - - const cronTimes: string[] = [minute, '*', '*', '*', '*']; - const cronExpression: string = cronTimes.join(' '); - if (hour === 1) { - const cronJob = new CronJob( - cronExpression, - async () => await executeTrigger({ activated: false } as IRecurrenceRule), - undefined, - true, - timezone, - ); - cronJobs.push(cronJob); - } else { - const cronJob = new CronJob( - cronExpression, - async () => - await executeTrigger({ - activated: true, - index: i, - intervalSize: hour, - typeInterval: 'hours', - } as IRecurrenceRule), - undefined, - true, - timezone, - ); - cronJobs.push(cronJob); + if (interval.field === 'days') { + const day = interval.daysInterval; + if (day !== 1) { + recurrence = { + activated: true, + index: i, + intervalSize: day, + typeInterval: 'days', + }; } } - if (interval[i].field === 'days') { - const day = interval[i].daysInterval as number; - const hour = interval[i].triggerAtHour?.toString() as string; - const minute = fallbackToZero(interval[i].triggerAtMinute?.toString() as string); - const cronTimes: string[] = [minute, hour, '*', '*', '*']; - const cronExpression: string = cronTimes.join(' '); - if (day === 1) { - const cronJob = new CronJob( - cronExpression, - async () => await executeTrigger({ activated: false } as IRecurrenceRule), - undefined, - true, - timezone, - ); - cronJobs.push(cronJob); - } else { - const cronJob = new CronJob( - cronExpression, - async () => - await executeTrigger({ - activated: true, - index: i, - intervalSize: day, - typeInterval: 'days', - } as IRecurrenceRule), - undefined, - true, - timezone, - ); - cronJobs.push(cronJob); + if (interval.field === 'weeks') { + const week = interval.weeksInterval; + if (week !== 1) { + recurrence = { + activated: true, + index: i, + intervalSize: week, + typeInterval: 'weeks', + }; } } - if (interval[i].field === 'weeks') { - const hour = interval[i].triggerAtHour?.toString() as string; - const minute = fallbackToZero(interval[i].triggerAtMinute?.toString() as string); - const week = interval[i].weeksInterval as number; - const days = interval[i].triggerAtDay as IDataObject[]; - const day = days.length === 0 ? '*' : days.join(','); - const cronTimes: string[] = [minute, hour, '*', '*', day]; - const cronExpression = cronTimes.join(' '); - if (week === 1) { - const cronJob = new CronJob( - cronExpression, - async () => await executeTrigger({ activated: false } as IRecurrenceRule), - undefined, - true, - timezone, - ); - cronJobs.push(cronJob); - } else { - const cronJob = new CronJob( - cronExpression, - async () => - await executeTrigger({ - activated: true, - index: i, - intervalSize: week, - typeInterval: 'weeks', - } as IRecurrenceRule), - undefined, - true, - timezone, - ); - cronJobs.push(cronJob); + if (interval.field === 'months') { + const month = interval.monthsInterval; + if (month !== 1) { + recurrence = { + activated: true, + index: i, + intervalSize: month, + typeInterval: 'months', + }; } } - if (interval[i].field === 'months') { - const month = interval[i].monthsInterval; - const day = interval[i].triggerAtDayOfMonth?.toString() as string; - const hour = interval[i].triggerAtHour?.toString() as string; - const minute = fallbackToZero(interval[i].triggerAtMinute?.toString() as string); - const cronTimes: string[] = [minute, hour, day, '*', '*']; - const cronExpression: string = cronTimes.join(' '); - if (month === 1) { - const cronJob = new CronJob( - cronExpression, - async () => await executeTrigger({ activated: false } as IRecurrenceRule), - undefined, - true, - timezone, - ); - cronJobs.push(cronJob); + try { + this.helpers.registerCron(cronExpression, async () => await executeTrigger(recurrence)); + } catch (error) { + if (interval.field === 'cronExpression') { + throw new NodeOperationError(this.getNode(), 'Invalid cron expression', { + description: 'More information on how to build them at https://crontab.guru/', + }); } else { - const cronJob = new CronJob( - cronExpression, - async () => - await executeTrigger({ - activated: true, - index: i, - intervalSize: month, - typeInterval: 'months', - } as IRecurrenceRule), - undefined, - true, - timezone, - ); - cronJobs.push(cronJob); + throw error; } } } - async function closeFunction() { - for (const cronJob of cronJobs) { - cronJob.stop(); - } - for (const entry of intervalArr) { - clearInterval(entry); - } - } - async function manualTriggerFunction() { - void executeTrigger({ activated: false } as IRecurrenceRule); + void executeTrigger({ activated: false }); } return { - closeFunction, manualTriggerFunction, }; } diff --git a/packages/nodes-base/nodes/Schedule/SchedulerInterface.ts b/packages/nodes-base/nodes/Schedule/SchedulerInterface.ts index 9367eab02dd09..1acc9ee9edd51 100644 --- a/packages/nodes-base/nodes/Schedule/SchedulerInterface.ts +++ b/packages/nodes-base/nodes/Schedule/SchedulerInterface.ts @@ -1,6 +1,53 @@ -export interface IRecurrenceRule { - activated: boolean; - index?: number; - intervalSize?: number; - typeInterval?: string; +import type { CronExpression } from 'n8n-workflow'; + +export type IRecurrenceRule = + | { activated: false } + | { + activated: true; + index: number; + intervalSize: number; + typeInterval: 'hours' | 'days' | 'weeks' | 'months'; + }; + +export type ScheduleInterval = + | { + field: 'cronExpression'; + expression: CronExpression; + } + | { + field: 'seconds'; + secondsInterval: number; + } + | { + field: 'minutes'; + minutesInterval: number; + } + | { + field: 'hours'; + hoursInterval: number; + triggerAtMinute?: number; + } + | { + field: 'days'; + daysInterval: number; + triggerAtHour?: number; + triggerAtMinute?: number; + } + | { + field: 'weeks'; + weeksInterval: number; + triggerAtDay: number[]; + triggerAtHour?: number; + triggerAtMinute?: number; + } + | { + field: 'months'; + monthsInterval: number; + triggerAtDayOfMonth?: number; + triggerAtHour?: number; + triggerAtMinute?: number; + }; + +export interface Rule { + interval: ScheduleInterval[]; } diff --git a/packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts b/packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts new file mode 100644 index 0000000000000..fff01284d4939 --- /dev/null +++ b/packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts @@ -0,0 +1,115 @@ +import { recurrenceCheck, toCronExpression } from '../GenericFunctions'; + +describe('toCronExpression', () => { + it('should return cron expression for cronExpression field', () => { + const result = toCronExpression({ + field: 'cronExpression', + expression: '1 2 3 * * *', + }); + expect(result).toEqual('1 2 3 * * *'); + }); + + it('should return cron expression for seconds interval', () => { + const result = toCronExpression({ + field: 'seconds', + secondsInterval: 10, + }); + expect(result).toEqual('*/10 * * * * *'); + }); + + it('should return cron expression for minutes interval', () => { + const result = toCronExpression({ + field: 'minutes', + minutesInterval: 30, + }); + expect(result).toEqual('* */30 * * * *'); + }); + + it('should return cron expression for hours interval', () => { + const result = toCronExpression({ + field: 'hours', + hoursInterval: 3, + triggerAtMinute: 22, + }); + expect(result).toEqual('* 22 */3 * * *'); + }); + + it('should return cron expression for days interval', () => { + const result = toCronExpression({ + field: 'days', + daysInterval: 4, + triggerAtMinute: 30, + triggerAtHour: 12, + }); + expect(result).toEqual('* 30 12 * * *'); + }); + + it('should return cron expression for weeks interval', () => { + const result = toCronExpression({ + field: 'weeks', + weeksInterval: 2, + triggerAtMinute: 0, + triggerAtHour: 9, + triggerAtDay: [1, 3, 5], + }); + + expect(result).toEqual('* 0 9 * * 1,3,5'); + }); + + it('should return cron expression for months interval', () => { + const result = toCronExpression({ + field: 'months', + monthsInterval: 3, + triggerAtMinute: 0, + triggerAtHour: 0, + triggerAtDayOfMonth: 1, + }); + + expect(result).toEqual('* 0 0 1 */3 *'); + }); +}); + +describe('recurrenceCheck', () => { + describe('recurrenceCheck', () => { + it('should return true if recurrence rule is in the recurrence rules list', () => { + const result = recurrenceCheck( + { + activated: true, + index: 0, + intervalSize: 2, + typeInterval: 'days', + }, + [], + 'UTC', + ); + + expect(result).toBe(true); + }); + + // it('should return false if recurrence rule is not in the recurrence rules list', () => { + // const recurrence = { + // field: 'weeks', + // weeksInterval: 2, + // triggerAtMinute: 0, + // triggerAtHour: 9, + // triggerAtDay: [1, 3, 5], + // }; + // const recurrenceRules = [1, 2, 4, 5]; + // const timezone = 'UTC'; + + // const result = recurrenceCheck(recurrence, recurrenceRules, timezone); + + // expect(result).toBe(false); + // }); + + // it('should return false if recurrence rule is empty', () => { + // const recurrence = {}; + // const recurrenceRules = [1, 2, 3, 4, 5]; + // const timezone = 'UTC'; + + // const result = recurrenceCheck(recurrence, recurrenceRules, timezone); + + // expect(result).toBe(false); + // }); + }); +}); diff --git a/packages/nodes-base/package.json b/packages/nodes-base/package.json index 7b5a73cfb691a..aa1e8b41ba50e 100644 --- a/packages/nodes-base/package.json +++ b/packages/nodes-base/package.json @@ -804,7 +804,6 @@ "@types/aws4": "^1.5.1", "@types/basic-auth": "^1.1.3", "@types/cheerio": "^0.22.15", - "@types/cron": "~1.7.1", "@types/eventsource": "^1.1.2", "@types/express": "^4.17.21", "@types/html-to-text": "^9.0.1", @@ -838,7 +837,6 @@ "change-case": "4.1.2", "cheerio": "1.0.0-rc.6", "chokidar": "3.5.2", - "cron": "1.7.2", "csv-parse": "5.5.0", "currency-codes": "2.1.0", "eventsource": "2.0.2", diff --git a/packages/workflow/src/Cron.ts b/packages/workflow/src/Cron.ts index 7f41e9ccd5a0c..e6022e8887782 100644 --- a/packages/workflow/src/Cron.ts +++ b/packages/workflow/src/Cron.ts @@ -1,10 +1,10 @@ +import type { CronExpression } from './Interfaces'; import { randomInt } from './utils'; interface BaseTriggerTime { mode: T; } -type CronExpression = string; interface CustomTrigger extends BaseTriggerTime<'custom'> { cronExpression: CronExpression; } @@ -66,5 +66,5 @@ export const toCronExpression = (item: TriggerTime): CronExpression => { if (item.mode === 'everyMonth') return `${randomSecond()} ${item.minute} ${item.hour} ${item.dayOfMonth} * *`; - return item.cronExpression.trim(); + return item.cronExpression.trim() as CronExpression; }; diff --git a/packages/workflow/src/Interfaces.ts b/packages/workflow/src/Interfaces.ts index 68dd5bea06d81..a1a9860198ec3 100644 --- a/packages/workflow/src/Interfaces.ts +++ b/packages/workflow/src/Interfaces.ts @@ -840,6 +840,14 @@ export interface SSHTunnelFunctions { getSSHClient(credentials: SSHCredentials): Promise; } +type CronUnit = number | '' | '*' | `*/${number}` | string; +export type CronExpression = + `${CronUnit} ${CronUnit} ${CronUnit} ${CronUnit} ${CronUnit} ${CronUnit}`; + +export interface SchedulingFunctions { + registerCron(cronExpression: CronExpression, onTick: () => void): void; +} + export type NodeTypeAndVersion = { name: string; type: string; @@ -992,6 +1000,7 @@ export interface IPollFunctions helpers: RequestHelperFunctions & BaseHelperFunctions & BinaryHelperFunctions & + SchedulingFunctions & JsonHelperFunctions; } @@ -1012,6 +1021,7 @@ export interface ITriggerFunctions BaseHelperFunctions & BinaryHelperFunctions & SSHTunnelFunctions & + SchedulingFunctions & JsonHelperFunctions; } @@ -1434,14 +1444,10 @@ export type IParameterLabel = { size?: 'small' | 'medium'; }; -export interface IPollResponse { - closeFunction?: CloseFunction; -} - export interface ITriggerResponse { closeFunction?: CloseFunction; // To manually trigger the run - manualTriggerFunction?: CloseFunction; + manualTriggerFunction?: () => Promise; // Gets added automatically at manual workflow runs resolves with // the first emitted data manualTriggerResponse?: Promise; diff --git a/packages/workflow/src/Workflow.ts b/packages/workflow/src/Workflow.ts index 29d789562a014..a836e99d19161 100644 --- a/packages/workflow/src/Workflow.ts +++ b/packages/workflow/src/Workflow.ts @@ -58,6 +58,7 @@ import { STARTING_NODE_TYPES, } from './Constants'; import { ApplicationError } from './errors/application.error'; +import { getGlobalState } from './GlobalState'; function dedupe(arr: T[]): T[] { return [...new Set(arr)]; @@ -155,6 +156,10 @@ export class Workflow { this.expression = new Expression(this); } + get timezone() { + return this.settings.timezone ?? getGlobalState().defaultTimezone; + } + /** * The default connections are by source node. This function rewrites them by destination nodes * to easily find parent nodes. diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 62c6a8ffe6ee4..2d43e63787644 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -903,8 +903,8 @@ importers: specifier: 2.0.0 version: 2.0.0 cron: - specifier: 1.7.2 - version: 1.7.2 + specifier: 3.1.7 + version: 3.1.7 fast-glob: specifier: 3.2.12 version: 3.2.12 @@ -957,9 +957,6 @@ importers: '@types/concat-stream': specifier: ^2.0.0 version: 2.0.0 - '@types/cron': - specifier: ~1.7.1 - version: 1.7.3 '@types/express': specifier: ^4.17.21 version: 4.17.21 @@ -1353,9 +1350,6 @@ importers: chokidar: specifier: 3.5.2 version: 3.5.2 - cron: - specifier: 1.7.2 - version: 1.7.2 csv-parse: specifier: 5.5.0 version: 5.5.0 @@ -1525,9 +1519,6 @@ importers: '@types/cheerio': specifier: ^0.22.15 version: 0.22.31 - '@types/cron': - specifier: ~1.7.1 - version: 1.7.3 '@types/eventsource': specifier: ^1.1.2 version: 1.1.9 @@ -5165,9 +5156,6 @@ packages: '@types/cookiejar@2.1.5': resolution: {integrity: sha512-he+DHOWReW0nghN24E1WUqM0efK4kI9oTqDm6XmK8ZPe2djZ90BSNdGnIyCLzCPw7/pogPlGbzI2wHGGmi4O/Q==} - '@types/cron@1.7.3': - resolution: {integrity: sha512-iPmUXyIJG1Js+ldPYhOQcYU3kCAQ2FWrSkm1FJPoii2eYSn6wEW6onPukNTT0bfiflexNSRPl6KWmAIqS+36YA==} - '@types/cross-spawn@6.0.2': resolution: {integrity: sha512-KuwNhp3eza+Rhu8IFI5HUXRP0LIhqH5cAjubUvGXXthh4YYBuP2ntwEX+Cz8GJoZUHlKo247wPWOfA9LYEq4cw==} @@ -5303,6 +5291,9 @@ packages: '@types/luxon@3.2.0': resolution: {integrity: sha512-lGmaGFoaXHuOLXFvuju2bfvZRqxAqkHPx9Y9IQdQABrinJJshJwfNCKV+u7rR3kJbiqfTF/NhOkcxxAFrObyaA==} + '@types/luxon@3.4.2': + resolution: {integrity: sha512-TifLZlFudklWlMBfhubvgqTXRzLDI5pCbGa4P8a3wPyUQSW+1xQ5eDsreP9DWHX3tjq1ke96uYG/nwundroWcA==} + '@types/mailparser@3.4.4': resolution: {integrity: sha512-C6Znp2QVS25JqtuPyxj38Qh+QoFcLycdxsvcc6IZCGekhaMBzbdTXzwGzhGoYb3TfKu8IRCNV0sV1o3Od97cEQ==} @@ -6795,8 +6786,8 @@ packages: resolution: {integrity: sha512-p0SaNjrHOnQeR8/VnfGbmg9te2kfyYSQ7Sc/j/6DtPL3JQvKxmjO9TSjNFpujqV3vEYYBvNNvXSxzyksBWAx1Q==} engines: {node: '>=12.0.0'} - cron@1.7.2: - resolution: {integrity: sha512-+SaJ2OfeRvfQqwXQ2kgr0Y5pzBR/lijf5OpnnaruwWnmI799JfWr2jN2ItOV9s3A/+TFOt6mxvKzQq5F0Jp6VQ==} + cron@3.1.7: + resolution: {integrity: sha512-tlBg7ARsAMQLzgwqVxy8AZl/qlTc5nibqYwtNGoCrd+cV+ugI+tvZC1oT/8dFH8W455YrywGykx/KMmAqOr7Jw==} cross-env@7.0.3: resolution: {integrity: sha512-+/HKd6EgcQCJGh2PSjZuUitQBQynKor4wrFbRg4DtAgS1aWO+gU52xpH7M9ScGgXSYmAVS9bIJ8EzuaGw0oNAw==} @@ -18425,11 +18416,6 @@ snapshots: '@types/cookiejar@2.1.5': {} - '@types/cron@1.7.3': - dependencies: - '@types/node': 18.16.16 - moment: 2.29.4 - '@types/cross-spawn@6.0.2': dependencies: '@types/node': 18.16.16 @@ -18572,6 +18558,8 @@ snapshots: '@types/luxon@3.2.0': {} + '@types/luxon@3.4.2': {} + '@types/mailparser@3.4.4': dependencies: '@types/node': 18.16.16 @@ -20355,9 +20343,10 @@ snapshots: dependencies: luxon: 3.4.4 - cron@1.7.2: + cron@3.1.7: dependencies: - moment-timezone: 0.5.37 + '@types/luxon': 3.4.2 + luxon: 3.4.4 cross-env@7.0.3: dependencies: From 7a49bd7e7ab12a54a1b58caa9dd0f984dea73312 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Mon, 15 Jul 2024 11:07:27 +0200 Subject: [PATCH 03/17] update toCronExpression, and add tests --- .../nodes/Schedule/GenericFunctions.ts | 13 +-- .../Schedule/test/GenericFunctions.test.ts | 87 ++++++++----------- 2 files changed, 42 insertions(+), 58 deletions(-) diff --git a/packages/nodes-base/nodes/Schedule/GenericFunctions.ts b/packages/nodes-base/nodes/Schedule/GenericFunctions.ts index 892ce5fbc77f1..ea31feb69ded1 100644 --- a/packages/nodes-base/nodes/Schedule/GenericFunctions.ts +++ b/packages/nodes-base/nodes/Schedule/GenericFunctions.ts @@ -53,20 +53,23 @@ export function recurrenceCheck( export const toCronExpression = (interval: ScheduleInterval): CronExpression => { if (interval.field === 'cronExpression') return interval.expression; if (interval.field === 'seconds') return `*/${interval.secondsInterval} * * * * *`; - if (interval.field === 'minutes') return `* */${interval.minutesInterval} * * * *`; + + const randomSecond = randomInt(0, 60); + if (interval.field === 'minutes') return `${randomSecond} */${interval.minutesInterval} * * * *`; const minute = interval.triggerAtMinute ?? randomInt(0, 60); - if (interval.field === 'hours') return `* ${minute} */${interval.hoursInterval} * * *`; + if (interval.field === 'hours') + return `${randomSecond} ${minute} */${interval.hoursInterval} * * *`; // Since Cron does not support `*/` for days or weeks, all following expressions trigger more often, but are then filtered by `recurrenceCheck` const hour = interval.triggerAtHour ?? randomInt(0, 24); - if (interval.field === 'days') return `* ${minute} ${hour} * * *`; + if (interval.field === 'days') return `${randomSecond} ${minute} ${hour} * * *`; if (interval.field === 'weeks') { const days = interval.triggerAtDay; const daysOfWeek = days.length === 0 ? '*' : days.join(','); - return `* ${minute} ${hour} * * ${daysOfWeek}`; + return `${randomSecond} ${minute} ${hour} * * ${daysOfWeek}`; } const dayOfMonth = interval.triggerAtDayOfMonth ?? randomInt(0, 31); - return `* ${minute} ${hour} ${dayOfMonth} */${interval.monthsInterval} *`; + return `${randomSecond} ${minute} ${hour} ${dayOfMonth} */${interval.monthsInterval} *`; }; diff --git a/packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts b/packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts index fff01284d4939..5ca8b60289c18 100644 --- a/packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts +++ b/packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts @@ -1,6 +1,11 @@ -import { recurrenceCheck, toCronExpression } from '../GenericFunctions'; +import { toCronExpression } from '../GenericFunctions'; +import * as n8nWorkflow from 'n8n-workflow'; describe('toCronExpression', () => { + Object.defineProperty(n8nWorkflow, 'randomInt', { + value: (min: number, max: number) => Math.floor((min + max) / 2), + }); + it('should return cron expression for cronExpression field', () => { const result = toCronExpression({ field: 'cronExpression', @@ -22,7 +27,7 @@ describe('toCronExpression', () => { field: 'minutes', minutesInterval: 30, }); - expect(result).toEqual('* */30 * * * *'); + expect(result).toEqual('30 */30 * * * *'); }); it('should return cron expression for hours interval', () => { @@ -31,7 +36,13 @@ describe('toCronExpression', () => { hoursInterval: 3, triggerAtMinute: 22, }); - expect(result).toEqual('* 22 */3 * * *'); + expect(result).toEqual('30 22 */3 * * *'); + + const result1 = toCronExpression({ + field: 'hours', + hoursInterval: 3, + }); + expect(result1).toEqual('30 30 */3 * * *'); }); it('should return cron expression for days interval', () => { @@ -39,9 +50,15 @@ describe('toCronExpression', () => { field: 'days', daysInterval: 4, triggerAtMinute: 30, - triggerAtHour: 12, + triggerAtHour: 10, + }); + expect(result).toEqual('30 30 10 * * *'); + + const result1 = toCronExpression({ + field: 'days', + daysInterval: 4, }); - expect(result).toEqual('* 30 12 * * *'); + expect(result1).toEqual('30 30 12 * * *'); }); it('should return cron expression for weeks interval', () => { @@ -52,8 +69,13 @@ describe('toCronExpression', () => { triggerAtHour: 9, triggerAtDay: [1, 3, 5], }); - - expect(result).toEqual('* 0 9 * * 1,3,5'); + expect(result).toEqual('30 0 9 * * 1,3,5'); + const result1 = toCronExpression({ + field: 'weeks', + weeksInterval: 2, + triggerAtDay: [1, 3, 5], + }); + expect(result1).toEqual('30 30 12 * * 1,3,5'); }); it('should return cron expression for months interval', () => { @@ -64,52 +86,11 @@ describe('toCronExpression', () => { triggerAtHour: 0, triggerAtDayOfMonth: 1, }); - - expect(result).toEqual('* 0 0 1 */3 *'); - }); -}); - -describe('recurrenceCheck', () => { - describe('recurrenceCheck', () => { - it('should return true if recurrence rule is in the recurrence rules list', () => { - const result = recurrenceCheck( - { - activated: true, - index: 0, - intervalSize: 2, - typeInterval: 'days', - }, - [], - 'UTC', - ); - - expect(result).toBe(true); + expect(result).toEqual('30 0 0 1 */3 *'); + const result1 = toCronExpression({ + field: 'months', + monthsInterval: 3, }); - - // it('should return false if recurrence rule is not in the recurrence rules list', () => { - // const recurrence = { - // field: 'weeks', - // weeksInterval: 2, - // triggerAtMinute: 0, - // triggerAtHour: 9, - // triggerAtDay: [1, 3, 5], - // }; - // const recurrenceRules = [1, 2, 4, 5]; - // const timezone = 'UTC'; - - // const result = recurrenceCheck(recurrence, recurrenceRules, timezone); - - // expect(result).toBe(false); - // }); - - // it('should return false if recurrence rule is empty', () => { - // const recurrence = {}; - // const recurrenceRules = [1, 2, 3, 4, 5]; - // const timezone = 'UTC'; - - // const result = recurrenceCheck(recurrence, recurrenceRules, timezone); - - // expect(result).toBe(false); - // }); + expect(result1).toEqual('30 30 12 15 */3 *'); }); }); From a32519ed34ad385449287f3286563f566c4bf9e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Mon, 15 Jul 2024 11:54:08 +0200 Subject: [PATCH 04/17] some unit tests for recurrenceCheck --- .../nodes/Schedule/GenericFunctions.ts | 11 +++++---- .../nodes/Schedule/ScheduleTrigger.node.ts | 11 ++++----- .../Schedule/test/GenericFunctions.test.ts | 23 ++++++++++++++++++- 3 files changed, 33 insertions(+), 12 deletions(-) diff --git a/packages/nodes-base/nodes/Schedule/GenericFunctions.ts b/packages/nodes-base/nodes/Schedule/GenericFunctions.ts index ea31feb69ded1..1e7ae2dc5798a 100644 --- a/packages/nodes-base/nodes/Schedule/GenericFunctions.ts +++ b/packages/nodes-base/nodes/Schedule/GenericFunctions.ts @@ -3,19 +3,22 @@ import { type CronExpression, randomInt } from 'n8n-workflow'; import type { IRecurrenceRule, ScheduleInterval } from './SchedulerInterface'; export function recurrenceCheck( - recurrence: IRecurrenceRule & { activated: true }, + recurrence: IRecurrenceRule, recurrenceRules: number[], timezone: string, ): boolean { - const momentTz = moment.tz(timezone); - const recurrenceRuleIndex = recurrence.index; + if (!recurrence.activated) return true; + const intervalSize = recurrence.intervalSize; - const typeInterval = recurrence.typeInterval; if (!intervalSize) return false; + const recurrenceRuleIndex = recurrence.index; + const typeInterval = recurrence.typeInterval; + const lastExecution = recurrenceRuleIndex !== undefined ? recurrenceRules[recurrenceRuleIndex] : undefined; + const momentTz = moment.tz(timezone); if (recurrenceRuleIndex !== undefined && typeInterval === 'weeks') { const week = momentTz.week(); if ( diff --git a/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts b/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts index c8eaa1092b21b..6d6f02f0b5d32 100644 --- a/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts +++ b/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts @@ -420,6 +420,9 @@ export class ScheduleTrigger implements INodeType { } const executeTrigger = async (recurrence: IRecurrenceRule) => { + const shouldTrigger = recurrenceCheck(recurrence, staticData.recurrenceRules, timezone); + if (!shouldTrigger) return; + const momentTz = moment.tz(timezone); const resultData = { timestamp: momentTz.toISOString(true), @@ -435,13 +438,7 @@ export class ScheduleTrigger implements INodeType { Timezone: momentTz.format('z Z'), }; - if (!recurrence.activated) { - this.emit([this.helpers.returnJsonArray([resultData])]); - } else { - if (recurrenceCheck(recurrence, staticData.recurrenceRules, timezone)) { - this.emit([this.helpers.returnJsonArray([resultData])]); - } - } + this.emit([this.helpers.returnJsonArray([resultData])]); }; for (let i = 0; i < intervals.length; i++) { diff --git a/packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts b/packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts index 5ca8b60289c18..173f6d4654093 100644 --- a/packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts +++ b/packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts @@ -1,4 +1,4 @@ -import { toCronExpression } from '../GenericFunctions'; +import { recurrenceCheck, toCronExpression } from '../GenericFunctions'; import * as n8nWorkflow from 'n8n-workflow'; describe('toCronExpression', () => { @@ -94,3 +94,24 @@ describe('toCronExpression', () => { expect(result1).toEqual('30 30 12 15 */3 *'); }); }); + +describe.only('recurrenceCheck', () => { + it('should return true if activated=false', () => { + const result = recurrenceCheck({ activated: false }, [], 'UTC'); + expect(result).toBe(true); + }); + + it('should return false if intervalSize is falsey', () => { + const result = recurrenceCheck( + { + activated: true, + index: 0, + intervalSize: 0, + typeInterval: 'days', + }, + [], + 'UTC', + ); + expect(result).toBe(false); + }); +}); From 5ae64f8cf236d3dd3d2f2c9aca621e252d06f0df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Mon, 15 Jul 2024 11:55:31 +0200 Subject: [PATCH 05/17] Do not setup actual crons in manual executions --- .../nodes/Schedule/ScheduleTrigger.node.ts | 108 +++++++++--------- 1 file changed, 55 insertions(+), 53 deletions(-) diff --git a/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts b/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts index 6d6f02f0b5d32..2d364eba1dc19 100644 --- a/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts +++ b/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts @@ -441,68 +441,70 @@ export class ScheduleTrigger implements INodeType { this.emit([this.helpers.returnJsonArray([resultData])]); }; - for (let i = 0; i < intervals.length; i++) { - const interval = intervals[i]; - const cronExpression = toCronExpression(interval); - let recurrence: IRecurrenceRule = { activated: false }; + if (this.getMode() !== 'manual') { + for (let i = 0; i < intervals.length; i++) { + const interval = intervals[i]; + const cronExpression = toCronExpression(interval); + let recurrence: IRecurrenceRule = { activated: false }; - if (interval.field === 'hours') { - const hour = interval.hoursInterval; - if (hour !== 1) { - recurrence = { - activated: true, - index: i, - intervalSize: hour, - typeInterval: 'hours', - }; + if (interval.field === 'hours') { + const hour = interval.hoursInterval; + if (hour !== 1) { + recurrence = { + activated: true, + index: i, + intervalSize: hour, + typeInterval: 'hours', + }; + } } - } - if (interval.field === 'days') { - const day = interval.daysInterval; - if (day !== 1) { - recurrence = { - activated: true, - index: i, - intervalSize: day, - typeInterval: 'days', - }; + if (interval.field === 'days') { + const day = interval.daysInterval; + if (day !== 1) { + recurrence = { + activated: true, + index: i, + intervalSize: day, + typeInterval: 'days', + }; + } } - } - if (interval.field === 'weeks') { - const week = interval.weeksInterval; - if (week !== 1) { - recurrence = { - activated: true, - index: i, - intervalSize: week, - typeInterval: 'weeks', - }; + if (interval.field === 'weeks') { + const week = interval.weeksInterval; + if (week !== 1) { + recurrence = { + activated: true, + index: i, + intervalSize: week, + typeInterval: 'weeks', + }; + } } - } - if (interval.field === 'months') { - const month = interval.monthsInterval; - if (month !== 1) { - recurrence = { - activated: true, - index: i, - intervalSize: month, - typeInterval: 'months', - }; + if (interval.field === 'months') { + const month = interval.monthsInterval; + if (month !== 1) { + recurrence = { + activated: true, + index: i, + intervalSize: month, + typeInterval: 'months', + }; + } } - } - try { - this.helpers.registerCron(cronExpression, async () => await executeTrigger(recurrence)); - } catch (error) { - if (interval.field === 'cronExpression') { - throw new NodeOperationError(this.getNode(), 'Invalid cron expression', { - description: 'More information on how to build them at https://crontab.guru/', - }); - } else { - throw error; + try { + this.helpers.registerCron(cronExpression, async () => await executeTrigger(recurrence)); + } catch (error) { + if (interval.field === 'cronExpression') { + throw new NodeOperationError(this.getNode(), 'Invalid cron expression', { + description: 'More information on how to build them at https://crontab.guru/', + }); + } else { + throw error; + } } } } From b378bffec52a75c9e109737a004485ad4b747f8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Mon, 15 Jul 2024 12:20:27 +0200 Subject: [PATCH 06/17] more tests for recurrenceCheck --- .../nodes/Schedule/GenericFunctions.ts | 22 ++++++++--------- .../nodes/Schedule/ScheduleTrigger.node.ts | 24 +++++++++---------- .../Schedule/test/GenericFunctions.test.ts | 17 ++++++++++++- 3 files changed, 38 insertions(+), 25 deletions(-) diff --git a/packages/nodes-base/nodes/Schedule/GenericFunctions.ts b/packages/nodes-base/nodes/Schedule/GenericFunctions.ts index 1e7ae2dc5798a..ea49870770454 100644 --- a/packages/nodes-base/nodes/Schedule/GenericFunctions.ts +++ b/packages/nodes-base/nodes/Schedule/GenericFunctions.ts @@ -12,39 +12,37 @@ export function recurrenceCheck( const intervalSize = recurrence.intervalSize; if (!intervalSize) return false; - const recurrenceRuleIndex = recurrence.index; + const index = recurrence.index; const typeInterval = recurrence.typeInterval; - - const lastExecution = - recurrenceRuleIndex !== undefined ? recurrenceRules[recurrenceRuleIndex] : undefined; + const lastExecution = recurrenceRules[index]; const momentTz = moment.tz(timezone); - if (recurrenceRuleIndex !== undefined && typeInterval === 'weeks') { + if (typeInterval === 'weeks') { const week = momentTz.week(); if ( lastExecution === undefined || // First time executing this rule week === (intervalSize + lastExecution) % 52 || // not first time, but minimum interval has passed week === lastExecution // Trigger on multiple days in the same week ) { - recurrenceRules[recurrenceRuleIndex] = week; + recurrenceRules[index] = week; return true; } - } else if (recurrenceRuleIndex !== undefined && typeInterval === 'days') { + } else if (typeInterval === 'days') { const dayOfYear = momentTz.dayOfYear(); if (lastExecution === undefined || dayOfYear === (intervalSize + lastExecution) % 365) { - recurrenceRules[recurrenceRuleIndex] = dayOfYear; + recurrenceRules[index] = dayOfYear; return true; } - } else if (recurrenceRuleIndex !== undefined && typeInterval === 'hours') { + } else if (typeInterval === 'hours') { const hour = momentTz.hour(); if (lastExecution === undefined || hour === (intervalSize + lastExecution) % 24) { - recurrenceRules[recurrenceRuleIndex] = hour; + recurrenceRules[index] = hour; return true; } - } else if (recurrenceRuleIndex !== undefined && typeInterval === 'months') { + } else if (typeInterval === 'months') { const month = momentTz.month(); if (lastExecution === undefined || month === (intervalSize + lastExecution) % 12) { - recurrenceRules[recurrenceRuleIndex] = month; + recurrenceRules[index] = month; return true; } } else { diff --git a/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts b/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts index 2d364eba1dc19..81c3bf56ed0e8 100644 --- a/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts +++ b/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts @@ -448,48 +448,48 @@ export class ScheduleTrigger implements INodeType { let recurrence: IRecurrenceRule = { activated: false }; if (interval.field === 'hours') { - const hour = interval.hoursInterval; - if (hour !== 1) { + const { hoursInterval } = interval; + if (hoursInterval !== 1) { recurrence = { activated: true, index: i, - intervalSize: hour, + intervalSize: hoursInterval, typeInterval: 'hours', }; } } if (interval.field === 'days') { - const day = interval.daysInterval; - if (day !== 1) { + const { daysInterval } = interval; + if (daysInterval !== 1) { recurrence = { activated: true, index: i, - intervalSize: day, + intervalSize: daysInterval, typeInterval: 'days', }; } } if (interval.field === 'weeks') { - const week = interval.weeksInterval; - if (week !== 1) { + const { weeksInterval } = interval; + if (weeksInterval !== 1) { recurrence = { activated: true, index: i, - intervalSize: week, + intervalSize: weeksInterval, typeInterval: 'weeks', }; } } if (interval.field === 'months') { - const month = interval.monthsInterval; - if (month !== 1) { + const { monthsInterval } = interval; + if (monthsInterval !== 1) { recurrence = { activated: true, index: i, - intervalSize: month, + intervalSize: monthsInterval, typeInterval: 'months', }; } diff --git a/packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts b/packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts index 173f6d4654093..f1e1fdb6d6e99 100644 --- a/packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts +++ b/packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts @@ -1,5 +1,6 @@ -import { recurrenceCheck, toCronExpression } from '../GenericFunctions'; import * as n8nWorkflow from 'n8n-workflow'; +import { recurrenceCheck, toCronExpression } from '../GenericFunctions'; +import type { IRecurrenceRule } from '../SchedulerInterface'; describe('toCronExpression', () => { Object.defineProperty(n8nWorkflow, 'randomInt', { @@ -114,4 +115,18 @@ describe.only('recurrenceCheck', () => { ); expect(result).toBe(false); }); + + it('should return true only once for a day cron', () => { + const recurrence: IRecurrenceRule = { + activated: true, + index: 0, + intervalSize: 2, + typeInterval: 'days', + }; + const recurrenceRules: number[] = []; + const result1 = recurrenceCheck(recurrence, recurrenceRules, 'UTC'); + expect(result1).toBe(true); + const result2 = recurrenceCheck(recurrence, recurrenceRules, 'UTC'); + expect(result2).toBe(false); + }); }); From aebdfec9c235e3a9825c455ee8ed58b89c985e6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Mon, 15 Jul 2024 12:26:27 +0200 Subject: [PATCH 07/17] make displayed timezone less confusing --- packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts b/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts index 81c3bf56ed0e8..0b95aff0f302b 100644 --- a/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts +++ b/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts @@ -435,7 +435,7 @@ export class ScheduleTrigger implements INodeType { Hour: momentTz.format('HH'), Minute: momentTz.format('mm'), Second: momentTz.format('ss'), - Timezone: momentTz.format('z Z'), + Timezone: momentTz.format('z (UTC Z)'), }; this.emit([this.helpers.returnJsonArray([resultData])]); From 574dff86efddbf699b55e7d90c30a59ef9ad3bb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Mon, 15 Jul 2024 12:36:11 +0200 Subject: [PATCH 08/17] improve the rendered timezone even more --- packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts b/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts index 0b95aff0f302b..40e7591d003f6 100644 --- a/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts +++ b/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts @@ -435,7 +435,7 @@ export class ScheduleTrigger implements INodeType { Hour: momentTz.format('HH'), Minute: momentTz.format('mm'), Second: momentTz.format('ss'), - Timezone: momentTz.format('z (UTC Z)'), + Timezone: `${timezone} (UTC${momentTz.format('Z')})`, }; this.emit([this.helpers.returnJsonArray([resultData])]); From f3864c4d382a77302a946f64de7c484cb74340b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Mon, 15 Jul 2024 13:11:11 +0200 Subject: [PATCH 09/17] extract intervalToRecurrence, add unit tests, and use the first rule in manual testing --- .../nodes/Schedule/GenericFunctions.ts | 80 +++++++++-- .../nodes/Schedule/ScheduleTrigger.node.ts | 69 ++-------- .../Schedule/test/GenericFunctions.test.ts | 129 +++++++++++++++++- 3 files changed, 205 insertions(+), 73 deletions(-) diff --git a/packages/nodes-base/nodes/Schedule/GenericFunctions.ts b/packages/nodes-base/nodes/Schedule/GenericFunctions.ts index ea49870770454..65503cfb5bfd6 100644 --- a/packages/nodes-base/nodes/Schedule/GenericFunctions.ts +++ b/packages/nodes-base/nodes/Schedule/GenericFunctions.ts @@ -17,14 +17,10 @@ export function recurrenceCheck( const lastExecution = recurrenceRules[index]; const momentTz = moment.tz(timezone); - if (typeInterval === 'weeks') { - const week = momentTz.week(); - if ( - lastExecution === undefined || // First time executing this rule - week === (intervalSize + lastExecution) % 52 || // not first time, but minimum interval has passed - week === lastExecution // Trigger on multiple days in the same week - ) { - recurrenceRules[index] = week; + if (typeInterval === 'hours') { + const hour = momentTz.hour(); + if (lastExecution === undefined || hour === (intervalSize + lastExecution) % 24) { + recurrenceRules[index] = hour; return true; } } else if (typeInterval === 'days') { @@ -33,10 +29,14 @@ export function recurrenceCheck( recurrenceRules[index] = dayOfYear; return true; } - } else if (typeInterval === 'hours') { - const hour = momentTz.hour(); - if (lastExecution === undefined || hour === (intervalSize + lastExecution) % 24) { - recurrenceRules[index] = hour; + } else if (typeInterval === 'weeks') { + const week = momentTz.week(); + if ( + lastExecution === undefined || // First time executing this rule + week === (intervalSize + lastExecution) % 52 || // not first time, but minimum interval has passed + week === lastExecution // Trigger on multiple days in the same week + ) { + recurrenceRules[index] = week; return true; } } else if (typeInterval === 'months') { @@ -45,8 +45,6 @@ export function recurrenceCheck( recurrenceRules[index] = month; return true; } - } else { - return true; } return false; } @@ -74,3 +72,57 @@ export const toCronExpression = (interval: ScheduleInterval): CronExpression => const dayOfMonth = interval.triggerAtDayOfMonth ?? randomInt(0, 31); return `${randomSecond} ${minute} ${hour} ${dayOfMonth} */${interval.monthsInterval} *`; }; + +export function intervalToRecurrence(interval: ScheduleInterval, index: number) { + let recurrence: IRecurrenceRule = { activated: false }; + + if (interval.field === 'hours') { + const { hoursInterval } = interval; + if (hoursInterval !== 1) { + recurrence = { + activated: true, + index, + intervalSize: hoursInterval, + typeInterval: 'hours', + }; + } + } + + if (interval.field === 'days') { + const { daysInterval } = interval; + if (daysInterval !== 1) { + recurrence = { + activated: true, + index, + intervalSize: daysInterval, + typeInterval: 'days', + }; + } + } + + if (interval.field === 'weeks') { + const { weeksInterval } = interval; + if (weeksInterval !== 1) { + recurrence = { + activated: true, + index, + intervalSize: weeksInterval, + typeInterval: 'weeks', + }; + } + } + + if (interval.field === 'months') { + const { monthsInterval } = interval; + if (monthsInterval !== 1) { + recurrence = { + activated: true, + index, + intervalSize: monthsInterval, + typeInterval: 'months', + }; + } + } + + return recurrence; +} diff --git a/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts b/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts index 40e7591d003f6..cf710ab092e6b 100644 --- a/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts +++ b/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts @@ -3,12 +3,13 @@ import type { INodeType, INodeTypeDescription, ITriggerResponse, + CronExpression, } from 'n8n-workflow'; import { NodeOperationError } from 'n8n-workflow'; import moment from 'moment-timezone'; import type { IRecurrenceRule, Rule } from './SchedulerInterface'; -import { recurrenceCheck, toCronExpression } from './GenericFunctions'; +import { intervalToRecurrence, recurrenceCheck, toCronExpression } from './GenericFunctions'; export class ScheduleTrigger implements INodeType { description: INodeTypeDescription = { @@ -419,7 +420,7 @@ export class ScheduleTrigger implements INodeType { staticData.recurrenceRules = []; } - const executeTrigger = async (recurrence: IRecurrenceRule) => { + const executeTrigger = (recurrence: IRecurrenceRule) => { const shouldTrigger = recurrenceCheck(recurrence, staticData.recurrenceRules, timezone); if (!shouldTrigger) return; @@ -441,62 +442,16 @@ export class ScheduleTrigger implements INodeType { this.emit([this.helpers.returnJsonArray([resultData])]); }; - if (this.getMode() !== 'manual') { - for (let i = 0; i < intervals.length; i++) { - const interval = intervals[i]; - const cronExpression = toCronExpression(interval); - let recurrence: IRecurrenceRule = { activated: false }; - - if (interval.field === 'hours') { - const { hoursInterval } = interval; - if (hoursInterval !== 1) { - recurrence = { - activated: true, - index: i, - intervalSize: hoursInterval, - typeInterval: 'hours', - }; - } - } - - if (interval.field === 'days') { - const { daysInterval } = interval; - if (daysInterval !== 1) { - recurrence = { - activated: true, - index: i, - intervalSize: daysInterval, - typeInterval: 'days', - }; - } - } - - if (interval.field === 'weeks') { - const { weeksInterval } = interval; - if (weeksInterval !== 1) { - recurrence = { - activated: true, - index: i, - intervalSize: weeksInterval, - typeInterval: 'weeks', - }; - } - } - - if (interval.field === 'months') { - const { monthsInterval } = interval; - if (monthsInterval !== 1) { - recurrence = { - activated: true, - index: i, - intervalSize: monthsInterval, - typeInterval: 'months', - }; - } - } + const rules = intervals.map((interval, i) => ({ + interval, + cronExpression: toCronExpression(interval), + recurrence: intervalToRecurrence(interval, i), + })); + if (this.getMode() !== 'manual') { + for (const { interval, cronExpression, recurrence } of rules) { try { - this.helpers.registerCron(cronExpression, async () => await executeTrigger(recurrence)); + this.helpers.registerCron(cronExpression, () => executeTrigger(recurrence)); } catch (error) { if (interval.field === 'cronExpression') { throw new NodeOperationError(this.getNode(), 'Invalid cron expression', { @@ -510,7 +465,7 @@ export class ScheduleTrigger implements INodeType { } async function manualTriggerFunction() { - void executeTrigger({ activated: false }); + executeTrigger(rules[0].recurrence); } return { diff --git a/packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts b/packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts index f1e1fdb6d6e99..d6c124cc4c269 100644 --- a/packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts +++ b/packages/nodes-base/nodes/Schedule/test/GenericFunctions.test.ts @@ -1,5 +1,5 @@ import * as n8nWorkflow from 'n8n-workflow'; -import { recurrenceCheck, toCronExpression } from '../GenericFunctions'; +import { intervalToRecurrence, recurrenceCheck, toCronExpression } from '../GenericFunctions'; import type { IRecurrenceRule } from '../SchedulerInterface'; describe('toCronExpression', () => { @@ -96,7 +96,7 @@ describe('toCronExpression', () => { }); }); -describe.only('recurrenceCheck', () => { +describe('recurrenceCheck', () => { it('should return true if activated=false', () => { const result = recurrenceCheck({ activated: false }, [], 'UTC'); expect(result).toBe(true); @@ -130,3 +130,128 @@ describe.only('recurrenceCheck', () => { expect(result2).toBe(false); }); }); + +describe('intervalToRecurrence', () => { + it('should return recurrence rule for seconds interval', () => { + const result = intervalToRecurrence( + { + field: 'seconds', + secondsInterval: 10, + }, + 0, + ); + expect(result.activated).toBe(false); + }); + + it('should return recurrence rule for minutes interval', () => { + const result = intervalToRecurrence( + { + field: 'minutes', + minutesInterval: 30, + }, + 1, + ); + expect(result.activated).toBe(false); + }); + + it('should return recurrence rule for hours interval', () => { + const result = intervalToRecurrence( + { + field: 'hours', + hoursInterval: 3, + triggerAtMinute: 22, + }, + 2, + ); + expect(result).toEqual({ + activated: true, + index: 2, + intervalSize: 3, + typeInterval: 'hours', + }); + + const result1 = intervalToRecurrence( + { + field: 'hours', + hoursInterval: 3, + }, + 3, + ); + expect(result1).toEqual({ + activated: true, + index: 3, + intervalSize: 3, + typeInterval: 'hours', + }); + }); + + it('should return recurrence rule for days interval', () => { + const result = intervalToRecurrence( + { + field: 'days', + daysInterval: 4, + triggerAtMinute: 30, + triggerAtHour: 10, + }, + 4, + ); + expect(result).toEqual({ + activated: true, + index: 4, + intervalSize: 4, + typeInterval: 'days', + }); + + const result1 = intervalToRecurrence( + { + field: 'days', + daysInterval: 4, + }, + 5, + ); + expect(result1).toEqual({ + activated: true, + index: 5, + intervalSize: 4, + typeInterval: 'days', + }); + }); + + it('should return recurrence rule for weeks interval', () => { + const result = intervalToRecurrence( + { + field: 'weeks', + weeksInterval: 2, + triggerAtMinute: 0, + triggerAtHour: 9, + triggerAtDay: [1, 3, 5], + }, + 6, + ); + expect(result).toEqual({ + activated: true, + index: 6, + intervalSize: 2, + typeInterval: 'weeks', + }); + }); + + it('should return recurrence rule for months interval', () => { + const result = intervalToRecurrence( + { + field: 'months', + monthsInterval: 3, + triggerAtMinute: 0, + triggerAtHour: 0, + triggerAtDayOfMonth: 1, + }, + 8, + ); + expect(result).toEqual({ + activated: true, + index: 8, + intervalSize: 3, + typeInterval: 'months', + }); + }); +}); From 23d3128d2d22c4007e9256afc9de4f754ba4ff1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Mon, 15 Jul 2024 13:30:41 +0200 Subject: [PATCH 10/17] validate user defined cron expressions in manual executions as well --- .../nodes/Schedule/ScheduleTrigger.node.ts | 22 ++++++++++++++----- packages/nodes-base/package.json | 1 + pnpm-lock.yaml | 19 +++++++++------- 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts b/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts index cf710ab092e6b..05b26e39ea53d 100644 --- a/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts +++ b/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts @@ -3,11 +3,11 @@ import type { INodeType, INodeTypeDescription, ITriggerResponse, - CronExpression, } from 'n8n-workflow'; import { NodeOperationError } from 'n8n-workflow'; - import moment from 'moment-timezone'; +import { sendAt } from 'cron'; + import type { IRecurrenceRule, Rule } from './SchedulerInterface'; import { intervalToRecurrence, recurrenceCheck, toCronExpression } from './GenericFunctions'; @@ -401,7 +401,7 @@ export class ScheduleTrigger implements INodeType { field: ['cronExpression'], }, }, - hint: 'Format: [Minute] [Hour] [Day of Month] [Month] [Day of Week]', + hint: 'Format: [Second] [Minute] [Hour] [Day of Month] [Month] [Day of Week]', }, ], }, @@ -464,9 +464,19 @@ export class ScheduleTrigger implements INodeType { } } - async function manualTriggerFunction() { - executeTrigger(rules[0].recurrence); - } + const manualTriggerFunction = async () => { + const { interval, cronExpression, recurrence } = rules[0]; + if (interval.field === 'cronExpression') { + try { + sendAt(cronExpression); + } catch (error) { + throw new NodeOperationError(this.getNode(), 'Invalid cron expression', { + description: 'More information on how to build them at https://crontab.guru/', + }); + } + } + executeTrigger(recurrence); + }; return { manualTriggerFunction, diff --git a/packages/nodes-base/package.json b/packages/nodes-base/package.json index aa1e8b41ba50e..cba80c5f312cf 100644 --- a/packages/nodes-base/package.json +++ b/packages/nodes-base/package.json @@ -837,6 +837,7 @@ "change-case": "4.1.2", "cheerio": "1.0.0-rc.6", "chokidar": "3.5.2", + "cron": "3.1.7", "csv-parse": "5.5.0", "currency-codes": "2.1.0", "eventsource": "2.0.2", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index fb2a00bacf270..1fe8fce642a38 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1353,6 +1353,9 @@ importers: chokidar: specifier: 3.5.2 version: 3.5.2 + cron: + specifier: 3.1.7 + version: 3.1.7 csv-parse: specifier: 5.5.0 version: 5.5.0 @@ -21206,7 +21209,7 @@ snapshots: eslint-import-resolver-node@0.3.9: dependencies: - debug: 3.2.7(supports-color@8.1.1) + debug: 3.2.7(supports-color@5.5.0) is-core-module: 2.13.1 resolve: 1.22.8 transitivePeerDependencies: @@ -21231,7 +21234,7 @@ snapshots: eslint-module-utils@2.8.0(@typescript-eslint/parser@7.2.0(eslint@8.57.0)(typescript@5.5.2))(eslint-import-resolver-node@0.3.9)(eslint-import-resolver-typescript@3.6.1(@typescript-eslint/parser@7.2.0(eslint@8.57.0)(typescript@5.5.2))(eslint-plugin-import@2.29.1)(eslint@8.57.0))(eslint@8.57.0): dependencies: - debug: 3.2.7(supports-color@8.1.1) + debug: 3.2.7(supports-color@5.5.0) optionalDependencies: '@typescript-eslint/parser': 7.2.0(eslint@8.57.0)(typescript@5.5.2) eslint: 8.57.0 @@ -21251,7 +21254,7 @@ snapshots: array.prototype.findlastindex: 1.2.3 array.prototype.flat: 1.3.2 array.prototype.flatmap: 1.3.2 - debug: 3.2.7(supports-color@8.1.1) + debug: 3.2.7(supports-color@5.5.0) doctrine: 2.1.0 eslint: 8.57.0 eslint-import-resolver-node: 0.3.9 @@ -21781,7 +21784,7 @@ snapshots: follow-redirects@1.15.6(debug@3.2.7): optionalDependencies: - debug: 3.2.7(supports-color@8.1.1) + debug: 3.2.7(supports-color@5.5.0) follow-redirects@1.15.6(debug@4.3.4): optionalDependencies: @@ -22122,7 +22125,7 @@ snapshots: array-parallel: 0.1.3 array-series: 0.1.5 cross-spawn: 4.0.2 - debug: 3.2.7(supports-color@8.1.1) + debug: 3.2.7(supports-color@5.5.0) transitivePeerDependencies: - supports-color @@ -24887,7 +24890,7 @@ snapshots: pdf-parse@1.1.1: dependencies: - debug: 3.2.7(supports-color@8.1.1) + debug: 3.2.7(supports-color@5.5.0) node-ensure: 0.0.0 transitivePeerDependencies: - supports-color @@ -25769,7 +25772,7 @@ snapshots: rhea@1.0.24: dependencies: - debug: 3.2.7(supports-color@8.1.1) + debug: 3.2.7(supports-color@5.5.0) transitivePeerDependencies: - supports-color @@ -26143,7 +26146,7 @@ snapshots: binascii: 0.0.2 bn.js: 5.2.1 browser-request: 0.3.3 - debug: 3.2.7(supports-color@8.1.1) + debug: 3.2.7(supports-color@5.5.0) expand-tilde: 2.0.2 extend: 3.0.2 fast-xml-parser: 4.2.7 From 12dd52f467ed50e6bc8592516b787374984cdf53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Tue, 16 Jul 2024 11:43:13 +0200 Subject: [PATCH 11/17] workflow timezone doesn't change. --- packages/workflow/src/Workflow.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/workflow/src/Workflow.ts b/packages/workflow/src/Workflow.ts index a836e99d19161..3c74480b3864f 100644 --- a/packages/workflow/src/Workflow.ts +++ b/packages/workflow/src/Workflow.ts @@ -95,6 +95,8 @@ export class Workflow { settings: IWorkflowSettings; + readonly timezone: string; + // To save workflow specific static data like for example // ids of registered webhooks of nodes staticData: IDataObject; @@ -152,14 +154,11 @@ export class Workflow { }); this.settings = parameters.settings || {}; + this.timezone = this.settings.timezone ?? getGlobalState().defaultTimezone; this.expression = new Expression(this); } - get timezone() { - return this.settings.timezone ?? getGlobalState().defaultTimezone; - } - /** * The default connections are by source node. This function rewrites them by destination nodes * to easily find parent nodes. From d6a6a2d25b88601e8a6a131c6be2748bfb08ab95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Tue, 16 Jul 2024 11:55:33 +0200 Subject: [PATCH 12/17] improve CronExpression type --- .../nodes/Schedule/GenericFunctions.ts | 2 +- packages/workflow/src/Cron.ts | 18 +++++++++--------- packages/workflow/src/Interfaces.ts | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/nodes-base/nodes/Schedule/GenericFunctions.ts b/packages/nodes-base/nodes/Schedule/GenericFunctions.ts index 65503cfb5bfd6..42d31bd582652 100644 --- a/packages/nodes-base/nodes/Schedule/GenericFunctions.ts +++ b/packages/nodes-base/nodes/Schedule/GenericFunctions.ts @@ -66,7 +66,7 @@ export const toCronExpression = (interval: ScheduleInterval): CronExpression => if (interval.field === 'weeks') { const days = interval.triggerAtDay; const daysOfWeek = days.length === 0 ? '*' : days.join(','); - return `${randomSecond} ${minute} ${hour} * * ${daysOfWeek}`; + return `${randomSecond} ${minute} ${hour} * * ${daysOfWeek}` as CronExpression; } const dayOfMonth = interval.triggerAtDayOfMonth ?? randomInt(0, 31); diff --git a/packages/workflow/src/Cron.ts b/packages/workflow/src/Cron.ts index e6022e8887782..4039e02aa973d 100644 --- a/packages/workflow/src/Cron.ts +++ b/packages/workflow/src/Cron.ts @@ -49,22 +49,22 @@ export type TriggerTime = | EveryWeek | EveryMonth; -const randomSecond = () => randomInt(60).toString(); - export const toCronExpression = (item: TriggerTime): CronExpression => { - if (item.mode === 'everyMinute') return `${randomSecond()} * * * * *`; - if (item.mode === 'everyHour') return `${randomSecond()} ${item.minute} * * * *`; + const randomSecond = randomInt(60); + + if (item.mode === 'everyMinute') return `${randomSecond} * * * * *`; + if (item.mode === 'everyHour') return `${randomSecond} ${item.minute} * * * *`; if (item.mode === 'everyX') { - if (item.unit === 'minutes') return `${randomSecond()} */${item.value} * * * *`; - if (item.unit === 'hours') return `${randomSecond()} 0 */${item.value} * * *`; + if (item.unit === 'minutes') return `${randomSecond} */${item.value} * * * *`; + if (item.unit === 'hours') return `${randomSecond} 0 */${item.value} * * *`; } - if (item.mode === 'everyDay') return `${randomSecond()} ${item.minute} ${item.hour} * * *`; + if (item.mode === 'everyDay') return `${randomSecond} ${item.minute} ${item.hour} * * *`; if (item.mode === 'everyWeek') - return `${randomSecond()} ${item.minute} ${item.hour} * * ${item.weekday}`; + return `${randomSecond} ${item.minute} ${item.hour} * * ${item.weekday}`; if (item.mode === 'everyMonth') - return `${randomSecond()} ${item.minute} ${item.hour} ${item.dayOfMonth} * *`; + return `${randomSecond} ${item.minute} ${item.hour} ${item.dayOfMonth} * *`; return item.cronExpression.trim() as CronExpression; }; diff --git a/packages/workflow/src/Interfaces.ts b/packages/workflow/src/Interfaces.ts index a1a9860198ec3..e9d6141c5fa0f 100644 --- a/packages/workflow/src/Interfaces.ts +++ b/packages/workflow/src/Interfaces.ts @@ -840,7 +840,7 @@ export interface SSHTunnelFunctions { getSSHClient(credentials: SSHCredentials): Promise; } -type CronUnit = number | '' | '*' | `*/${number}` | string; +type CronUnit = number | '*' | `*/${number}`; export type CronExpression = `${CronUnit} ${CronUnit} ${CronUnit} ${CronUnit} ${CronUnit} ${CronUnit}`; From a600fcdfd3f071ea52557d251768890fef6c9f74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Tue, 16 Jul 2024 11:58:27 +0200 Subject: [PATCH 13/17] scatter triggers on the old cron node as well, to prevent all "every hour" crons from all triggering at the same minute --- packages/workflow/src/Cron.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/workflow/src/Cron.ts b/packages/workflow/src/Cron.ts index 4039e02aa973d..ad83eec682e09 100644 --- a/packages/workflow/src/Cron.ts +++ b/packages/workflow/src/Cron.ts @@ -57,7 +57,9 @@ export const toCronExpression = (item: TriggerTime): CronExpression => { if (item.mode === 'everyX') { if (item.unit === 'minutes') return `${randomSecond} */${item.value} * * * *`; - if (item.unit === 'hours') return `${randomSecond} 0 */${item.value} * * *`; + + const randomMinute = randomInt(60); + if (item.unit === 'hours') return `${randomSecond} ${randomMinute} */${item.value} * * *`; } if (item.mode === 'everyDay') return `${randomSecond} ${item.minute} ${item.hour} * * *`; if (item.mode === 'everyWeek') From d49e6c4406018463281f57c7788a863da3903118 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Tue, 16 Jul 2024 12:00:51 +0200 Subject: [PATCH 14/17] address PR feedback --- packages/core/src/ScheduledTaskManager.ts | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/packages/core/src/ScheduledTaskManager.ts b/packages/core/src/ScheduledTaskManager.ts index 68f105f5ddcf2..ce656f3716303 100644 --- a/packages/core/src/ScheduledTaskManager.ts +++ b/packages/core/src/ScheduledTaskManager.ts @@ -7,15 +7,13 @@ export class ScheduledTaskManager { readonly cronJobs = new Map(); registerCron(workflow: Workflow, cronExpression: CronExpression, onTick: () => void) { - const cronJob = new CronJob( - cronExpression as string, - onTick, - undefined, - true, - workflow.timezone, - ); - if (!this.cronJobs.has(workflow.id)) this.cronJobs.set(workflow.id, []); - this.cronJobs.get(workflow.id)!.push(cronJob); + const cronJob = new CronJob(cronExpression, onTick, undefined, true, workflow.timezone); + const cronJobsForWorkflow = this.cronJobs.get(workflow.id); + if (cronJobsForWorkflow) { + cronJobsForWorkflow.push(cronJob); + } else { + this.cronJobs.set(workflow.id, [cronJob]); + } } deregisterCrons(workflowId: string) { From a224cf7ef6136fc6f874560cbfab1ea4785ee729 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Tue, 16 Jul 2024 12:11:24 +0200 Subject: [PATCH 15/17] fix old Cron tests --- packages/workflow/test/Cron.test.ts | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/packages/workflow/test/Cron.test.ts b/packages/workflow/test/Cron.test.ts index d1e48f9f85e4e..0b8c45fd330a9 100644 --- a/packages/workflow/test/Cron.test.ts +++ b/packages/workflow/test/Cron.test.ts @@ -1,4 +1,5 @@ import { toCronExpression } from '@/Cron'; +import type { CronExpression } from '@/Interfaces'; describe('Cron', () => { describe('toCronExpression', () => { @@ -6,7 +7,7 @@ describe('Cron', () => { const expression = toCronExpression({ mode: 'everyMinute', }); - expect(expression).toMatch(/^[1-6]?[0-9] \* \* \* \* \*$/); + expect(expression).toMatch(/^[1-5]?[0-9] \* \* \* \* \*$/); }); test('should generate a valid cron for `everyHour` triggers', () => { @@ -14,7 +15,7 @@ describe('Cron', () => { mode: 'everyHour', minute: 11, }); - expect(expression).toMatch(/^[1-6]?[0-9] 11 \* \* \* \*$/); + expect(expression).toMatch(/^[1-5]?[0-9] 11 \* \* \* \*$/); }); test('should generate a valid cron for `everyX[minutes]` triggers', () => { @@ -23,7 +24,7 @@ describe('Cron', () => { unit: 'minutes', value: 42, }); - expect(expression).toMatch(/^[1-6]?[0-9] \*\/42 \* \* \* \*$/); + expect(expression).toMatch(/^[1-5]?[0-9] \*\/42 \* \* \* \*$/); }); test('should generate a valid cron for `everyX[hours]` triggers', () => { @@ -32,7 +33,7 @@ describe('Cron', () => { unit: 'hours', value: 3, }); - expect(expression).toMatch(/^[1-6]?[0-9] 0 \*\/3 \* \* \*$/); + expect(expression).toMatch(/^[1-5]?[0-9] [1-5]?[0-9] \*\/3 \* \* \*$/); }); test('should generate a valid cron for `everyDay` triggers', () => { @@ -41,7 +42,7 @@ describe('Cron', () => { hour: 13, minute: 17, }); - expect(expression).toMatch(/^[1-6]?[0-9] 17 13 \* \* \*$/); + expect(expression).toMatch(/^[1-5]?[0-9] 17 13 \* \* \*$/); }); test('should generate a valid cron for `everyWeek` triggers', () => { @@ -51,7 +52,7 @@ describe('Cron', () => { minute: 17, weekday: 4, }); - expect(expression).toMatch(/^[1-6]?[0-9] 17 13 \* \* 4$/); + expect(expression).toMatch(/^[1-5]?[0-9] 17 13 \* \* 4$/); }); test('should generate a valid cron for `everyMonth` triggers', () => { @@ -61,13 +62,13 @@ describe('Cron', () => { minute: 17, dayOfMonth: 12, }); - expect(expression).toMatch(/^[1-6]?[0-9] 17 13 12 \* \*$/); + expect(expression).toMatch(/^[1-5]?[0-9] 17 13 12 \* \*$/); }); test('should trim custom cron expressions', () => { const expression = toCronExpression({ mode: 'custom', - cronExpression: ' 0 9-17 * * * ', + cronExpression: ' 0 9-17 * * * ' as CronExpression, }); expect(expression).toEqual('0 9-17 * * *'); }); From 2b72adb03bd7b0a1d25bab1865c23c3fedf7e1f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Tue, 16 Jul 2024 18:57:19 +0200 Subject: [PATCH 16/17] do not return manualTriggerFunction for non-manual executions --- .../nodes/Schedule/ScheduleTrigger.node.ts | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts b/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts index 05b26e39ea53d..8808664af3ccb 100644 --- a/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts +++ b/packages/nodes-base/nodes/Schedule/ScheduleTrigger.node.ts @@ -462,24 +462,23 @@ export class ScheduleTrigger implements INodeType { } } } - } - - const manualTriggerFunction = async () => { - const { interval, cronExpression, recurrence } = rules[0]; - if (interval.field === 'cronExpression') { - try { - sendAt(cronExpression); - } catch (error) { - throw new NodeOperationError(this.getNode(), 'Invalid cron expression', { - description: 'More information on how to build them at https://crontab.guru/', - }); + return {}; + } else { + const manualTriggerFunction = async () => { + const { interval, cronExpression, recurrence } = rules[0]; + if (interval.field === 'cronExpression') { + try { + sendAt(cronExpression); + } catch (error) { + throw new NodeOperationError(this.getNode(), 'Invalid cron expression', { + description: 'More information on how to build them at https://crontab.guru/', + }); + } } - } - executeTrigger(recurrence); - }; + executeTrigger(recurrence); + }; - return { - manualTriggerFunction, - }; + return { manualTriggerFunction }; + } } } From d0192f364072629e37315cf5401c04a021cf8d4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Tue, 16 Jul 2024 18:59:10 +0200 Subject: [PATCH 17/17] add unit tests for the ScheduleTrigger node --- .../tests/ScheduleTrigger.node.test.ts | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 packages/nodes-base/nodes/Schedule/tests/ScheduleTrigger.node.test.ts diff --git a/packages/nodes-base/nodes/Schedule/tests/ScheduleTrigger.node.test.ts b/packages/nodes-base/nodes/Schedule/tests/ScheduleTrigger.node.test.ts new file mode 100644 index 0000000000000..fa1d2cd615c9c --- /dev/null +++ b/packages/nodes-base/nodes/Schedule/tests/ScheduleTrigger.node.test.ts @@ -0,0 +1,83 @@ +import * as n8nWorkflow from 'n8n-workflow'; +import type { INode, ITriggerFunctions, Workflow } from 'n8n-workflow'; +import { returnJsonArray } from 'n8n-core'; +import { ScheduledTaskManager } from 'n8n-core/dist/ScheduledTaskManager'; +import { mock } from 'jest-mock-extended'; +import { ScheduleTrigger } from '../ScheduleTrigger.node'; + +describe('ScheduleTrigger', () => { + Object.defineProperty(n8nWorkflow, 'randomInt', { + value: (min: number, max: number) => Math.floor((min + max) / 2), + }); + + const HOUR = 60 * 60 * 1000; + const mockDate = new Date('2023-12-28 12:34:56.789Z'); + const timezone = 'Europe/Berlin'; + jest.useFakeTimers(); + jest.setSystemTime(mockDate); + + const node = mock({ typeVersion: 1 }); + const workflow = mock({ timezone }); + const scheduledTaskManager = new ScheduledTaskManager(); + const helpers = mock({ + returnJsonArray, + registerCron: (cronExpression, onTick) => + scheduledTaskManager.registerCron(workflow, cronExpression, onTick), + }); + + const triggerFunctions = mock({ + helpers, + getTimezone: () => timezone, + getNode: () => node, + getMode: () => 'trigger', + }); + + const scheduleTrigger = new ScheduleTrigger(); + + beforeEach(() => { + jest.clearAllMocks(); + }); + + describe('trigger', () => { + it('should emit on defined schedule', async () => { + triggerFunctions.getNodeParameter.calledWith('rule', expect.anything()).mockReturnValueOnce({ + interval: [{ field: 'hours', hoursInterval: 3 }], + }); + triggerFunctions.getWorkflowStaticData.mockReturnValueOnce({ recurrenceRules: [] }); + + const result = await scheduleTrigger.trigger.call(triggerFunctions); + // Assert that no manualTriggerFunction is returned + expect(result).toEqual({}); + + expect(triggerFunctions.emit).not.toHaveBeenCalled(); + + jest.advanceTimersByTime(HOUR); + expect(triggerFunctions.emit).not.toHaveBeenCalled(); + + jest.advanceTimersByTime(2 * HOUR); + expect(triggerFunctions.emit).toHaveBeenCalledTimes(1); + + const firstTriggerData = triggerFunctions.emit.mock.calls[0][0][0][0]; + expect(firstTriggerData.json).toEqual({ + 'Day of month': '28', + 'Day of week': 'Thursday', + Hour: '15', + Minute: '30', + Month: 'December', + 'Readable date': 'December 28th 2023, 3:30:30 pm', + 'Readable time': '3:30:30 pm', + Second: '30', + Timezone: 'Europe/Berlin (UTC+01:00)', + Year: '2023', + timestamp: '2023-12-28T15:30:30.000+01:00', + }); + + jest.setSystemTime(new Date(firstTriggerData.json.timestamp as string)); + + jest.advanceTimersByTime(2 * HOUR); + expect(triggerFunctions.emit).toHaveBeenCalledTimes(1); + jest.advanceTimersByTime(HOUR); + expect(triggerFunctions.emit).toHaveBeenCalledTimes(2); + }); + }); +});