Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JsonFormat.Feature.WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS not respected when deserialising Instants #272

Closed
pmahony893 opened this issue Apr 14, 2023 · 8 comments · Fixed by #277
Labels
Milestone

Comments

@pmahony893
Copy link

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
Copy link
Member

cowtowncoder commented Apr 14, 2023

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
Copy link
Author

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
Copy link
Member

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
Copy link
Member

Ok, since it's just a small thing I went ahead and did FasterXML/jackson-annotations#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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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.

@cowtowncoder cowtowncoder added the good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project label May 29, 2023
@raman-babich
Copy link
Contributor

JFYI, working on pr.

@cowtowncoder cowtowncoder added 2.16 and removed good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project labels Jun 19, 2023
@cowtowncoder cowtowncoder added this to the 2.16.0 milestone Jun 19, 2023
@cowtowncoder cowtowncoder changed the title JsonFormat.Feature.WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS not respected when deserialising Instants JsonFormat.Feature.WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS not respected when deserialising Instants Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants