Add feature toggle to read numeric strings as numeric timestamps

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

Add feature toggle to read numeric strings as numeric timestamps #269

Closed mpkorstanje closed 9 months ago

mpkorstanje commented 1 year ago

There is an apparent inconsistency in the way Jackson de-serializes numbers that are shaped as a string into an instant.

When not providing a custom date format, quoted numbers are treated as epoch milis/nanos
When not providing a custom date, quoted numbers are assumed to be handled by the pattern.

There is however no way to construct a pattern that handles both ISO dates and epoch milis/nanos. This feature toggle allow numeric strings to be read as numeric timestamps.

Fixes: https://github.com/FasterXML/jackson-modules-java8/issues/263.

This PR is paired with https://github.com/FasterXML/jackson-databind/pull/3830.

mpkorstanje commented 1 year ago

Contributing - Paper Work All you need to do is to download the CLA document, print it, fill and sign, scan (or take photo on your phone) and email that copy to info at fasterxml dot com.

Please hold one moment while I print liquid gold on dead tree.

cowtowncoder commented 1 year ago

Ok: as per my comments, I'd like to use a module configuration setting here.

But looking at the way deserializers are implemented, this appears a bit tricky: not so much wrt configuring JavaTimeModule, but in passing configuration setting(s) from module to InstantDeserializer... some refactoring would be needed, so that instead of pre-created instances, we'd create instances when registering deserializers. Alas, InstantDeserializer.INSTANT and 2 others are public static singletons so we must leave something like that. Let me think about this a bit.

cowtowncoder commented 9 months ago

Ok, I found time to implement #281, and for that introduced JavaTimeFeature enumeration -- this now allows adding a setting for this PR as well. The main/only concern for me is just naming the feature to reflect semantics.

mpkorstanje commented 9 months ago

Cool. I'll rework the PR to use that.

cowtowncoder commented 9 months ago

Looks like I got CLA too, great!

cowtowncoder commented 9 months ago

@mpkorstanje I renamed the feature, made some other minor tweaks and so I think this is ready for merge from my POV. But I wanted to let you have a look if you want some other changes (even better feature name? :) ) Just LMK either way, looking forward to getting this nice feature merged.

mpkorstanje commented 9 months ago

Sorry for the delay. Yesterday was a bit full.

Naming aside, it looks good to me.

cowtowncoder commented 9 months ago

Thank you for all the work here & improvement suggestions @mpkorstanje !