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

Fix java time formatters that round up #37604

Merged
merged 11 commits into from
Jan 22, 2019
Merged

Fix java time formatters that round up #37604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
221f6d1
Fix java time formatters that round up
spinscale Jan 18, 2019
d4fe6bc
Switch from exception based error handling to building proper default
spinscale Jan 20, 2019
42e053f
simplify code
spinscale Jan 20, 2019
082b90f
Merge branch 'master' into 1901-fix-round-up-formatters
spinscale Jan 20, 2019
e686257
Merge branch 'master' into 1901-fix-round-up-formatters
spinscale Jan 21, 2019
fa72994
refactor conditionals in JavaDateFormatter ctor to sth more readable
spinscale Jan 21, 2019
7463c9b
make field private again
spinscale Jan 21, 2019
3deaec8
Merge branch 'master' into 1901-fix-round-up-formatters
spinscale Jan 21, 2019
cd1f003
review comment: add proper comment
spinscale Jan 21, 2019
75b3d9c
Merge branch 'master' into 1901-fix-round-up-formatters
spinscale Jan 21, 2019
e75925a
Merge branch 'master' into 1901-fix-round-up-formatters
spinscale Jan 21, 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
6 changes: 4 additions & 2 deletions server/src/main/java/org/elasticsearch/common/time/DateFormatter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,11 @@ static DateFormatter forPattern(String input) {
return Joda.forPattern(input);
}

// force java 8 date format
// dates starting with 8 will not be using joda but java time formatters
input = input.substring(1);
Copy link
Member

Choose a reason for hiding this comment

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

This is to remove the leading "8" from the input pattern but I always find this confusing without a comment.


