From fc38e503e04194da01e719810c267be4d2799698 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 29 Jun 2016 13:36:11 -0400 Subject: [PATCH] Clearer error when handling fractional time values In 2f638b5a23597967a98b1ced1deac91d64af5a44, support for fractional time values was removed. While this change is documented, the error message presented does not give an indication that fractional inputs are not supported. This commit fixes this by detecting when the input is a time value that would successfully parse as a double but will not parse as a long and presenting a clear error message that fractional time values are not supported. Relates #19158 --- .../elasticsearch/common/unit/TimeValue.java | 68 ++++++++++--------- .../common/unit/TimeValueTests.java | 31 +++++++++ 2 files changed, 68 insertions(+), 31 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 5a56603dad7..db8299cdc9a 100644 --- a/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java +++ b/core/src/main/java/org/elasticsearch/common/unit/TimeValue.java @@ -317,40 +317,46 @@ public class TimeValue implements Writeable { if (sValue == null) { return defaultValue; } - try { - String lowerSValue = sValue.toLowerCase(Locale.ROOT).trim(); - if (lowerSValue.endsWith("nanos")) { - return new TimeValue(parse(lowerSValue, 5), TimeUnit.NANOSECONDS); - } else if (lowerSValue.endsWith("micros")) { - return new TimeValue(parse(lowerSValue, 6), TimeUnit.MICROSECONDS); - } else if (lowerSValue.endsWith("ms")) { - return new TimeValue(parse(lowerSValue, 2), TimeUnit.MILLISECONDS); - } else if (lowerSValue.endsWith("s")) { - return new TimeValue(parse(lowerSValue, 1), TimeUnit.SECONDS); - } else if (lowerSValue.endsWith("m")) { - return new TimeValue(parse(lowerSValue, 1), TimeUnit.MINUTES); - } else if (lowerSValue.endsWith("h")) { - return new TimeValue(parse(lowerSValue, 1), TimeUnit.HOURS); - } else if (lowerSValue.endsWith("d")) { - return new TimeValue(parse(lowerSValue, 1), TimeUnit.DAYS); - } else if (lowerSValue.matches("-0*1")) { - return TimeValue.MINUS_ONE; - } else if (lowerSValue.matches("0+")) { - return TimeValue.ZERO; - } else { - // Missing units: - throw new ElasticsearchParseException( - "failed to parse setting [{}] with value [{}] as a time value: unit is missing or unrecognized", - settingName, - sValue); - } - } catch (NumberFormatException e) { - throw new ElasticsearchParseException("failed to parse [{}]", e, sValue); + final String normalized = sValue.toLowerCase(Locale.ROOT).trim(); + if (normalized.endsWith("nanos")) { + return new TimeValue(parse(sValue, normalized, 5), TimeUnit.NANOSECONDS); + } else if (normalized.endsWith("micros")) { + return new TimeValue(parse(sValue, normalized, 6), TimeUnit.MICROSECONDS); + } else if (normalized.endsWith("ms")) { + 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")) { + return new TimeValue(parse(sValue, normalized, 1), TimeUnit.MINUTES); + } else if (normalized.endsWith("h")) { + return new TimeValue(parse(sValue, normalized, 1), TimeUnit.HOURS); + } else if (normalized.endsWith("d")) { + return new TimeValue(parse(sValue, normalized, 1), TimeUnit.DAYS); + } else if (normalized.matches("-0*1")) { + return TimeValue.MINUS_ONE; + } else if (normalized.matches("0+")) { + return TimeValue.ZERO; + } else { + // Missing units: + throw new ElasticsearchParseException( + "failed to parse setting [{}] with value [{}] as a time value: unit is missing or unrecognized", + settingName, + sValue); } } - private static long parse(String s, int suffixLength) { - return Long.parseLong(s.substring(0, s.length() - suffixLength).trim()); + private static long parse(final String initialInput, final String normalized, final int suffixLength) { + final String s = normalized.substring(0, normalized.length() - suffixLength).trim(); + try { + return Long.parseLong(s); + } catch (final NumberFormatException e) { + try { + @SuppressWarnings("unused") final double ignored = Double.parseDouble(s); + throw new ElasticsearchParseException("failed to parse [{}], fractional time values are not supported", e, initialInput); + } catch (final NumberFormatException ignored) { + throw new ElasticsearchParseException("failed to parse [{}]", e, initialInput); + } + } } private static final long C0 = 1L; 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 9b73f2f99af..78afc9e514f 100644 --- a/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java +++ b/core/src/test/java/org/elasticsearch/common/unit/TimeValueTests.java @@ -30,9 +30,12 @@ import java.util.concurrent.TimeUnit; import static org.elasticsearch.common.unit.TimeValue.timeValueNanos; import static org.elasticsearch.common.unit.TimeValue.timeValueSeconds; +import static org.hamcrest.CoreMatchers.instanceOf; +import static org.hamcrest.CoreMatchers.not; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.lessThan; +import static org.hamcrest.object.HasToString.hasToString; public class TimeValueTests extends ESTestCase { @@ -125,6 +128,34 @@ public class TimeValueTests extends ESTestCase { assertThat(TimeValue.parseTimeValue(t.getStringRep(), null, "test"), equalTo(t)); } + private static final String FRACTIONAL_TIME_VALUES_ARE_NOT_SUPPORTED = "fractional time values are not supported"; + + public void testNonFractionalTimeValues() { + final String s = randomAsciiOfLength(10) + randomTimeUnit(); + final ElasticsearchParseException e = + expectThrows(ElasticsearchParseException.class, () -> TimeValue.parseTimeValue(s, null, "test")); + assertThat(e, hasToString(containsString("failed to parse [" + s + "]"))); + assertThat(e, not(hasToString(containsString(FRACTIONAL_TIME_VALUES_ARE_NOT_SUPPORTED)))); + assertThat(e.getCause(), instanceOf(NumberFormatException.class)); + } + + public void testFractionalTimeValues() { + double value; + do { + value = randomDouble(); + } while (value == 0); + final String s = Double.toString(randomIntBetween(0, 128) + value) + randomTimeUnit(); + final ElasticsearchParseException e = + expectThrows(ElasticsearchParseException.class, () -> TimeValue.parseTimeValue(s, null, "test")); + assertThat(e, hasToString(containsString("failed to parse [" + s + "]"))); + assertThat(e, hasToString(containsString(FRACTIONAL_TIME_VALUES_ARE_NOT_SUPPORTED))); + assertThat(e.getCause(), instanceOf(NumberFormatException.class)); + } + + private String randomTimeUnit() { + return randomFrom("nanos", "micros", "ms", "s", "m", "h", "d"); + } + private void assertEqualityAfterSerialize(TimeValue value, int expectedSize) throws IOException { BytesStreamOutput out = new BytesStreamOutput(); value.writeTo(out);