Normalize zone id during ZonedDateTime deserialization

[dscalzi]

Resolves #204

https://stackoverflow.com/a/39507023/5295111
https://docs.oracle.com/javase/8/docs/api/java/time/ZoneId.html#normalized--

[dscalzi]

Might have to tweak the solution, thought of another test case and this should probably pass also

    @Test
    public void testDeserializationComparedToStandard2() throws Throwable
    {
        String inputString = "2021-02-01T19:49:04.0513486Z[UTC]";

        assertEquals("The value is not correct.",
                DateTimeFormatter.ISO_ZONED_DATE_TIME.parse(inputString, ZonedDateTime::from),
                READER.readValue(q(inputString)));
    }

   The value is not correct. expected:<2021-02-01T19:49:04.051348600Z[UTC]> but was:<2021-02-01T19:49:04.051348600Z>
   at com.fasterxml.jackson.datatype.jsr310.deser.ZonedDateTimeDeserTest.testDeserializationComparedToStandard2(ZonedDateTimeDeserTest.java:60)

[dscalzi]

Summary of this as it stands:

ADJUST_DATES_TO_CONTEXT_TIME_ZONE is enabled by default, which is what most users want. The system default (defined in databind) is set to TimeZone.getTimeZone("UTC").

In InstantDeserializer the timezone adjustment happens by default (again, desired behavior).

if (shouldAdjustToContextTimezone(ctxt)) {
    return adjust.apply(value, getZone(ctxt));
}

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

From here there are two possible paths.

1. The consumer does not set a timezone. In this case, the default set by jackson (TimeZone.getTimeZone("UTC")) is used. When going through getZone(), the ZoneId is not normalized, returning the equivalent of ZoneId.of("UTC"). When this is passed to the ZonedDateTime::withZoneSameInstant (value of adjust in the above snippet) we get the issue where the zone is "UTC" instead of "Z" (ie 2021-02-01T19:49:04.051348600Z[UTC] instead of 2021-02-01T19:49:04.051348600Z). Note again, that this is default without user intervention.
2. The consumer does set a timezone. The question here is should the zone that the user passes be normalized. Arguably, yes. If no normalized zone is available, it will still just use whatever the user set. However if a normalized zone is available, then the resultant ZonedDateTime will match more accurately with a standard timestamp.

Why does this happen? Review the output of the following calls.

toZoneId()

System.out.println(TimeZone.getTimeZone("UTC").toZoneId());  // UTC
System.out.println(TimeZone.getTimeZone(ZoneOffset.UTC).toZoneId()); // UTC
System.out.println(TimeZone.getTimeZone("Z").toZoneId()); // GMT
System.out.println(TimeZone.getTimeZone("GMT").toZoneId()); // GMT

normalized()

System.out.println(TimeZone.getTimeZone("UTC").toZoneId().normalized()); // Z
System.out.println(TimeZone.getTimeZone(ZoneOffset.UTC).toZoneId().normalized()); // Z
System.out.println(TimeZone.getTimeZone("Z").toZoneId().normalized()); // Z
System.out.println(TimeZone.getTimeZone("GMT").toZoneId().normalized()); // Z

ZonedDateTimes with adjustments

String inputString = "2021-02-01T19:49:04.0513486Z";
ZonedDateTime standard = DateTimeFormatter.ISO_ZONED_DATE_TIME.parse(inputString, ZonedDateTime::from);
System.out.println(standard.withZoneSameInstant(ZoneId.of("UTC"))); // 2021-02-01T19:49:04.051348600Z[UTC]
System.out.println(standard.withZoneSameInstant(ZoneId.of("GMT"))); // 2021-02-01T19:49:04.051348600Z[GMT]
System.out.println(standard.withZoneSameInstant(ZoneId.of("Z"))); // 2021-02-01T19:49:04.051348600Z

Fundamentally, there is a disconnect between UTC and GTM here. When the Zone Ids are normalized, all 4 produce the same result, which is consistent with the ISO standard.

The question then boils down to this: If the consumer has set their TimeZone manually to GMT, would they want their ZoneDateTime objects to all say 2021-02-01T19:49:04.051348600Z[GMT]? The current Jackson default would emit 2021-02-01T19:49:04.051348600Z[UTC]. Fundamentally, these are the same times. If the zone were to be normalized, then the result for both of these scenarios would be 2021-02-01T19:49:04.051348600Z. In my opinion, the normalized value is desired.

Reference: FasterXML/jackson-databind#915

I would strongly advocate for changing the default behavior. If needed, I suppose a DeserializationFeature could be implemented to disable normalization of the ZoneId if by the off chance someone is relying on the existing behavior, but it should be opted into during an upgrade.

