Support for OffsetDateTime and ZonedDateTime in jackson-jr-extension-javatime
#201
Conversation
…TimeValueReader not to crash when a string was fed that accidentally included a timezone.
|
First of all: thank you for the contribution! It sounds like useful addition. It looks like there's a test failure (possible related to what is mentioned as non-compatible change)? |
|
I need to review this with some more time, but one process thing we need (if not done earlier), before I can merge it (pending review etc), is to get CLA. It's here: https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf and the usual was is to print it, fill & sign, scan/photo, email to Looking forward to getting this merged! |
|
I will look into CLA thanks. It looks like my own unit test failed because I did not take into account that the server obviously does not share my time zone. |
|
FYI: I submitted the CLA, fixed the unit test, and made some extra improvements to be even kinder on ISO 8601 input. |
|
@pjfanning @JooHyukKim WDYT? (CLA exists fwtw just need to review and evaluate change itself) |
|
Will review tonight, idea itself seems... yes, why not? |
...time/src/main/java/com/fasterxml/jackson/jr/extension/javatime/LocalDateTimeValueReader.java
Outdated
Show resolved
Hide resolved
|
It will be good to add even a bit of documentation, like version Otherwise change seems okay |
The original code hardly had any Javadoc comments, so I sort of went with it. If you'd prefer me to write some Javadoc code for each file that is lacking, I would be happy to do so! |
|
Yeah My bigger concerns have to do with things like timezones, defaulting, however. |
.../src/main/java/com/fasterxml/jackson/jr/extension/javatime/JavaTimeReaderWriterProvider.java
Outdated
Show resolved
Hide resolved
| protected static final DateTimeFormatter LOCAL_FORMATTER = createFormatter(false); | ||
|
|
||
| public JavaTimeReaderWriterProvider() { | ||
| _fallbackLocalZoneId = ZoneId.systemDefault(); |
There was a problem hiding this comment.
This seems like it is different from standard Jackson defaulting which for time zones/offsets tries to use global "neutral" default (UTC where possible, GMT otherwise), to avoid relying on local defaults that are arbitrary.
But I am not sure such choice exists for ZoneId?
There was a problem hiding this comment.
I understand what you mean. I personally think the default for a LocalDateTime object should probably always be in the local time for the user. Which this probably comes closest to. But it wouldn't hurt setting it to UTC to be safe, considering the user can change it.
I have been doubting another time zone related segment myself, which is the includeUtcDefault boolean that occurs inside of JavaTimeReaderWriterProvider. Because that segment defaults ZonedDateTime and OffsetDateTimes to UTC when no time zone is included in its ISO 8601 string. I was thinking that maybe it would make more sense to default to the time zone that the user configured. But I am sort of undecided on that.
There was a problem hiding this comment.
Thought this over. Wikipedia says the following:
If no UTC relation information is given with a time representation, the time is assumed to be in local time.
So I suggest I'll change the formatter to use the user's selected time zone for dates that do not contain offset information. That will automatically mean the end of the static formatter.
There was a problem hiding this comment.
The static formatters are still here. I found it to be really difficult to apply offsets automatically from the DateTimeFormatter. I did change the static method to be private though.
…ed a small change to be kind to null values.
|
I found out that my code (and the original too) wasn't very kind on null values so I went ahead and changed that while I was adding Javadoc documentation. |
...me/src/main/java/com/fasterxml/jackson/jr/extension/javatime/DefaultDateTimeValueReader.java
Outdated
Show resolved
Hide resolved
|
@StanB-EKZ looks good, sounds good. With minor touches ready to go. LMK when you think pr is complete and I can merge it in, right in time for 2.20.0 release (soon). |
…liant behavior for ISO 8601 strings that lack an offset.
|
@cowtowncoder I made more changes than I wanted to, but I think I'm probably there now. I'm not super happy about the offset check that I introduced in |
|
@StanB-EKZ Thank you for doing all this. I hope to get this merged ASAP (bit overloaded but excited to get this in 2.20 release!) |
cowtowncoder
changed the title
OffsetDateTime and ZonedDateTime in jackson-jr-extension-javatime
| import com.fasterxml.jackson.jr.ob.JSON; | ||
| import com.fasterxml.jackson.jr.ob.JSONObjectException; | ||
|
|
||
| public class LocalDateTimeReaderTest { |
There was a problem hiding this comment.
Forgot to comment: these should have been split into different test classes (or test class renamed) -- by read/write, type.
Will rename post-merge now.
There was a problem hiding this comment.
You are right, I did notice that but completely forgot. My apologies.
Added support for OffsetDateTime and ZonedDateTime to jackson-jr-extension-javatime.
The most striking change here is the introduction of a static
FORMATTERfield inJavaTimeReaderWriterProvider. This field is used by the ValueReader classes. I introduced it because ISO 8601 string representations that included a time offset could causeLocalDateTimeValueReaderto throw a DateTimeParsingException. Apparently reading requires a slightly more forgiving approach than writing.The static formatter can parse ISO 8601 dates, optionally containing: milliseconds, hour offsets or zone IDs.
This did require me to get rid of the
withDateTimeFormatter()method inJavaTimeReaderWriterProvider. Because allowing users to change the format, could let them break ISO 8601 related behavior. If anyone has a good idea of how to reliably restore that behavior, I would be happy to.I also added a fairly straightforward unit test to check if parsing and formatting works as expected.