Skip to content

Commit d9a7bce

Browse files
pgomulkapgomulka
authored
Support optional parsers in any order with DateMathParser Backport(46654) (#47217)
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 backport #46654
1 parent 7eed3e5 commit d9a7bce

File tree

8 files changed

+146
-61
lines changed

8 files changed

+146
-61
lines changed

‎server/src/main/java/org/elasticsearch/common/time/DateFormatter.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ static DateFormatter forPattern(String input) {
147147
return formatters.get(0);
148148
}
149149

150-
return DateFormatters.merge(input, formatters);
150+
return JavaDateFormatter.combined(input, formatters);
151151
}
152152

153153
static List<String> splitCombinedPatterns(String input) {

‎server/src/main/java/org/elasticsearch/common/time/DateFormatters.java‎

Lines changed: 1 addition & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,6 @@
4040
import java.time.temporal.TemporalAdjusters;
4141
import java.time.temporal.TemporalQueries;
4242
import java.time.temporal.WeekFields;
43-
import java.util.ArrayList;
44-
import java.util.List;
4543
import java.util.Locale;
4644

4745
import static java.time.temporal.ChronoField.DAY_OF_MONTH;
@@ -1033,7 +1031,7 @@ public class DateFormatters {
10331031
new DateTimeFormatterBuilder().appendValue(WeekFields.ISO.weekBasedYear()).toFormatter(Locale.ROOT));
10341032

10351033
/*
1036-
* Returns a formatter for a four digit weekyear. (uuuu)
1034+
* Returns a formatter for a four digit year. (uuuu)
10371035
*/
10381036
private static final DateFormatter YEAR = new JavaDateFormatter("year",
10391037
new DateTimeFormatterBuilder().appendValue(ChronoField.YEAR).toFormatter(Locale.ROOT));
@@ -1605,27 +1603,6 @@ static DateFormatter forPattern(String input) {
16051603
}
16061604
}
16071605

1608-
static JavaDateFormatter merge(String pattern, List<DateFormatter> formatters) {
1609-
assert formatters.size() > 0;
1610-
1611-
List<DateTimeFormatter> dateTimeFormatters = new ArrayList<>(formatters.size());
1612-
DateTimeFormatterBuilder roundupBuilder = new DateTimeFormatterBuilder();
1613-
DateTimeFormatter printer = null;
1614-
for (DateFormatter formatter : formatters) {
1615-
assert formatter instanceof JavaDateFormatter;
1616-
JavaDateFormatter javaDateFormatter = (JavaDateFormatter) formatter;
1617-
if (printer == null) {
1618-
printer = javaDateFormatter.getPrinter();
1619-
}
1620-
dateTimeFormatters.addAll(javaDateFormatter.getParsers());
1621-
roundupBuilder.appendOptional(javaDateFormatter.getRoundupParser());
1622-
}
1623-
DateTimeFormatter roundUpParser = roundupBuilder.toFormatter(Locale.ROOT);
1624-
1625-
return new JavaDateFormatter(pattern, printer, builder -> builder.append(roundUpParser),
1626-
dateTimeFormatters.toArray(new DateTimeFormatter[0]));
1627-
}
1628-
16291606
private static final LocalDate LOCALDATE_EPOCH = LocalDate.of(1970, 1, 1);
16301607

