From 64d4952f2254f03e679f60fadbec7ba5adf71e1f Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Fri, 31 Aug 2018 12:35:12 +0200 Subject: [PATCH 1/3] Core: Fix epoch millis java time formatter The existing implemention could not deal with negative numbers as well as +- 999 milliseconds around the epoch. This commit uses Instant.ofEpochMilli() and tries to parse the input to a number instead of using a date formatter. --- .../common/time/DateFormatters.java | 37 +++++++++++- .../joda/JavaJodaTimeDuellingTests.java | 8 +++ .../common/time/DateFormattersTests.java | 58 +++++++++++++++++++ 3 files changed, 101 insertions(+), 2 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java diff --git a/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java b/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java index 4017e43b071fd..55b2b6b69f989 100644 --- a/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java +++ b/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java @@ -25,10 +25,12 @@ import java.time.DayOfWeek; import java.time.Instant; import java.time.LocalDate; +import java.time.ZoneId; import java.time.ZoneOffset; import java.time.ZonedDateTime; import java.time.format.DateTimeFormatter; import java.time.format.DateTimeFormatterBuilder; +import java.time.format.DateTimeParseException; import java.time.format.ResolverStyle; import java.time.format.SignStyle; import java.time.temporal.ChronoField; @@ -879,11 +881,42 @@ public class DateFormatters { /* * Returns a formatter for parsing the milliseconds since the epoch + * This one needs a custom implementation, because the standard date formatter can not parse negative values + * or anything +- 999 milliseconds around the eopch + * + * This implementation just resorts to parsing the input directly to an Instant by trying to parse a number. */ - private static final CompoundDateTimeFormatter EPOCH_MILLIS = new CompoundDateTimeFormatter(new DateTimeFormatterBuilder() + private static final DateTimeFormatter EPOCH_MILLIS_FORMATTER = new DateTimeFormatterBuilder() .appendValue(ChronoField.INSTANT_SECONDS, 1, 19, SignStyle.NEVER) .appendValue(ChronoField.MILLI_OF_SECOND, 3) - .toFormatter(Locale.ROOT)); + .toFormatter(Locale.ROOT); + + private static final class EpochDateTimeFormatter extends CompoundDateTimeFormatter { + + EpochDateTimeFormatter() { + super(EPOCH_MILLIS_FORMATTER); + } + + EpochDateTimeFormatter(ZoneId zoneId) { + super(EPOCH_MILLIS_FORMATTER.withZone(zoneId)); + } + + @Override + public TemporalAccessor parse(String input) { + try { + return Instant.ofEpochMilli(Long.valueOf(input)).atZone(ZoneOffset.UTC); + } catch (NumberFormatException e) { + throw new DateTimeParseException("invalid number", input, 0, e); + } + } + + @Override + public CompoundDateTimeFormatter withZone(ZoneId zoneId) { + return new EpochDateTimeFormatter(zoneId); + } + } + + private static final CompoundDateTimeFormatter EPOCH_MILLIS = new EpochDateTimeFormatter(); /* * Returns a formatter that combines a full date and two digit hour of diff --git a/server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java b/server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java index 1551a315b26d9..cea83bfbccfdc 100644 --- a/server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java +++ b/server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java @@ -71,7 +71,15 @@ public void testCustomTimeFormats() { public void testDuellingFormatsValidParsing() { assertSameDate("1522332219", "epoch_second"); + assertSameDate("0", "epoch_second"); + assertSameDate("1", "epoch_second"); + assertSameDate("-1", "epoch_second"); + assertSameDate("-1522332219", "epoch_second"); assertSameDate("1522332219321", "epoch_millis"); + assertSameDate("0", "epoch_millis"); + assertSameDate("1", "epoch_millis"); + assertSameDate("-1", "epoch_millis"); + assertSameDate("-1522332219321", "epoch_millis"); assertSameDate("20181126", "basic_date"); assertSameDate("20181126T121212.123Z", "basic_date_time"); diff --git a/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java b/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java new file mode 100644 index 0000000000000..86224dacdb6a0 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java @@ -0,0 +1,58 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.common.time; + +import org.elasticsearch.test.ESTestCase; + +import java.time.ZoneId; +import java.time.ZonedDateTime; +import java.time.format.DateTimeParseException; + +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.is; + +public class DateFormattersTests extends ESTestCase { + + // the epoch milli parser is a bit special, as it does not use date formatter, see comments in DateFormatters + public void testEpochMilliParser() { + CompoundDateTimeFormatter formatter = DateFormatters.forPattern("epoch_millis"); + + DateTimeParseException e = expectThrows(DateTimeParseException.class, () -> formatter.parse("invalid")); + assertThat(e.getMessage(), containsString("invalid number")); + + // different zone, should still yield the same output, as epoch is time zoned independent + ZoneId zoneId = randomZone(); + CompoundDateTimeFormatter zonedFormatter = formatter.withZone(zoneId); + assertThat(zonedFormatter.printer.getZone(), is(zoneId)); + + // test with negative and non negative values + assertThatSameDateTime(formatter, zonedFormatter, String.valueOf(randomNonNegativeLong() * -1)); + assertThatSameDateTime(formatter, zonedFormatter, String.valueOf(randomNonNegativeLong())); + assertThatSameDateTime(formatter, zonedFormatter, String.valueOf(0)); + assertThatSameDateTime(formatter, zonedFormatter, String.valueOf(-1)); + assertThatSameDateTime(formatter, zonedFormatter, String.valueOf(1)); + } + + private void assertThatSameDateTime(CompoundDateTimeFormatter formatter, CompoundDateTimeFormatter zonedFormatter, String value) { + ZonedDateTime formatterZonedDateTime = DateFormatters.toZonedDateTime(formatter.parse(value)); + ZonedDateTime zonedFormatterZonedDateTime = DateFormatters.toZonedDateTime(zonedFormatter.parse(value)); + assertThat(formatterZonedDateTime.toInstant().toEpochMilli(), is(zonedFormatterZonedDateTime.toInstant().toEpochMilli())); + } +} From 1b5ae9e9f2f13d37aec99f831e1148241f274a58 Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Fri, 31 Aug 2018 16:49:36 +0200 Subject: [PATCH 2/3] address review comments --- .../org/elasticsearch/common/time/DateFormatters.java | 11 ++++++++--- .../common/time/DateFormattersTests.java | 6 ++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java b/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java index 55b2b6b69f989..69b8cb0c85bfa 100644 --- a/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java +++ b/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java @@ -882,7 +882,7 @@ public class DateFormatters { /* * Returns a formatter for parsing the milliseconds since the epoch * This one needs a custom implementation, because the standard date formatter can not parse negative values - * or anything +- 999 milliseconds around the eopch + * or anything +- 999 milliseconds around the epoch * * This implementation just resorts to parsing the input directly to an Instant by trying to parse a number. */ @@ -893,11 +893,11 @@ public class DateFormatters { private static final class EpochDateTimeFormatter extends CompoundDateTimeFormatter { - EpochDateTimeFormatter() { + private EpochDateTimeFormatter() { super(EPOCH_MILLIS_FORMATTER); } - EpochDateTimeFormatter(ZoneId zoneId) { + private EpochDateTimeFormatter(ZoneId zoneId) { super(EPOCH_MILLIS_FORMATTER.withZone(zoneId)); } @@ -914,6 +914,11 @@ public TemporalAccessor parse(String input) { public CompoundDateTimeFormatter withZone(ZoneId zoneId) { return new EpochDateTimeFormatter(zoneId); } + + @Override + public String format(TemporalAccessor accessor) { + return String.valueOf(Instant.from(accessor).toEpochMilli()); + } } private static final CompoundDateTimeFormatter EPOCH_MILLIS = new EpochDateTimeFormatter(); 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 86224dacdb6a0..8b062959cf9ef 100644 --- a/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java +++ b/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java @@ -24,6 +24,7 @@ import java.time.ZoneId; import java.time.ZonedDateTime; import java.time.format.DateTimeParseException; +import java.time.temporal.TemporalAccessor; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; @@ -48,6 +49,11 @@ public void testEpochMilliParser() { assertThatSameDateTime(formatter, zonedFormatter, String.valueOf(0)); assertThatSameDateTime(formatter, zonedFormatter, String.valueOf(-1)); assertThatSameDateTime(formatter, zonedFormatter, String.valueOf(1)); + + // format() output should be equal as well + long randomMillis = randomLong(); + TemporalAccessor accessor = formatter.parse(randomMillis + ""); + assertThat(randomMillis + "", is(formatter.format(accessor))); } private void assertThatSameDateTime(CompoundDateTimeFormatter formatter, CompoundDateTimeFormatter zonedFormatter, String value) { From 246c20c3d392d4d52b8603d0900a5013c385d00d Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Fri, 31 Aug 2018 16:58:07 +0200 Subject: [PATCH 3/3] minor test changes --- .../common/time/DateFormattersTests.java | 31 ++++++++++++------- 1 file changed, 20 insertions(+), 11 deletions(-) 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 8b062959cf9ef..f96674cf7a4a9 100644 --- a/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java +++ b/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java @@ -44,21 +44,30 @@ public void testEpochMilliParser() { assertThat(zonedFormatter.printer.getZone(), is(zoneId)); // test with negative and non negative values - assertThatSameDateTime(formatter, zonedFormatter, String.valueOf(randomNonNegativeLong() * -1)); - assertThatSameDateTime(formatter, zonedFormatter, String.valueOf(randomNonNegativeLong())); - assertThatSameDateTime(formatter, zonedFormatter, String.valueOf(0)); - assertThatSameDateTime(formatter, zonedFormatter, String.valueOf(-1)); - assertThatSameDateTime(formatter, zonedFormatter, String.valueOf(1)); + assertThatSameDateTime(formatter, zonedFormatter, randomNonNegativeLong() * -1); + assertThatSameDateTime(formatter, zonedFormatter, randomNonNegativeLong()); + assertThatSameDateTime(formatter, zonedFormatter, 0); + assertThatSameDateTime(formatter, zonedFormatter, -1); + assertThatSameDateTime(formatter, zonedFormatter, 1); // format() output should be equal as well - long randomMillis = randomLong(); - TemporalAccessor accessor = formatter.parse(randomMillis + ""); - assertThat(randomMillis + "", is(formatter.format(accessor))); + assertSameFormat(formatter, randomNonNegativeLong() * -1); + assertSameFormat(formatter, randomNonNegativeLong()); + assertSameFormat(formatter, 0); + assertSameFormat(formatter, -1); + assertSameFormat(formatter, 1); } - private void assertThatSameDateTime(CompoundDateTimeFormatter formatter, CompoundDateTimeFormatter zonedFormatter, String value) { - ZonedDateTime formatterZonedDateTime = DateFormatters.toZonedDateTime(formatter.parse(value)); - ZonedDateTime zonedFormatterZonedDateTime = DateFormatters.toZonedDateTime(zonedFormatter.parse(value)); + private void assertThatSameDateTime(CompoundDateTimeFormatter formatter, CompoundDateTimeFormatter zonedFormatter, long millis) { + String millisAsString = String.valueOf(millis); + ZonedDateTime formatterZonedDateTime = DateFormatters.toZonedDateTime(formatter.parse(millisAsString)); + ZonedDateTime zonedFormatterZonedDateTime = DateFormatters.toZonedDateTime(zonedFormatter.parse(millisAsString)); assertThat(formatterZonedDateTime.toInstant().toEpochMilli(), is(zonedFormatterZonedDateTime.toInstant().toEpochMilli())); } + + private void assertSameFormat(CompoundDateTimeFormatter formatter, long millis) { + String millisAsString = String.valueOf(millis); + TemporalAccessor accessor = formatter.parse(millisAsString); + assertThat(millisAsString, is(formatter.format(accessor))); + } }