issue#4055: Fix bug in OpenSearch date time auto-detection causing in…

…dex error JavaDateFormatter is introduced during Joda-Datetime deprecation, it supports composition of multiple parsers/formatter and return the first one succeeded to parse the input. DateTimeException from new Java DataTime APIs is not handled at all causing the bug in date time auto-detection in some corner cases. Signed-off-by: Pengxiaolong <[email protected]>
opensearch-project · Nov 9, 2023 · b65860f · b65860f
1 parent 8673fa9
commit b65860f
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 21 deletions.
diff --git a/server/src/main/java/org/opensearch/common/time/DateFormatter.java b/server/src/main/java/org/opensearch/common/time/DateFormatter.java
@@ -39,7 +39,6 @@
 import java.time.ZoneId;
 import java.time.ZoneOffset;
 import java.time.ZonedDateTime;
-import java.time.format.DateTimeParseException;
 import java.time.temporal.TemporalAccessor;
 import java.util.ArrayList;
 import java.util.List;
@@ -56,7 +55,7 @@ public interface DateFormatter {
     /**
      * Try to parse input to a java time TemporalAccessor
      * @param input                   An arbitrary string resembling the string representation of a date or time
-     * @throws DateTimeParseException If parsing fails, this exception will be thrown.
+     * @throws IllegalArgumentException If parsing fails, this exception will be thrown.
      *                                Note that it can contained suppressed exceptions when several formatters failed parse this value
      * @return                        The java time object containing the parsed input
      */

diff --git a/server/src/main/java/org/opensearch/common/time/JavaDateFormatter.java b/server/src/main/java/org/opensearch/common/time/JavaDateFormatter.java
@@ -36,6 +36,7 @@
 import org.opensearch.core.common.Strings;
 
 import java.text.ParsePosition;
+import java.time.DateTimeException;
 import java.time.ZoneId;
 import java.time.format.DateTimeFormatter;
 import java.time.format.DateTimeFormatterBuilder;
@@ -52,7 +53,6 @@
 import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
-import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.function.BiConsumer;
 import java.util.stream.Collectors;
 
@@ -285,27 +285,54 @@ public TemporalAccessor parse(String input) {
      * @throws DateTimeParseException when unable to parse with any parsers
      */
     private TemporalAccessor doParse(String input) {
-        if (parsers.size() > 1) {
-            Object object = null;
-            if (canCacheLastParsedFormatter && lastParsedformatter != null) {
-                ParsePosition pos = new ParsePosition(0);
-                object = lastParsedformatter.toFormat().parseObject(input, pos);
+        if (parsers.size() != 1) {
+            // Copy to local from volatile to avoid error in multi-threads,
+            // other threads may update lastParsedformatter
+            final DateTimeFormatter cachedFormatter = lastParsedformatter;
+            if (canCacheLastParsedFormatter && cachedFormatter != null) {
+                final ParsePosition pos = new ParsePosition(0);
+                Object object = tryParseObject(cachedFormatter, input, pos);
                 if (parsingSucceeded(object, input, pos)) {
                     return (TemporalAccessor) object;
                 }
             }
             for (DateTimeFormatter formatter : parsers) {
-                ParsePosition pos = new ParsePosition(0);
-                object = formatter.toFormat().parseObject(input, pos);
-                if (parsingSucceeded(object, input, pos)) {
-                    lastParsedformatter = formatter;
-                    return (TemporalAccessor) object;
+                if (formatter != cachedFormatter || !canCacheLastParsedFormatter) {
+                    final ParsePosition pos = new ParsePosition(0);
+                    Object object = tryParseObject(formatter, input, pos);
+                    if (parsingSucceeded(object, input, pos)) {
+                        if (canCacheLastParsedFormatter) {
+                            lastParsedformatter = formatter;
+                        }
+                        return (TemporalAccessor) object;
+                    }
                 }
             }
 
-            throw new DateTimeParseException("Failed to parse with all enclosed parsers", input, 0);
+            throw new DateTimeParseException("failed to parse date field [" + input + "] with format [" + format + "]", input, 0);
+        } else {
+            return parsers.get(0).parse(input);
+        }
+    }
+
+    /**
+     * Try to parse input with given formatter, return null if it fails to parse input.
+     * Because of JDK bug https://bugs.openjdk.org/browse/JDK-8319640,
+     * {@link java.text.Format#parseObject(String, ParsePosition)} may throw ${@link DateTimeException}
+     * rather than {@link DateTimeParseException}, causing early termination of the loop in {@link #doParse(String)},
+     * this method catches DateTimeException to avoid the exception is leaked to invoker.
+     *
+     * @param formatter formatter used to parse input.
+     * @param input input to parse
+     * @param pos initial ParsePosition
+     * @return parse result if there is {@link DateTimeException}
+     */
+    private Object tryParseObject(final DateTimeFormatter formatter, final String input, final ParsePosition pos) {
+        try {
+            return formatter.toFormat().parseObject(input, pos);
+        } catch (final DateTimeException ex) {
+            return null;
         }
-        return this.parsers.get(0).parse(input);
     }
 
     private boolean parsingSucceeded(Object object, String input, ParsePosition pos) {
@@ -318,9 +345,7 @@ public DateFormatter withZone(ZoneId zoneId) {
         if (zoneId.equals(zone())) {
             return this;
         }
-        List<DateTimeFormatter> parsers = new CopyOnWriteArrayList<>(
-            this.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))
@@ -334,9 +359,7 @@ public DateFormatter withLocale(Locale locale) {
         if (locale.equals(locale())) {
             return this;
         }
-        List<DateTimeFormatter> parsers = new CopyOnWriteArrayList<>(
-            this.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))

diff --git a/server/src/test/java/org/opensearch/common/time/DateFormattersTests.java b/server/src/test/java/org/opensearch/common/time/DateFormattersTests.java
@@ -683,4 +683,15 @@ public void testCamelCaseDeprecation() {
             }
         }
     }
+
+    public void testToVerifyFixForDateTimeException() {
+        DateFormatter formatter = DateFormatter.forPattern("strict_date_optional_time||epoch_millis");
+        assertEquals(1522431028842L, Instant.from(formatter.parse("2018-03-30T17:30:28.842Z")).toEpochMilli());
+        assertEquals(1522431028842L, Instant.from(formatter.parse("1522431028842")).toEpochMilli());
+        assertThrows(IllegalArgumentException.class, () -> formatter.parse(""));
+        // text causes DateTimeException from DateTimeFormatter#parseObject for the pattern in this test,
+        // and causes parse failure.
+        assertThrows(IllegalArgumentException.class, () -> formatter.parse("2018-03-30T17-30-28.842Z"));
+    }
+
 }