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?

[kupci]

I haven't seen that but taking a look, it does seem others are running into this problem, so you might find some solutions below:

• https://intellij-support.jetbrains.com/hc/en-us/community/posts/360007996699-IntelliJ-2020-java-error-package-com-fasterxml-jackson-dataformat-yaml-does-not-exist
• https://youtrack.jetbrains.com/issue/IDEA-237320?_ga=2.130115023.1981242890.1602084021-20389993.1602084021#focus=streamItem-27-4081510.0-0

[obarcelonap]

I managed to run the test after deleting some files in idea config dir :?

Now the issue is about the testing. After adding my tests in DurationDeserTest some of the other ones are failing.

Is like my tests are polluting somehow the whole test suite because if I comment them everything is on green.
I pushed latest version of the code in the branch, could you check again?
https://github.com/FasterXML/jackson-modules-java8/compare/master...obarcelonap:duration-deserializer-respects-json-format-pattern?expand=1

I think after solving this, code is ready. Let me know if I am missing something.
Thanks

[cowtowncoder]

Not sure if this is the root cause but I know that some test classes incorrectly modify shared ObjectMapper instance and if tests are run concurrently that could cause bogus failures. I have fixed most of these over time, and looking at

datetime/src/test/java/com/fasterxml/jackson/datatype/jsr310/deser/DurationDeserTest.java

it does not have this problem.

[cowtowncoder]

@obarcelonap Did not see anything that would explain test fails, although one change seemed bit odd:

            if (format.hasPattern()) {
                DurationPattern.from(format.getPattern())
                    .ifPresent(deser::withPattern);
            }

as if it was missing return statement?

[obarcelonap]

@cowtowncoder that was the thing, missing to reset the value to null. Created the PR :)

[cowtowncoder]

@obarcelonap Thanks! I think this is getting ready. One thing I'll need is CLA, from:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

and usually the easiest way is to print, fill & sign, scan/photo, email to info at fasterxml dot com.

Thank you once again for the PR!

[obarcelonap]

Just sent the CLA by email.

[cowtowncoder]

@obarcelonap thank you again for contributing this, taking your time. Thank you @kupci for comments, pointing out issues.

First, a bigger thing to discuss: Seconds vs Milli-seconds defaulting (Very Broken Behavior).
I'll outline my thinking below.

It took me a while to get the context again, and I finally realized what was pointed out earlier: existing handling is pretty much broken wrt unit handling, assuming:

