FasterXML / jackson-modules-java8

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

`JsonFormat.Feature.WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS` not respected when deserialising `Instant`s #272

Closed pmahony893 closed 1 year ago

pmahony893 commented 1 year ago

I have a class as follows:

public class InstantWrapper {
    @JsonFormat(without = Feature.WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS)
    private Instant myInstant;

    public Instant getMyInstant() { return myInstant; }
    public void setMyInstant(Instant myInstant) { this.myInstant = myInstant; }
}

When serialising, the result uses milliseconds since the epoch; however, when deserialising, it is interpreted as nanoseconds.

I understand that the JavaDoc says "Override for SerializationFeature.WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS" (saying nothing about DeserializationFeature.READ_DATE_TIMESTAMPS_AS_NANOSECONDS), however in my view it's undesirable that a round-trip modifies the data when this option is used.

Is there a way (other than writing my own deserialiser) to override the ObjectMapper's settings so that timestamps are interpreted using milliseconds?

cowtowncoder commented 1 year ago

So does DeserializationFeature.READ_DATE_TIMESTAMPS_AS_NANOSECONDS not work? Or is this about there not being JsonFormat.Feature.READ_DATE_TIMESTAMPS_AS_NANOSECONDS to go with write option?

If latter, contributions for adding support would be most welcome: I think its missing is an omission; new per-property Features are added over time as requested. (note that since there is just "WRITE" in name I do not think it should apply to "READ"s, but I agree that for users this is not a good situation).

So it'd be good to get this use case covered. I do not think semantics of "WRITE_xxx" should be changed, but I do think there should be "READ_xxx" counterpart (and renaming of existing Enums is, unfortunately, major backwards-incompatibility so not doable for 2.x either)

pmahony893 commented 1 year ago

Thanks for the response! Yes, this was more about the inability to override DeserializationFeature.READ_DATE_TIMESTAMPS_AS_NANOSECONDS from the JsonFormat annotation. Naively I'd expect changing JsonFormat.Feature.WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS to cover both operations, but you're right, it doesn't feel good to do that as it just has WRITE in the name.

I could try my hand at adding support, but unfortunately I'm not sure how much time I'd be able to give it. Am I right in thinking it would first need a change in the jackson-annotations repo, and then here? And would there be changes needed to handle non-java.time classes as well?

cowtowncoder commented 1 year ago

Correct, a change would be needed in jackson-annotations. As to non-java.time (mostly Joda), I don't think this is supported outside java.time types, so it'd be enough to add support here. For plumbing, looking at serialization side should be helpful; this would not necessarily be super difficult.

One thing is timing: if jackson-annotations change was contributed Really Soon Now (in next couple of days), I could merge that in for 2.15.0; and support could be added in one of 2.15.x versions for date/time module. But annotations changes only go in .0 version so if not now, it'll need to go in 2.16.x which will take a while.

cowtowncoder commented 1 year ago

Ok, since it's just a small thing I went ahead and did https://github.com/FasterXML/jackson-annotations/issues/221 -- so there is now JsonFormat.Feature.WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS. I'll see how easy it'd be to do the rest now...

cowtowncoder commented 1 year ago

Ok. So, good news is that JSR310DateTimeDeserializerBase actually gets JsonFormat.Value already, through which newly added JsonFormat.Feature is readily accessible. Method _withFormatOverrides() is called with it.

The bad news is that plumbing may get bit hairy: looks like at least 4 deserializer implementations support nano-second format; ideally handling would be shared to some degree...

cowtowncoder commented 1 year ago

Lol. Looking at DurationDeserializer... meaning of "READ_DATE_TIMESTAMPS_AS_NANOSECONDS" is absolutely crazy -- it's taken to mean "read as SECONDS". I can't even. What happened there?

cowtowncoder commented 1 year ago

At this point I do not have time to work on this unfortunately: I can help if someone has time & itch; all pieces are ready (ability to override reading with JsonFormat.Feature.READ_DATE_TIMESTAMPS_AS_NANOSECONDS) so it should be doable, I just do not have bandwidth myself at this point.

raman-babich commented 1 year ago

JFYI, working on pr.