Require units for all time values

This commit is a backwards compatibilty break for all watcher indices
that had a `throttel_period` set on their watches. `throttle_period` used
to be a numeric value but now is stored as a string AND requires a unit
like seconds or minutes etc. to prevent errors. All other time valiues like
http timeouts also require units now.

Closes elastic/elasticsearch#598

Original commit: elastic/x-pack-elasticsearch@e3b2c2a4af

rest-api-spec/test/put_watch/20_put_watch_with_throttle_period.yaml

@@ -44,4 +44,4 @@
         id: "my_watch1"
   - match: { found : true}
   - match: { _id: "my_watch1" }
-  - match: { watch.throttle_period: 10000 }
+  - match: { watch.throttle_period: "10s" }

rest-api-spec/test/put_watch/30_put_watch_with_action_throttle_period.yaml

@@ -44,4 +44,4 @@
         id: "my_watch1"
   - match: { found : true}
   - match: { _id: "my_watch1" }
-  - match: { watch.actions.test_index.throttle_period: 10000 }
+  - match: { watch.actions.test_index.throttle_period: "10s" }

src/main/java/org/elasticsearch/watcher/actions/ActionWrapper.java

@@ -122,7 +122,7 @@ public class ActionWrapper implements ToXContent {
         builder.startObject();
         TimeValue throttlePeriod = throttler.throttlePeriod();
         if (throttlePeriod != null) {
-            builder.field(Throttler.Field.THROTTLE_PERIOD.getPreferredName(), throttlePeriod.getMillis());
+            builder.field(Throttler.Field.THROTTLE_PERIOD.getPreferredName(), throttlePeriod);
         }
         if (transform != null) {
             builder.startObject(Transform.Field.TRANSFORM.getPreferredName())
@@ -153,7 +153,7 @@ public class ActionWrapper implements ToXContent {
                     transform = transformRegistry.parse(watchId, parser);
                 } else if (Throttler.Field.THROTTLE_PERIOD.match(currentFieldName)) {
                     try {
-                        throttlePeriod = WatcherDateTimeUtils.parseTimeValue(parser, null, Throttler.Field.THROTTLE_PERIOD.toString());
+                        throttlePeriod = WatcherDateTimeUtils.parseTimeValue(parser, Throttler.Field.THROTTLE_PERIOD.toString());
                     } catch (WatcherDateTimeUtils.ParseException pe) {
                         throw new ActionException("could not parse action [{}/{}]. failed to parse field [{}] as time value", pe, watchId, actionId, currentFieldName);
                     }

src/main/java/org/elasticsearch/watcher/client/WatchSourceBuilder.java

@@ -141,7 +141,7 @@ public class WatchSourceBuilder implements ToXContent {
         }
 
         if (defaultThrottlePeriod != null) {
-            builder.field(Watch.Field.THROTTLE_PERIOD.getPreferredName(), defaultThrottlePeriod.getMillis());
+            builder.field(Watch.Field.THROTTLE_PERIOD.getPreferredName(), defaultThrottlePeriod.toString());
         }
 
         builder.startObject(Watch.Field.ACTIONS.getPreferredName());
@@ -185,7 +185,7 @@ public class WatchSourceBuilder implements ToXContent {
         public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
             builder.startObject();
             if (throttlePeriod != null) {
-                builder.field(Throttler.Field.THROTTLE_PERIOD.getPreferredName(), throttlePeriod.getMillis());
+                builder.field(Throttler.Field.THROTTLE_PERIOD.getPreferredName(), throttlePeriod);
             }
             if (transform != null) {
                 builder.startObject(Transform.Field.TRANSFORM.getPreferredName())

src/main/java/org/elasticsearch/watcher/support/WatcherDateTimeUtils.java

@@ -109,14 +109,6 @@ public class WatcherDateTimeUtils {
         return builder.field(fieldName, formatDate(date));
     }
 
-    public static void writeDate(StreamOutput out, DateTime date) throws IOException {
-        out.writeLong(date.getMillis());
-    }
-
-    public static DateTime readDate(StreamInput in, DateTimeZone timeZone) throws IOException {
-        return new DateTime(in.readLong(), timeZone);
-    }
-
     public static void writeOptionalDate(StreamOutput out, DateTime date) throws IOException {
         if (date == null) {
             out.writeBoolean(false);
@@ -130,25 +122,14 @@ public class WatcherDateTimeUtils {
         return in.readBoolean() ? new DateTime(in.readLong(), timeZone) : null;
     }
 
-    public static TimeValue parseTimeValue(XContentParser parser, TimeValue defaultValue, String settingName) throws IOException {
+    public static TimeValue parseTimeValue(XContentParser parser, String settingName) throws IOException {
-        return parseTimeValue(parser, TimeUnit.MILLISECONDS, defaultValue, settingName);
+        final XContentParser.Token token = parser.currentToken();
-    }
-
-    public static TimeValue parseTimeValue(XContentParser parser, TimeUnit defaultTimeUnit, TimeValue defaultValue, String settingName) throws IOException {
-        XContentParser.Token token = parser.currentToken();
         if (token == XContentParser.Token.VALUE_NULL) {
-            return defaultValue;
+            return null;
-        }
-        if (token == XContentParser.Token.VALUE_NUMBER) {
-            long millis = parser.longValue();
-            if (millis < 0) {
-                throw new ParseException("could not parse milli-seconds time value [{}]. Time value cannot be negative.", millis);
-            }
-            return new TimeValue(millis, defaultTimeUnit);
         }
         if (token == XContentParser.Token.VALUE_STRING) {
             try {
-                TimeValue value = TimeValue.parseTimeValue(parser.text(), defaultValue, settingName);
+                TimeValue value = TimeValue.parseTimeValue(parser.text(), null, settingName);
                 if (value.millis() < 0) {
                     throw new ParseException("could not parse time value [{}]. Time value cannot be negative.", parser.text());
                 }
@@ -158,7 +139,7 @@ public class WatcherDateTimeUtils {
             }
 
         }
-        throw new ParseException("could not parse time value. expected either a string or a numeric value but found [{}] instead", token);
+        throw new ParseException("could not parse time value. expected either a string or a null value but found [{}] instead", token);
     }
 
     public static class ParseException extends WatcherException {

src/main/java/org/elasticsearch/watcher/support/http/HttpRequest.java

@@ -123,10 +123,10 @@ public class HttpRequest implements ToXContent {
             builder.field(Field.BODY.getPreferredName(), body);
         }
         if (connectionTimeout != null) {
-            builder.field(Field.CONNECTION_TIMEOUT.getPreferredName(), connectionTimeout.toString());
+            builder.field(Field.CONNECTION_TIMEOUT.getPreferredName(), connectionTimeout);
         }
         if (readTimeout != null) {
-            builder.field(Field.READ_TIMEOUT.getPreferredName(), readTimeout.toString());
+            builder.field(Field.READ_TIMEOUT.getPreferredName(), readTimeout);
         }
         return builder.endObject();
     }
@@ -206,13 +206,13 @@ public class HttpRequest implements ToXContent {
                     builder.auth(httpAuthRegistry.parse(parser));
                 } else if (Field.CONNECTION_TIMEOUT.match(currentFieldName)) {
                     try {
-                        builder.connectionTimeout(WatcherDateTimeUtils.parseTimeValue(parser, null, Field.CONNECTION_TIMEOUT.toString()));
+                        builder.connectionTimeout(WatcherDateTimeUtils.parseTimeValue(parser, Field.CONNECTION_TIMEOUT.toString()));
                     } catch (WatcherDateTimeUtils.ParseException pe) {
                         throw new ParseException("could not parse http request. invalid time value for [{}] field", pe, currentFieldName);
                     }
                 } else if (Field.READ_TIMEOUT.match(currentFieldName)) {
                     try {
-                        builder.readTimeout(WatcherDateTimeUtils.parseTimeValue(parser, null, Field.READ_TIMEOUT.toString()));
+                        builder.readTimeout(WatcherDateTimeUtils.parseTimeValue(parser, Field.READ_TIMEOUT.toString()));
                     } catch (WatcherDateTimeUtils.ParseException pe) {
                         throw new ParseException("could not parse http request. invalid time value for [{}] field", pe, currentFieldName);
                     }

src/main/java/org/elasticsearch/watcher/support/http/HttpRequestTemplate.java

@@ -178,10 +178,10 @@ public class HttpRequestTemplate implements ToXContent {
             builder.field(Field.BODY.getPreferredName(), body, params);
         }
         if (connectionTimeout != null) {
-            builder.field(Field.CONNECTION_TIMEOUT.getPreferredName(), connectionTimeout.toString());
+            builder.field(Field.CONNECTION_TIMEOUT.getPreferredName(), connectionTimeout);
         }
         if (readTimeout != null) {
-            builder.field(Field.READ_TIMEOUT.getPreferredName(), readTimeout.toString());
+            builder.field(Field.READ_TIMEOUT.getPreferredName(), readTimeout);
         }
         return builder.endObject();
     }
@@ -254,13 +254,13 @@ public class HttpRequestTemplate implements ToXContent {
                     builder.body(parseFieldTemplate(currentFieldName, parser));
                 } else if (Field.CONNECTION_TIMEOUT.match(currentFieldName)) {
                     try {
-                        builder.connectionTimeout(WatcherDateTimeUtils.parseTimeValue(parser, null, Field.CONNECTION_TIMEOUT.toString()));
+                        builder.connectionTimeout(WatcherDateTimeUtils.parseTimeValue(parser, Field.CONNECTION_TIMEOUT.toString()));
                     } catch (WatcherDateTimeUtils.ParseException pe) {
                         throw new ParseException("could not parse http request template. invalid time value for [{}] field", pe, currentFieldName);
                     }
                 } else if (Field.READ_TIMEOUT.match(currentFieldName)) {
                     try {
-                        builder.readTimeout(WatcherDateTimeUtils.parseTimeValue(parser, null, Field.READ_TIMEOUT.toString()));
+                        builder.readTimeout(WatcherDateTimeUtils.parseTimeValue(parser, Field.READ_TIMEOUT.toString()));
                     } catch (WatcherDateTimeUtils.ParseException pe) {
                         throw new ParseException("could not parse http request template. invalid time value for [{}] field", pe, currentFieldName);
                     }

src/main/java/org/elasticsearch/watcher/watch/Watch.java

@@ -167,7 +167,7 @@ public class Watch implements TriggerEngine.Job, ToXContent {
             if (builder.humanReadable()) {
                 builder.field(Field.THROTTLE_PERIOD.getPreferredName(), throttlePeriod.format(PeriodType.seconds()));
             } else {
-                builder.field(Field.THROTTLE_PERIOD.getPreferredName(), throttlePeriod.getMillis());
+                builder.field(Field.THROTTLE_PERIOD.getPreferredName(), throttlePeriod);
             }
         }
         builder.field(Field.ACTIONS.getPreferredName(), actions, params);
@@ -292,7 +292,7 @@ public class Watch implements TriggerEngine.Job, ToXContent {
                     transform = transformRegistry.parse(id, parser);
                 } else if (Field.THROTTLE_PERIOD.match(currentFieldName)) {
                     try {
-                        throttlePeriod = WatcherDateTimeUtils.parseTimeValue(parser, null, Field.THROTTLE_PERIOD.toString());
+                        throttlePeriod = WatcherDateTimeUtils.parseTimeValue(parser, Field.THROTTLE_PERIOD.toString());
                     } catch (WatcherDateTimeUtils.ParseException pe) {
                         throw new ParseException("could not parse watch [{}]. failed to parse time value for field [{}]", pe, id, currentFieldName);
                     }

src/main/resources/watches.json

@@ -33,7 +33,8 @@
           "dynamic" : true
         },
         "throttle_period": {
-          "type": "long"
+          "type": "string",
+          "index" : "no"
         },
         "transform": {
           "type" : "object",

src/test/java/org/elasticsearch/watcher/support/WatcherDateTimeUtilsTests.java

@@ -27,7 +27,7 @@ import static org.hamcrest.Matchers.notNullValue;
  */
 public class WatcherDateTimeUtilsTests extends ElasticsearchTestCase {
 
-    @Test
+    @Test(expected = WatcherDateTimeUtils.ParseException.class)
     public void testParseTimeValue_Numeric() throws Exception {
         TimeValue value = new TimeValue(randomInt(100), randomFrom(TimeUnit.values()));
 
@@ -36,7 +36,7 @@ public class WatcherDateTimeUtilsTests extends ElasticsearchTestCase {
         parser.nextToken(); // field name
         parser.nextToken(); // value
 
-        TimeValue parsed = WatcherDateTimeUtils.parseTimeValue(parser, null, "test");
+        TimeValue parsed = WatcherDateTimeUtils.parseTimeValue(parser, "test");
         assertThat(parsed, notNullValue());
         assertThat(parsed.millis(), is(value.millis()));
     }
@@ -50,7 +50,7 @@ public class WatcherDateTimeUtilsTests extends ElasticsearchTestCase {
         parser.nextToken(); // field name
         parser.nextToken(); // value
 
-        WatcherDateTimeUtils.parseTimeValue(parser, null, "test");
+        WatcherDateTimeUtils.parseTimeValue(parser, "test");
     }
 
     @Test
@@ -69,7 +69,7 @@ public class WatcherDateTimeUtilsTests extends ElasticsearchTestCase {
         parser.nextToken(); // field name
         parser.nextToken(); // value
 
-        TimeValue parsed = WatcherDateTimeUtils.parseTimeValue(parser, null, "test");
+        TimeValue parsed = WatcherDateTimeUtils.parseTimeValue(parser, "test");
         assertThat(parsed, notNullValue());
         assertThat(parsed.millis(), is(values.get(key).millis()));
     }
@@ -90,31 +90,16 @@ public class WatcherDateTimeUtilsTests extends ElasticsearchTestCase {
         parser.nextToken(); // field name
         parser.nextToken(); // value
 
-        WatcherDateTimeUtils.parseTimeValue(parser, null, "test");
+        WatcherDateTimeUtils.parseTimeValue(parser, "test");
     }
 
-    @Test
     public void testParseTimeValue_Null() throws Exception {
         XContentParser parser = xContentParser(jsonBuilder().startObject().nullField("value").endObject());
         parser.nextToken(); // start object
         parser.nextToken(); // field name
         parser.nextToken(); // value
 
-        TimeValue parsed = WatcherDateTimeUtils.parseTimeValue(parser, null, "test");
+        TimeValue parsed = WatcherDateTimeUtils.parseTimeValue(parser, "test");
         assertThat(parsed, nullValue());
     }
-
-    @Test
-    public void testParseTimeValue_Null_DefaultValue() throws Exception {
-        XContentParser parser = xContentParser(jsonBuilder().startObject().nullField("value").endObject());
-        parser.nextToken(); // start object
-        parser.nextToken(); // field name
-        parser.nextToken(); // value
-
-        TimeValue defaultValue = new TimeValue(randomInt(100), randomFrom(TimeUnit.values()));
-
-        TimeValue parsed = WatcherDateTimeUtils.parseTimeValue(parser, defaultValue, "test");
-        assertThat(parsed, notNullValue());
-        assertThat(parsed, sameInstance(defaultValue));
-    }
 }