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

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);
     }
 }

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<DateFormatter> formatters) {
-        assert formatters.size() > 0;
-
-        List<DateTimeFormatter> 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);
 

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<DateTimeFormatter> parsers;
-    private final DateTimeFormatter roundupParser;
+    private final JavaDateFormatter roundupParser;
 
-    private JavaDateFormatter(String format, DateTimeFormatter printer, DateTimeFormatter roundupParser, List<DateTimeFormatter> parsers) {
-        this.format = format;
-        this.printer = printer;
-        this.roundupParser = roundupParser;
-        this.parsers = parsers;
+    static class RoundUpFormatter extends JavaDateFormatter{
+
+        RoundUpFormatter(String format, List<DateTimeFormatter> roundUpParsers) {
+            super(format,  firstFrom(roundUpParsers),null, roundUpParsers);
+        }
+
+        private static DateTimeFormatter firstFrom(List<DateTimeFormatter> 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<DateTimeFormatterBuilder> roundupParserConsumer,
+    // subclasses override roundUpParser
+    JavaDateFormatter(String format,
+                      DateTimeFormatter printer,
+                      Consumer<DateTimeFormatterBuilder> 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<DateTimeFormatter> roundUp = createRoundUpParser(format, roundupParserConsumer);
+        this.roundupParser = new RoundUpFormatter(format, roundUp) ;
+    }
 
-        DateTimeFormatterBuilder builder = new DateTimeFormatterBuilder();
+    private List<DateTimeFormatter> createRoundUpParser(String format,
+                                                        Consumer<DateTimeFormatterBuilder> 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<DateFormatter> formatters) {
+        assert formatters.size() > 0;
+
+        List<DateTimeFormatter> parsers = new ArrayList<>(formatters.size());
+        List<DateTimeFormatter> 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<DateTimeFormatter> roundUpParsers,
+                               List<DateTimeFormatter> 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<DateTimeFormatter> parsers = this.parsers.stream().map(p -> p.withZone(zoneId)).collect(Collectors.toList());
+        List<DateTimeFormatter> 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<DateTimeFormatter> parsers = this.parsers.stream().map(p -> p.withLocale(locale)).collect(Collectors.toList());
+        List<DateTimeFormatter> roundUpParsers = this.roundupParser.getParsers()
+                                                                   .stream()
+                                                                   .map(p -> p.withLocale(locale))
+                                                                   .collect(Collectors.toList());
+        return new JavaDateFormatter(format, printer.withLocale(locale), roundUpParsers, parsers);
     }
 
     @Override

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<String,TemporalAccessor> 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;

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);
+    }
 }

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");