`DurationDeserializer` should use `@JsonFormat.pattern` (and config override) to support configurable `ChronoUnit`

[chisui]

When deserializing DurationDeserializer treats all durations of number values as seconds and delegates string values to Duration.parse.

Versions

com.fasterxml.jackson.core:jackson-core:jar:2.11.2
com.fasterxml.jackson.core:jackson-databind:jar:2.11.2
com.fasterxml.jackson.datatype:jackson-datatype-jdk8:jar:2.11.2
com.fasterxml.jackson.datatype:jackson-datatype-jsr310:jar:2.11.2
com.fasterxml.jackson.core:jackson-annotations:jar:2.11.2

Example

class A {
    @JsonFormat(pattern = "h")
    Duration dur;

    public static void main(String[] args) throws Exception {
        JsonMapper m = JsonMapper.builder()
                .addModule(new JavaTimeModule())
                .build();
        A a0 = m.readValue("{\"dur\":2}".getBytes(), A.class);
        System.out.println(a0.dur.equals(Duration.ofHours(2))); // should print "true" but prints "false

        A a1 = m.readValue("{\"dur\":\"2\"}".getBytes(), A.class);
        System.out.println(a1.dur.equals(Duration.ofHours(2))); // should print "true" but throws Exception
    }
}

The Exception thrown by the second readValue call:

com.fasterxml.jackson.databind.exc.InvalidFormatException: Cannot deserialize value of type `java.time.Duration` from String "2": Failed to deserialize java.time.Duration: (java.time.format.DateTimeParseException) Text cannot be parsed to a Duration
 at [Source: (byte[])"{"dur":"2"}"; line: 1, column: 8] (through reference chain: *****)

	at com.fasterxml.jackson.databind.exc.InvalidFormatException.from(InvalidFormatException.java:67)
	at com.fasterxml.jackson.databind.DeserializationContext.weirdStringException(DeserializationContext.java:1702)
	at com.fasterxml.jackson.databind.DeserializationContext.handleWeirdStringValue(DeserializationContext.java:947)
	at com.fasterxml.jackson.datatype.jsr310.deser.JSR310DeserializerBase._handleDateTimeException(JSR310DeserializerBase.java:129)
	at com.fasterxml.jackson.datatype.jsr310.deser.DurationDeserializer.deserialize(DurationDeserializer.java:92)
	at com.fasterxml.jackson.datatype.jsr310.deser.DurationDeserializer.deserialize(DurationDeserializer.java:43)
	at com.fasterxml.jackson.databind.deser.impl.FieldProperty.deserializeAndSet(FieldProperty.java:138)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.vanillaDeserialize(BeanDeserializer.java:293)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:156)
	at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4524)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3527)
        ...
Caused by: java.time.format.DateTimeParseException: Text cannot be parsed to a Duration
	at java.base/java.time.Duration.parse(Duration.java:417)
	at com.fasterxml.jackson.datatype.jsr310.deser.DurationDeserializer.deserialize(DurationDeserializer.java:90)
	... 70 more

Even though JsonFormat doesn't explicitly mention any jsr310 classes in its documentation they should still work with it. Especially since DurationDeserializer already respects JsonFormat#lenient.

[kupci]

Yes, JsonFormat should be supported, but it was added after the module was written, so it will have to be added incrementally. See related item #168.

[cowtowncoder]

Marking as targeting 2.12 since probably needs refactoring of base serializers.

[obarcelonap]

Hey @cowtowncoder I am interested in this one. Can you guide me a bit through the required changes? Thanks in advance

[cowtowncoder]

@obarcelonap first of all, that would be excellent!

It might make sense to have a look at Joda module (https://github.com/FasterXML/jackson-datatype-joda/) and compare deserializer implementations. But the basic idea is that annotation information is available in createContextual() call and needs to be accessed, stored (by creating a new instance of deserializer).

If you want to try working on this, use 2.12 branch (master is for 3.0 and that is quite far out).
I am making some changes for 2.12 right now for both modules (for unrelated changes), but should be done in an hour or so.
I can also merge 2.12 -> master changes eventually so no need to worry about that (there are some changes but I can resolve them).

[Bluexin]

Additionally, I noticed a suspicious looking piece of code related to the DurationDeserializer configuration :

jackson-modules-java8/datetime/src/main/java/com/fasterxml/jackson/datatype/jsr310/deser/DurationDeserializer.java

Lines 134 to 139 in 37f55ed

	protected Duration _fromTimestamp(DeserializationContext ctxt, long ts) {
	if (ctxt.isEnabled(DeserializationFeature.READ_DATE_TIMESTAMPS_AS_NANOSECONDS)) {
	return Duration.ofSeconds(ts);
	}
	return Duration.ofMillis(ts);
	}

The Feature is about deserializing to nanos, but it deserializes as seconds. Do you want me to make a separate issue for this or solve it as part of this one? Was already like this in 2.10.2

[obarcelonap]

@cowtowncoder did some prototyping today, could you check how it looks like?
https://github.com/FasterXML/jackson-modules-java8/compare/master...obarcelonap:duration-deserializer-respects-json-format-pattern?expand=1

If that's ok I will test it.

thanks!!

[kupci]

The Feature is about deserializing to nanos, but it deserializes as seconds. Do you want me to make a separate issue for this or solve it as part of this one? Was already like this in 2.10.2

Should be a separate bug, with test case and so on. But I think that is the correct behavior, see #165, sometimes the flags are a little ambiguous.

[kupci]

protected enum DurationPattern {
      DAYS("d", Duration::ofDays),

Looks good so far, only thought is for the above enum, the patterns ("d", "h", ...) should be available somewhere already, in the Jackson core, and then you can map DurationPattern to that?

[obarcelonap]

I am not experienced with jackson-core but I can't find anything similar.

Other option is just rely on Duration::of method (instead of specific methods) so the enum is not needed as the received format could be parsed as ChronoUnit.
https://docs.oracle.com/javase/8/docs/api/java/time/Duration.html#of-long-java.time.temporal.TemporalUnit-
https://docs.oracle.com/javase/8/docs/api/java/time/temporal/ChronoUnit.html?is-external=true
Actually I think this approach is better: less code and it will support more units that does not have specialized methods (eg. months, weeks, micros,...)

[obarcelonap]

I am trying to unit test the class and I am having compilation issues when executing any test through intellij. Compilation error is related to missing jackson dependencies like jackson-core. But it works if I run tests trough mvn.

Any clue? Do I need to do something special when importing the mvn project in intellij?