Support use of "pattern" (ChronoUnit) for DurationSerializer too

[cowtowncoder]

(see #184 for background)

So, we should support use of @JsonFormat(pattern = "HOURS") (and "config override" equivalent) for writing too. As discussed on #184, there is gnarly challenge with backwards-compatibility -- change should only affect explicit pattern-using case.

Another challenge is the possibility of fractional values: given Duration may not be exactly divisible by desired unit: for example, we might have "90 seconds" as value, but configured to use "minutes", resulting in 1.5 (ideally).
If it is possible to calculate this accurately (enough), I'd like to do that, but I am not sure it is.

If not, I suggest that only one specific case of fractions would be supported: that of "SECONDS" as explicit unit. It would be output as fractions as needed, based on underlying seconds/nanoseconds values.

[cowtowncoder]

Also note that above logic should only be invoked when configured to output "as timestamp" (as number) -- "as string" uses Duration.toString(), which in turn produces Period style textual output. We should leave that aspect unchanged as it is at least quite well defined.

[obarcelonap]

Hey guys, can I take this one too?
Thanks in advance

[cowtowncoder]

@obarcelonap sure! I made some changes to the first one so make sure sync (some renaming). Was thinking that helper class probably should go under new .util package, but did not yet move there.

[obarcelonap]

Hey guys just worked a bit today on this

• As @cowtowncoder mentioned, extracted DurationUnitConverter into a utils package. Also extended the class to support serialization.
• Serialization for micros and half days is not directly supported by Duration, you may want to take a decision there (keep a custom impl or remove the support for deserialization).
• I needed to abstract & override couple of methods from base serializer which is a bit convoluted. Actually not sure if makes sense to use most of it in the case of Duration.
• The pattern only applies when SerializationFeature.WRITE_DURATIONS_AS_TIMESTAMPS is enabled and format pattern is detected.
• Not supporting fractional values yet, I need to iterate on this. My idea is to enable fractions only when SerializationFeature.WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS is enabled and a format pattern is detected

Branch here in case you want to take a look https://github.com/FasterXML/jackson-modules-java8/compare/master...obarcelonap:support-pattern-for-duration-serializer?expand=1

[kupci]

One comment on code below:

  public DurationSerializer(DurationSerializer base, DurationUnitConverter converter) {
       super(base, base._useTimestamp, base._useNanoseconds, base._formatter, base._shape);
       _durationUnitConverter = converter;
   }

So is the idea to allow someone to provide their own DurationUnitConverter? If not, "pattern" should be configured in the DurationSerializer constructor, much like DateTimeFormatter, but think DurationUnitConverter would work like DecimalUtils, i.e. static calls:
return DecimalUtils.extractSecondsAndNanos(value, Duration::ofSeconds);

Not supporting fractional values yet,

I'd incline to 'leave that as an exercise to the customer', i.e. the Duration itself doesn't support fractions, except for two Duration methods:

ofSeconds(long seconds, long nanoAdjustment)
Obtains a Duration representing a number of seconds and an adjustment in nanoseconds.

and

parse(CharSequence text)
Obtains a Duration from a text string such as PnDTnHnMn.nS.

There is access to JsonFormat.Shape in JSR310FormattedSerializerBase where it is used in to support the case below:

       // Does not look like we can make any use of DateTimeFormatter here?
        generator.writeString(duration.toString());

[cowtowncoder]

I think that constructor is probably called from createContextual(), and should not be public.

[obarcelonap]

I think that constructor is probably called from createContextual(), and should not be public.

exactly, I need to reduce the visibility of the constructor

[obarcelonap]

There is access to JsonFormat.Shape in JSR310FormattedSerializerBase where it is used in to support the case below:

The thing about using JsonFormat.Shape is that is already used by the base class to determine _useTimestamp in JSR310FormattedSerializerBase#createContextual and later one is also used by useTimestamp and useNanoseconds functions in base class.
To be honest, not sure if makes sense to use createContextual, useTimestamp and useNanoseconds from base class for Duration since its handling is fairly different (no dtf formatter, shape usage) and its usage from DurationSerializer is quite convoluted.
I'd go for a rewrite of createContextual in DurationSerializer without using base class.

But in any case, if I am not mistaken, current serialization of duration is as follows:

WRITE_DURATIONS_AS_TIMESTAMPS	WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS	JsonFormat.Shape	Serialization
true	false	null	Duration::toMillis
true	true	null	_toNanos
X	X	NUMBER_INT	Duration::toMillis
X	X	NUMBER_FLOAT	_toNanos
If not in the table, value is seralized as string (Duration::toString)

And I would adapt the code to consider JsonFormat.Pattern too but keeping backwards compatibility:

WRITE_DURATIONS_AS_TIMESTAMPS	WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS	JsonFormat.Shape	JsonFormat.Pattern	Serialization
true	false	null	null	Duration::toMillis
false	false	null	ChronoUnit	unit conversion
true	false	null	ChronoUnit	unit conversion
true	true	null	null	_toNanos
true	true	null	ChronoUnit	unit conversion with fractions
X	X	NUMBER_INT	null	Duration::toMillis
X	X	NUMBER_INT	ChronoUnit	unit conversion
X	X	NUMBER_FLOAT	null	_toNanos
X	X	NUMBER_FLOAT	ChronoUnit	unit conversion with fractions
If not in the table, value is seralized as string (Duration::toString)

Would that make sense to you?

[cowtowncoder]

In general this sounds good, but one question I have about case where both

• Numeric Shape (NUMBER_INT or NUMBER_FLOAT) is specified
• non-empty pattern is specified

Table above suggests that existence of "pattern" would sort of override shape being indicated as numeric (timestamp-y).
I would have guessed the opposite (that Shape trumps over pattern, if specified)?
I don't have super strong opinion on this as user should be able to control both aspects, but wanted to clarify this.

I do agree that global WRITE_DURATIONS_AS_TIMESTAMPS should have lower precedence than various JsonFormat overrides.

I will try to follow up changes with lower delay this week so we can maybe get PR for this soon -- hoping now to get 2.12.0-rc2 over upcoming weekend, getting close to closing most things I really want for RC.

[obarcelonap]

Thanks for the feedback 🙆‍♂️ 🙆‍♂️

Table above suggests that existence of "pattern" would sort of override shape being indicated as numeric (timestamp-y).
I would have guessed the opposite (that Shape trumps over pattern, if specified)?
I don't have super strong opinion on this as user should be able to control both aspects, but wanted to clarify this.

It made more sense to me when both are set, my reason behind is pattern is more specific since it changes the scale of the value, not only the precision (fractions).

Since you're familiarized with the experience using the lib, I will go for your suggestion. Also, I reduced the scope a bit, getting rid of unit conversion with fractions since there is no direct support on it in Duration. It would work like this:

WRITE_DURATIONS_AS_TIMESTAMPS	WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS	JsonFormat.Shape	JsonFormat.Pattern	Serialization
true	false	null	null	Duration::toMillis
false	false	null	ChronoUnit	unit conversion
true	false	null	ChronoUnit	unit conversion
true	true	null	X	_toNanos
X	X	NUMBER_INT	X	Duration::toMillis
X	X	NUMBER_FLOAT	X	_toNanos
If not in the table, value is seralized as string (Duration::toString)

I will try to follow up changes with lower delay this week so we can maybe get PR for this soon -- hoping now to get 2.12.0-rc2 over upcoming weekend, getting close to closing most things I really want for RC.

For the moment I will create the PR without unit fractions so we can get this merged and later on we can iterate if fractions are needed.

[cowtowncoder]

@obarcelonap Sounds like a plan, yes. Getting non-fractional handling in quickly makes sense.

[cowtowncoder]

Merged.