Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Document cron-vs-quartz parsing convention for dayOfWeek part in CronExpression #32128

Closed
tvahrst opened this issue Jan 26, 2024 · 1 comment
Closed
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: backported An issue that has been backported to maintenance branches type: documentation A documentation task
Milestone

Comments

@tvahrst
Copy link

tvahrst commented Jan 26, 2024

Cron expressions with a quartz specific dayOfWeek part like 0 30 17 ? * 2#3 are not parsed correctly by Spring's CronExpression:

2#3 means 'third Monday every month' but CronExpression parses this as "third tuesday in month".

It looks like the parsing logic in QuartzCronField#parseDayOfWeek is not correct:

	private static DayOfWeek parseDayOfWeek(String value) {
		int dayOfWeek = Integer.parseInt(value);
		if (dayOfWeek == 0) {
			dayOfWeek = 7; // cron is 0 based; java.time 1 based
		}
		try {
			return DayOfWeek.of(dayOfWeek);
		}...
	}

This methods transforms the input-value (cron/quartz dayOfWeek) into a java.time dayOfWeek index, then returning the corresponding java.time.DayOfWeek

The problem is, that cron, quartz and java.time have different dayOfTime indices. cron and quartz use same indices for su - fr, but Saturday differs. java.time is 1 based, beginning with Monday:

    cron quartz    java.time
sa    0    7         6
su    1    1         7
mo    2    2         1
tu    3    3         2
...

So the input-value has to be decremented by 1 and the overflow handled, like this:

		// Offset cron/quartz => java.time:
		dayOfWeek -= 1;

		if (dayOfWeek < 1) {
			dayOfWeek += 7;
		}

Tests:

	@Test
	public void testDayOfWeek_7() {
		// every third saturday in month.
		QuartzCronField quartzCronField = QuartzCronField.parseDaysOfWeek("7#3");
		LocalDateTime next = quartzCronField.nextOrSame(LocalDateTime.parse("2024-01-01T17:00:00"));

		assertThat(next.getDayOfMonth()).isEqualTo(20);  // expected: 2024-10-20, saturday

	}
	@Test
	public void testDayOfWeek_0(){
		// 0 is not a valid dayOfWeek for quartz, but for cron. To be tolerant for cron dayOfWeek, we handle 0 as saturday.
		QuartzCronField quartzCronField = QuartzCronField.parseDaysOfWeek("0#3");
		LocalDateTime next = quartzCronField.nextOrSame(LocalDateTime.parse("2024-01-01T17:00:00"));

		assertThat(next.getDayOfMonth()).isEqualTo(20);  // expected: 2024-10-20, saturday
	}

	@Test
	public void testDayOfWeek_1(){
		// every third sunday in month.
		QuartzCronField quartzCronField = QuartzCronField.parseDaysOfWeek("1#3");
		LocalDateTime next = quartzCronField.nextOrSame(LocalDateTime.parse("2024-01-01T17:00:00"));

		assertThat(next.getDayOfMonth()).isEqualTo(21);  // expected: 2024-10-21, sunday
	}

	@Test
	public void testDayOfWeek_2(){
		// every third monday in month.
		QuartzCronField quartzCronField = QuartzCronField.parseDaysOfWeek("2#3");
		LocalDateTime next = quartzCronField.nextOrSame(LocalDateTime.parse("2024-01-01T17:00:00"));

		assertThat(next.getDayOfMonth()).isEqualTo(15);  // expected: 2024-10-15, monday

	}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 26, 2024
@jhoeller jhoeller added type: bug A general bug in: core Issues in core modules (aop, beans, core, context, expression) and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 26, 2024
@jhoeller jhoeller self-assigned this Jan 26, 2024
@jhoeller jhoeller added this to the 6.1.4 milestone Jan 26, 2024
@jhoeller jhoeller added the for: backport-to-6.0.x Marks an issue as a candidate for backport to 6.0.x label Jan 26, 2024
@github-actions github-actions bot added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-6.0.x Marks an issue as a candidate for backport to 6.0.x labels Jan 26, 2024
@jhoeller jhoeller added the for: backport-to-5.3.x Marks an issue as a candidate for backport to 5.3.x label Jan 26, 2024
@github-actions github-actions bot removed the for: backport-to-5.3.x Marks an issue as a candidate for backport to 5.3.x label Jan 26, 2024
@jhoeller
Copy link
Contributor

jhoeller commented Jan 26, 2024

On review, the inconsistency seems to be between Cron and Quartz:
quartz-scheduler/quartz#524
https://stackoverflow.com/questions/18919151/crontab-day-of-the-week-syntax

We currently parse the day of the week according to common Cron rules which Quartz seems to disagree with. Since we have been doing this for a number of years already, it'll be hard to change this, even if this is arguably Quartz-specific syntax to begin with... so we might have to turn this into a documentation issue.

@jhoeller jhoeller changed the title CronExpression wrong parsing of quartz specific expression Document cron-vs-quartz parsing convention for dayOfWeek part in CronExpression Jan 26, 2024
@jhoeller jhoeller added type: documentation A documentation task and removed type: bug A general bug labels Jan 26, 2024
jhoeller added a commit that referenced this issue Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: backported An issue that has been backported to maintenance branches type: documentation A documentation task
Projects
None yet
Development

No branches or pull requests

3 participants