Skip to content

Commit eab5ceb

Browse files
committed
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
1 parent 9fa33b6 commit eab5ceb

File tree

2 files changed

+15
-5
lines changed

2 files changed

+15
-5
lines changed

core/src/main/java/org/elasticsearch/common/unit/TimeValue.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -326,7 +326,10 @@ public static TimeValue parseTimeValue(String sValue, TimeValue defaultValue, St
326326
return new TimeValue(parse(sValue, normalized, 2), TimeUnit.MILLISECONDS);
327327
} else if (normalized.endsWith("s")) {
328328
return new TimeValue(parse(sValue, normalized, 1), TimeUnit.SECONDS);
329-
} else if (normalized.endsWith("m")) {
329+
} else if (sValue.endsWith("m")) {
330+
// parsing minutes should be case sensitive as `M` is generally
331+
// accepted to mean months not minutes. This is the only case where
332+
// the upper and lower case forms indicate different time units
330333
return new TimeValue(parse(sValue, normalized, 1), TimeUnit.MINUTES);
331334
} else if (normalized.endsWith("h")) {
332335
return new TimeValue(parse(sValue, normalized, 1), TimeUnit.HOURS);

core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -92,10 +92,6 @@ public void testParseTimeValue() {
9292
TimeValue.parseTimeValue("10 m", null, "test"));
9393
assertEquals(new TimeValue(10, TimeUnit.MINUTES),
9494
TimeValue.parseTimeValue("10m", null, "test"));
95-
assertEquals(new TimeValue(10, TimeUnit.MINUTES),
96-
TimeValue.parseTimeValue("10 M", null, "test"));
97-
assertEquals(new TimeValue(10, TimeUnit.MINUTES),
98-
TimeValue.parseTimeValue("10M", null, "test"));
9995

10096
assertEquals(new TimeValue(10, TimeUnit.HOURS),
10197
TimeValue.parseTimeValue("10 h", null, "test"));
@@ -115,6 +111,17 @@ public void testParseTimeValue() {
115111
assertEquals(new TimeValue(10, TimeUnit.DAYS),
116112
TimeValue.parseTimeValue("10D", null, "test"));
117113

114+
// Time values of months should throw an exception as months are not
115+
// supported. Note that this is the only unit that is not case sensitive
116+
// as `m` is the only character that is overloaded in terms of which
117+
// time unit is expected between the upper and lower case versions
118+
expectThrows(ElasticsearchParseException.class, () -> {
119+
TimeValue.parseTimeValue("10 M", null, "test");
120+
});
121+
expectThrows(ElasticsearchParseException.class, () -> {
122+
TimeValue.parseTimeValue("10M", null, "test");
123+
});
124+
118125
final int length = randomIntBetween(0, 8);
119126
final String zeros = new String(new char[length]).replace('\0', '0');
120127
assertTrue(TimeValue.parseTimeValue("-" + zeros + "1", null, "test") == TimeValue.MINUS_ONE);

0 commit comments

Comments
 (0)