1. Second-based numeric values for floating-point numbers -- more reasonable, I think, given internal representation of Duration
2. Milli-/nano-second based values for integral numbers: while I can see why this might have come about (Java timestamps often are millisecond-based, and there is that nano-seconds feature), I think this is just Wrong. But it is also by now Defined Behavior :-(

Regarding this, I don't think we can change defaults, even if they are wrong. Systems that use Jackson have had to rely on these defaults, work around them. If we change semantics we will break systems.
But I think we can improve things for 2.x, and fix the issue from 3.0 where bigger changes are needed.

Regarding this particular issue, I propose that we only change time-unit when user explicitly uses pattern to specify ChronoUnit to use; but leave defaults to strange-but-established preceding logic.

We COULD additional specify a configuration setting for the module to allow fixing this issue: something like "setDefaultToSecondsForDuration()" in JavaTimeModule. Or, for even fancier, create JavaTimeFeature enum, add enable()/disable() methods.
If so, let's create separate issue for this, as follow-up work, and see if there's time to do this for 2.12 (or if not, 2.13).

This problem also concerns DurationSerializer, for what that is worth; we have to align handling there, too.

[cowtowncoder]

After explaining how I see the biggest problem, here are more targeted notes about this specific PR.

1. Keep behavior wrt integer number handling working wrt millis/nanos UNLESS there is explicit unit override via @JsonFormat or config override (latter SHOULD be handled by base class but we need to test it -- I can help)
2. Require strict name-matching for ChronoUnit (no case-insensitive)
3. I think we can allow "FOREVER" (since it is supported by JDK, more work NOT to allow), but if so, unit test has to verify it -- I think numeric value is effectively ignored so "0 (forever)" and "5789 (forever)" result in same
4. For 2.12, we should follow up with serializer change that follows logic of "if no pattern, do what you already did; if pattern, use more sensible approach". But I will file a new issue for DurationSerializer changes: let's not do them here.
5. Make sure we follow same logic for both integer- and floating-point values, in case of explicit pattern (as mentioned earlier, case of default handling is... broken).

I think above changes are quite close to PR but I wanted to reach consensus now.

[kupci]

5. Make sure we follow same logic for both integer- and floating-point values,

Agree, though one question on "same logic", you mean the original (pre-PR) logic right? One addition to this is that, playing around a bit with the Duration.of(long amount,TemporalUnit unit) method, which is used to create the Duration value in the PR, then seems we can limit the PR in scope to only integer values? In other words (and I added a comment in the PR also), keep the case statement below as per original (before PR)
case JsonTokenId.ID_NUMBER_FLOAT:
and only change
case JsonTokenId.ID_NUMBER_INT:

[cowtowncoder]

@kupci Yes, true. There does not seem to be a way to really support fractions for anything but SECONDS, unfortunately. So maybe pattern has to be effectively either ignored for floating-point, or even throw an exception if non-SECONDS units are chosen and there is fractional part... or, could even throw an exception if any value other than SECONDS is attempted.

So for 2.12, it does sound like we should start by supporting only ID_NUMBER_INT case (and that special case of "String value from CSV/XML that is a valid integer").

But then again, I am beginning to wonder whether we have time to do proper job here for 2.12 or not.
And also... whether there is value in adding all this logic, just to support use of pattern instead of user using String with Period compatible ISO-8601 notation that seems superior in most ways.

I'd rather not make changes unless they clearly improve handling.

[obarcelonap]

Hey guys, thank you for all your comments, I try to disconnect in the weekends and have good quality family time and now checking the email I see all this interesting discussion, amazing job.

If I am not wrong, I'd need to fulfill the following requirements:

1. Keep behavior wrt integer number handling working wrt millis/nanos UNLESS there is explicit unit override via @JsonFormat or config override (latter SHOULD be handled by base class but we need to test it -- I can help)
2. Require strict name-matching for ChronoUnit (no case-insensitive)
3. I think we can allow "FOREVER" (since it is supported by JDK, more work NOT to allow), but if so, unit test has to verify it -- I think numeric value is effectively ignored so "0 (forever)" and "5789 (forever)" result in same
4. For 2.12, we should follow up with serializer change that follows logic of "if no pattern, do what you already did; if pattern, use more sensible approach". But I will file a new issue for DurationSerializer changes: let's not do them here.
5. Supporting only ID_NUMBER_INT case (and that special case of "String value from CSV/XML that is a valid integer").

Mainly @cowtowncoder 's notes adapting slightly the last point to do not support floating point numbers.

But then again, I am beginning to wonder whether we have time to do proper job here for 2.12 or not.

Is there any planned schedule for releasing 2.12 version? I do think changes are really narrowed in a class and if there is an agreement in the bullet list above I can make that happen :)

But then again, I am beginning to wonder whether we have time to do proper job here for 2.12 or not.
And also... whether there is value in adding all this logic, just to support use of pattern instead of user using String with > > Period compatible ISO-8601 notation that seems superior in most ways.

I'd rather not make changes unless they clearly improve handling.

IMO this change will provide better Duration handling will avoid the accidental complexity added by the library just because it only supports seconds units gracefully.
For instance, I want to deserialize a payload with a field called retentionDays. When designing the interface I'd need to know that this is not supported by the lib and change to use something like retentionSeconds, retentionPeriod..
So changing the app domain just because the underlying lib is not supporting what is needed.
This is the best case, worst case is I design the API without knowing the limitation and I agree with an external contractor the field will be parsed in days and I do not realize until on implementation phase that this is not supported.

If there are so many questions left, maybe retargetting to 3.0 makes sense, I can try to take a look on how it would this changes in that version.

Let me know your thoughts before adapting the code with the latest comments in the current PR.

[cowtowncoder]

@obarcelonap Ah, just to clarify: if not 2.12, I'd target 2.13, not 3.0 -- 3.0 is still too far out (or undefined).
And np at all with response -- this is all voluntary for everyone, and amount of time available varies a lot. I would not expect that every comment would receive a response within a day or two (nor can provide quick responses myself).

So, on timing, probably we can get this in 2.12 still. I think we'll iterate over this.
And just to summarize, I think that the main goal here is to:

• Allow explicit definition of unit to use for integer values of Duration, using @JsonFormat.pattern (and equivalent config overrides)

with secondary constraints;

• Starting with deserializer, but
• Following up with matching serializer
• Keeping existing usage working as-is
• Documenting new added functionality

That seems doable to me. Looking forward to modified version!

[obarcelonap]

Pushed latest changes, please let me know your feedback.

[cowtowncoder]

Merged, will be in 2.12.0-rc2