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

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

Conversation

mpkorstanje
Copy link
Contributor

@mpkorstanje mpkorstanje commented Mar 15, 2023

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: #263.

This PR is paired with FasterXML/jackson-databind#3830.

@mpkorstanje
Copy link
Contributor Author

mpkorstanje commented Mar 15, 2023

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

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

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

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

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 FasterXML#263
@mpkorstanje mpkorstanje force-pushed the feature/toggle-to-parse-qouted-numbers-with-custom-formatter branch from e5b0adb to d93b065 Compare November 12, 2023 13:53
@mpkorstanje mpkorstanje changed the base branch from 2.15 to 2.16 November 12, 2023 13:53
@cowtowncoder
Copy link
Member

Looks like I got CLA too, great!

@cowtowncoder
Copy link
Member

@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.

@cowtowncoder cowtowncoder marked this pull request as ready for review November 13, 2023 02:57
* Note that when a pattern is not explicitly defined numeric strings are
* interpreted as a numeric timestamp.
*/
ALWAYS_ALLOW_STRINGIFIED_TIMESTAMPS(false)
Copy link
Contributor Author

@mpkorstanje mpkorstanje Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be consistent with READ_DATE_TIMESTAMPS_AS_NANOSECONDS I reckon ALWAYS_READ_STRINGIFIED_DATE_TIMESTAMPS would be more accurate. Especially when used together, because they then would appear to refer to the same thing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add DATE_ in there, good suggestion.

The reason I think "ALLOW" is important is because this is used in addition to -- but not instead of -- custom pattern.

* Flag set from
* {@link com.fasterxml.jackson.datatype.jsr310.JavaTimeFeature#ALWAYS_ALLOW_STRINGIFIED_TIMESTAMPS}
* to determine whether numeric strings are interpreted as numeric timestamps
* (enabled) nor not (disabled) in addition to an explicitly defined pattern.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we replace "pattern" with "custom DateTimeFormatter" as you added to the changelog?

@mpkorstanje
Copy link
Contributor Author

Sorry for the delay. Yesterday was a bit full.

Naming aside, it looks good to me.

@cowtowncoder cowtowncoder merged commit ea1fc92 into FasterXML:2.16 Nov 15, 2023
4 checks passed
@cowtowncoder
Copy link
Member

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

@mpkorstanje mpkorstanje deleted the feature/toggle-to-parse-qouted-numbers-with-custom-formatter branch November 15, 2023 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants