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

iso 8601 date format #552

Merged
merged 2 commits into from Sep 23, 2014
Merged

iso 8601 date format #552

merged 2 commits into from Sep 23, 2014

Conversation

geronimo-iia
Copy link

Hello,

After reading several post about iso 8601 date format,( like this one http://www.w3.org/TR/NOTE-datetime for example), I think that it could be easier to parse iso 8601 date if we could use both "strict standard" and short format.

We could parse format according this:
[yyyy-MM-dd|yyyyMMdd][T(hh:mm[:ss[.sss]]|hhmm[ss[.sss]])]?[Z|[+-]hh:mm]]

For example:
2007-08-13T19:51:23.2Z
20070813T19:51:23Z
2007-08-13T195123Z
2007-08-13T19:51Z
20070813T19:51Z
20070813Z

I hope it can help a little,

Thanks for you're job, time

Best Regards

Jérôme

Jerome Guibert added 2 commits September 19, 2014 15:41
…MM-dd|yyyyMMdd][T(hh:mm[:ss[.sss]]|hhmm[ss[.sss]])]?[Z|[+-]hh:mm]]
}

// extract timezone
String timezoneId;
if (date.length() <= offset) {
throw new IndexOutOfBoundsException("No time zone indicator ");
Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be best to use some more refined exception, like ParseException of IllegalArgumentException. I know it's behavioral change, but IndexOutOfBounds is sort of low-level errror.

Copy link
Author

Choose a reason for hiding this comment

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

I'm agree with you.
But it's not an IndexOutOfBounds that client see, it's a ParseException because of line 217/218.

I can replace the line 177 by:
throw new ParseException("Failed to parse date ["' + date + "']: No time zone indicator", pos.getIndex());

But it break a little the logic of this function,

Let me known what your prefer

Copy link
Member

Choose a reason for hiding this comment

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

Ah. Ok, so it is to keep handling working the way it did -- got it. I can see if I can tweak it after merge, but if it gets translated to different exception by code that is fine.

@cowtowncoder
Copy link
Member

Sounds good to me; thank you for the contribution!

One thing I will have to ask for before merging this is a filled Contributor License Agreement (CLA), from:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

so that we know it is safe to distribute code. We have to ask one of these from all contributors to Jackson project; but fortunately only once before the first contribution.

If you could print, fill-in+sign and scan it, and send resulting image (pdf, png, jpeg whatever) to info at fasterxml.com that would be great.

Apologies for this, but it is one (and only!) formality we need here. :)

@geronimo-iia
Copy link
Author

You're welcome,
I will send you the CLA soon

@geronimo-iia
Copy link
Author

Cla send :)

cowtowncoder added a commit that referenced this pull request Sep 23, 2014
@cowtowncoder cowtowncoder merged commit d28c947 into FasterXML:master Sep 23, 2014
cowtowncoder added a commit that referenced this pull request Sep 23, 2014
@newyankeecodeshop
Copy link

I think this change is incorrect for ISO dates without a time component. In the examples above, "20070813Z" is not a valid date-only value. There should be no time zone in a date-only value, since the value has no time component at all. Logically, it's that day in any time zone, and if it's being converted to a date/time instance, the conversion would always happen in the local time zone.
Also, by my reading of the ISO 8601 spec and RFC 3339, the "Z" designation should only be applied to values that have a time component. (http://www.ietf.org/rfc/rfc3339.txt)

@cowtowncoder
Copy link
Member

@newyankeecodeshop I am not sure which way to go with this: while I can see your argument, it is also true that since in Java date/time values are typically stored as "milliseconds-since-epoch" notation, and thereby value of a date-only value may well change depending on timezone information.
Put another way: there is difference between date-only and date-time-for-midnight short cut. And it may not be obvious which usage is in effect.

But the point about ISO-8601 spec and RFC would be valid; if they suggest use of timezone offset / 'Z' is not legal then it probably was not a good idea to allow it.

And then again, if such usage is wide-spread, we have to balance inter-operability (of being more liberal in accepting values, conservative in what we output) and standards-compliancy.

What do you think would be the right thing to do, considering that this behavior is included in 2.5.0?

@newyankeecodeshop
Copy link

@cowtowncoder You're right that Java Date objects represent an instant in time. In my experience, to make date-only values work in that environment, they always have to be parsed in the local time, so that the Date instance will be midnight in the local time zone. Otherwise, when the date is formatted for display, it can possibly show as being in a different day. For example, since the American Eastern time zone is GMT-5:00 (or -4:00 without daylight savings), if a date-only value is parsed and turned that day at midnight GMT, when shown to the user in their time zone, it will be the previous day.
"2007-08-13" becomes "2007-08-13T00:00:00Z" becomes "2007-08-12T20:00:00-04:00" becomes "12th August 2007" when formatted in the Eastern time zone.

I agree there is a semantic issue because a date-only value is being stored in a type that stores date+time values. This is probably a reason why we should encourage folks to use Joda LocalDate instead of j.u.Date in all cases, since it removes the uncertainty. At any rate, I think if Jackson has this feature, it should allow the 'Z' to be optional and treat the value as midnight in the local time zone.

@cowtowncoder
Copy link
Member

@newyankeecodeshop Fair enough. If missing Z is not allowed by current functionality, a new issue may be filed to resolve that..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants