Deserialization of timestamps with UTC timezone to LocalDateTime doesn't yield correct time

[alankila]

Hello,

I'm using LocalDateTime to represent my timestamps on server side, but would like to transmit data in instant-like format in and out of my API, using the Z timezone. One of the advantages of doing this is that all clients treat the string as referring to the same instance, e.g. "new Date(string)" will result in same interpretation in all browsers, and built-in function such as date.toISOString() exists for formatting the string into an instant in time.

I ran into this behavior in Jackson during deserializing:

jackson-modules-java8/datetime/src/main/java/com/fasterxml/jackson/datatype/jsr310/deser/LocalDateTimeDeserializer.java

Line 75 in 66dce02

return LocalDateTime.ofInstant(Instant.parse(string), ZoneOffset.UTC);

The behavior of this line is equivalent to just removing the Z character at end of timestamp and treating the rest as LocalDateTime, e.g. it will return the LocalDateTime in the UTC timezone. Wouldn't it make a lot more sense to convert UTC timestamp to LocalDateTime using system's own timezone, i.e.

LocalDateTime.ofInstant(Instant.parse(string), ZoneId.systemDefault())

so that the time as written by JavaScript would be understood as the same time after Jackson is processing it? I don't think a lot of people expect that a LocalDateTime should be interpreted to be in UTC timezone. For most of us, it's expected to be exactly what it says on the class name: a date and time in the local timezone.

[kupci]

This makes sense, and as a further confirmation that this is should be the expected behavior, if you call now() on a LocalDateTime instance, it will return the

the current date-time from the system clock in the default time-zone.
This will query the system clock in the default time-zone to obtain the current date-time.

Though how to implement change, since would be a breaking change? There is the ADJUST_DATES_TO_CONTEXT_TIME_ZONE feature already (see issue #118) which uses this logic to get the Zone (if the feature is enabled):

private ZoneId getZone(DeserializationContext context)
{
    // Instants are always in UTC, so don't waste compute cycles
    return (_valueClass == Instant.class) ? null : context.getTimeZone().toZoneId();
}

One thing that isn't clear is whether Oracle's idea of default time-zone is the same as Jackson's context time zone (above). If it is, the existing feature could be used and implemented for LocalDateTimes, otherwise, another solution would be needed.

[cowtowncoder]

Jackson does not use the local timezone as default for anything, anywhere, so I do not like the idea of starting to do that in some cases at this point.

But even more fundamental problem I think is this: "local" values are not bound to ANY timezone: they are abstract. They should not get parsed to anything with timezone; and ideally I actually think that parsing should just FAIL if timezone information is encountered. If timezone information is to be used, proper type -- "zoned" date/time -- needs to be used. LocalXxx just will not do.
That sounds like what should happen in "strict" mode.

But for "lenient" handling (which is currently the default), I suppose I could see either parsing into ZonedDateTime, then only using date/time part... or. Just dropping the timezone. I actually am not sure if former serves user better -- since LocalDateTime will not have timezone information (sort of similar how java.util.Date does not have it, except... not quite) this will inevitably lead to information loss.

[kupci]

If timezone information is to be used, proper type -- "zoned" date/time -- needs to be used

Ah, good point. This strikes me as the cleanest and simplest of solutions here.

.

[ewirch]

I actually think that parsing should just FAIL if timezone information is encountered

Exactly. Treating zone-less and zone-full dates the same is just wrong. I looked up the history. FasterXML/jackson-datatype-jsr310#68 introduced the change, copying code from FasterXML/jackson-datatype-jsr310#56. Rationale behind the change:

what I needed was to be able to parse this, coming from JavaScript:

new Date().toJSON()
"2016-01-18T14:09:30.163Z"

But the change did it wrong. JavaScript's Date is time zone aware. Asuming the user's device has a zone offset of +02:00, and the local time is 18:00, toJSON() would serialize to 2016-01-18T16:00:00.0Z. Right now, parsing LocalDateTime Jackson, running on the server, would parse this to 16:00 local time. Assuming the server implementation is correct, the server will apply user's zone offset correct (it assumes local date!), which will result in 2016-01-18T14:00:00.0Z!

My opinion is, it shouldn't even happen for "lenient" handling, because it is simply wrong.

[kupci]

I took a look at that change (68) and it looks like that scenario you mention was never tested (only happy path) so thank you for the points, this will definitely have to be reviewed.

[cowtowncoder]

It'd be great to get a fix in for 2.12 -- has to be minor release for behavioral change in most cases, at least if there is a reasonable chance that someone somewhere is counting on current behavior (even if objectively wrong one).

[cowtowncoder]

We now have a PR (yay!) -- so do we still think this is a good idea, without need to configure anything? I just want to double-check if anyone (@kupci ?) has further concerns.

[kupci]

ideally I actually think that parsing should just FAIL if timezone information is encountered. If timezone information is to be used, proper type -- "zoned" date/time -- needs to be used.

@cowtowncoder Given your point, and also @ewirch 's, I think this is the right way to go.

[cowtowncoder]

Ok, so... I changed my mind. :)

So: code WILL allow "local-time plus 'Z' at the end" if (and only if!) "lenient" mode is enabled. This to still solve #68 case.
But no attempt is made for other offsets/timezones; and in strict case specific exception message is used for "Zulu" case.
Further I added tests to ensure that value produced is correct, regardless of mapper-configured default timezone.

I think this makes sense all in all, and will go in 2.12.0. Thanks to everyone who participated in discussion!