-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Allow parsing timezone without fully provided time #50178
Conversation
Pinging @elastic/es-core-infra (:Core/Infra/Core) |
I'm not sure this is something we want to do. I don't think ISO 8601 allows for timezone without a time. I thought we were moving towards deprecating joda specific functionality? This especially seems a weird feature to support since based on your description it appears joda doesn't actually support this as it intended, hence why the dueling tests are commented out. |
FYI, you'll need to merge in the latest changes from |
@elasticmachine update branch |
…lka/elasticsearch into fix/joda_partial_timezone
|
||
public void testTimezoneParsing() { | ||
//with Timezone | ||
// assertSameDateAs("2016-11-30T+01", "strict_date_optional_time", "strict_date_optional_time"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we perhaps want a comment here about Joda as well?
Thank you for fixing this! |
@jamshid as per my comment here https://github.com/elastic/elasticsearch/pull/50178/files#diff-216a9f905247015f48298c3b6b1bb3a0R68 please bear in mind, I need to confirm with ISO8601 if this should be merged at all. |
@elasticmachine update branch |
@rjernst it looks that this should indeed be allowed as |
I actually re-read the spec and it looks that timezone can only by applied with time.
where
allows reduced accuracy of local time. This is defined in
and allows only Therefore 2016-11-30T12+01 is correct |
…icsearch into fix/joda_partial_timezone
@elasticmachine update branch |
strict_date_optional_time changes to have optional minute part. It already allowed optional second and fraction of second part. This allows parsing 2018-01-01T00+01 , 2018-01-01T00:00+01 , 2018-01-01T00:00:00+01 , 2018-01-01T00:00:00.000+01 It won't allow parsing a timezone without an hour part as this is not allowed by iso8601 spec closes elastic#49351
…50740) strict_date_optional_time changes to have optional minute part. It already allowed optional second and fraction of second part. This allows parsing 2018-01-01T00+01 , 2018-01-01T00:00+01 , 2018-01-01T00:00:00+01 , 2018-01-01T00:00:00.000+01 It won't allow parsing a timezone without an hour part as this is not allowed by iso8601 spec closes #49351
strict_date_optional_time changes to have optional minute part. It already allowed optional second and fraction of second part. This allows parsing 2018-01-01T00+01 , 2018-01-01T00:00+01 , 2018-01-01T00:00:00+01 , 2018-01-01T00:00:00.000+01 It won't allow parsing a timezone without an hour part as this is not allowed by iso8601 spec closes elastic#49351
strict_date_optional_time in Joda allowed to provide just a timezone without fully provided time. So dates like
2018-01-01T00+01 , 2018-01-01T00:00+01 , 2018-01-01T00:00:00+01 , 2018-01-01T00:00:00.000+01
were allowed.This should be fixed in java.time implementation too.
However, Joda was incorrectly parsing
2018-01-01T+01
when no time part was provided. It is because +01 which should be considered an offset, is parsed as a signed hour part.java.time implementation does not suffer from this and correctly parses this date. We are not allowing this though, as this is not allowed be iso8601 spec.
In essence this PR is only fixing parsing for
2018-01-01T00+01
. Other formats were allowed already.closes #49351