Details
diff --git a/azkaban-common/src/main/java/azkaban/utils/Utils.java b/azkaban-common/src/main/java/azkaban/utils/Utils.java
index ca53c76..48ed0ab 100644
--- a/azkaban-common/src/main/java/azkaban/utils/Utils.java
+++ b/azkaban-common/src/main/java/azkaban/utils/Utils.java
@@ -28,6 +28,7 @@ import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.Collection;
+import java.util.Date;
import java.util.Enumeration;
import java.util.Random;
import java.util.TimeZone;
@@ -489,7 +490,25 @@ public class Utils {
} else return null;
}
- public static boolean isCronExpressionValid(String cronExpression) {
- return CronExpression.isValidExpression(cronExpression);
+ /**
+ *
+ * @param cronExpression
+ * @param timezone
+ * @return if the cronExpression is valid or not.
+ */
+ public static boolean isCronExpressionValid(String cronExpression, DateTimeZone timezone) {
+ if (!CronExpression.isValidExpression(cronExpression)) {
+ return false;
+ }
+
+ /*
+ * The below code is aimed at checking some cases that the above code can not identify,
+ * e.g. <0 0 3 ? * * 22> OR <0 0 3 ? * 8>. Under these cases, the below code is able to tell.
+ */
+ CronExpression cronExecutionTime = parseCronExpression(cronExpression, timezone);
+ if (cronExecutionTime == null || cronExecutionTime.getNextValidTimeAfter(new Date()) == null) {
+ return false;
+ }
+ return true;
}
}
diff --git a/azkaban-common/src/test/java/azkaban/utils/UtilsTest.java b/azkaban-common/src/test/java/azkaban/utils/UtilsTest.java
index 4ea0930..526cc36 100644
--- a/azkaban-common/src/test/java/azkaban/utils/UtilsTest.java
+++ b/azkaban-common/src/test/java/azkaban/utils/UtilsTest.java
@@ -16,6 +16,7 @@
package azkaban.utils;
+import org.joda.time.DateTimeZone;
import org.junit.Assert;
import org.junit.Test;
@@ -52,4 +53,29 @@ public class UtilsTest {
Assert.assertTrue(Utils.isValidPort(3030));
Assert.assertTrue(Utils.isValidPort(1045));
}
+
+ /* Test CronExpression valid cases*/
+ @Test
+ public void testValidCronExpressionV() {
+
+ DateTimeZone timezone = DateTimeZone.getDefault();
+ Assert.assertTrue(Utils.isCronExpressionValid("0 0 3 ? * *", timezone));
+ Assert.assertTrue(Utils.isCronExpressionValid("0 0 3 ? * * 2017", timezone));
+ Assert.assertTrue(Utils.isCronExpressionValid("0 0 * ? * *", timezone));
+ Assert.assertTrue(Utils.isCronExpressionValid("0 0 * ? * FRI", timezone));
+
+ // This is a bug from Quartz Cron. It looks like Quartz will parse the preceding 7 fields of a String.
+ Assert.assertTrue(Utils.isCronExpressionValid("0 0 3 ? * * 2017 22", timezone));
+ }
+
+ /* Test CronExpression invalid cases*/
+ @Test
+ public void testInvalidCronExpression() {
+
+ DateTimeZone timezone = DateTimeZone.getDefault();
+ Assert.assertFalse(Utils.isCronExpressionValid("0 0 3 * * *", timezone));
+ Assert.assertFalse(Utils.isCronExpressionValid("0 66 * ? * *", timezone));
+ Assert.assertFalse(Utils.isCronExpressionValid("0 * * ? * 8", timezone));
+ Assert.assertFalse(Utils.isCronExpressionValid("0 * 25 ? * FRI", timezone));
+ }
}
diff --git a/azkaban-web-server/src/main/java/azkaban/webapp/servlet/ScheduleServlet.java b/azkaban-web-server/src/main/java/azkaban/webapp/servlet/ScheduleServlet.java
index 14d5b89..32a4f4a 100644
--- a/azkaban-web-server/src/main/java/azkaban/webapp/servlet/ScheduleServlet.java
+++ b/azkaban-web-server/src/main/java/azkaban/webapp/servlet/ScheduleServlet.java
@@ -731,7 +731,7 @@ public class ScheduleServlet extends LoginAbstractAzkabanServlet {
// everything in Azkaban functions is at the minute granularity, so we add 0 here
// to let the expression to be complete.
cronExpression = getParam(req, "cronExpression");
- if(azkaban.utils.Utils.isCronExpressionValid(cronExpression) == false) {
+ if(azkaban.utils.Utils.isCronExpressionValid(cronExpression, timezone) == false) {
ret.put("error", "This expression <" + cronExpression + "> can not be parsed to quartz cron.");
return;
}