List<DateFormatter> formatters = new ArrayList<>();
for (String pattern : Strings.delimitedListToStringArray(input.substring(1), "||")) {
for (String pattern : Strings.delimitedListToStringArray(input, "||")) {
if (Strings.hasLength(pattern) == false) {
throw new IllegalArgumentException("Cannot have empty element in multi date format pattern: " + input);
}
Expand Down
44 changes: 23 additions & 21 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 @@ -46,7 +46,6 @@
import static java.time.temporal.ChronoField.DAY_OF_WEEK;
import static java.time.temporal.ChronoField.DAY_OF_YEAR;
import static java.time.temporal.ChronoField.HOUR_OF_DAY;
import static java.time.temporal.ChronoField.MILLI_OF_SECOND;
import static java.time.temporal.ChronoField.MINUTE_OF_HOUR;
import static java.time.temporal.ChronoField.MONTH_OF_YEAR;
import static java.time.temporal.ChronoField.NANO_OF_SECOND;
Expand Down Expand Up @@ -90,7 +89,7 @@ public class DateFormatters {
.appendLiteral('T')
.append(STRICT_HOUR_MINUTE_SECOND_FORMATTER)
.optionalStart()
.appendFraction(MILLI_OF_SECOND, 3, 3, true)
.appendFraction(NANO_OF_SECOND, 3, 3, true)
.optionalEnd()
.optionalStart()
.appendZoneOrOffsetId()
Expand Down Expand Up @@ -159,14 +158,14 @@ public class DateFormatters {
.appendValue(HOUR_OF_DAY, 2, 2, SignStyle.NOT_NEGATIVE)
.appendValue(MINUTE_OF_HOUR, 2, 2, SignStyle.NOT_NEGATIVE)
.appendValue(SECOND_OF_MINUTE, 2, 2, SignStyle.NOT_NEGATIVE)
.appendFraction(MILLI_OF_SECOND, 1, 3, true)
.appendFraction(NANO_OF_SECOND, 1, 3, true)
.toFormatter(Locale.ROOT);

private static final DateTimeFormatter BASIC_TIME_PRINTER = new DateTimeFormatterBuilder()
.appendValue(HOUR_OF_DAY, 2, 2, SignStyle.NOT_NEGATIVE)
.appendValue(MINUTE_OF_HOUR, 2, 2, SignStyle.NOT_NEGATIVE)
.appendValue(SECOND_OF_MINUTE, 2, 2, SignStyle.NOT_NEGATIVE)
.appendFraction(MILLI_OF_SECOND, 3, 3, true)
.appendFraction(NANO_OF_SECOND, 3, 3, true)
.toFormatter(Locale.ROOT);

/*
Expand Down Expand Up @@ -372,7 +371,7 @@ public class DateFormatters {
.appendValue(HOUR_OF_DAY, 2, 2, SignStyle.NOT_NEGATIVE)
.appendValue(MINUTE_OF_HOUR, 2, 2, SignStyle.NOT_NEGATIVE)
.appendValue(SECOND_OF_MINUTE, 2, 2, SignStyle.NOT_NEGATIVE)
.appendFraction(MILLI_OF_SECOND, 3, 3, true)
.appendFraction(NANO_OF_SECOND, 3, 3, true)
.appendZoneOrOffsetId()
.toFormatter(Locale.ROOT),
new DateTimeFormatterBuilder()
Expand All @@ -381,7 +380,7 @@ public class DateFormatters {
.appendValue(HOUR_OF_DAY, 2, 2, SignStyle.NOT_NEGATIVE)
.appendValue(MINUTE_OF_HOUR, 2, 2, SignStyle.NOT_NEGATIVE)
.appendValue(SECOND_OF_MINUTE, 2, 2, SignStyle.NOT_NEGATIVE)
.appendFraction(MILLI_OF_SECOND, 3, 3, true)
.appendFraction(NANO_OF_SECOND, 3, 3, true)
.append(TIME_ZONE_FORMATTER_NO_COLON)
.toFormatter(Locale.ROOT)
);
Expand Down Expand Up @@ -438,7 +437,7 @@ public class DateFormatters {
.appendLiteral('T')
.append(STRICT_HOUR_MINUTE_SECOND_FORMATTER)
.optionalStart()
.appendFraction(MILLI_OF_SECOND, 3, 3, true)
.appendFraction(NANO_OF_SECOND, 3, 3, true)
.optionalEnd()
.toFormatter(Locale.ROOT);

Expand Down Expand Up @@ -495,12 +494,12 @@ public class DateFormatters {
// NOTE: this is not a strict formatter to retain the joda time based behaviour, even though it's named like this
private static final DateTimeFormatter STRICT_HOUR_MINUTE_SECOND_MILLIS_FORMATTER = new DateTimeFormatterBuilder()
.append(STRICT_HOUR_MINUTE_SECOND_FORMATTER)
.appendFraction(MILLI_OF_SECOND, 1, 3, true)
.appendFraction(NANO_OF_SECOND, 1, 3, true)
.toFormatter(Locale.ROOT);

private static final DateTimeFormatter STRICT_HOUR_MINUTE_SECOND_MILLIS_PRINTER = new DateTimeFormatterBuilder()
.append(STRICT_HOUR_MINUTE_SECOND_FORMATTER)
.appendFraction(MILLI_OF_SECOND, 3, 3, true)
.appendFraction(NANO_OF_SECOND, 3, 3, true)
.toFormatter(Locale.ROOT);

/*
Expand Down Expand Up @@ -534,7 +533,7 @@ public class DateFormatters {
.appendLiteral("T")
.append(STRICT_HOUR_MINUTE_SECOND_FORMATTER)
// this one here is lenient as well to retain joda time based bwc compatibility
.appendFraction(MILLI_OF_SECOND, 1, 3, true)
.appendFraction(NANO_OF_SECOND, 1, 3, true)
.toFormatter(Locale.ROOT)
);

Expand Down Expand Up @@ -562,7 +561,7 @@ public class DateFormatters {
.optionalStart()
.appendLiteral(':')
.appendValue(SECOND_OF_MINUTE, 2, 2, SignStyle.NOT_NEGATIVE)
.appendFraction(MILLI_OF_SECOND, 3, 3, true)
.appendFraction(NANO_OF_SECOND, 3, 3, true)
.optionalEnd()
.toFormatter(Locale.ROOT);

Expand All @@ -586,7 +585,7 @@ public class DateFormatters {
.appendValue(MINUTE_OF_HOUR, 2, 2, SignStyle.NOT_NEGATIVE)
.appendLiteral(':')
.appendValue(SECOND_OF_MINUTE, 2, 2, SignStyle.NOT_NEGATIVE)
.appendFraction(MILLI_OF_SECOND, 1, 3, true)
.appendFraction(NANO_OF_SECOND, 1, 3, true)
.toFormatter(Locale.ROOT);

private static final DateTimeFormatter STRICT_TIME_PRINTER = new DateTimeFormatterBuilder()
Expand All @@ -595,7 +594,7 @@ public class DateFormatters {
.appendValue(MINUTE_OF_HOUR, 2, 2, SignStyle.NOT_NEGATIVE)
.appendLiteral(':')
.appendValue(SECOND_OF_MINUTE, 2, 2, SignStyle.NOT_NEGATIVE)
.appendFraction(MILLI_OF_SECOND, 3, 3, true)
.appendFraction(NANO_OF_SECOND, 3, 3, true)
.toFormatter(Locale.ROOT);

/*
Expand Down Expand Up @@ -819,7 +818,7 @@ public class DateFormatters {
.appendValue(SECOND_OF_MINUTE, 1, 2, SignStyle.NOT_NEGATIVE)
.optionalEnd()
.optionalStart()
.appendFraction(MILLI_OF_SECOND, 1, 3, true)
.appendFraction(NANO_OF_SECOND, 1, 3, true)
.optionalEnd()
.optionalStart().appendZoneOrOffsetId().optionalEnd()
.optionalStart().appendOffset("+HHmm", "Z").optionalEnd()
Expand All @@ -840,7 +839,7 @@ public class DateFormatters {
.appendValue(MINUTE_OF_HOUR, 1, 2, SignStyle.NOT_NEGATIVE)
.appendLiteral(':')
.appendValue(SECOND_OF_MINUTE, 1, 2, SignStyle.NOT_NEGATIVE)
.appendFraction(MILLI_OF_SECOND, 1, 3, true)
.appendFraction(NANO_OF_SECOND, 1, 3, true)
.toFormatter(Locale.ROOT);

private static final DateTimeFormatter ORDINAL_DATE_FORMATTER = new DateTimeFormatterBuilder()
Expand Down Expand Up @@ -875,7 +874,7 @@ public class DateFormatters {

private static final DateTimeFormatter TIME_PREFIX = new DateTimeFormatterBuilder()
.append(TIME_NO_MILLIS_FORMATTER)
.appendFraction(MILLI_OF_SECOND, 1, 3, true)
.appendFraction(NANO_OF_SECOND, 1, 3, true)
.toFormatter(Locale.ROOT);

private static final DateTimeFormatter WEEK_DATE_FORMATTER = new DateTimeFormatterBuilder()
Expand Down Expand Up @@ -963,7 +962,7 @@ public class DateFormatters {
.optionalStart()
.appendLiteral(':')
.appendValue(SECOND_OF_MINUTE, 1, 2, SignStyle.NOT_NEGATIVE)
.appendFraction(MILLI_OF_SECOND, 1, 3, true)
.appendFraction(NANO_OF_SECOND, 1, 3, true)
.optionalEnd()
.toFormatter(Locale.ROOT);

Expand Down Expand Up @@ -1068,7 +1067,7 @@ public class DateFormatters {
.optionalStart()
.appendLiteral(':')
.appendValue(SECOND_OF_MINUTE, 1, 2, SignStyle.NOT_NEGATIVE)
.appendFraction(MILLI_OF_SECOND, 1, 3, true)
.appendFraction(NANO_OF_SECOND, 1, 3, true)
.optionalEnd()
.toFormatter(Locale.ROOT);

Expand Down Expand Up @@ -1453,18 +1452,21 @@ 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;
DateTimeFormatter dateTimeFormatter = javaDateFormatter.getParser();
if (printer == null) {
printer = javaDateFormatter.getPrinter();
}
dateTimeFormatters.add(dateTimeFormatter);
dateTimeFormatters.add(javaDateFormatter.getParser());
roundupBuilder.appendOptional(javaDateFormatter.getRoundupParser());
}
DateTimeFormatter roundUpParser = roundupBuilder.toFormatter(Locale.ROOT);

return new JavaDateFormatter(pattern, printer, dateTimeFormatters.toArray(new DateTimeFormatter[0]));
return new JavaDateFormatter(pattern, printer, builder -> builder.append(roundUpParser),
dateTimeFormatters.toArray(new DateTimeFormatter[0]));
}

private static final ZonedDateTime EPOCH_ZONED_DATE_TIME = Instant.EPOCH.atZone(ZoneOffset.UTC);
Expand Down
2 changes: 2 additions & 0 deletions server/src/main/java/org/elasticsearch/common/time/EpochTime.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,11 @@ public long getFrom(TemporalAccessor temporal) {
.toFormatter(Locale.ROOT);

static final DateFormatter SECONDS_FORMATTER = new JavaDateFormatter("epoch_second", SECONDS_FORMATTER3,
builder -> builder.parseDefaulting(ChronoField.NANO_OF_SECOND, 999_999_999L),
SECONDS_FORMATTER1, SECONDS_FORMATTER2, SECONDS_FORMATTER3);

static final DateFormatter MILLIS_FORMATTER = new JavaDateFormatter("epoch_millis", MILLISECONDS_FORMATTER3,
builder -> builder.parseDefaulting(EpochTime.NANOS_OF_MILLI, 999_999L),
MILLISECONDS_FORMATTER1, MILLISECONDS_FORMATTER2, MILLISECONDS_FORMATTER3);

private abstract static class EpochField implements TemporalField {
Expand Down
41 changes: 31 additions & 10 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 @@ -32,6 +32,7 @@
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
import java.util.function.Consumer;

class JavaDateFormatter implements DateFormatter {

Expand All @@ -43,14 +44,27 @@ class JavaDateFormatter implements DateFormatter {
ROUND_UP_BASE_FIELDS.put(ChronoField.HOUR_OF_DAY, 23L);
ROUND_UP_BASE_FIELDS.put(ChronoField.MINUTE_OF_HOUR, 59L);
ROUND_UP_BASE_FIELDS.put(ChronoField.SECOND_OF_MINUTE, 59L);
ROUND_UP_BASE_FIELDS.put(ChronoField.MILLI_OF_SECOND, 999L);
ROUND_UP_BASE_FIELDS.put(ChronoField.NANO_OF_SECOND, 999_999_999L);
}

private final String format;
private final DateTimeFormatter printer;
private final DateTimeFormatter parser;
private final DateTimeFormatter roundupParser;

private JavaDateFormatter(String format, DateTimeFormatter printer, DateTimeFormatter roundupParser, DateTimeFormatter parser) {
this.format = 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) {
if (printer == null) {
throw new IllegalArgumentException("printer may not be null");
}
Expand All @@ -75,6 +89,19 @@ class JavaDateFormatter implements DateFormatter {
}
this.format = format;
this.printer = printer;

DateTimeFormatterBuilder builder = new DateTimeFormatterBuilder();
builder.append(this.parser);
roundupParserConsumer.accept(builder);
DateTimeFormatter roundupFormatter = builder.toFormatter(parser.getLocale());
if (printer.getZone() != null) {
roundupFormatter = roundupFormatter.withZone(printer.getZone());
}
this.roundupParser = roundupFormatter;
}

DateTimeFormatter getRoundupParser() {
return roundupParser;
}

DateTimeFormatter getParser() {
Expand All @@ -100,7 +127,7 @@ public DateFormatter withZone(ZoneId zoneId) {
return this;
}

return new JavaDateFormatter(format, printer.withZone(zoneId), parser.withZone(zoneId));
return new JavaDateFormatter(format, printer.withZone(zoneId), roundupParser.withZone(zoneId), parser.withZone(zoneId));
}

@Override
Expand All @@ -110,7 +137,7 @@ public DateFormatter withLocale(Locale locale) {
return this;
}

return new JavaDateFormatter(format, printer.withLocale(locale), parser.withLocale(locale));
return new JavaDateFormatter(format, printer.withLocale(locale), roundupParser.withLocale(locale), parser.withLocale(locale));
}

@Override
Expand All @@ -123,12 +150,6 @@ public String pattern() {
return format;
}

JavaDateFormatter parseDefaulting(Map<TemporalField, Long> fields) {
final DateTimeFormatterBuilder parseDefaultingBuilder = new DateTimeFormatterBuilder().append(printer);
fields.forEach(parseDefaultingBuilder::parseDefaulting);
return new JavaDateFormatter(format, parseDefaultingBuilder.toFormatter(Locale.ROOT));
}

@Override
public Locale locale() {
return this.printer.getLocale();
Expand All @@ -141,7 +162,7 @@ public ZoneId zone() {

@Override
public DateMathParser toDateMathParser() {
return new JavaDateMathParser(this, this.parseDefaulting(ROUND_UP_BASE_FIELDS));
return new JavaDateMathParser(parser, roundupParser);
}

@Override
Expand Down
16 changes: 10 additions & 6 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 @@ -20,6 +20,7 @@
package org.elasticsearch.common.time;

import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.Strings;

import java.time.DateTimeException;
import java.time.DayOfWeek;
Expand All @@ -28,6 +29,7 @@
import java.time.ZoneId;
import java.time.ZoneOffset;
import java.time.ZonedDateTime;
import java.time.format.DateTimeFormatter;
import java.time.temporal.ChronoField;
import java.time.temporal.TemporalAccessor;
import java.time.temporal.TemporalAdjusters;
Expand All @@ -44,12 +46,10 @@
*/
public class JavaDateMathParser implements DateMathParser {

private final DateTimeFormatter formatter;
private final DateTimeFormatter roundUpFormatter;


private final DateFormatter formatter;
private final DateFormatter roundUpFormatter;

public JavaDateMathParser(DateFormatter formatter, DateFormatter roundUpFormatter) {
public JavaDateMathParser(DateTimeFormatter formatter, DateTimeFormatter roundUpFormatter) {
Objects.requireNonNull(formatter);
this.formatter = formatter;
this.roundUpFormatter = roundUpFormatter;
Expand Down Expand Up @@ -208,7 +208,11 @@ private long parseMath(final String mathString, final long time, final boolean r
}

private long parseDateTime(String value, ZoneId timeZone, boolean roundUpIfNoTime) {
DateFormatter formatter = roundUpIfNoTime ? this.roundUpFormatter : this.formatter;
if (Strings.isNullOrEmpty(value)) {
throw new IllegalArgumentException("cannot parse empty date");
}

DateTimeFormatter formatter = roundUpIfNoTime ? this.roundUpFormatter : this.formatter;
try {
if (timeZone == null) {
return DateFormatters.toZonedDateTime(formatter.parse(value)).toInstant().toEpochMilli();
Expand Down
Loading