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

Make timezone in "strict_date_time" format optional #16701

Closed
wants to merge 2 commits into from
Closed

Make timezone in "strict_date_time" format optional #16701

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
85dcb45
Make timezone in strict_date_time optional
Nov 22, 2024
d54ff3c
Changelog
Nov 22, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- Support prefix list for remote repository attributes([#16271](https://github.com/opensearch-project/OpenSearch/pull/16271))
- Add new configuration setting `synonym_analyzer`, to the `synonym` and `synonym_graph` filters, enabling the specification of a custom analyzer for reading the synonym file ([#16488](https://github.com/opensearch-project/OpenSearch/pull/16488)).
- Add stats for remote publication failure and move download failure stats to remote methods([#16682](https://github.com/opensearch-project/OpenSearch/pull/16682/))
- Make timezone optional for strict_date_time format ([#16701](https://github.com/opensearch-project/OpenSearch/pull/16701))

### Dependencies
- Bump `com.google.cloud:google-cloud-core-http` from 2.23.0 to 2.47.0 ([#16504](https://github.com/opensearch-project/OpenSearch/pull/16504))
Expand Down
4 changes: 4 additions & 0 deletions server/src/main/java/org/opensearch/common/time/DateFormatters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -732,6 +732,8 @@ public class DateFormatters {
/*
* Returns a formatter that combines a full date and time, separated by a 'T'
* (uuuu-MM-dd'T'HH:mm:ss.SSSZZ).
* Timezone is optional and defaults to UTC,
* as Python's isoformat() can outputs ISO 8601-compliant dates that lack a timezone.
*/
private static final DateFormatter STRICT_DATE_TIME = new JavaDateFormatter(
"strict_date_time",
Expand All @@ -741,7 +743,9 @@ public class DateFormatters {
.toFormatter(Locale.ROOT)
.withResolverStyle(ResolverStyle.STRICT),
new DateTimeFormatterBuilder().append(STRICT_DATE_FORMATTER)
.optionalStart()
Copy link
Collaborator

Choose a reason for hiding this comment

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

One of the reasons it is called STRICT_DATE_TIME is because all components are required, changing that would lead to surprising results. We have a large number of relaxed formats like STRICT_DATE_OPTIONAL_TIME, STRICT_DATE_OPTIONAL_TIME_NANOS, those are explicit on optionality of some components.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed that we probably don't want to change the definition of what STRICT_DATE_TIME means.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems reasonable. In this case I'll close this PR and recommend that the person who created the original issue changes their mapping to use a different format.

.append(TIME_ZONE_FORMATTER_NO_COLON)
.optionalEnd()
.toFormatter(Locale.ROOT)
.withResolverStyle(ResolverStyle.STRICT)
);
Expand Down
16 changes: 16 additions & 0 deletions server/src/test/java/org/opensearch/common/time/DateFormattersTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -828,6 +828,22 @@ public void testCamelCaseDeprecation() {
}
}

public void testStrictDateTimeDefaultsToUTC() {
DateFormatter dateFormatter = DateFormatter.forPattern("strict_date_time");
// "strict_date_time" formatter should be able to parse with and without a timezone specified, as both are valid in ISO 8601
// See https://github.com/opensearch-project/OpenSearch/issues/16673
assertEquals(
ZonedDateTime.of(2024, 11, 21, 0, 0, 0, 0, ZoneOffset.UTC),
DateFormatters.from(dateFormatter.parse("2024-11-21T00:00:00Z"))
);

// Should default to UTC without a timezone specified
assertEquals(
ZonedDateTime.of(2024, 11, 21, 0, 0, 0, 0, ZoneOffset.UTC),
DateFormatters.from(dateFormatter.parse("2024-11-21T00:00:00"))
);
}

void assertDateTimeEquals(String toTest, DateFormatter candidateParser, DateFormatter baselineParser) {
Instant gotInstant = DateFormatters.from(candidateParser.parse(toTest)).toInstant();
Instant expectedInstant = DateFormatters.from(baselineParser.parse(toTest)).toInstant();
Expand Down
Loading