Repository: kelektiv/node-cron
Pull request #937 (2024-12-29 16:02:49)

feat: throw instead of silently rewriting invalid cron expressions
## Description

Removes an outdated function that would **silently rewrite** invalid expressions, e.g. `0 0 30 FEB *` to `0 0 1 FEB *`.

## Related Issues

N/A

## Motivation and Context

@Anurag-Bansal1 stumbled upon the `MONTH_CONSTRAINTS` constant while exploring the codebase and asked why `29` was chosen for February.

This constant is only used by the `_verifyParse` function, and upon careful review, it looks like this function was [added a very long time ago](https://github.com/kelektiv/node-cron/commit/f1239db59e8952faf6cf855ac2e91b9fbd8d6701) to [prevent infinite loops](https://github.com/kelektiv/node-cron/issues/79), which is a problem that we've since [resolved at a deeper level](https://github.com/kelektiv/node-cron/pull/686).

## How Has This Been Tested?

Added a new test case. See the [failed test in CI](https://github.com/kelektiv/node-cron/actions/runs/12536830900/job/34960265861?pr=937) before I pushed the code changes.

## Screenshots (if appropriate):

Initially [discussed on Discord](https://discord.com/channels/1075597081017851934/1075597081860915242/1322867512559534171):

![image](https://github.com/user-attachments/assets/d842ca35-3e19-4df7-a683-b78c3385ef9f)

![image](https://github.com/user-attachments/assets/75fdc179-6b78-40be-bcd3-2f28b768de0d)

## Types of changes

<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->

- [ ] Bug fix (non-breaking change which fixes an issue)
- [X] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Checklist:

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [X] My code follows the code style of this project.
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [X] I have added tests to cover my changes.
- [X] All new and existing tests passed.
- [ ] If my change introduces a breaking change, I have added a `!` after the type/scope in the title (see the Conventional Commits standard).

## Related Commits

From most to least recent:
- https://github.com/kelektiv/node-cron/issues/686
- https://github.com/kelektiv/node-cron/issues/442 
- https://github.com/kelektiv/node-cron/issues/360
- https://github.com/kelektiv/node-cron/issues/127
- https://github.com/kelektiv/node-cron/issues/79 (fixed in https://github.com/kelektiv/node-cron/commit/f1239db59e8952faf6cf855ac2e91b9fbd8d6701)


Patch:
diff --git a/src/constants.ts b/src/constants.ts
index 0b6856c..a642b6c 100644
--- a/src/constants.ts
+++ b/src/constants.ts
@@ -6,20 +6,6 @@ export const CONSTRAINTS = Object.freeze({
 	month: [1, 12],
 	dayOfWeek: [0, 7]
 } as const);
-export const MONTH_CONSTRAINTS = Object.freeze({
-	1: 31,
-	2: 29, // support leap year...not perfect
-	3: 31,
-	4: 30,
-	5: 31,
-	6: 30,
-	7: 31,
-	8: 31,
-	9: 30,
-	10: 31,
-	11: 30,
-	12: 31
-} as const);
 export const PARSE_DEFAULTS = Object.freeze({
 	second: '0',
 	minute: '*',
diff --git a/src/time.ts b/src/time.ts
index 0c8cb8b..488602e 100644
--- a/src/time.ts
+++ b/src/time.ts
@@ -3,7 +3,6 @@ import { DateTime, Zone } from 'luxon';
 import {
 	ALIASES,
 	CONSTRAINTS,
-	MONTH_CONSTRAINTS,
 	PARSE_DEFAULTS,
 	PRESETS,
 	RE_RANGE,
@@ -15,13 +14,10 @@ import {
 import { CronError, ExclusiveParametersError } from './errors';
 import {
 	CronJobParams,
-	DayOfMonthRange,
-	MonthRange,
 	Ranges,
 	TimeUnit,
 	TimeUnitField
 } from './types/cron.types';
-import { getRecordKeys } from './utils';
 
 export class CronTime {
 	source: string | DateTime;
@@ -76,7 +72,6 @@ export class CronTime {
 		} else {
 			this.source = source;
 			this._parse(this.source);
-			this._verifyParse();
 		}
 	}
 
@@ -84,51 +79,6 @@ export class CronTime {
 		return date.weekday === 7 ? 0 : date.weekday;
 	}
 
-	/**
-	 * Ensure that the syntax parsed correctly and correct the specified values if needed.
-	 */
-	private _verifyParse() {
-		const months = getRecordKeys(this.month);
-		const daysOfMonth = getRecordKeys(this.dayOfMonth);
-
-		let isOk = false;
-
-		/**
-		 * if a dayOfMonth is not found in all months, we only need to fix the last
-		 * wrong month to prevent infinite loop
-		 */
-		let lastWrongMonth: MonthRange | null = null;
-		for (const m of months) {
-			const con = MONTH_CONSTRAINTS[m];
-
-			for (const day of daysOfMonth) {
-				if (day <= con) {
-					isOk = true;
-				}
-			}
-
-			if (!isOk) {
-				// save the month in order to be fixed if all months fails (infinite loop)
-				lastWrongMonth = m;
-				console.warn(`Month '${m}' is limited to '${con}' days.`);
-			}
-		}
-
-		// infinite loop detected (dayOfMonth is not found in all months)
-		// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
-		if (!isOk && lastWrongMonth !== null) {
-			const notOkCon = MONTH_CONSTRAINTS[lastWrongMonth];
-			for (const notOkDay of daysOfMonth) {
-				if (notOkDay > notOkCon) {
-					// eslint-disable-next-line @typescript-eslint/no-dynamic-delete
-					delete this.dayOfMonth[notOkDay];
-					const fixedDay = (notOkDay % notOkCon) as DayOfMonthRange;
-					this.dayOfMonth[fixedDay] = true;
-				}
-			}
-		}
-	}
-
 	/**
 	 * Calculate the "next" scheduled time
 	 */


Test patch:
diff --git a/tests/cron.test.ts b/tests/cron.test.ts
index 4572948..aec84c6 100644
--- a/tests/cron.test.ts
+++ b/tests/cron.test.ts
@@ -616,28 +616,24 @@ describe('cron', () => {
 
 	it('should not get into an infinite loop on invalid times', () => {
 		expect(() => {
-			new CronJob(
-				'* 60 * * * *',
-				() => {
-					expect(true).toBe(true);
-				},
-				null,
-				true
-			);
+			new CronJob('* 60 * * * *', () => {}, null, true);
 		}).toThrow();
 
 		expect(() => {
-			new CronJob(
-				'* * 24 * * *',
-				() => {
-					expect(true).toBe(true);
-				},
-				null,
-				true
-			);
+			new CronJob('* * 24 * * *', () => {}, null, true);
+		}).toThrow();
+
+		expect(() => {
+			new CronJob('0 0 30 FEB *', callback, null, true);
 		}).toThrow();
 	});
 
+	it('should not throw if at least one time is valid', () => {
+		expect(() => {
+			new CronJob('0 0 30 JAN,FEB *', callback, null, true);
+		}).not.toThrow();
+	});
+
 	it('should test start of month', () => {
 		const d = new Date('12/31/2014');
 		d.setSeconds(59);
@@ -856,6 +852,7 @@ describe('cron', () => {
 
 		// tick by 1 day
 		clock.tick(24 * 60 * 60 * 1000);
+		clock.restore();
 		job.stop();
 		expect(callback).toHaveBeenCalledTimes(1);
 	});