With normalization on by default, failure of the test case I posted above is expected. With ADJUST_DATES_TO_CONTEXT_TIME_ZONE enabled, the new behavior would be that it is adjusted to a normalized zone.

To read that string in with the zone set to the non-normalized UTC zone, the following test case would work. If a DeserializationFeature to disable normalization were implemented, that could also be used to achieve the desired result.

    @Test
    public void testDeserializationComparedToStandard2() throws Throwable
    {
        String inputString = "2021-02-01T19:49:04.0513486Z[UTC]";

        ZonedDateTime converted = newMapper()
                .configure(DeserializationFeature.ADJUST_DATES_TO_CONTEXT_TIME_ZONE, false)
                .readerFor(ZonedDateTime.class).readValue(q(inputString));

        assertEquals("The value is not correct.",
                DateTimeFormatter.ISO_ZONED_DATE_TIME.parse(inputString, ZonedDateTime::from),
                converted);
    }

Hopefully this now paints a complete picture, since the issue was pretty obscure to start with. What are your thoughts @cowtowncoder?

[cowtowncoder]

Hmmh. Ok, this change fails a ton of tests; I can't quite judge if this is expected (some are, probably, but all)?

[dscalzi]

I'll check, the tests passed locally

[dscalzi]

I believe the failures are due to the system defaults on the linux box running the test. I'll see if I can mock the environment locally and figure out what it's returning. All the tests pass on my local Windows 11 (EST zone).

[dscalzi]

That change should fix it. The GitHub runner's system zone is UTC. It worked for me locally because EST normalized is still EST.

[cowtowncoder]

@dscalzi Ok great.

So, just one more thing wrt merging this: if I haven't yet asked for CLA, from:

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

I would need that submitted before merging. Only needs to be done once before the first contribution.
The usual way is to print it, fill & sign, scan/photo, email to info at fasterxml dot com.
Once I have it I think I can merge this PR for inclusion 2.15.0 release.

Looking forward to merging!

[dscalzi]

Just sent, and added a couple notes to the PR

[cowtowncoder]

Good, I like references like this!

[cowtowncoder]

Thank you again @dscalzi ! Merged in for inclusion in 2.15.0.

[cowtowncoder]

Hi @dscalzi !

I just noticed a new failure in

https://github.com/FasterXML/jackson-integration-tests/

(when starting to prepare for upcoming 2.15.0-rc1 release)

where it looks like timezone marker Z becomes ZoneId.of("Z") (instead of formerly ZoneId.of("UTC")).
Not sure if this is intentional; test in question was:

        ZonedDateTime read = MAPPER.readValue(q("2000-01-01T12:00Z"), ZonedDateTime.class);
        assertEquals("The value is not correct.",
                ZonedDateTime.of(2000, 1, 1, 12, 0, 0, 0, ZoneId.of("UTC")),
                read);

but I can "fix" it by changing "UTC" to "Z". But I didn't think "Z" would be a "real" timezone.

[dscalzi]

hey @cowtowncoder, could you try replacing ZoneId.of("UTC") with ZoneOffset.UTC?

[cowtowncoder]

Sure, will do (and now I wonder if that wasn't the one already).

[cowtowncoder]

@dscalzi yes, that fixes it. Thank you!

[indyana]

This change does seem to have broken our application. There are many places where we're comparing ZonedDateTime fields deserialized from stored JSON strings (format written out: "2023-02-02T20:23:11.057Z"), to ZonedDateTime objects created in code with ZoneId.of("UTC") .
Comparing a ZonedDateTime object created by application code to one deserialized from JSON now no longer works because the zones are not equal. (Zone of the deserialized JSON got normalized to "Z").

Fixing seems to mean replacing anywhere in code using ZoneId "UTC" with the offset zone Z, but that's a lot of application code to go through. Would be nice to have a way of disabling normalization!

org.opentest4j.AssertionFailedError:
Expected :2023-02-02T20:23:11.057Z[UTC]
Actual :2023-02-02T20:23:11.057Z

[cowtowncoder]

@indyana If you'd like to see configurability, please file a new issue as RFE, asking for configurability, referencing back to this issue. Then if anyone has time and interest they could work on adding this feature -- it does sound useful.

One practical complication here is just that module does not yet have configurability, so would probably need to add a JavaTimeModule.Feature enum and a bit of scaffolding. Aside from that should be quite straight-forward.

[dscalzi]

@indyana Depending on your usage, it would largely just be a find/replace of ZoneId.of("UTC") with ZoneOffset.UTC as you mentioned.

[indyana]

Yep, I understand, just a good number of repos to go through to fix the issue and honestly the behavior isn't what I would expect if I specifically configure Jackson to adjust dates to time zone UTC.