16311608
/**

‎server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java‎

Lines changed: 72 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import java.time.temporal.ChronoField;
3030
import java.time.temporal.TemporalAccessor;
3131
import java.time.temporal.TemporalField;
32+
import java.util.ArrayList;
3233
import java.util.Arrays;
3334
import java.util.Collection;
3435
import java.util.Collections;
@@ -57,19 +58,33 @@ class JavaDateFormatter implements DateFormatter {
5758
private final String format;
5859
private final DateTimeFormatter printer;
5960
private final List<DateTimeFormatter> parsers;
60-
private final DateTimeFormatter roundupParser;
61+
private final JavaDateFormatter roundupParser;
6162

62-
private JavaDateFormatter(String format, DateTimeFormatter printer, DateTimeFormatter roundupParser, List<DateTimeFormatter> parsers) {
63-
this.format = format;
64-
this.printer = printer;
65-
this.roundupParser = roundupParser;
66-
this.parsers = parsers;
63+
static class RoundUpFormatter extends JavaDateFormatter{
64+
65+
RoundUpFormatter(String format, List<DateTimeFormatter> roundUpParsers) {
66+
super(format, firstFrom(roundUpParsers),null, roundUpParsers);
67+
}
68+
69+
private static DateTimeFormatter firstFrom(List<DateTimeFormatter> roundUpParsers) {
70+
return roundUpParsers.get(0);
71+
}
72+
73+
@Override
74+
JavaDateFormatter getRoundupParser() {
75+
throw new UnsupportedOperationException("RoundUpFormatter does not have another roundUpFormatter");
76+
}
6777
}
78+
79+
// named formatters use default roundUpParser
6880
JavaDateFormatter(String format, DateTimeFormatter printer, DateTimeFormatter... parsers) {
6981
this(format, printer, builder -> ROUND_UP_BASE_FIELDS.forEach(builder::parseDefaulting), parsers);
7082
}
7183

72-
JavaDateFormatter(String format, DateTimeFormatter printer, Consumer<DateTimeFormatterBuilder> roundupParserConsumer,
84+
// subclasses override roundUpParser
85+
JavaDateFormatter(String format,
86+
DateTimeFormatter printer,
87+
Consumer<DateTimeFormatterBuilder> roundupParserConsumer,
7388
DateTimeFormatter... parsers) {
7489
if (printer == null) {
7590
throw new IllegalArgumentException("printer may not be null");
@@ -90,20 +105,51 @@ private JavaDateFormatter(String format, DateTimeFormatter printer, DateTimeForm
90105
} else {
91106
this.parsers = Arrays.asList(parsers);
92107
}
108+
//this is when the RoundUp Formatter is created. In further merges (with ||) it will only append this one to a list.
109+
List<DateTimeFormatter> roundUp = createRoundUpParser(format, roundupParserConsumer);
110+
this.roundupParser = new RoundUpFormatter(format, roundUp) ;
111+
}
93112

94-
DateTimeFormatterBuilder builder = new DateTimeFormatterBuilder();
113+
private List<DateTimeFormatter> createRoundUpParser(String format,
114+
Consumer<DateTimeFormatterBuilder> roundupParserConsumer) {
95115
if (format.contains("||") == false) {
116+
DateTimeFormatterBuilder builder = new DateTimeFormatterBuilder();
96117
builder.append(this.parsers.get(0));
118+
roundupParserConsumer.accept(builder);
119+
return Arrays.asList(builder.toFormatter(locale()));
97120
}
98-
roundupParserConsumer.accept(builder);
99-
DateTimeFormatter roundupFormatter = builder.toFormatter(locale());
100-
if (printer.getZone() != null) {
101-
roundupFormatter = roundupFormatter.withZone(zone());
121+
return null;
122+
}
123+
124+
public static DateFormatter combined(String input, List<DateFormatter> formatters) {
125+
assert formatters.size() > 0;
126+
127+
List<DateTimeFormatter> parsers = new ArrayList<>(formatters.size());
128+
List<DateTimeFormatter> roundUpParsers = new ArrayList<>(formatters.size());
129+
130+
DateTimeFormatter printer = null;
131+
for (DateFormatter formatter : formatters) {
132+
assert formatter instanceof JavaDateFormatter;
133+
JavaDateFormatter javaDateFormatter = (JavaDateFormatter) formatter;
134+
if (printer == null) {
135+
printer = javaDateFormatter.getPrinter();
136+
}
137+
parsers.addAll(javaDateFormatter.getParsers());
138+
roundUpParsers.addAll(javaDateFormatter.getRoundupParser().getParsers());
102139
}
103-
this.roundupParser = roundupFormatter;
140+
141+
return new JavaDateFormatter(input, printer, roundUpParsers, parsers);
142+
}
143+
144+
private JavaDateFormatter(String format, DateTimeFormatter printer, List<DateTimeFormatter> roundUpParsers,
145+
List<DateTimeFormatter> parsers) {
146+
this.format = format;
147+
this.printer = printer;
148+
this.roundupParser = roundUpParsers != null ? new RoundUpFormatter(format, roundUpParsers ) : null;
149+
this.parsers = parsers;
104150
}
105151

106-
DateTimeFormatter getRoundupParser() {
152+
JavaDateFormatter getRoundupParser() {
107153
return roundupParser;
108154
}
109155

@@ -162,8 +208,12 @@ public DateFormatter withZone(ZoneId zoneId) {
162208
if (zoneId.equals(zone())) {
163209
return this;
164210
}
165-
return new JavaDateFormatter(format, printer.withZone(zoneId), getRoundupParser().withZone(zoneId),
166-
parsers.stream().map(p -> p.withZone(zoneId)).collect(Collectors.toList()));
211+
List<DateTimeFormatter> parsers = this.parsers.stream().map(p -> p.withZone(zoneId)).collect(Collectors.toList());
212+
List<DateTimeFormatter> roundUpParsers = this.roundupParser.getParsers()
213+
.stream()
214+
.map(p -> p.withZone(zoneId))
215+
.collect(Collectors.toList());
216+
return new JavaDateFormatter(format, printer.withZone(zoneId), roundUpParsers, parsers);
167217
}
168218

169219
@Override
@@ -172,8 +222,12 @@ public DateFormatter withLocale(Locale locale) {
172222
if (locale.equals(locale())) {
173223
return this;
174224
}
175-
return new JavaDateFormatter(format, printer.withLocale(locale), getRoundupParser().withLocale(locale),
176-
parsers.stream().map(p -> p.withLocale(locale)).collect(Collectors.toList()));
225+
List<DateTimeFormatter> parsers = this.parsers.stream().map(p -> p.withLocale(locale)).collect(Collectors.toList());
226+
List<DateTimeFormatter> roundUpParsers = this.roundupParser.getParsers()
227+
.stream()
228+
.map(p -> p.withLocale(locale))
229+
.collect(Collectors.toList());
230+
return new JavaDateFormatter(format, printer.withLocale(locale), roundUpParsers, parsers);
177231
}
178232

179233
@Override

‎server/src/main/java/org/elasticsearch/common/time/JavaDateMathParser.java‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,14 +28,12 @@
2828
import java.time.ZoneId;
2929
import java.time.ZoneOffset;
3030
import java.time.ZonedDateTime;
31-
import java.time.format.DateTimeFormatter;
3231
import java.time.format.DateTimeParseException;
3332
import java.time.temporal.ChronoField;
3433
import java.time.temporal.TemporalAccessor;
3534
import java.time.temporal.TemporalAdjusters;
3635
import java.time.temporal.TemporalQueries;
3736
import java.util.Objects;
38-
import java.util.function.Function;
3937
import java.util.function.LongSupplier;
4038

4139
/**
@@ -48,14 +46,14 @@
4846
public class JavaDateMathParser implements DateMathParser {
4947

5048
private final JavaDateFormatter formatter;
51-
private final DateTimeFormatter roundUpFormatter;
5249
private final String format;
50+
private final JavaDateFormatter roundupParser;
5351

54-
JavaDateMathParser(String format, JavaDateFormatter formatter, DateTimeFormatter roundUpFormatter) {
52+
JavaDateMathParser(String format, JavaDateFormatter formatter, JavaDateFormatter roundupParser) {
5553
this.format = format;
54+
this.roundupParser = roundupParser;
5655
Objects.requireNonNull(formatter);
5756
this.formatter = formatter;
58-
this.roundUpFormatter = roundUpFormatter;
5957
}
6058

6159
@Override
@@ -217,12 +215,12 @@ private Instant parseDateTime(String value, ZoneId timeZone, boolean roundUpIfNo
217215
throw new ElasticsearchParseException("cannot parse empty date");
218216
}
219217

220-
Function<String,TemporalAccessor> formatter = roundUpIfNoTime ? this.roundUpFormatter::parse : this.formatter::parse;
218+
DateFormatter formatter = roundUpIfNoTime ? this.roundupParser : this.formatter;
221219
try {
222220
if (timeZone == null) {
223-
return DateFormatters.from(formatter.apply(value)).toInstant();
221+
return DateFormatters.from(formatter.parse(value)).toInstant();
224222
} else {
225-
TemporalAccessor accessor = formatter.apply(value);
223+
TemporalAccessor accessor = formatter.parse(value);
226224
ZoneId zoneId = TemporalQueries.zone().queryFrom(accessor);
227225
if (zoneId != null) {
228226
timeZone = zoneId;

‎server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java‎

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,15 +19,18 @@
1919

2020
package org.elasticsearch.common.joda;
2121

22+
import org.elasticsearch.ElasticsearchParseException;
2223
import org.elasticsearch.bootstrap.JavaVersion;
2324
import org.elasticsearch.common.time.DateFormatter;
2425
import org.elasticsearch.common.time.DateFormatters;
26+
import org.elasticsearch.common.time.DateMathParser;
2527
import org.elasticsearch.test.ESTestCase;
2628
import org.joda.time.DateTime;
2729
import org.joda.time.DateTimeZone;
28-
import org.joda.time.format.ISODateTimeFormat;
2930
import org.joda.time.format.DateTimeFormat;
31+
import org.joda.time.format.ISODateTimeFormat;
3032

33+
import java.time.ZoneId;
3134
import java.time.ZoneOffset;
3235
import java.time.ZonedDateTime;
3336
import java.time.format.DateTimeFormatter;
@@ -39,6 +42,34 @@
3942
import static org.hamcrest.Matchers.is;
4043

4144
public class JavaJodaTimeDuellingTests extends ESTestCase {
45+
public void testCompositeDateMathParsing(){
46+
//in all these examples the second pattern will be used
47+
assertDateMathEquals("2014-06-06T12:01:02.123", "yyyy-MM-dd'T'HH:mm:ss||yyyy-MM-dd'T'HH:mm:ss.SSS");
48+
assertDateMathEquals("2014-06-06T12:01:02.123", "strictDateTimeNoMillis||yyyy-MM-dd'T'HH:mm:ss.SSS");
49+
assertDateMathEquals("2014-06-06T12:01:02.123", "yyyy-MM-dd'T'HH:mm:ss+HH:MM||yyyy-MM-dd'T'HH:mm:ss.SSS");
50+
}
51+
52+
public void testExceptionWhenCompositeParsingFailsDateMath(){
53+
//both parsing failures should contain pattern and input text in exception
54+
//both patterns fail parsing the input text due to only 2 digits of millis. Hence full text was not parsed.
55+
String pattern = "yyyy-MM-dd'T'HH:mm:ss||yyyy-MM-dd'T'HH:mm:ss.SS";
56+
String text = "2014-06-06T12:01:02.123";
57+
ElasticsearchParseException e1 = expectThrows(ElasticsearchParseException.class,
58+
() -> dateMathToMillis(text, DateFormatter.forPattern(pattern)));
59+
assertThat(e1.getMessage(), containsString(pattern));
60+
assertThat(e1.getMessage(), containsString(text));
61+
62+
ElasticsearchParseException e2 = expectThrows(ElasticsearchParseException.class,
63+
() -> dateMathToMillis(text, Joda.forPattern(pattern)));
64+
assertThat(e2.getMessage(), containsString(pattern));
65+
assertThat(e2.getMessage(), containsString(text));
66+
}
67+
68+
private long dateMathToMillis(String text, DateFormatter dateFormatter) {
69+
DateFormatter javaFormatter = dateFormatter.withLocale(randomLocale(random()));
70+
DateMathParser javaDateMath = javaFormatter.toDateMathParser();
71+
return javaDateMath.parse(text, () -> 0, true, (ZoneId) null).toEpochMilli();
72+
}
4273

4374
//these parsers should allow both ',' and '.' as a decimal point
4475
public void testDecimalPointParsing(){
@@ -871,4 +902,11 @@ private void assertJavaTimeParseException(String input, String format) {
871902
assertThat(e.getMessage(), containsString(input));
872903
assertThat(e.getMessage(), containsString(format));
873904
}
905+
906+
private void assertDateMathEquals(String text, String pattern) {
907+
long gotMillisJava = dateMathToMillis(text, DateFormatter.forPattern(pattern));
908+
long gotMillisJoda = dateMathToMillis(text, Joda.forPattern(pattern));
909+
910+
assertEquals(gotMillisJoda, gotMillisJava);
911+
}
874912
}

‎server/src/test/java/org/elasticsearch/common/joda/JodaDateMathParserTests.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727

2828
import java.time.Instant;
2929
import java.time.ZoneId;
30+
import java.time.ZoneOffset;
3031
import java.util.concurrent.atomic.AtomicBoolean;
3132
import java.util.function.LongSupplier;
3233

@@ -59,6 +60,19 @@ void assertDateEquals(long gotMillis, String original, String expected) {
5960
}
6061
}
6162

63+
public void testOverridingLocaleOrZoneAndCompositeRoundUpParser() {
64+
//the pattern has to be composite and the match should not be on the first one
65+
DateFormatter formatter = Joda.forPattern("date||epoch_millis").withLocale(randomLocale(random()));
66+
DateMathParser parser = formatter.toDateMathParser();
67+
long gotMillis = parser.parse("297276785531", () -> 0, true, (ZoneId) null).toEpochMilli();
68+
assertDateEquals(gotMillis, "297276785531", "297276785531");
69+
70+
formatter = Joda.forPattern("date||epoch_millis").withZone(ZoneOffset.UTC);
71+
parser = formatter.toDateMathParser();
72+
gotMillis = parser.parse("297276785531", () -> 0, true, (ZoneId) null).toEpochMilli();
73+
assertDateEquals(gotMillis, "297276785531", "297276785531");
74+
}
75+
6276
public void testBasicDates() {
6377
assertDateMathEquals("2014", "2014-01-01T00:00:00.000");
6478
assertDateMathEquals("2014-05", "2014-05-01T00:00:00.000");

‎server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
import java.time.ZoneId;
2727
import java.time.ZoneOffset;
2828
import java.time.ZonedDateTime;
29-
import java.time.format.DateTimeFormatter;
3029
import java.time.temporal.ChronoField;
3130
import java.time.temporal.TemporalAccessor;
3231
import java.util.Locale;
@@ -253,7 +252,7 @@ public void testIso8601Parsing() {
253252
public void testRoundupFormatterWithEpochDates() {
254253
assertRoundupFormatter("epoch_millis", "1234567890", 1234567890L);
255254
// also check nanos of the epoch_millis formatter if it is rounded up to the nano second
256-
DateTimeFormatter roundUpFormatter = ((JavaDateFormatter) DateFormatter.forPattern("8epoch_millis")).getRoundupParser();
255+
JavaDateFormatter roundUpFormatter = ((JavaDateFormatter) DateFormatter.forPattern("8epoch_millis")).getRoundupParser();
257256
Instant epochMilliInstant = DateFormatters.from(roundUpFormatter.parse("1234567890")).toInstant();
258257
assertThat(epochMilliInstant.getLong(ChronoField.NANO_OF_SECOND), is(890_999_999L));
259258

@@ -266,7 +265,7 @@ public void testRoundupFormatterWithEpochDates() {
266265

267266
assertRoundupFormatter("epoch_second", "1234567890", 1234567890999L);
268267
// also check nanos of the epoch_millis formatter if it is rounded up to the nano second
269-
DateTimeFormatter epochSecondRoundupParser = ((JavaDateFormatter) DateFormatter.forPattern("8epoch_second")).getRoundupParser();
268+
JavaDateFormatter epochSecondRoundupParser = ((JavaDateFormatter) DateFormatter.forPattern("8epoch_second")).getRoundupParser();
270269
Instant epochSecondInstant = DateFormatters.from(epochSecondRoundupParser.parse("1234567890")).toInstant();
271270
assertThat(epochSecondInstant.getLong(ChronoField.NANO_OF_SECOND), is(999_999_999L));
272271

@@ -280,7 +279,7 @@ public void testRoundupFormatterWithEpochDates() {
280279
private void assertRoundupFormatter(String format, String input, long expectedMilliSeconds) {
281280
JavaDateFormatter dateFormatter = (JavaDateFormatter) DateFormatter.forPattern(format);
282281
dateFormatter.parse(input);
283-
DateTimeFormatter roundUpFormatter = dateFormatter.getRoundupParser();
282+
JavaDateFormatter roundUpFormatter = dateFormatter.getRoundupParser();
284283
long millis = DateFormatters.from(roundUpFormatter.parse(input)).toInstant().toEpochMilli();
285284
assertThat(millis, is(expectedMilliSeconds));
286285
}
@@ -290,8 +289,8 @@ public void testRoundupFormatterZone() {
290289
String format = randomFrom("epoch_second", "epoch_millis", "strict_date_optional_time", "uuuu-MM-dd'T'HH:mm:ss.SSS",
291290
"strict_date_optional_time||date_optional_time");
292291
JavaDateFormatter formatter = (JavaDateFormatter) DateFormatter.forPattern(format).withZone(zoneId);
293-
DateTimeFormatter roundUpFormatter = formatter.getRoundupParser();
294-
assertThat(roundUpFormatter.getZone(), is(zoneId));
292+
JavaDateFormatter roundUpFormatter = formatter.getRoundupParser();
293+
assertThat(roundUpFormatter.zone(), is(zoneId));
295294
assertThat(formatter.zone(), is(zoneId));
296295
}
297296

@@ -300,8 +299,8 @@ public void testRoundupFormatterLocale() {
300299
String format = randomFrom("epoch_second", "epoch_millis", "strict_date_optional_time", "uuuu-MM-dd'T'HH:mm:ss.SSS",
301300
"strict_date_optional_time||date_optional_time");
302301
JavaDateFormatter formatter = (JavaDateFormatter) DateFormatter.forPattern(format).withLocale(locale);
303-
DateTimeFormatter roundupParser = formatter.getRoundupParser();
304-
assertThat(roundupParser.getLocale(), is(locale));
302+
JavaDateFormatter roundupParser = formatter.getRoundupParser();
303+
assertThat(roundupParser.locale(), is(locale));
305304
assertThat(formatter.locale(), is(locale));
306305
}
307306

0 commit comments

Comments
 (0)