From b50164f37536ab1eb0eab64d7d6b803d2363a680 Mon Sep 17 00:00:00 2001 From: Przemyslaw Gomulka Date: Fri, 27 Sep 2019 15:49:58 +0200 Subject: [PATCH] Support optional parsers in any order with DateMathParser and roundup (#46654) Currently DateMathParser with roundUp = true is relying on the DateFormatter build with combined optional sub parsers with defaulted fields (depending on the formatter). That means that for yyyy-MM-dd'T'HH:mm:ss||yyyy-MM-dd'T'HH:mm:ss.SSS Java.time implementation expects optional parsers in order from most specific to least specific (reverse in the example above). It is causing a problem because the first parsing succeeds but does not consume the full input. The second parser should be used. We can work around this with keeping a list of RoundUpParsers and iterate over them choosing the one that parsed full input. The same approach we used for regular (non date math) in relates #40100 The jdk is not considering this to be a bug https://bugs.openjdk.java.net/browse/JDK-8188771 Those below will expect this change first relates #46242 relates #45284 --- .../common/time/DateFormatter.java | 2 +- .../common/time/DateFormatters.java | 25 +----- .../common/time/JavaDateFormatter.java | 90 +++++++++++++++---- .../common/time/JavaDateMathParser.java | 14 ++- .../joda/JavaJodaTimeDuellingTests.java | 39 ++++++++ .../common/joda/JodaDateMathParserTests.java | 14 +++ .../common/time/DateFormattersTests.java | 15 ++-- .../common/time/JavaDateMathParserTests.java | 7 +- 8 files changed, 147 insertions(+), 59 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/time/DateFormatter.java b/server/src/main/java/org/elasticsearch/common/time/DateFormatter.java index bf7999067b05a..3798cf1744bda 100644 --- a/server/src/main/java/org/elasticsearch/common/time/DateFormatter.java +++ b/server/src/main/java/org/elasticsearch/common/time/DateFormatter.java @@ -149,6 +149,6 @@ static DateFormatter forPattern(String input) { return formatters.get(0); } - return DateFormatters.merge(input, formatters); + return JavaDateFormatter.combined(input, formatters); } } 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 8cb71866ad252..d3bf066e18481 100644 --- a/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java +++ b/server/src/main/java/org/elasticsearch/common/time/DateFormatters.java @@ -40,8 +40,6 @@ import java.time.temporal.TemporalAdjusters; import java.time.temporal.TemporalQueries; import java.time.temporal.WeekFields; -import java.util.ArrayList; -import java.util.List; import static java.time.temporal.ChronoField.DAY_OF_MONTH; import static java.time.temporal.ChronoField.DAY_OF_WEEK; @@ -1045,7 +1043,7 @@ public class DateFormatters { new DateTimeFormatterBuilder().appendValue(WeekFields.ISO.weekBasedYear()).toFormatter(IsoLocale.ROOT)); /* - * Returns a formatter for a four digit weekyear. (uuuu) + * Returns a formatter for a four digit year. (uuuu) */ private static final DateFormatter YEAR = new JavaDateFormatter("year", new DateTimeFormatterBuilder().appendValue(ChronoField.YEAR).toFormatter(IsoLocale.ROOT)); @@ -1440,7 +1438,7 @@ public class DateFormatters { .appendValue(WeekFields.ISO.dayOfWeek()) .toFormatter(IsoLocale.ROOT) ); - + ///////////////////////////////////////// // @@ -1628,26 +1626,7 @@ static DateFormatter forPattern(String input) { } } - static JavaDateFormatter merge(String pattern, List formatters) { - assert formatters.size() > 0; - - List dateTimeFormatters = new ArrayList<>(formatters.size()); - DateTimeFormatterBuilder roundupBuilder = new DateTimeFormatterBuilder(); - DateTimeFormatter printer = null; - for (DateFormatter formatter : formatters) { - assert formatter instanceof JavaDateFormatter; - JavaDateFormatter javaDateFormatter = (JavaDateFormatter) formatter; - if (printer == null) { - printer = javaDateFormatter.getPrinter(); - } - dateTimeFormatters.addAll(javaDateFormatter.getParsers()); - roundupBuilder.appendOptional(javaDateFormatter.getRoundupParser()); - } - DateTimeFormatter roundUpParser = roundupBuilder.toFormatter(IsoLocale.ROOT); - return new JavaDateFormatter(pattern, printer, builder -> builder.append(roundUpParser), - dateTimeFormatters.toArray(new DateTimeFormatter[0])); - } private static final LocalDate LOCALDATE_EPOCH = LocalDate.of(1970, 1, 1); diff --git a/server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java b/server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java index 3513600582278..e1ac6db119609 100644 --- a/server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java +++ b/server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java @@ -29,6 +29,7 @@ import java.time.temporal.ChronoField; import java.time.temporal.TemporalAccessor; import java.time.temporal.TemporalField; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -57,19 +58,33 @@ class JavaDateFormatter implements DateFormatter { private final String format; private final DateTimeFormatter printer; private final List parsers; - private final DateTimeFormatter roundupParser; + private final JavaDateFormatter roundupParser; - private JavaDateFormatter(String format, DateTimeFormatter printer, DateTimeFormatter roundupParser, List parsers) { - this.format = format; - this.printer = printer; - this.roundupParser = roundupParser; - this.parsers = parsers; + static class RoundUpFormatter extends JavaDateFormatter{ + + RoundUpFormatter(String format, List roundUpParsers) { + super(format, firstFrom(roundUpParsers),null, roundUpParsers); + } + + private static DateTimeFormatter firstFrom(List roundUpParsers) { + return roundUpParsers.get(0); + } + + @Override + JavaDateFormatter getRoundupParser() { + throw new UnsupportedOperationException("RoundUpFormatter does not have another roundUpFormatter"); + } } + + // named formatters use default roundUpParser JavaDateFormatter(String format, DateTimeFormatter printer, DateTimeFormatter... parsers) { this(format, printer, builder -> ROUND_UP_BASE_FIELDS.forEach(builder::parseDefaulting), parsers); } - JavaDateFormatter(String format, DateTimeFormatter printer, Consumer roundupParserConsumer, + // subclasses override roundUpParser + JavaDateFormatter(String format, + DateTimeFormatter printer, + Consumer roundupParserConsumer, DateTimeFormatter... parsers) { if (printer == null) { throw new IllegalArgumentException("printer may not be null"); @@ -90,20 +105,51 @@ private JavaDateFormatter(String format, DateTimeFormatter printer, DateTimeForm } else { this.parsers = Arrays.asList(parsers); } + //this is when the RoundUp Formatter is created. In further merges (with ||) it will only append this one to a list. + List roundUp = createRoundUpParser(format, roundupParserConsumer); + this.roundupParser = new RoundUpFormatter(format, roundUp) ; + } - DateTimeFormatterBuilder builder = new DateTimeFormatterBuilder(); + private List createRoundUpParser(String format, + Consumer roundupParserConsumer) { if (format.contains("||") == false) { + DateTimeFormatterBuilder builder = new DateTimeFormatterBuilder(); builder.append(this.parsers.get(0)); + roundupParserConsumer.accept(builder); + return Arrays.asList(builder.toFormatter(locale())); } - roundupParserConsumer.accept(builder); - DateTimeFormatter roundupFormatter = builder.toFormatter(locale()); - if (printer.getZone() != null) { - roundupFormatter = roundupFormatter.withZone(zone()); + return null; + } + + public static DateFormatter combined(String input, List formatters) { + assert formatters.size() > 0; + + List parsers = new ArrayList<>(formatters.size()); + List roundUpParsers = new ArrayList<>(formatters.size()); + + DateTimeFormatter printer = null; + for (DateFormatter formatter : formatters) { + assert formatter instanceof JavaDateFormatter; + JavaDateFormatter javaDateFormatter = (JavaDateFormatter) formatter; + if (printer == null) { + printer = javaDateFormatter.getPrinter(); + } + parsers.addAll(javaDateFormatter.getParsers()); + roundUpParsers.addAll(javaDateFormatter.getRoundupParser().getParsers()); } - this.roundupParser = roundupFormatter; + + return new JavaDateFormatter(input, printer, roundUpParsers, parsers); + } + + private JavaDateFormatter(String format, DateTimeFormatter printer, List roundUpParsers, + List parsers) { + this.format = format; + this.printer = printer; + this.roundupParser = roundUpParsers != null ? new RoundUpFormatter(format, roundUpParsers ) : null; + this.parsers = parsers; } - DateTimeFormatter getRoundupParser() { + JavaDateFormatter getRoundupParser() { return roundupParser; } @@ -162,8 +208,12 @@ public DateFormatter withZone(ZoneId zoneId) { if (zoneId.equals(zone())) { return this; } - return new JavaDateFormatter(format, printer.withZone(zoneId), getRoundupParser().withZone(zoneId), - parsers.stream().map(p -> p.withZone(zoneId)).collect(Collectors.toList())); + List parsers = this.parsers.stream().map(p -> p.withZone(zoneId)).collect(Collectors.toList()); + List roundUpParsers = this.roundupParser.getParsers() + .stream() + .map(p -> p.withZone(zoneId)) + .collect(Collectors.toList()); + return new JavaDateFormatter(format, printer.withZone(zoneId), roundUpParsers, parsers); } @Override @@ -172,8 +222,12 @@ public DateFormatter withLocale(Locale locale) { if (locale.equals(locale())) { return this; } - return new JavaDateFormatter(format, printer.withLocale(locale), getRoundupParser().withLocale(locale), - parsers.stream().map(p -> p.withLocale(locale)).collect(Collectors.toList())); + List parsers = this.parsers.stream().map(p -> p.withLocale(locale)).collect(Collectors.toList()); + List roundUpParsers = this.roundupParser.getParsers() + .stream() + .map(p -> p.withLocale(locale)) + .collect(Collectors.toList()); + return new JavaDateFormatter(format, printer.withLocale(locale), roundUpParsers, parsers); } @Override diff --git a/server/src/main/java/org/elasticsearch/common/time/JavaDateMathParser.java b/server/src/main/java/org/elasticsearch/common/time/JavaDateMathParser.java index 78d4f10d87cbf..b3fd8fa0f277e 100644 --- a/server/src/main/java/org/elasticsearch/common/time/JavaDateMathParser.java +++ b/server/src/main/java/org/elasticsearch/common/time/JavaDateMathParser.java @@ -28,14 +28,12 @@ import java.time.ZoneId; import java.time.ZoneOffset; import java.time.ZonedDateTime; -import java.time.format.DateTimeFormatter; import java.time.format.DateTimeParseException; import java.time.temporal.ChronoField; import java.time.temporal.TemporalAccessor; import java.time.temporal.TemporalAdjusters; import java.time.temporal.TemporalQueries; import java.util.Objects; -import java.util.function.Function; import java.util.function.LongSupplier; /** @@ -48,14 +46,14 @@ public class JavaDateMathParser implements DateMathParser { private final JavaDateFormatter formatter; - private final DateTimeFormatter roundUpFormatter; private final String format; + private final JavaDateFormatter roundupParser; - JavaDateMathParser(String format, JavaDateFormatter formatter, DateTimeFormatter roundUpFormatter) { + JavaDateMathParser(String format, JavaDateFormatter formatter, JavaDateFormatter roundupParser) { this.format = format; + this.roundupParser = roundupParser; Objects.requireNonNull(formatter); this.formatter = formatter; - this.roundUpFormatter = roundUpFormatter; } @Override @@ -217,12 +215,12 @@ private Instant parseDateTime(String value, ZoneId timeZone, boolean roundUpIfNo throw new ElasticsearchParseException("cannot parse empty date"); } - Function formatter = roundUpIfNoTime ? this.roundUpFormatter::parse : this.formatter::parse; + DateFormatter formatter = roundUpIfNoTime ? this.roundupParser : this.formatter; try { if (timeZone == null) { - return DateFormatters.from(formatter.apply(value)).toInstant(); + return DateFormatters.from(formatter.parse(value)).toInstant(); } else { - TemporalAccessor accessor = formatter.apply(value); + TemporalAccessor accessor = formatter.parse(value); ZoneId zoneId = TemporalQueries.zone().queryFrom(accessor); if (zoneId != null) { timeZone = zoneId; 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 99121fdc835d3..a4f5ba637c764 100644 --- a/server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java +++ b/server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java @@ -19,15 +19,18 @@ package org.elasticsearch.common.joda; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.bootstrap.JavaVersion; import org.elasticsearch.common.time.DateFormatter; import org.elasticsearch.common.time.DateFormatters; +import org.elasticsearch.common.time.DateMathParser; import org.elasticsearch.test.ESTestCase; import org.joda.time.DateTime; import org.joda.time.DateTimeZone; import org.joda.time.format.ISODateTimeFormat; import java.time.LocalDateTime; +import java.time.ZoneId; import java.time.ZoneOffset; import java.time.ZonedDateTime; import java.time.format.DateTimeFormatter; @@ -44,6 +47,35 @@ protected boolean enableWarningsCheck() { return false; } + public void testCompositeDateMathParsing(){ + //in all these examples the second pattern will be used + assertDateMathEquals("2014-06-06T12:01:02.123", "yyyy-MM-dd'T'HH:mm:ss||yyyy-MM-dd'T'HH:mm:ss.SSS"); + assertDateMathEquals("2014-06-06T12:01:02.123", "strictDateTimeNoMillis||yyyy-MM-dd'T'HH:mm:ss.SSS"); + assertDateMathEquals("2014-06-06T12:01:02.123", "yyyy-MM-dd'T'HH:mm:ss+HH:MM||yyyy-MM-dd'T'HH:mm:ss.SSS"); + } + + public void testExceptionWhenCompositeParsingFailsDateMath(){ + //both parsing failures should contain pattern and input text in exception + //both patterns fail parsing the input text due to only 2 digits of millis. Hence full text was not parsed. + String pattern = "yyyy-MM-dd'T'HH:mm:ss||yyyy-MM-dd'T'HH:mm:ss.SS"; + String text = "2014-06-06T12:01:02.123"; + ElasticsearchParseException e1 = expectThrows(ElasticsearchParseException.class, + () -> dateMathToMillis(text, DateFormatter.forPattern(pattern))); + assertThat(e1.getMessage(), containsString(pattern)); + assertThat(e1.getMessage(), containsString(text)); + + ElasticsearchParseException e2 = expectThrows(ElasticsearchParseException.class, + () -> dateMathToMillis(text, Joda.forPattern(pattern))); + assertThat(e2.getMessage(), containsString(pattern)); + assertThat(e2.getMessage(), containsString(text)); + } + + private long dateMathToMillis(String text, DateFormatter dateFormatter) { + DateFormatter javaFormatter = dateFormatter.withLocale(randomLocale(random())); + DateMathParser javaDateMath = javaFormatter.toDateMathParser(); + return javaDateMath.parse(text, () -> 0, true, (ZoneId) null).toEpochMilli(); + } + public void testDayOfWeek() { //7 (ok joda) vs 1 (java by default) but 7 with customized org.elasticsearch.common.time.IsoLocale.ISO8601 ZonedDateTime now = LocalDateTime.of(2009,11,15,1,32,8,328402) @@ -851,4 +883,11 @@ private void assertJavaTimeParseException(String input, String format) { assertThat(e.getMessage(), containsString(input)); assertThat(e.getMessage(), containsString(format)); } + + private void assertDateMathEquals(String text, String pattern) { + long gotMillisJava = dateMathToMillis(text, DateFormatter.forPattern(pattern)); + long gotMillisJoda = dateMathToMillis(text, Joda.forPattern(pattern)); + + assertEquals(gotMillisJoda, gotMillisJava); + } } diff --git a/server/src/test/java/org/elasticsearch/common/joda/JodaDateMathParserTests.java b/server/src/test/java/org/elasticsearch/common/joda/JodaDateMathParserTests.java index f6382b92343ec..d1a90328dfb65 100644 --- a/server/src/test/java/org/elasticsearch/common/joda/JodaDateMathParserTests.java +++ b/server/src/test/java/org/elasticsearch/common/joda/JodaDateMathParserTests.java @@ -27,6 +27,7 @@ import java.time.Instant; import java.time.ZoneId; +import java.time.ZoneOffset; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.LongSupplier; @@ -59,6 +60,19 @@ void assertDateEquals(long gotMillis, String original, String expected) { } } + public void testOverridingLocaleOrZoneAndCompositeRoundUpParser() { + //the pattern has to be composite and the match should not be on the first one + DateFormatter formatter = Joda.forPattern("date||epoch_millis").withLocale(randomLocale(random())); + DateMathParser parser = formatter.toDateMathParser(); + long gotMillis = parser.parse("297276785531", () -> 0, true, (ZoneId) null).toEpochMilli(); + assertDateEquals(gotMillis, "297276785531", "297276785531"); + + formatter = Joda.forPattern("date||epoch_millis").withZone(ZoneOffset.UTC); + parser = formatter.toDateMathParser(); + gotMillis = parser.parse("297276785531", () -> 0, true, (ZoneId) null).toEpochMilli(); + assertDateEquals(gotMillis, "297276785531", "297276785531"); + } + public void testBasicDates() { assertDateMathEquals("2014", "2014-01-01T00:00:00.000"); assertDateMathEquals("2014-05", "2014-05-01T00:00:00.000"); 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 1f18fb1655d44..e478b6dfcc499 100644 --- a/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java +++ b/server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java @@ -26,7 +26,6 @@ import java.time.ZoneId; import java.time.ZoneOffset; import java.time.ZonedDateTime; -import java.time.format.DateTimeFormatter; import java.time.temporal.ChronoField; import java.time.temporal.TemporalAccessor; import java.util.Locale; @@ -253,7 +252,7 @@ public void testIso8601Parsing() { public void testRoundupFormatterWithEpochDates() { assertRoundupFormatter("epoch_millis", "1234567890", 1234567890L); // also check nanos of the epoch_millis formatter if it is rounded up to the nano second - DateTimeFormatter roundUpFormatter = ((JavaDateFormatter) DateFormatter.forPattern("8epoch_millis")).getRoundupParser(); + JavaDateFormatter roundUpFormatter = ((JavaDateFormatter) DateFormatter.forPattern("8epoch_millis")).getRoundupParser(); Instant epochMilliInstant = DateFormatters.from(roundUpFormatter.parse("1234567890")).toInstant(); assertThat(epochMilliInstant.getLong(ChronoField.NANO_OF_SECOND), is(890_999_999L)); @@ -266,7 +265,7 @@ public void testRoundupFormatterWithEpochDates() { assertRoundupFormatter("epoch_second", "1234567890", 1234567890999L); // also check nanos of the epoch_millis formatter if it is rounded up to the nano second - DateTimeFormatter epochSecondRoundupParser = ((JavaDateFormatter) DateFormatter.forPattern("8epoch_second")).getRoundupParser(); + JavaDateFormatter epochSecondRoundupParser = ((JavaDateFormatter) DateFormatter.forPattern("8epoch_second")).getRoundupParser(); Instant epochSecondInstant = DateFormatters.from(epochSecondRoundupParser.parse("1234567890")).toInstant(); assertThat(epochSecondInstant.getLong(ChronoField.NANO_OF_SECOND), is(999_999_999L)); @@ -280,7 +279,7 @@ public void testRoundupFormatterWithEpochDates() { private void assertRoundupFormatter(String format, String input, long expectedMilliSeconds) { JavaDateFormatter dateFormatter = (JavaDateFormatter) DateFormatter.forPattern(format); dateFormatter.parse(input); - DateTimeFormatter roundUpFormatter = dateFormatter.getRoundupParser(); + JavaDateFormatter roundUpFormatter = dateFormatter.getRoundupParser(); long millis = DateFormatters.from(roundUpFormatter.parse(input)).toInstant().toEpochMilli(); assertThat(millis, is(expectedMilliSeconds)); } @@ -290,8 +289,8 @@ public void testRoundupFormatterZone() { String format = randomFrom("epoch_second", "epoch_millis", "strict_date_optional_time", "uuuu-MM-dd'T'HH:mm:ss.SSS", "strict_date_optional_time||date_optional_time"); JavaDateFormatter formatter = (JavaDateFormatter) DateFormatter.forPattern(format).withZone(zoneId); - DateTimeFormatter roundUpFormatter = formatter.getRoundupParser(); - assertThat(roundUpFormatter.getZone(), is(zoneId)); + JavaDateFormatter roundUpFormatter = formatter.getRoundupParser(); + assertThat(roundUpFormatter.zone(), is(zoneId)); assertThat(formatter.zone(), is(zoneId)); } @@ -300,8 +299,8 @@ public void testRoundupFormatterLocale() { String format = randomFrom("epoch_second", "epoch_millis", "strict_date_optional_time", "uuuu-MM-dd'T'HH:mm:ss.SSS", "strict_date_optional_time||date_optional_time"); JavaDateFormatter formatter = (JavaDateFormatter) DateFormatter.forPattern(format).withLocale(locale); - DateTimeFormatter roundupParser = formatter.getRoundupParser(); - assertThat(roundupParser.getLocale(), is(locale)); + JavaDateFormatter roundupParser = formatter.getRoundupParser(); + assertThat(roundupParser.locale(), is(locale)); assertThat(formatter.locale(), is(locale)); } diff --git a/server/src/test/java/org/elasticsearch/common/time/JavaDateMathParserTests.java b/server/src/test/java/org/elasticsearch/common/time/JavaDateMathParserTests.java index 2fb524608968d..c50ec185dc5d5 100644 --- a/server/src/test/java/org/elasticsearch/common/time/JavaDateMathParserTests.java +++ b/server/src/test/java/org/elasticsearch/common/time/JavaDateMathParserTests.java @@ -44,7 +44,7 @@ public void testOverridingLocaleOrZoneAndCompositeRoundUpParser() { long gotMillis = parser.parse("297276785531", () -> 0, true, (ZoneId) null).toEpochMilli(); assertDateEquals(gotMillis, "297276785531", "297276785531"); - formatter = DateFormatter.forPattern("date||epoch_millis").withZone(randomZone()); + formatter = DateFormatter.forPattern("date||epoch_millis").withZone(ZoneOffset.UTC); parser = formatter.toDateMathParser(); gotMillis = parser.parse("297276785531", () -> 0, true, (ZoneId) null).toEpochMilli(); assertDateEquals(gotMillis, "297276785531", "297276785531"); @@ -301,6 +301,11 @@ private void assertDateMathEquals(String toTest, String expected) { } private void assertDateMathEquals(String toTest, String expected, final long now, boolean roundUp, ZoneId timeZone) { + assertDateMathEquals(parser, toTest, expected, now, roundUp, timeZone); + } + + private void assertDateMathEquals(DateMathParser parser, String toTest, String expected, final long now, + boolean roundUp, ZoneId timeZone) { long gotMillis = parser.parse(toTest, () -> now, roundUp, timeZone).toEpochMilli(); assertDateEquals(gotMillis, toTest, expected); }