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.

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

[Java.time] Calculate week of a year with ISO rules #48209

Merged
merged 17 commits into from
Oct 22, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -861,6 +861,9 @@ class BuildPlugin implements Plugin<Project> {
'tests.security.manager': 'true',
'jna.nosys': 'true'

//TODO remove once jvm.options are added to test system properties
test.systemProperty ('java.locale.providers','SPI,COMPAT')
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a comment that this won't be needed once we include jvm.options in tests sysprops?


// ignore changing test seed when build is passed -Dignore.tests.seed for cacheability experimentation
if (System.getProperty('ignore.tests.seed') != null) {
nonInputProperties.systemProperty('tests.seed', project.property('testSeed'))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import org.elasticsearch.gradle.testclusters.RestTestRunnerTask
import org.elasticsearch.gradle.tool.Boilerplate
import org.elasticsearch.gradle.tool.ClasspathUtils
import org.gradle.api.DefaultTask
import org.gradle.api.JavaVersion
import org.gradle.api.Task
import org.gradle.api.file.FileCopyDetails
import org.gradle.api.tasks.Copy
Expand Down
2 changes: 1 addition & 1 deletion distribution/src/config/jvm.options
Original file line number Diff line number Diff line change
Expand Up @@ -104,4 +104,4 @@ ${error.file}
-Xlog:gc*,gc+age=trace,safepoint:file=${loggc}:utctime,pid,tags:filecount=32,filesize=64m
# due to internationalization enhancements in JDK 9 Elasticsearch need to set the provider to COMPAT otherwise
# time/date parsing will break in an incompatible way for some date patterns and locals
-Djava.locale.providers=COMPAT
-Djava.locale.providers=SPI,COMPAT
448 changes: 227 additions & 221 deletions server/src/main/java/org/elasticsearch/common/time/DateFormatters.java

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -125,27 +125,27 @@ public long getFrom(TemporalAccessor temporal) {
.optionalStart() // optional is used so isSupported will be called when printing
.appendFraction(NANOS_OF_SECOND, 0, 9, true)
.optionalEnd()
.toFormatter(IsoLocale.ROOT);
.toFormatter(Locale.ROOT);

// this supports seconds ending in dot
private static final DateTimeFormatter SECONDS_FORMATTER2 = new DateTimeFormatterBuilder()
.appendValue(SECONDS, 1, 19, SignStyle.NORMAL)
.appendLiteral('.')
.toFormatter(IsoLocale.ROOT);
.toFormatter(Locale.ROOT);

// this supports milliseconds without any fraction
private static final DateTimeFormatter MILLISECONDS_FORMATTER1 = new DateTimeFormatterBuilder()
.appendValue(MILLIS, 1, 19, SignStyle.NORMAL)
.optionalStart()
.appendFraction(NANOS_OF_MILLI, 0, 6, true)
.optionalEnd()
.toFormatter(IsoLocale.ROOT);
.toFormatter(Locale.ROOT);

// this supports milliseconds ending in dot
private static final DateTimeFormatter MILLISECONDS_FORMATTER2 = new DateTimeFormatterBuilder()
.append(MILLISECONDS_FORMATTER1)
.appendLiteral('.')
.toFormatter(IsoLocale.ROOT);
.toFormatter(Locale.ROOT);

static final DateFormatter SECONDS_FORMATTER = new JavaDateFormatter("epoch_second", SECONDS_FORMATTER1,
builder -> builder.parseDefaulting(ChronoField.NANO_OF_SECOND, 999_999_999L),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,24 @@
*/
package org.elasticsearch.common.time;

import java.util.Calendar;
import java.util.Locale;
import java.util.spi.CalendarDataProvider;

/**
* Locale constants to be used across elasticsearch code base.
* java.util.Locale.ROOT should not be used as it defaults start of the week incorrectly to Sunday.
*/
public final class IsoLocale {
private IsoLocale() {
throw new UnsupportedOperationException();
public class IsoCalendarDataProvider extends CalendarDataProvider {

@Override
public int getFirstDayOfWeek(Locale locale) {
return Calendar.MONDAY;
}

/**
* We want to use Locale.ROOT but with a start of the week as defined in ISO8601 to be compatible with the behaviour in joda-time
* https://github.com/elastic/elasticsearch/issues/42588
* @see java.time.temporal.WeekFields#of(Locale)
*/
public static final Locale ROOT = new Locale.Builder()
.setLocale(Locale.ROOT)
.setUnicodeLocaleKeyword("fw", "mon").build();
@Override
public int getMinimalDaysInFirstWeek(Locale locale) {
return 4;
}

@Override
public Locale[] getAvailableLocales() {
return new Locale[]{Locale.ROOT};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@
import org.elasticsearch.common.time.DateFormatters;
import org.elasticsearch.common.time.DateMathParser;
import org.elasticsearch.common.time.DateUtils;
import org.elasticsearch.common.time.IsoLocale;
import org.elasticsearch.common.util.LocaleUtils;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.support.XContentMapValues;
Expand Down Expand Up @@ -136,7 +135,7 @@ public static class Builder extends FieldMapper.Builder<Builder, DateFieldMapper
public Builder(String name) {
super(name, new DateFieldType(), new DateFieldType());
builder = this;
locale = IsoLocale.ROOT;
locale = Locale.ROOT;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import org.elasticsearch.common.collect.CopyOnWriteHashMap;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.time.IsoLocale;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.support.XContentMapValues;
Expand All @@ -42,6 +41,7 @@
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Locale;
import java.util.Map;

public class ObjectMapper extends Mapper implements Cloneable {
Expand Down Expand Up @@ -530,7 +530,7 @@ public void toXContent(XContentBuilder builder, Params params, ToXContent custom
builder.field("type", CONTENT_TYPE);
}
if (dynamic != null) {
builder.field("dynamic", dynamic.name().toLowerCase(IsoLocale.ROOT));
builder.field("dynamic", dynamic.name().toLowerCase(Locale.ROOT));
}
if (enabled != Defaults.ENABLED) {
builder.field("enabled", enabled);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.time.DateFormatter;
import org.elasticsearch.common.time.DateMathParser;
import org.elasticsearch.common.time.IsoLocale;
import org.elasticsearch.common.util.LocaleUtils;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
Expand Down Expand Up @@ -80,7 +79,7 @@ public static class Defaults {

public static class Builder extends FieldMapper.Builder<Builder, RangeFieldMapper> {
private Boolean coerce;
private Locale locale = IsoLocale.ROOT;
private Locale locale = Locale.ROOT;
private String pattern;

public Builder(String name, RangeType type) {
Expand Down Expand Up @@ -424,7 +423,7 @@ && fieldType().dateTimeFormatter().pattern().equals(DateFieldMapper.DEFAULT_DATE
}
if (fieldType().rangeType == RangeType.DATE
&& (includeDefaults || (fieldType().dateTimeFormatter() != null
&& fieldType().dateTimeFormatter().locale() != IsoLocale.ROOT))) {
&& fieldType().dateTimeFormatter().locale() != Locale.ROOT))) {
builder.field("locale", fieldType().dateTimeFormatter().locale());
}
if (includeDefaults || coerce.explicit()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.elasticsearch.common.SuppressForbidden;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.time.DateFormatter;
import org.elasticsearch.common.time.DateFormatters;
import org.elasticsearch.common.time.DateUtils;
import org.joda.time.DateTime;

Expand Down Expand Up @@ -50,7 +51,6 @@
import java.time.temporal.TemporalQuery;
import java.time.temporal.TemporalUnit;
import java.time.temporal.ValueRange;
import java.time.temporal.WeekFields;
import java.util.Locale;
import java.util.Objects;

Expand Down Expand Up @@ -474,14 +474,14 @@ public int getSecondOfMinute() {

@Deprecated
public int getWeekOfWeekyear() {
logDeprecatedMethod("getWeekOfWeekyear()", "get(WeekFields.ISO.weekOfWeekBasedYear())");
return dt.get(WeekFields.ISO.weekOfWeekBasedYear());
logDeprecatedMethod("getWeekOfWeekyear()", "get(DateFormatters.WEEK_FIELDS.weekOfWeekBasedYear())");
return dt.get(DateFormatters.WEEK_FIELDS.weekOfWeekBasedYear());
}

@Deprecated
public int getWeekyear() {
logDeprecatedMethod("getWeekyear()", "get(WeekFields.ISO.weekBasedYear())");
return dt.get(WeekFields.ISO.weekBasedYear());
logDeprecatedMethod("getWeekyear()", "get(DateFormatters.WEEK_FIELDS.weekBasedYear())");
return dt.get(DateFormatters.WEEK_FIELDS.weekBasedYear());
}

@Deprecated
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
org.elasticsearch.common.time.IsoCalendarDataProvider
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@
import java.time.temporal.TemporalAccessor;
import java.util.Locale;

import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.core.IsEqual.equalTo;

public class JavaJodaTimeDuellingTests extends ESTestCase {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,32 @@

public class DateFormattersTests extends ESTestCase {

public void testWeekBasedDates() {
// as per WeekFields.ISO first week starts on Monday and has minimum 4 days
DateFormatter dateFormatter = DateFormatters.forPattern("YYYY-ww");

// first week of 2016 starts on Monday 2016-01-04 as previous week in 2016 has only 3 days
assertThat(DateFormatters.from(dateFormatter.parse("2016-01")) ,
equalTo(ZonedDateTime.of(2016,01,04, 0,0,0,0,ZoneOffset.UTC)));

// first week of 2015 starts on Monday 2014-12-29 because 4days belong to 2019
assertThat(DateFormatters.from(dateFormatter.parse("2015-01")) ,
equalTo(ZonedDateTime.of(2014,12,29, 0,0,0,0,ZoneOffset.UTC)));


// as per WeekFields.ISO first week starts on Monday and has minimum 4 days
dateFormatter = DateFormatters.forPattern("YYYY");

// first week of 2016 starts on Monday 2016-01-04 as previous week in 2016 has only 3 days
assertThat(DateFormatters.from(dateFormatter.parse("2016")) ,
equalTo(ZonedDateTime.of(2016,01,04, 0,0,0,0,ZoneOffset.UTC)));

// first week of 2015 starts on Monday 2014-12-29 because 4days belong to 2019
assertThat(DateFormatters.from(dateFormatter.parse("2015")) ,
equalTo(ZonedDateTime.of(2014,12,29, 0,0,0,0,ZoneOffset.UTC)));
}


// this is not in the duelling tests, because the epoch millis parser in joda time drops the milliseconds after the comma
// but is able to parse the rest
// as this feature is supported it also makes sense to make it exact
Expand Down Expand Up @@ -115,7 +141,7 @@ public void testParsersWithMultipleInternalFormats() throws Exception {
}

public void testLocales() {
assertThat(DateFormatters.forPattern("strict_date_optional_time").locale(), is(IsoLocale.ROOT));
assertThat(DateFormatters.forPattern("strict_date_optional_time").locale(), is(Locale.ROOT));
Locale locale = randomLocale(random());
assertThat(DateFormatters.forPattern("strict_date_optional_time").withLocale(locale).locale(), is(locale));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,20 @@ public void testOverridingLocaleOrZoneAndCompositeRoundUpParser() {
assertDateEquals(gotMillis, "297276785531", "297276785531");
}

public void testWeekDates() {
DateFormatter formatter = DateFormatter.forPattern("YYYY-ww");
assertDateMathEquals(formatter.toDateMathParser(), "2016-01", "2016-01-04T23:59:59.999Z", 0, true, ZoneOffset.UTC);

formatter = DateFormatter.forPattern("YYYY");
assertDateMathEquals(formatter.toDateMathParser(), "2016", "2016-01-04T23:59:59.999Z", 0, true, ZoneOffset.UTC);

formatter = DateFormatter.forPattern("YYYY-ww");
assertDateMathEquals(formatter.toDateMathParser(), "2015-01", "2014-12-29T23:59:59.999Z", 0, true, ZoneOffset.UTC);

formatter = DateFormatter.forPattern("YYYY");
assertDateMathEquals(formatter.toDateMathParser(), "2015", "2014-12-29T23:59:59.999Z", 0, true, ZoneOffset.UTC);
}

public void testBasicDates() {
assertDateMathEquals("2014-05-30", "2014-05-30T00:00:00.000");
assertDateMathEquals("2014-05-30T20", "2014-05-30T20:00:00.000");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ public void testChangeLocale() throws IOException {
mapper.parse(new SourceToParse("test", "1", BytesReference
.bytes(XContentFactory.jsonBuilder()
.startObject()
.field("field", "Mi., 06 Dez. 2000 02:55:00 -0800")
.field("field", "Mi, 06 Dez 2000 02:55:00 -0800")
.endObject()),
XContentType.JSON));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -213,12 +213,12 @@ public void testSecondOfMinute() {

public void testWeekOfWeekyear() {
assertMethodDeprecation(() -> assertThat(javaTime.getWeekOfWeekyear(), equalTo(jodaTime.getWeekOfWeekyear())),
"getWeekOfWeekyear()", "get(WeekFields.ISO.weekOfWeekBasedYear())");
"getWeekOfWeekyear()", "get(DateFormatters.WEEK_FIELDS.weekOfWeekBasedYear())");
}

public void testWeekyear() {
assertMethodDeprecation(() -> assertThat(javaTime.getWeekyear(), equalTo(jodaTime.getWeekyear())),
"getWeekyear()", "get(WeekFields.ISO.weekBasedYear())");
"getWeekyear()", "get(DateFormatters.WEEK_FIELDS.weekBasedYear())");
}

public void testYearOfCentury() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1579,21 +1579,21 @@ public void testRangeQueryWithLocaleMapping() throws Exception {
.endObject().endObject().endObject()));

indexRandom(true,
client().prepareIndex("test", "type1", "1").setSource("date_field", "Mi., 06 Dez. 2000 02:55:00 -0800"),
client().prepareIndex("test", "type1", "2").setSource("date_field", "Do., 07 Dez. 2000 02:55:00 -0800")
client().prepareIndex("test", "type1", "1").setSource("date_field", "Mi, 06 Dez 2000 02:55:00 -0800"),
client().prepareIndex("test", "type1", "2").setSource("date_field", "Do, 07 Dez 2000 02:55:00 -0800")
);

SearchResponse searchResponse = client().prepareSearch("test")
.setQuery(QueryBuilders.rangeQuery("date_field")
.gte("Di., 05 Dez. 2000 02:55:00 -0800")
.lte("Do., 07 Dez. 2000 00:00:00 -0800"))
.gte("Di, 05 Dez 2000 02:55:00 -0800")
.lte("Do, 07 Dez 2000 00:00:00 -0800"))
.get();
assertHitCount(searchResponse, 1L);

searchResponse = client().prepareSearch("test")
.setQuery(QueryBuilders.rangeQuery("date_field")
.gte("Di., 05 Dez. 2000 02:55:00 -0800")
.lte("Fr., 08 Dez. 2000 00:00:00 -0800"))
.gte("Di, 05 Dez 2000 02:55:00 -0800")
.lte("Fr, 08 Dez 2000 00:00:00 -0800"))
.get();
assertHitCount(searchResponse, 2L);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,11 @@
/**
* Due to changes in JDK9 where locale data is used from CLDR, the licence message will differ in jdk 8 and jdk9+
* https://openjdk.java.net/jeps/252
* We run ES with -Djava.locale.providers=SPI,COMPAT and same option has to be applied when running this test from IDE
*/
public class LicenseServiceTests extends ESTestCase {

public void testLogExpirationWarningOnJdk9AndNewer() {
assumeTrue("this is for JDK9+", JavaVersion.current().compareTo(JavaVersion.parse("9")) >= 0);

long time = LocalDate.of(2018, 11, 15).atStartOfDay(ZoneOffset.UTC).toInstant().toEpochMilli();
final boolean expired = randomBoolean();
final String message = LicenseService.buildExpirationMessage(time, expired).toString();
if (expired) {
assertThat(message, startsWith("LICENSE [EXPIRED] ON [THU, NOV 15, 2018].\n"));
} else {
assertThat(message, startsWith("License [will expire] on [Thu, Nov 15, 2018].\n"));
}
}

public void testLogExpirationWarningOnJdk8() {
public void testLogExpirationWarning() {
assumeTrue("this is for JDK8 only", JavaVersion.current().equals(JavaVersion.parse("8")));
Copy link
Contributor

Choose a reason for hiding this comment

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

@pgomulka I think this part of the change might have been incorrect...
We collapsed the 2 tests into 1, but that one test is set to only run on JDK8. I think this assumeTrue should be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tvernum you are right, the same applies to 7.x branch as the output should be the same.
thanks for taking a look!


long time = LocalDate.of(2018, 11, 15).atStartOfDay(ZoneOffset.UTC).toInstant().toEpochMilli();
Expand All @@ -45,5 +33,4 @@ public void testLogExpirationWarningOnJdk8() {
assertThat(message, startsWith("License [will expire] on [Thursday, November 15, 2018].\n"));
}
}

}
Loading