OffsetDateTime deserialization does not handle nano seconds correctly

[cowtowncoder]

(moved from https://github.com/FasterXML/jackson-datatype-jsr310/issues/96 by @xzer)

when InstantDeserializer get a long value for OffsetDateTime, it will entering the following method:

    protected T _fromLong(DeserializationContext context, long timestamp)
    {
        if(context.isEnabled(DeserializationFeature.READ_DATE_TIMESTAMPS_AS_NANOSECONDS)){
            return fromNanoseconds.apply(new FromDecimalArguments(
                    timestamp, 0, this.getZone(context)
            ));
        }
        return fromMilliseconds.apply(new FromIntegerArguments(
                timestamp, this.getZone(context)));
    }

Notice that, with the default configuration, READ_DATE_TIMESTAMPS_AS_NANOSECONDS is true, it will create a FromDecimalArguments with the passed timestamp value, and which will be passed to the following source:

    public static final InstantDeserializer<OffsetDateTime> OFFSET_DATE_TIME = new InstantDeserializer<>(
            OffsetDateTime.class, DateTimeFormatter.ISO_OFFSET_DATE_TIME,
            OffsetDateTime::from,
            a -> OffsetDateTime.ofInstant(Instant.ofEpochMilli(a.value), a.zoneId),
            a -> OffsetDateTime.ofInstant(Instant.ofEpochSecond(a.integer, a.fraction), a.zoneId),
            (d, z) -> d.withOffsetSameInstant(z.getRules().getOffset(d.toLocalDateTime())),
            true // yes, replace +0000 with Z
    );

to be precision, the following row:

a -> OffsetDateTime.ofInstant(Instant.ofEpochSecond(a.integer, a.fraction), a.zoneId),

which will handle the value as epoch seconds, rather than nano second.

Is there anything that I misunderstand? Or it is actually a bug?

[kupci]

which will handle the value as epoch seconds, rather than nano second.
The 2nd value is the nano seconds. From the JavaDoc:

public static Instant ofEpochSecond(long epochSecond, long nanoAdjustment)

[xzer]

the timestamp value is treated at the integer part, not the fraction part.

[kupci]

Ok, now I see the point, and agree, this does seem incorrect, but maybe more of a documentation issue at the very least, or deprecating the READ_DATE_TIMESTAMPS_AS_NANOSECONDS variable name, and changing it to READ_DATE_TIMESTAMPS_AS_EPOCH_SECONDS?

[kupci]

See similar issue #109

[xzer]

At deserialization time, decimal numbers are always read as fractional second timestamps with up-to-nanosecond resolution, since the meaning of the decimal is unambiguous. The more ambiguous integer types are read as fractional seconds without a decimal point if READ_DATE_TIMESTAMPS_AS_NANOSECONDS is enabled (it is by default), and otherwise they are read as milliseconds.

TBH, as an non English speaker, I totally cannot understand what the last sentence starting by "The more ambiguous integer types..." is speaking...

Then I tried to read the code to understand it, but in the code, the nano part (the fractional number) is always 0, I was totally confused at that time.

Even if we change the name to READ_DATE_TIMESTAMPS_AS_SECONDS, the source is still not making sense: why do we need to handle fraction here?

I think there are 2 issues here:

• the feature is trying to handle decimal point as a mark of nano seconds (now I understood it), but there may be a bug in the implementation (why the faction is always 0?)
• the name of the feature is too confusing, I believe most people would be confused by the name.

I would suggest to deprecate this feature, or change it to a simple form that, just treat the string as an long integer of nanosecond.

And further, as some languages sdks use second as default precision, thus it would be convinient have a configuration of read data as second.

Last, since default java time stamp is millisecond, I would suggest use millisecond as default configuration.

[cowtowncoder]

I can not comment on all the details as I did not author this module, but here are some of my thoughts that are relevant.

First, on ambiguousness: I think what sentence tries to say is this:

1. With floating-point ("decimal") numbers, use of seconds is unambiguous; precision may vary (down to nanosecond range) but not unit
2. With integer numbers (that is, numbers with no fractional part) , there is no natural base unit because while JDK (and Java generally) used to be based on millisecond as units, Java 8 date/time change values to use nanoseconds as base unit.

Given (2), it is not very sensible to use milliseconds as base unit with Java 8 date/time types as that would make it impossible to use full accuracy. I am not sure use of seconds for that case would make sense either.

At the same time, I do see it problematic that other systems / standards use millisecond ("old" Java) or second (Unix) as base unit for timestamps, since their integer values can then easily be misinterpreted.

[GedMarc]

Hi Guys,

Finally got something here - You can actually specify your precision it seems

	@JsonFormat(pattern = "yyyy-MM-dd'T'HH:mm:ss.SSS")
	private LocalDateTime lastActionTime;
@JsonFormat(pattern = "yyyy-MM-dd'T'HH:mm:ss,SSSSSS")
	private ZonedDateTime lastActionZone;
@JsonFormat(pattern = "yyyy-MM-dd'T'HH:mm:ss.SSSSSSSSS")
	private ZonedDateTime lastActionMillis;

DateTimeFormatter formatter = new DateTimeFormatterBuilder()
    // here is the same as your code
    .append(DateTimeFormatter.BASIC_ISO_DATE).appendLiteral('T')
    // time (hour/minute/seconds)
    .appendPattern("HH:mm:ss")
    // optional nanos with 9 digits (including decimal point)
    .optionalStart()
    .appendFraction(ChronoField.NANO_OF_SECOND, 9, 9, true)
    .optionalEnd()
    // optional nanos with 6 digits (including decimal point)
    .optionalStart()
    .appendFraction(ChronoField.NANO_OF_SECOND, 6, 6, true)
    .optionalEnd()
    // optional nanos with 3 digits (including decimal point)
    .optionalStart()
    .appendFraction(ChronoField.NANO_OF_SECOND, 3, 3, true)
    .optionalEnd()
    // offset
    .appendOffset("+HH:mm", "Z")
    // create formatter
    .toFormatter();

I find myself happy with the implementation, also found this out because JDK 12 no longer accepts null as a pattern and forces you to specify the format accurately, can't have any java.time without a @jsonformat anymore

https://stackoverflow.com/questions/44373144/java-8-how-to-create-datetimeformatter-with-milli-micro-or-nano-seconds

[cowtowncoder]

@GedMarc I know you added a note on Afterburner issue, but could you also create an issue here, along with "easy" label (I think)? We have some new contributors who might be able to resolve the null formatter problem faster than I can -- I think there is a default to use, or alternatively call different method. There may be some logic in base class that assumes null formatter means default, and so on, but I don't think there is anything fundamentally difficult to resolve it, just takes some time to go through it.