CronSequenceGenerator.isValidExpression actually validates cron fields

Issue: SPR-15604

Author: jhoeller

spring-context/src/main/java/org/springframework/scheduling/support/CronSequenceGenerator.java

@@ -17,7 +17,6 @@
 package org.springframework.scheduling.support;
 
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.BitSet;
 import java.util.Calendar;
 import java.util.Collections;
@@ -26,6 +25,7 @@
 import java.util.List;
 import java.util.TimeZone;
 
+import org.springframework.lang.Nullable;
 import org.springframework.util.StringUtils;
 
 /**
@@ -50,13 +50,15 @@
  *
  * @author Dave Syer
  * @author Juergen Hoeller
+ * @author Ruslan Sibgatullin
  * @since 3.0
  * @see CronTrigger
  */
 public class CronSequenceGenerator {
 
 	private final String expression;
 
+	@Nullable
 	private final TimeZone timeZone;
 
 	private final BitSet months = new BitSet(12);
@@ -96,6 +98,12 @@ public CronSequenceGenerator(String expression, TimeZone timeZone) {
 		parse(expression);
 	}
 
+	private CronSequenceGenerator(String expression, String[] fields) {
+		this.expression = expression;
+		this.timeZone = null;
+		doParse(fields);
+	}
+
 
 	/**
 	 * Return the cron pattern that this sequence generator has been built for.
@@ -234,7 +242,7 @@ private int findNext(BitSet bits, int value, Calendar calendar, int field, int n
 		// roll over if needed
 		if (nextValue == -1) {
 			calendar.add(nextField, 1);
-			reset(calendar, Arrays.asList(field));
+			reset(calendar, Collections.singletonList(field));
 			nextValue = bits.nextSetBit(0);
 		}
 		if (nextValue != value) {
@@ -265,12 +273,17 @@ private void parse(String expression) throws IllegalArgumentException {
 			throw new IllegalArgumentException(String.format(
 					"Cron expression must consist of 6 fields (found %d in \"%s\")", fields.length, expression));
 		}
+		doParse(fields);
+	}
+
+	private void doParse(String[] fields) {
 		setNumberHits(this.seconds, fields[0], 0, 60);
 		setNumberHits(this.minutes, fields[1], 0, 60);
 		setNumberHits(this.hours, fields[2], 0, 24);
 		setDaysOfMonth(this.daysOfMonth, fields[3]);
 		setMonths(this.months, fields[4]);
 		setDays(this.daysOfWeek, replaceOrdinals(fields[5], "SUN,MON,TUE,WED,THU,FRI,SAT"), 8);
+
 		if (this.daysOfWeek.get(7)) {
 			// Sunday can be represented as 0 or 7
 			this.daysOfWeek.set(0);
@@ -388,19 +401,29 @@ private int[] getRange(String field, int min, int max) {
 
 	/**
 	 * Determine whether the specified expression represents a valid cron pattern.
-	 * <p>Specifically, this method verifies that the expression contains six
-	 * fields separated by single spaces.
 	 * @param expression the expression to evaluate
 	 * @return {@code true} if the given expression is a valid cron expression
 	 * @since 4.3
 	 */
-	public static boolean isValidExpression(String expression) {
+	public static boolean isValidExpression(@Nullable String expression) {
+		if (expression == null) {
+			return false;
+		}
 		String[] fields = StringUtils.tokenizeToStringArray(expression, " ");
-		return areValidCronFields(fields);
+		if (!areValidCronFields(fields)) {
+			return false;
+		}
+		try {
+			new CronSequenceGenerator(expression, fields);
+			return true;
+		}
+		catch (IllegalArgumentException ex) {
+			return false;
+		}
 	}
 
-	private static boolean areValidCronFields(String[] fields) {
-		return (fields.length == 6);
+	private static boolean areValidCronFields(@Nullable String[] fields) {
+		return (fields != null && fields.length == 6);
 	}
 
 

spring-context/src/test/java/org/springframework/scheduling/support/CronSequenceGeneratorTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2017 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -24,6 +24,7 @@
 
 /**
  * @author Juergen Hoeller
+ * @author Ruslan Sibgatullin
  */
 @SuppressWarnings("deprecation")
 public class CronSequenceGeneratorTests {
@@ -82,10 +83,20 @@ public void validExpression() {
 	}
 
 	@Test
-	public void invalidExpression() {
+	public void invalidExpressionWithLength() {
 		assertFalse(CronSequenceGenerator.isValidExpression("0 */2 1-4 * * * *"));
 	}
 
+	@Test
+	public void invalidExpressionWithSeconds() {
+		assertFalse(CronSequenceGenerator.isValidExpression("100 */2 1-4 * * *"));
+	}
+
+	@Test
+	public void invalidExpressionWithMonths() {
+		assertFalse(CronSequenceGenerator.isValidExpression("0 */2 1-4 * INVALID *"));
+	}
+
 	@Test
 	public void nullExpression() {
 		assertFalse(CronSequenceGenerator.isValidExpression(null));