From eab5ceb9decde80ff26a24191c8cdd075bb6ce9d Mon Sep 17 00:00:00 2001 From: Colin Goodheart-Smithe Date: Thu, 28 Jul 2016 11:23:06 +0100 Subject: [PATCH] Makes `m` case sensitive in TimeValue The reason for this change is that currently if a user specifies e.g.`2M` meaning 2 months as a time value instead of throwing an exception explaining that time units in months are not supported (due to months having variable time spans) we instead will parse this to 2 minutes. This could be surprising to a user and could mean put a lot of load on the cluster performing a task that was never intended and whose results will be useless anyway. It is generally accepted that `m` indicates minutes and `M` indicates months with time values so this is consistent with the expectations a user might have around specifying time units. A concrete example of where this causes issues is in the decay score function which uses TimeValue to parse the scale and offset parameters of the decay into millisecond values to use in the calculation. Relates to #19619 --- .../org/elasticsearch/common/unit/TimeValue.java | 5 ++++- .../elasticsearch/common/unit/TimeValueTests.java | 15 +++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java b/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java index db8299cdc9ac4..ed67019c10399 100644 --- a/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java +++ b/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java @@ -326,7 +326,10 @@ public static TimeValue parseTimeValue(String sValue, TimeValue defaultValue, St return new TimeValue(parse(sValue, normalized, 2), TimeUnit.MILLISECONDS); } else if (normalized.endsWith("s")) { return new TimeValue(parse(sValue, normalized, 1), TimeUnit.SECONDS); - } else if (normalized.endsWith("m")) { + } else if (sValue.endsWith("m")) { + // parsing minutes should be case sensitive as `M` is generally + // accepted to mean months not minutes. This is the only case where + // the upper and lower case forms indicate different time units return new TimeValue(parse(sValue, normalized, 1), TimeUnit.MINUTES); } else if (normalized.endsWith("h")) { return new TimeValue(parse(sValue, normalized, 1), TimeUnit.HOURS); diff --git a/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java b/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java index 003d78ce42eca..4d0ac5257a31e 100644 --- a/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java +++ b/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java @@ -92,10 +92,6 @@ public void testParseTimeValue() { TimeValue.parseTimeValue("10 m", null, "test")); assertEquals(new TimeValue(10, TimeUnit.MINUTES), TimeValue.parseTimeValue("10m", null, "test")); - assertEquals(new TimeValue(10, TimeUnit.MINUTES), - TimeValue.parseTimeValue("10 M", null, "test")); - assertEquals(new TimeValue(10, TimeUnit.MINUTES), - TimeValue.parseTimeValue("10M", null, "test")); assertEquals(new TimeValue(10, TimeUnit.HOURS), TimeValue.parseTimeValue("10 h", null, "test")); @@ -115,6 +111,17 @@ public void testParseTimeValue() { assertEquals(new TimeValue(10, TimeUnit.DAYS), TimeValue.parseTimeValue("10D", null, "test")); + // Time values of months should throw an exception as months are not + // supported. Note that this is the only unit that is not case sensitive + // as `m` is the only character that is overloaded in terms of which + // time unit is expected between the upper and lower case versions + expectThrows(ElasticsearchParseException.class, () -> { + TimeValue.parseTimeValue("10 M", null, "test"); + }); + expectThrows(ElasticsearchParseException.class, () -> { + TimeValue.parseTimeValue("10M", null, "test"); + }); + final int length = randomIntBetween(0, 8); final String zeros = new String(new char[length]).replace('\0', '0'); assertTrue(TimeValue.parseTimeValue("-" + zeros + "1", null, "test") == TimeValue.MINUS_ONE);