From 86593a931fc8daa9429dbfc4514d178e093758c7 Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Thu, 24 Jan 2019 17:05:29 +0100 Subject: [PATCH 1/3] Fix java time epoch date formatters 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 | 40 +++++-------------- .../common/time/DateFormattersTests.java | 16 ++++++++ .../aggregations/bucket/DateRangeIT.java | 27 +++---------- 3 files changed, 33 insertions(+), 50 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 c824a7c7e7c35..1cdc32f3a40b4 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 @@ public TemporalAccessor resolve(Map fieldValues, 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,29 +115,24 @@ public boolean isSupportedBy(TemporalAccessor temporal) { } @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) @@ -159,24 +152,13 @@ public long getFrom(TemporalAccessor temporal) { .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, MILLISECONDS_FORMATTER3); 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 96ef39e430178..12d46eae09f36 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,22 @@ public void testSupportBackwardsJava8Format() { 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())); + + DateFormatter secondsFormatter = DateFormatter.forPattern("epoch_second"); + String formattedSeconds = secondsFormatter.format(instant); + Instant secondsInstant = Instant.from(secondsFormatter.parse(formattedSeconds)); + assertThat(secondsInstant.getEpochSecond(), is(instant.getEpochSecond())); + } + 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 f50c0bfd072b1..3fc9b3b554587 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 @@ -996,39 +996,24 @@ public void testRangeWithFormatNumericValue() throws Exception { .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 From caca18777fbb1b3272e9683d3813733ea0b71d5c Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Fri, 25 Jan 2019 09:34:29 +0100 Subject: [PATCH 2/3] added test --- .../java/org/elasticsearch/common/time/DateFormattersTests.java | 2 ++ 1 file changed, 2 insertions(+) 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 12d46eae09f36..ed90132586c7f 100644 --- a/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java +++ b/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java @@ -161,6 +161,8 @@ public void testEpochFormatting() { 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() { From b7e4164dd1da14ff135afcde1c815ab0bda5d572 Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Fri, 25 Jan 2019 10:13:10 +0100 Subject: [PATCH 3/3] refactor by using a fraction with a minsize of 0 --- .../org/elasticsearch/common/time/EpochTime.java | 13 ++++--------- .../common/time/DateFormattersTests.java | 2 ++ .../search/aggregations/bucket/DateRangeIT.java | 1 - 3 files changed, 6 insertions(+), 10 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 1cdc32f3a40b4..22b29bd0edf45 100644 --- a/server/src/main/java/org/elasticsearch/common/time/EpochTime.java +++ b/server/src/main/java/org/elasticsearch/common/time/EpochTime.java @@ -136,6 +136,9 @@ public long getFrom(TemporalAccessor temporal) { // 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 @@ -144,21 +147,13 @@ public long getFrom(TemporalAccessor temporal) { .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_FORMATTER1, builder -> builder.parseDefaulting(ChronoField.NANO_OF_SECOND, 999_999_999L), SECONDS_FORMATTER1, SECONDS_FORMATTER2); static final DateFormatter MILLIS_FORMATTER = new JavaDateFormatter("epoch_millis", MILLISECONDS_FORMATTER1, builder -> builder.parseDefaulting(EpochTime.NANOS_OF_MILLI, 999_999L), - MILLISECONDS_FORMATTER1, MILLISECONDS_FORMATTER2, MILLISECONDS_FORMATTER3); + 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 ed90132586c7f..caf622b2353dc 100644 --- a/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java +++ b/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java @@ -156,6 +156,8 @@ public void testEpochFormatting() { 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); 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 3fc9b3b554587..ae6e4cc984fbf 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.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;