From c02cd3e2fdc703ade351f43f5e733279b324e9fb Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Fri, 1 Feb 2019 09:03:48 +0100 Subject: [PATCH] Fix java time epoch date formatters (#37829) The self written epoch date formatters were not properly able to format an Instant to a string due to a misconfiguration. This fix also removes a until now existing runtime behaviour under java 8 regarding the names of the aggregation buckets, which are now the same as before and have been under java 11. --- .../elasticsearch/common/time/EpochTime.java | 51 +++++-------------- .../common/time/DateFormattersTests.java | 20 ++++++++ .../aggregations/bucket/DateRangeIT.java | 28 +++------- 3 files changed, 40 insertions(+), 59 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/time/EpochTime.java b/server/src/main/java/org/elasticsearch/common/time/EpochTime.java index c824a7c7e7c..22b29bd0edf 100644 --- a/server/src/main/java/org/elasticsearch/common/time/EpochTime.java +++ b/server/src/main/java/org/elasticsearch/common/time/EpochTime.java @@ -19,8 +19,6 @@ package org.elasticsearch.common.time; -import org.elasticsearch.bootstrap.JavaVersion; - import java.time.format.DateTimeFormatter; import java.time.format.DateTimeFormatterBuilder; import java.time.format.ResolverStyle; @@ -72,7 +70,7 @@ class EpochTime { private static final EpochField NANOS_OF_SECOND = new EpochField(ChronoUnit.NANOS, ChronoUnit.SECONDS, ValueRange.of(0, 999_999_999)) { @Override public boolean isSupportedBy(TemporalAccessor temporal) { - return temporal.isSupported(ChronoField.NANO_OF_SECOND) && temporal.getLong(ChronoField.NANO_OF_SECOND) != 0; + return temporal.isSupported(ChronoField.NANO_OF_SECOND); } @Override public long getFrom(TemporalAccessor temporal) { @@ -117,32 +115,30 @@ class EpochTime { } @Override public long getFrom(TemporalAccessor temporal) { - return temporal.getLong(ChronoField.NANO_OF_SECOND); + return temporal.getLong(ChronoField.NANO_OF_SECOND) % 1_000_000; } }; // this supports seconds without any fraction private static final DateTimeFormatter SECONDS_FORMATTER1 = new DateTimeFormatterBuilder() .appendValue(SECONDS, 1, 19, SignStyle.NORMAL) + .optionalStart() // optional is used so isSupported will be called when printing + .appendFraction(NANOS_OF_SECOND, 0, 9, true) + .optionalEnd() .toFormatter(Locale.ROOT); // this supports seconds ending in dot private static final DateTimeFormatter SECONDS_FORMATTER2 = new DateTimeFormatterBuilder() - .append(SECONDS_FORMATTER1) + .appendValue(SECONDS, 1, 19, SignStyle.NORMAL) .appendLiteral('.') .toFormatter(Locale.ROOT); - // this supports seconds with a fraction and is also used for printing - private static final DateTimeFormatter SECONDS_FORMATTER3 = new DateTimeFormatterBuilder() - .append(SECONDS_FORMATTER1) - .optionalStart() // optional is used so isSupported will be called when printing - .appendFraction(NANOS_OF_SECOND, 1, 9, true) - .optionalEnd() - .toFormatter(Locale.ROOT); - // this supports milliseconds without any fraction private static final DateTimeFormatter MILLISECONDS_FORMATTER1 = new DateTimeFormatterBuilder() .appendValue(MILLIS, 1, 19, SignStyle.NORMAL) + .optionalStart() + .appendFraction(NANOS_OF_MILLI, 0, 6, true) + .optionalEnd() .toFormatter(Locale.ROOT); // this supports milliseconds ending in dot @@ -151,32 +147,13 @@ class EpochTime { .appendLiteral('.') .toFormatter(Locale.ROOT); - // this supports milliseconds with a fraction and is also used for printing - private static final DateTimeFormatter MILLISECONDS_FORMATTER3 = new DateTimeFormatterBuilder() - .append(MILLISECONDS_FORMATTER1) - .optionalStart() // optional is used so isSupported will be called when printing - .appendFraction(NANOS_OF_MILLI, 1, 6, true) - .optionalEnd() - .toFormatter(Locale.ROOT); - - static final DateFormatter SECONDS_FORMATTER = new JavaDateFormatter("epoch_second", SECONDS_FORMATTER3, + static final DateFormatter SECONDS_FORMATTER = new JavaDateFormatter("epoch_second", SECONDS_FORMATTER1, builder -> builder.parseDefaulting(ChronoField.NANO_OF_SECOND, 999_999_999L), - SECONDS_FORMATTER1, SECONDS_FORMATTER2, SECONDS_FORMATTER3); + SECONDS_FORMATTER1, SECONDS_FORMATTER2); - static final DateFormatter MILLIS_FORMATTER = getEpochMillisFormatter(); - - private static DateFormatter getEpochMillisFormatter() { - // the third formatter fails under java 8 as a printer, so fall back to this one - final DateTimeFormatter printer; - if (JavaVersion.current().getVersion().get(0) == 8) { - printer = MILLISECONDS_FORMATTER1; - } else { - printer = MILLISECONDS_FORMATTER3; - } - return new JavaDateFormatter("epoch_millis", printer, - builder -> builder.parseDefaulting(EpochTime.NANOS_OF_MILLI, 999_999L), - MILLISECONDS_FORMATTER1, MILLISECONDS_FORMATTER2, MILLISECONDS_FORMATTER3); - } + static final DateFormatter MILLIS_FORMATTER = new JavaDateFormatter("epoch_millis", MILLISECONDS_FORMATTER1, + builder -> builder.parseDefaulting(EpochTime.NANOS_OF_MILLI, 999_999L), + MILLISECONDS_FORMATTER1, MILLISECONDS_FORMATTER2); private abstract static class EpochField implements TemporalField { diff --git a/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java b/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java index 90a9a76e6a4..7b535f9d4c9 100644 --- a/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java +++ b/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java @@ -147,6 +147,26 @@ public class DateFormattersTests extends ESTestCase { assertThat(formatter, instanceOf(JavaDateFormatter.class)); } + public void testEpochFormatting() { + long seconds = randomLongBetween(0, 130L * 365 * 86400); // from 1970 epoch till around 2100 + long nanos = randomLongBetween(0, 999_999_999L); + Instant instant = Instant.ofEpochSecond(seconds, nanos); + + DateFormatter millisFormatter = DateFormatter.forPattern("epoch_millis"); + String millis = millisFormatter.format(instant); + Instant millisInstant = Instant.from(millisFormatter.parse(millis)); + assertThat(millisInstant.toEpochMilli(), is(instant.toEpochMilli())); + assertThat(millisFormatter.format(Instant.ofEpochSecond(42, 0)), is("42000")); + assertThat(millisFormatter.format(Instant.ofEpochSecond(42, 123456789L)), is("42123.456789")); + + DateFormatter secondsFormatter = DateFormatter.forPattern("epoch_second"); + String formattedSeconds = secondsFormatter.format(instant); + Instant secondsInstant = Instant.from(secondsFormatter.parse(formattedSeconds)); + assertThat(secondsInstant.getEpochSecond(), is(instant.getEpochSecond())); + + assertThat(secondsFormatter.format(Instant.ofEpochSecond(42, 0)), is("42")); + } + public void testParsingStrictNanoDates() { DateFormatter formatter = DateFormatters.forPattern("strict_date_optional_time_nanos"); formatter.format(formatter.parse("2016-01-01T00:00:00.000")); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeIT.java index f50c0bfd072..ae6e4cc984f 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeIT.java @@ -22,7 +22,6 @@ import org.elasticsearch.ElasticsearchException; import org.elasticsearch.action.index.IndexRequestBuilder; import org.elasticsearch.action.search.SearchPhaseExecutionException; import org.elasticsearch.action.search.SearchResponse; -import org.elasticsearch.bootstrap.JavaVersion; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.script.Script; @@ -996,39 +995,24 @@ public class DateRangeIT extends ESIntegTestCase { .addAggregation(dateRange("date_range").field("date").addRange(1000, 3000).addRange(3000, 4000)).get(); assertThat(searchResponse.getHits().getTotalHits().value, equalTo(3L)); List buckets = checkBuckets(searchResponse.getAggregations().get("date_range"), "date_range", 2); - if (JavaVersion.current().getVersion().get(0) == 8) { - assertBucket(buckets.get(0), 2L, "1000.0-3000.0", 1000000L, 3000000L); - assertBucket(buckets.get(1), 1L, "3000.0-4000.0", 3000000L, 4000000L); - } else { - assertBucket(buckets.get(0), 2L, "1000-3000", 1000000L, 3000000L); - assertBucket(buckets.get(1), 1L, "3000-4000", 3000000L, 4000000L); - } + assertBucket(buckets.get(0), 2L, "1000-3000", 1000000L, 3000000L); + assertBucket(buckets.get(1), 1L, "3000-4000", 3000000L, 4000000L); // using no format should also work when and to/from are string values searchResponse = client().prepareSearch(indexName).setSize(0) .addAggregation(dateRange("date_range").field("date").addRange("1000", "3000").addRange("3000", "4000")).get(); assertThat(searchResponse.getHits().getTotalHits().value, equalTo(3L)); buckets = checkBuckets(searchResponse.getAggregations().get("date_range"), "date_range", 2); - if (JavaVersion.current().getVersion().get(0) == 8) { - assertBucket(buckets.get(0), 2L, "1000.0-3000.0", 1000000L, 3000000L); - assertBucket(buckets.get(1), 1L, "3000.0-4000.0", 3000000L, 4000000L); - } else { - assertBucket(buckets.get(0), 2L, "1000-3000", 1000000L, 3000000L); - assertBucket(buckets.get(1), 1L, "3000-4000", 3000000L, 4000000L); - } + assertBucket(buckets.get(0), 2L, "1000-3000", 1000000L, 3000000L); + assertBucket(buckets.get(1), 1L, "3000-4000", 3000000L, 4000000L); // also e-notation should work, fractional parts should be truncated searchResponse = client().prepareSearch(indexName).setSize(0) .addAggregation(dateRange("date_range").field("date").addRange(1.0e3, 3000.8123).addRange(3000.8123, 4.0e3)).get(); assertThat(searchResponse.getHits().getTotalHits().value, equalTo(3L)); buckets = checkBuckets(searchResponse.getAggregations().get("date_range"), "date_range", 2); - if (JavaVersion.current().getVersion().get(0) == 8) { - assertBucket(buckets.get(0), 2L, "1000.0-3000.0", 1000000L, 3000000L); - assertBucket(buckets.get(1), 1L, "3000.0-4000.0", 3000000L, 4000000L); - } else { - assertBucket(buckets.get(0), 2L, "1000-3000", 1000000L, 3000000L); - assertBucket(buckets.get(1), 1L, "3000-4000", 3000000L, 4000000L); - } + assertBucket(buckets.get(0), 2L, "1000-3000", 1000000L, 3000000L); + assertBucket(buckets.get(1), 1L, "3000-4000", 3000000L, 4000000L); // using different format should work when to/from is compatible with // format in aggregation