search

FasterXML / jackson-modules-java8

Set of support modules for Java 8 datatypes (Optionals, date/time) and features (parameter names)
Apache License 2.0
401 stars 117 forks source link

Discrepancy in deserialization of ZonedDateTime #204

Closed dscalzi closed 1 year ago

dscalzi commented 3 years ago

Given input string: 2021-02-01T19:49:04.0513486Z

Jackson will deserialize a ZonedDateTime with the following properties:

Using the standard call

DateTimeFormatter.ISO_ZONED_DATE_TIME.parse(inputString, ZonedDateTime::from);

produces:

This causes an issue in comparisons because id value "UTC" != id value "Z". This also produces a redundancy in the toString of the ZonedDateTime because it detects that the offset != zone.

toString for Jackson result: 2021-02-01T19:49:04.051348600Z[UTC] toString for standard result: 2021-02-01T19:49:04.051348600Z

The latter can be consumed without extra processing and is the desired result.

Would it be possible to update the deserializer to behave like the standard call?

cowtowncoder commented 3 years ago

I'd be open to contributions that would unify handling -- I don't know if and how to this myself. @kupci or @beamerblvd might know.

If anyone wants to tackle this, I suspect it could go in a minor release (but should NOT be introduced in a patch) at least: if so, branch to base it against would be 2.13 at this point.

dscalzi commented 2 years ago

@cowtowncoder I think it might just come down to this value being false https://github.com/FasterXML/jackson-modules-java8/blob/2.14/datetime/src/main/java/com/fasterxml/jackson/datatype/jsr310/deser/InstantDeserializer.java#L95

I haven't run any tests myself.

cowtowncoder commented 2 years ago

Thank you for extra information @dscalzi!

At this point I am open to PR if anyone wants to submit it, but will not pro-actively make changes without someone actually needing it, providing test.

dscalzi commented 1 year ago

Doing some investigating, it seems like this stems from DeserializationFeature.ADJUST_DATES_TO_CONTEXT_TIME_ZONE. When this feature is enabled, the timezone on the resolved ZonedDateTime overrided to be the value of ObjectMapper's set TimeZone. The function responsable for this (ZonedDateTime::withZoneSameInstant) https://github.com/FasterXML/jackson-modules-java8/blob/2.14/datetime/src/main/java/com/fasterxml/jackson/datatype/jsr310/deser/InstantDeserializer.java#L94

When calling this method with UTC timezone, it changes the zone to UTC (instead of Z), resulting in the discrepancy as noted in the original post.

dscalzi commented 1 year ago

Further, the output of that depends on the input you pass to withZoneSameInstant

ZonedDateTime manipulatedUTC = standard.withZoneSameInstant(ZoneId.of("UTC"));
System.out.println("UTC Manipulated " + manipulatedUTC.getZone());
// Prints UTC
// Resultant ZonedDateTIme would be 2021-02-01T19:49:04.051348600Z[UTC]

ZonedDateTime manipulatedUTCStd = standard.withZoneSameInstant(ZoneOffset.UTC);
System.out.println("UTC Std Manipulated " + manipulatedUTCStd.getZone());
// Prints Z
//  Resultant ZonedDateTIme would be 2021-02-01T19:49:04.051348600Z

https://stackoverflow.com/a/39507023/5295111

dscalzi commented 1 year ago

Just raised a PR that fixes this issue. I have only investigated this for ZonedDateTimes, as I havent noticed any issues with other java date types.

cowtowncoder commented 1 year ago

Ok unfortunately this gets deep into Date/Time area where my context is bit incomplete. What you say does make sense. And yes, backwards compatibility is usually the big challenge.

About the only contribution I have here is that we are trying to avoid adding new datatype-specific features into general DeserializationFeature / SerializationFeature, and have added concept of DatatypeFeature, with 2 implementations:

  1. EnumFeature (for new settings added that affect reading/writing of Enum types)
  2. JsonNodeFeature (similarly for JsonNode types, aka Tree Model)

I have planned to add DateTimeFeature, fwtw, when it is needed. It coudl be easily added in 2.15; adding new DatatypeFeatures is itself easy (as is adding entries). I wrote JSTEPs:

cowtowncoder commented 1 year ago

Hmmh. Ok, after re-reading full explanation, perhaps we do NOT need backwards-compatibility feature unless someone actually does request it during 2.15 release candidate phase. So DateTimeFeature is not needed, yet; but one can be added if and as necessary.