Skip to content

Commit

Permalink
Ensure JavaDateMathParserTests are calling java formatters (#37710)
Browse files Browse the repository at this point in the history 
The existing JavaDateMathParserTests were using joda formatters instead
of java ones and thus the tests were not running the expected code.

This fixes the above and some follow up failures due to that.

First some round up formatter issues are fixed, especially when using
epoch dates, the parsing could be wrong and result in exceptions.

Second, some dates cannot be handled the same in java8 like in joda time
due to more strict parsing, when used in combination with epoch dates.
  • Loading branch information
@spinscale
spinscale authored Jan 24, 2019
1 parent 9f13eac commit 8899714
Show file tree
Hide file tree
Showing 6 changed files with 16 additions and 22 deletions.
2 changes: 0 additions & 2 deletions server/src/main/java/org/elasticsearch/common/time/DateFormatters.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -782,14 +782,12 @@ public class DateFormatters {

private static final DateTimeFormatter DATE_FORMATTER = new DateTimeFormatterBuilder()
.appendValue(ChronoField.YEAR, 1, 5, SignStyle.NORMAL)
.optionalStart()
.appendLiteral('-')
.appendValue(MONTH_OF_YEAR, 1, 2, SignStyle.NOT_NEGATIVE)
.optionalStart()
.appendLiteral('-')
.appendValue(DAY_OF_MONTH, 1, 2, SignStyle.NOT_NEGATIVE)
.optionalEnd()
.optionalEnd()
.toFormatter(Locale.ROOT);

private static final DateTimeFormatter HOUR_MINUTE_FORMATTER = new DateTimeFormatterBuilder()
Expand Down
3 changes: 2 additions & 1 deletion server/src/main/java/org/elasticsearch/common/time/EpochTime.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,8 @@ public TemporalAccessor resolve(Map<TemporalField,Long> fieldValues,
private static final EpochField NANOS_OF_MILLI = new EpochField(ChronoUnit.NANOS, ChronoUnit.MILLIS, ValueRange.of(0, 999_999)) {
@Override
public boolean isSupportedBy(TemporalAccessor temporal) {
return temporal.isSupported(ChronoField.NANO_OF_SECOND) && temporal.getLong(ChronoField.NANO_OF_SECOND) % 1_000_000 != 0;
return temporal.isSupported(ChronoField.INSTANT_SECONDS) &&
temporal.isSupported(ChronoField.NANO_OF_SECOND) && temporal.getLong(ChronoField.NANO_OF_SECOND) % 1_000_000 != 0;
}
@Override
public long getFrom(TemporalAccessor temporal) {
Expand Down
6 changes: 4 additions & 2 deletions server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,9 @@ private JavaDateFormatter(String format, DateTimeFormatter printer, DateTimeForm
this.printer = printer;

DateTimeFormatterBuilder builder = new DateTimeFormatterBuilder();
builder.append(this.parser);
if (format.contains("||") == false) {
builder.append(this.parser);
}
roundupParserConsumer.accept(builder);
DateTimeFormatter roundupFormatter = builder.toFormatter(parser.getLocale());
if (printer.getZone() != null) {
Expand Down Expand Up @@ -162,7 +164,7 @@ public ZoneId zone() {

@Override
public DateMathParser toDateMathParser() {
return new JavaDateMathParser(parser, roundupParser);
return new JavaDateMathParser(format, parser, roundupParser);
}

@Override
Expand Down
6 changes: 4 additions & 2 deletions server/src/main/java/org/elasticsearch/common/time/JavaDateMathParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,11 @@ public class JavaDateMathParser implements DateMathParser {

private final DateTimeFormatter formatter;
private final DateTimeFormatter roundUpFormatter;
private final String format;

public JavaDateMathParser(DateTimeFormatter formatter, DateTimeFormatter roundUpFormatter) {
JavaDateMathParser(String format, DateTimeFormatter formatter, DateTimeFormatter roundUpFormatter) {
Objects.requireNonNull(formatter);
this.format = format;
this.formatter = formatter;
this.roundUpFormatter = roundUpFormatter;
}
Expand Down Expand Up @@ -226,7 +228,7 @@ private long parseDateTime(String value, ZoneId timeZone, boolean roundUpIfNoTim
return DateFormatters.toZonedDateTime(accessor).withZoneSameLocal(timeZone).toInstant().toEpochMilli();
}
} catch (IllegalArgumentException | DateTimeException e) {
throw new ElasticsearchParseException("failed to parse date field [{}]: [{}]", e, value, e.getMessage());
throw new ElasticsearchParseException("failed to parse date field [{}] in format [{}]: [{}]", e, value, format, e.getMessage());
}
}
}
4 changes: 0 additions & 4 deletions server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,6 @@ public void testDuellingFormatsValidParsing() {
assertSameDate("2018-12-31T12:12:12.1", "date_hour_minute_second_millis");
assertSameDate("2018-12-31T12:12:12.1", "date_hour_minute_second_fraction");

assertSameDate("10000", "date_optional_time");
assertSameDate("10000T", "date_optional_time");
assertSameDate("2018", "date_optional_time");
assertSameDate("2018T", "date_optional_time");
assertSameDate("2018-05", "date_optional_time");
assertSameDate("2018-05-30", "date_optional_time");
assertSameDate("2018-05-30T20", "date_optional_time");
Expand Down
17 changes: 6 additions & 11 deletions server/src/test/java/org/elasticsearch/common/time/JavaDateMathParserTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,10 @@

public class JavaDateMathParserTests extends ESTestCase {

private final DateFormatter formatter = DateFormatter.forPattern("dateOptionalTime||epoch_millis");
private final DateFormatter formatter = DateFormatter.forPattern("8dateOptionalTime||epoch_millis");
private final DateMathParser parser = formatter.toDateMathParser();

public void testBasicDates() {
assertDateMathEquals("2014", "2014-01-01T00:00:00.000");
assertDateMathEquals("2014-05", "2014-05-01T00:00:00.000");
assertDateMathEquals("2014-05-30", "2014-05-30T00:00:00.000");
assertDateMathEquals("2014-05-30T20", "2014-05-30T20:00:00.000");
assertDateMathEquals("2014-05-30T20:21", "2014-05-30T20:21:00.000");
Expand Down Expand Up @@ -176,7 +174,6 @@ public void testImplicitRounding() {
public void testExplicitRounding() {
assertDateMathEquals("2014-11-18||/y", "2014-01-01", 0, false, null);
assertDateMathEquals("2014-11-18||/y", "2014-12-31T23:59:59.999", 0, true, null);
assertDateMathEquals("2014||/y", "2014-01-01", 0, false, null);
assertDateMathEquals("2014-01-01T00:00:00.001||/y", "2014-12-31T23:59:59.999", 0, true, null);
// rounding should also take into account time zone
assertDateMathEquals("2014-11-18||/y", "2013-12-31T23:00:00.000Z", 0, false, ZoneId.of("CET"));
Expand Down Expand Up @@ -243,12 +240,10 @@ public void testTimestamps() {
long datetime = parser.parse("1418248078", () -> 0);
assertDateEquals(datetime, "1418248078", "2014-12-10T21:47:58.000");

// a timestamp before 10000 is a year
assertDateMathEquals("9999", "9999-01-01T00:00:00.000");
// 10000 is also a year, breaking bwc, used to be a timestamp
assertDateMathEquals("10000", "10000-01-01T00:00:00.000");
// but 10000 with T is still a date format
assertDateMathEquals("10000T", "10000-01-01T00:00:00.000");
// numbers only are always an epoch timestamp
assertDateMathEquals("9999", "1970-01-01T00:00:09.999Z");
assertDateMathEquals("10000", "1970-01-01T00:00:10.000Z");
assertDateMathEquals("10000-01-01T", "10000-01-01T00:00:00.000");
}

void assertParseException(String msg, String date, String exc) {
Expand All @@ -266,7 +261,7 @@ public void testIllegalMathFormat() {

public void testIllegalDateFormat() {
assertParseException("Expected bad timestamp exception", Long.toString(Long.MAX_VALUE) + "0", "failed to parse date field");
assertParseException("Expected bad date format exception", "123bogus", "Unrecognized chars at the end of [123bogus]");
assertParseException("Expected bad date format exception", "123bogus", "could not be parsed, unparsed text found at index 3");
}

public void testOnlyCallsNowIfNecessary() {
Expand Down

0 comments on commit 8899714

Please sign in to comment.