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

Make ISO8601 date parser accept timezone when time does not have seconds #41896

Merged
merged 3 commits into from May 8, 2019

Conversation

droberts195
Copy link
Contributor

Prior to this change the ISO8601 date parser would only
parse an optional timezone if seconds were specified.
This change moves the timezone to the same level of
optional components as hour, so that timestamps without
minutes or seconds may optionally contain a timezone.
It also adds a unit test to cover all the supported
formats.

Prior to this change the ISO8601 date parser would only
parse an optional timezone if seconds were specified.
This change moves the timezone to the same level of
optional components as hour, so that timestamps without
minutes or seconds may optionally contain a timezone.
It also adds a unit test to cover all the supported
formats.
minutes or seconds may have a timezone.
@droberts195 droberts195 added >bug :Core/Infra/Core Core issues without another label v8.0.0 v7.2.0 labels May 7, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@@ -178,7 +178,7 @@
/**
* Returns a ISO 8601 compatible date time formatter and parser.
* This is not fully compatible to the existing spec, which would require far more edge cases, but merely compatible with the
* existing joda time ISO data formater
* existing joda time ISO date formatter
Copy link
Contributor Author

Choose a reason for hiding this comment

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

but merely compatible with the existing joda time ISO date formatter

I have not tried to go beyond this, but note that the Joda ISO8601 parser did accept timezone for times that didn't include minutes and seconds - see https://www.joda.org/joda-time/apidocs/org/joda/time/format/ISODateTimeFormat.html#timeParser--. That is why I labelled this PR as >bug rather than >enhancement. (But if that's controversial I'm happy to relabel as >enhancement.)

@droberts195 droberts195 requested a review from pgomulka May 7, 2019 13:50
Copy link
Contributor

@pgomulka pgomulka left a comment

Choose a reason for hiding this comment

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

Did we have a bug to allow time zone to follow Hour or MInute?
I am not sure we can relax so much. I thought ISO-8601 is more strict about what field has to be present in local time

// timezone not allowed with just date
formatter.format(formatter.parse("2018-05-15"));

formatter.format(formatter.parse("2018-05-15T17"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure we need this? I am not sure if iso8601 is really specifying this all, but java.time'` DateTimeFormatter.ISO_DATE_TIME is a bit more strict. it would fail for this and many others:

DateTimeFormatter.ISO_DATE_TIME.parse("2018-05-15T17")
DateTimeFormatter.ISO_DATE_TIME.parse("2018-05-15T17+01:00")
etc..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

java.time DateTimeFormatter.ISO_DATE_TIME is a bit more strict

Yes, but Joda time's ISO8601 parser was not as strict. That's why a custom "iso8601" parser was added that is used instead of Java's one. The comment says this custom parser is supposed to be compatible with Joda's parser. This was because people who were successfully parsing ISO8601 timestamps in 6.x would not necessarily be able to do so in 7.x. So the custom parser currently adds support for either comma or decimal point as the fractional seconds separator, and colon or no colon in the timezone specifier, but before this PR it doesn't quite cover all the cases that Joda's ISO 8601 parser covers.

@droberts195
Copy link
Contributor Author

Did we have a bug to allow time zone to follow Hour or MInute?

Not a customer bug, but I found this while testing the ML find file structure endpoint.

I thought ISO-8601 is more strict about what field has to be present

https://en.wikipedia.org/wiki/ISO_8601#Combined_date_and_time_representations says "Either the seconds, or the minutes and seconds, may be omitted from the basic or extended time formats for greater brevity but decreased accuracy".

Copy link
Contributor

@pgomulka pgomulka left a comment

Choose a reason for hiding this comment

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

I see, it makes sens to me now. Approved.
Do you think we should extend JavaJodaDueling test to make sure all test cases behave as they were with joda?

@droberts195
Copy link
Contributor Author

Do you think we should extend JavaJodaDueling test to make sure all test cases behave as they were with joda?

Sure - I added extra tests in cf53119

@droberts195
Copy link
Contributor Author

Jenkins run elasticsearch-ci/1

2 similar comments
@droberts195
Copy link
Contributor Author

Jenkins run elasticsearch-ci/1

@droberts195
Copy link
Contributor Author

Jenkins run elasticsearch-ci/1

@droberts195 droberts195 merged commit cce1a88 into elastic:master May 8, 2019
@droberts195 droberts195 deleted the fix_iso8601_parsing branch May 8, 2019 12:47
droberts195 added a commit that referenced this pull request May 8, 2019
…nds (#41896)

Prior to this change the ISO8601 date parser would only
parse an optional timezone if seconds were specified.
This change moves the timezone to the same level of
optional components as hour, so that timestamps without
minutes or seconds may optionally contain a timezone.
It also adds a unit test to cover all the supported
formats.
Megamiun pushed a commit to Megamiun/elasticsearch that referenced this pull request May 18, 2019
…nds (elastic#41896)

Prior to this change the ISO8601 date parser would only
parse an optional timezone if seconds were specified.
This change moves the timezone to the same level of
optional components as hour, so that timestamps without
minutes or seconds may optionally contain a timezone.
It also adds a unit test to cover all the supported
formats.
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
…nds (elastic#41896)

Prior to this change the ISO8601 date parser would only
parse an optional timezone if seconds were specified.
This change moves the timezone to the same level of
optional components as hour, so that timestamps without
minutes or seconds may optionally contain a timezone.
It also adds a unit test to cover all the supported
formats.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants