Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse composite patterns using ClassicFormat.parseObject backport(#40100) #40503

Merged
merged 6 commits into from
Apr 4, 2019
Merged

Parse composite patterns using ClassicFormat.parseObject backport(#40100) #40503

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7a08082
Parse composite patterns using ClassicFormat.parseObject (#40100)
pgomulka Mar 27, 2019
860d8e1
refactor tests to new messages/exceptiosn throw
pgomulka Mar 27, 2019
b06cdcd
code style
pgomulka Mar 27, 2019
f3b03bb
Merge branch '6.7' of github.com:elastic/elasticsearch into 6.7
pgomulka Apr 2, 2019
d040c4d
javadoc and cleanup
pgomulka Apr 2, 2019
358d761
javadoc update
pgomulka Apr 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions server/src/main/java/org/elasticsearch/common/time/DateFormatters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1585,7 +1585,7 @@ static JavaDateFormatter merge(String pattern, List<DateFormatter> formatters) {
if (printer == null) {
printer = javaDateFormatter.getPrinter();
}
dateTimeFormatters.add(javaDateFormatter.getParser());
dateTimeFormatters.addAll(javaDateFormatter.getParsers());
roundupBuilder.appendOptional(javaDateFormatter.getRoundupParser());
}
DateTimeFormatter roundUpParser = roundupBuilder.toFormatter(Locale.ROOT);
Expand Down Expand Up @@ -1632,7 +1632,7 @@ public static ZonedDateTime from(TemporalAccessor accessor) {
if (zoneId == null) {
zoneId = ZoneOffset.UTC;
}

LocalDate localDate = accessor.query(TemporalQueries.localDate());
LocalTime localTime = accessor.query(TemporalQueries.localTime());
boolean isLocalDateSet = localDate != null;
Expand Down
101 changes: 67 additions & 34 deletions server/src/main/java/org/elasticsearch/common/time/JavaDateFormatter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,19 @@

import org.elasticsearch.common.Strings;

import java.text.ParsePosition;
import java.time.ZoneId;
import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeFormatterBuilder;
import java.time.format.DateTimeParseException;
import java.time.temporal.ChronoField;
import java.time.temporal.TemporalAccessor;
import java.time.temporal.TemporalField;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
Expand All @@ -38,6 +43,7 @@ class JavaDateFormatter implements DateFormatter {

// base fields which should be used for default parsing, when we round up for date math
private static final Map<TemporalField, Long> ROUND_UP_BASE_FIELDS = new HashMap<>(6);

{
ROUND_UP_BASE_FIELDS.put(ChronoField.MONTH_OF_YEAR, 1L);
ROUND_UP_BASE_FIELDS.put(ChronoField.DAY_OF_MONTH, 1L);
Expand All @@ -49,22 +55,15 @@ class JavaDateFormatter implements DateFormatter {

private final String format;
private final DateTimeFormatter printer;
private final DateTimeFormatter parser;
private final List<DateTimeFormatter> parsers;
private final DateTimeFormatter roundupParser;

private JavaDateFormatter(String format, DateTimeFormatter printer, DateTimeFormatter roundupParser, DateTimeFormatter parser) {
this.format = "8" + format;
this.printer = printer;
this.roundupParser = roundupParser;
this.parser = parser;
}

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,
DateTimeFormatter... parsers) {
DateTimeFormatter... parsers) {
if (printer == null) {
throw new IllegalArgumentException("printer may not be null");
}
Expand All @@ -76,28 +75,23 @@ private JavaDateFormatter(String format, DateTimeFormatter printer, DateTimeForm
if (distinctLocales > 1) {
throw new IllegalArgumentException("formatters must have the same locale");
}
this.printer = printer;
this.format = "8" + format;

if (parsers.length == 0) {
this.parser = printer;
} else if (parsers.length == 1) {
this.parser = parsers[0];
this.parsers = Collections.singletonList(printer);
} else {
DateTimeFormatterBuilder builder = new DateTimeFormatterBuilder();
for (DateTimeFormatter parser : parsers) {
builder.appendOptional(parser);
}
this.parser = builder.toFormatter(Locale.ROOT);
this.parsers = Arrays.asList(parsers);
}
this.format = "8" + format;
this.printer = printer;

DateTimeFormatterBuilder builder = new DateTimeFormatterBuilder();
if (format.contains("||") == false) {
builder.append(this.parser);
builder.append(this.parsers.get(0));
}
roundupParserConsumer.accept(builder);
DateTimeFormatter roundupFormatter = builder.toFormatter(parser.getLocale());
DateTimeFormatter roundupFormatter = builder.toFormatter(locale());
if (printer.getZone() != null) {
roundupFormatter = roundupFormatter.withZone(printer.getZone());
roundupFormatter = roundupFormatter.withZone(zone());
}
this.roundupParser = roundupFormatter;
}
Expand All @@ -106,10 +100,6 @@ DateTimeFormatter getRoundupParser() {
return roundupParser;
}

DateTimeFormatter getParser() {
return parser;
}

DateTimeFormatter getPrinter() {
return printer;
}
Expand All @@ -119,27 +109,66 @@ public TemporalAccessor parse(String input) {
if (Strings.isNullOrEmpty(input)) {
throw new IllegalArgumentException("cannot parse empty date");
}
return parser.parse(input);

try {
return doParse(input);
} catch (DateTimeParseException e) {
throw new IllegalArgumentException("failed to parse date field [" + input + "] with format [" + format + "]", e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that this method will never throw DateTimeParseException as mentioned in the Javadoc of DateFormatter#parse. Can you please update the Javadoc there to reflect this change?

Copy link
Contributor Author

@pgomulka pgomulka Apr 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the javadoc for the interface could state RuntimeException to be absolutely correct, but it would be very imprecise.

  • JodaDateFormatter can throw java.time.DateTimeException as well as UnsupportedOperationException and IllegalArgumentException
  • JavaDateFormatter only IllegalArgumentException. NullPointerException is stated in the underlying doc for Format.parseObject but I don't expect this to be thrown as we already check for input not being empty and provide the position of parsing.

I will refactor the javadoc to state that IllegalArgumentException is expected to be thrown when parsing fails (that applies in both cases). But I wonder if we should ommit the DateTimeException and UnsupportedOperationException (thrown only by joda)

This problem also exists in master

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is no point in digging through the complete implementation of Java time and Joda time in order to come up with a Javadoc that documents all possible exceptions but rather document the ones that a user can "reasonably" expect when a date cannot be parsed which is in our case java.time.DateTimeException from JodaDateFormatter and IllegalArgumentException (from both) so they know how to deal with them.

IMHO it is ok to document both of them on the interface. Although one of them is in fact thrown by only one of the implementations, the exception type itself is available in the JDK and thus generic enough and we should expect users to program against the interface.

}
}

/**
* Attempt parsing the input without throwing exception. If multiple parsers are provided,
* it will continue iterating if the previous parser failed. The pattern must fully match, meaning whole input was used.
* This also means that this method depends on <code>DateTimeFormatter.ClassicFormat.parseObject</code>
* which does not throw exceptions when parsing failed.
*
* The approach with collection of parsers was taken because java-time requires ordering on optional (composite)
* patterns. Joda does not suffer from this.
* https://bugs.openjdk.java.net/browse/JDK-8188771
*
* @param input An arbitrary string resembling the string representation of a date or time
* @return a TemporalAccessor if parsing was successful.
* @throws DateTimeParseException when unable to parse with any parsers
*/
private TemporalAccessor doParse(String input) {
if (parsers.size() > 1) {
for (DateTimeFormatter formatter : parsers) {
ParsePosition pos = new ParsePosition(0);
Object object = formatter.toFormat().parseObject(input, pos);
if (parsingSucceeded(object, input, pos) == true) {
return (TemporalAccessor) object;
}
}
throw new DateTimeParseException("Failed to parse with all enclosed parsers", input, 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we iterate over parsers we would not know exact failure reason for each parsers. Tests that assert about the exact reason (like position where it failed) had to be changed

}
return this.parsers.get(0).parse(input);
}

private boolean parsingSucceeded(Object object, String input, ParsePosition pos) {
return object != null && pos.getIndex() == input.length();
}

@Override
public DateFormatter withZone(ZoneId zoneId) {
// shortcurt to not create new objects unnecessarily
if (zoneId.equals(parser.getZone())) {
if (zoneId.equals(zone())) {
return this;
}

return new JavaDateFormatter(format, printer.withZone(zoneId), roundupParser.withZone(zoneId), parser.withZone(zoneId));
return new JavaDateFormatter(format, printer.withZone(zoneId),
parsers.stream().map(p -> p.withZone(zoneId)).toArray(size -> new DateTimeFormatter[size]));
}

@Override
public DateFormatter withLocale(Locale locale) {
// shortcurt to not create new objects unnecessarily
if (locale.equals(parser.getLocale())) {
if (locale.equals(locale())) {
return this;
}

return new JavaDateFormatter(format, printer.withLocale(locale), roundupParser.withLocale(locale), parser.withLocale(locale));
return new JavaDateFormatter(format, printer.withLocale(locale),
parsers.stream().map(p -> p.withLocale(locale)).toArray(size -> new DateTimeFormatter[size]));
}

@Override
Expand All @@ -164,7 +193,7 @@ public ZoneId zone() {

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

@Override
Expand All @@ -180,12 +209,16 @@ public boolean equals(Object obj) {
JavaDateFormatter other = (JavaDateFormatter) obj;

return Objects.equals(format, other.format) &&
Objects.equals(locale(), other.locale()) &&
Objects.equals(this.printer.getZone(), other.printer.getZone());
Objects.equals(locale(), other.locale()) &&
Objects.equals(this.printer.getZone(), other.printer.getZone());
}

@Override
public String toString() {
return String.format(Locale.ROOT, "format[%s] locale[%s]", format, locale());
}

Collection<DateTimeFormatter> getParsers() {
return parsers;
}
}
17 changes: 10 additions & 7 deletions server/src/main/java/org/elasticsearch/common/time/JavaDateMathParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import java.time.temporal.TemporalAdjusters;
import java.time.temporal.TemporalQueries;
import java.util.Objects;
import java.util.function.Function;
import java.util.function.LongSupplier;

/**
Expand All @@ -46,11 +47,11 @@
*/
public class JavaDateMathParser implements DateMathParser {

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

JavaDateMathParser(String format, DateTimeFormatter formatter, DateTimeFormatter roundUpFormatter) {
JavaDateMathParser(String format, JavaDateFormatter formatter, DateTimeFormatter roundUpFormatter) {
Objects.requireNonNull(formatter);
this.format = format;
this.formatter = formatter;
Expand Down Expand Up @@ -214,21 +215,23 @@ private long parseDateTime(String value, ZoneId timeZone, boolean roundUpIfNoTim
throw new IllegalArgumentException("cannot parse empty date");
}

DateTimeFormatter formatter = roundUpIfNoTime ? this.roundUpFormatter : this.formatter;
Function<String,TemporalAccessor> formatter = roundUpIfNoTime ? this.roundUpFormatter::parse : this.formatter::parse;
try {
if (timeZone == null) {
return DateFormatters.from(formatter.parse(value)).toInstant().toEpochMilli();
return DateFormatters.from(formatter.apply(value)).toInstant().toEpochMilli();
} else {
TemporalAccessor accessor = formatter.parse(value);
TemporalAccessor accessor = formatter.apply(value);
ZoneId zoneId = TemporalQueries.zone().queryFrom(accessor);
if (zoneId != null) {
timeZone = zoneId;
}

return DateFormatters.from(accessor).withZoneSameLocal(timeZone).toInstant().toEpochMilli();
}
} catch (IllegalArgumentException | DateTimeException e) {
throw new ElasticsearchParseException("failed to parse date field [{}] in format [{}]: [{}]", e, value, format, e.getMessage());
} catch (IllegalArgumentException | DateTimeException e) {;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: There is an additional unnecessary semicolon at the end of the line.


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Is there a reason for the additional empty line here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

accident when merging

throw new ElasticsearchParseException("failed to parse date field [{}] with format [{}]: [{}]",
e, value, format, e.getMessage());
}
}
}
32 changes: 21 additions & 11 deletions server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,11 @@
import java.time.ZoneOffset;
import java.time.ZonedDateTime;
import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeParseException;
import java.time.temporal.TemporalAccessor;
import java.util.Locale;

import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.startsWith;

public class JavaJodaTimeDuellingTests extends ESTestCase {

Expand Down Expand Up @@ -290,7 +288,7 @@ public void testDuellingFormatsValidParsing() {
// joda comes up with a different exception message here, so we have to adapt
assertJodaParseException("2012-W1-8", "week_date",
"Cannot parse \"2012-W1-8\": Value 8 for dayOfWeek must be in the range [1,7]");
assertJavaTimeParseException("2012-W1-8", "week_date", "Text '2012-W1-8' could not be parsed");
assertJavaTimeParseException("2012-W1-8", "week_date");

assertSameDate("2012-W48-6T10:15:30.123Z", "week_date_time");
assertSameDate("2012-W48-6T10:15:30.123456789Z", "week_date_time");
Expand Down Expand Up @@ -330,6 +328,17 @@ public void testDuellingFormatsValidParsing() {
assertSameDate("2012-W1-1", "weekyear_week_day");
}

public void testCompositeParsing(){
//in all these examples the second pattern will be used
assertSameDate("2014-06-06T12:01:02.123", "yyyy-MM-dd'T'HH:mm:ss||yyyy-MM-dd'T'HH:mm:ss.SSS");
assertSameDate("2014-06-06T12:01:02.123", "strictDateTimeNoMillis||yyyy-MM-dd'T'HH:mm:ss.SSS");
assertSameDate("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 testExceptionWhenCompositeParsingFails(){
assertParseException("2014-06-06T12:01:02.123", "yyyy-MM-dd'T'HH:mm:ss||yyyy-MM-dd'T'HH:mm:ss.SS");
}

public void testDuelingStrictParsing() {
assertSameDate("2018W313", "strict_basic_week_date");
assertParseException("18W313", "strict_basic_week_date");
Expand Down Expand Up @@ -477,7 +486,7 @@ public void testDuelingStrictParsing() {
// joda comes up with a different exception message here, so we have to adapt
assertJodaParseException("2012-W01-8", "strict_week_date",
"Cannot parse \"2012-W01-8\": Value 8 for dayOfWeek must be in the range [1,7]");
assertJavaTimeParseException("2012-W01-8", "strict_week_date", "Text '2012-W01-8' could not be parsed");
assertJavaTimeParseException("2012-W01-8", "strict_week_date");

assertSameDate("2012-W48-6T10:15:30.123Z", "strict_week_date_time");
assertSameDate("2012-W48-6T10:15:30.123456789Z", "strict_week_date_time");
Expand Down Expand Up @@ -624,7 +633,7 @@ public void testParsingMissingTimezone() {

private void assertSamePrinterOutput(String format, ZonedDateTime javaDate, DateTime jodaDate) {
assertThat(jodaDate.getMillis(), is(javaDate.toInstant().toEpochMilli()));
String javaTimeOut = DateFormatters.forPattern(format).format(javaDate);
String javaTimeOut = DateFormatter.forPattern(format).format(javaDate);
String jodaTimeOut = DateFormatter.forPattern(format).formatJoda(jodaDate);
if (JavaVersion.current().getVersion().get(0) == 8 && javaTimeOut.endsWith(".0")
&& (format.equals("epoch_second") || format.equals("epoch_millis"))) {
Expand All @@ -639,7 +648,7 @@ private void assertSamePrinterOutput(String format, ZonedDateTime javaDate, Date

private void assertSameDate(String input, String format) {
DateFormatter jodaFormatter = Joda.forPattern(format);
DateFormatter javaFormatter = DateFormatters.forPattern(format);
DateFormatter javaFormatter = DateFormatter.forPattern(format);
assertSameDate(input, format, jodaFormatter, javaFormatter);
}

Expand All @@ -657,7 +666,7 @@ private void assertSameDate(String input, String format, DateFormatter jodaForma

private void assertParseException(String input, String format) {
assertJodaParseException(input, format, "Invalid format: \"" + input);
assertJavaTimeParseException(input, format, "Text '" + input + "' could not be parsed");
assertJavaTimeParseException(input, format);
}

private void assertJodaParseException(String input, String format, String expectedMessage) {
Expand All @@ -666,9 +675,10 @@ private void assertJodaParseException(String input, String format, String expect
assertThat(e.getMessage(), containsString(expectedMessage));
}

private void assertJavaTimeParseException(String input, String format, String expectedMessage) {
DateFormatter javaTimeFormatter = DateFormatters.forPattern(format);
DateTimeParseException dateTimeParseException = expectThrows(DateTimeParseException.class, () -> javaTimeFormatter.parse(input));
assertThat(dateTimeParseException.getMessage(), startsWith(expectedMessage));
private void assertJavaTimeParseException(String input, String format) {
DateFormatter javaTimeFormatter = DateFormatter.forPattern("8"+format);
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> javaTimeFormatter.parse(input));
assertThat(e.getMessage(), containsString(input));
assertThat(e.getMessage(), containsString(format));
}
}
Loading