-
Notifications
You must be signed in to change notification settings - Fork 117
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
Add feature toggle to read numeric strings as numeric timestamps #269
Conversation
Please hold one moment while I print liquid gold on dead tree. |
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 |
Ok, I found time to implement #281, and for that introduced |
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
e5b0adb
to
d93b065
Compare
datetime/src/main/java/com/fasterxml/jackson/datatype/jsr310/JavaTimeFeature.java
Outdated
Show resolved
Hide resolved
Looks like I got CLA too, great! |
@mpkorstanje I renamed the feature, made some other minor tweaks and so I think this is ready for merge from my POV. |
* Note that when a pattern is not explicitly defined numeric strings are | ||
* interpreted as a numeric timestamp. | ||
*/ | ||
ALWAYS_ALLOW_STRINGIFIED_TIMESTAMPS(false) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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?
datetime/src/main/java/com/fasterxml/jackson/datatype/jsr310/deser/InstantDeserializer.java
Outdated
Show resolved
Hide resolved
datetime/src/main/java/com/fasterxml/jackson/datatype/jsr310/deser/InstantDeserializer.java
Outdated
Show resolved
Hide resolved
Sorry for the delay. Yesterday was a bit full. Naming aside, it looks good to me. |
Thank you for all the work here & improvement suggestions @mpkorstanje ! |
There is an apparent inconsistency in the way Jackson de-serializes numbers that are shaped as a string into an instant.
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.