Watcher: Properly find next valid date in cron expressions (#32734)

When a list/an array of cron expressions is provided, and one of those addresses is already expired, the expired one will be considered as an option instead of the valid next one. This commit also reduces the visibility of the CronnableSchedule and refactors a comparator to look like java 8.
elastic · Aug 21, 2018 · 0d95e5c · 0d95e5c
1 parent b1bf7c0
commit 0d95e5c
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 16 deletions.
diff --git a/...her/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/CronnableSchedule.java b/...her/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/CronnableSchedule.java
@@ -13,20 +13,15 @@
 
 public abstract class CronnableSchedule implements Schedule {
 
-    private static final Comparator<Cron> CRON_COMPARATOR = new Comparator<Cron>() {
-        @Override
-        public int compare(Cron c1, Cron c2) {
-            return c1.expression().compareTo(c2.expression());
-        }
-    };
+    private static final Comparator<Cron> CRON_COMPARATOR = Comparator.comparing(Cron::expression);
 
     protected final Cron[] crons;
 
-    public CronnableSchedule(String... expressions) {
+    CronnableSchedule(String... expressions) {
         this(crons(expressions));
     }
 
-    public CronnableSchedule(Cron... crons) {
+    private CronnableSchedule(Cron... crons) {
         assert crons.length > 0;
         this.crons = crons;
         Arrays.sort(crons, CRON_COMPARATOR);
@@ -37,7 +32,15 @@ public long nextScheduledTimeAfter(long startTime, long time) {
         assert time >= startTime;
         long nextTime = Long.MAX_VALUE;
         for (Cron cron : crons) {
-            nextTime = Math.min(nextTime, cron.getNextValidTimeAfter(time));
+            long nextValidTimeAfter = cron.getNextValidTimeAfter(time);
+
+            boolean previousCronExpired = nextTime == -1;
+            boolean currentCronValid = nextValidTimeAfter > -1;
+            if (previousCronExpired && currentCronValid) {
+                nextTime = nextValidTimeAfter;
+            } else {
+                nextTime = Math.min(nextTime, nextValidTimeAfter);
+            }
         }
         return nextTime;
     }

diff --git a/...her/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/CronScheduleTests.java b/...her/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/CronScheduleTests.java
@@ -11,11 +11,15 @@
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.json.JsonXContent;
 
+import java.time.ZoneOffset;
+import java.time.ZonedDateTime;
+
 import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
 import static org.hamcrest.Matchers.arrayWithSize;
 import static org.hamcrest.Matchers.hasItemInArray;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
 
 public class CronScheduleTests extends ScheduleTestCase {
     public void testInvalid() throws Exception {
@@ -54,18 +58,25 @@ public void testParseMultiple() throws Exception {
         assertThat(crons, hasItemInArray("0 0/3 * * * ?"));
     }
 
+    public void testMultipleCronsNextScheduledAfter() {
+        CronSchedule schedule = new CronSchedule("0 5 9 1 1 ? 2019", "0 5 9 1 1 ? 2020", "0 5 9 1 1 ? 2017");
+        ZonedDateTime start2019 = ZonedDateTime.of(2019, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC);
+        ZonedDateTime start2020 = ZonedDateTime.of(2020, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC);
+        long firstSchedule = schedule.nextScheduledTimeAfter(0, start2019.toInstant().toEpochMilli());
+        long secondSchedule = schedule.nextScheduledTimeAfter(0, start2020.toInstant().toEpochMilli());
+
+        assertThat(firstSchedule, is(not(-1L)));
+        assertThat(secondSchedule, is(not(-1L)));
+        assertThat(firstSchedule, is(not(secondSchedule)));
+    }
+
     public void testParseInvalidBadExpression() throws Exception {
         XContentBuilder builder = jsonBuilder().value("0 0/5 * * ?");
         BytesReference bytes = BytesReference.bytes(builder);
         XContentParser parser = createParser(JsonXContent.jsonXContent, bytes);
         parser.nextToken();
-        try {
-            new CronSchedule.Parser().parse(parser);
-            fail("expected cron parsing to fail when using invalid cron expression");
-        } catch (ElasticsearchParseException pe) {
-            // expected
-            assertThat(pe.getCause(), instanceOf(IllegalArgumentException.class));
-        }
+        ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class, () -> new CronSchedule.Parser().parse(parser));
+        assertThat(e.getCause(), instanceOf(IllegalArgumentException.class));
     }
 
     public void testParseInvalidEmpty() throws Exception {