FasterXML / jackson-datatype-joda

Extension module to properly support full datatype set of Joda datetime library
Apache License 2.0
140 stars 82 forks source link

`DurationDeserializer` does not accept valid ISO8601 duration values #90

Closed bpelakh closed 7 years ago

bpelakh commented 8 years ago

Using the simple Duration(Object) constructor only accepts durations of the [Pp][Tt]\d+[Ss] format, which is a tiny subset of ISO8601 syntax. Instead of

return new Duration(parser.getText())

should instead do

return ISOPeriodFormat.standard().parsePeriod(parser.getText()).toStandardDuration()

cowtowncoder commented 8 years ago

Sounds reasonable. Would you happen to have examples of valid values that are not handled but would be handled with this change?

bpelakh commented 8 years ago

Sure - PT1H (1 hour) and PT4H30M (4 hrs 30 min) are both valid ISO durations not accepted by the new Duration() constructor.

cowtowncoder commented 8 years ago

Actually looking deeper into this, I am not sure this makes sense after all. Seems to me that Period is designed to accept wide(r) range of representations, being more flexible, whereas Duration is defined as:

An immutable duration specifying a length of time in milliseconds.

and thus makes sense to be expressed only using smaller subset of ISO-8601 (which after all allows dizzying number of potential variations).

So if your data requires wider sets of representations, shouldn't it be Period?

squiddle commented 7 years ago

So i got bitten by this as well, and i would like to see the suggested fix as it is an improvement for me.

The main difference between Duration and Period is how it is applied to a date to get to another Instance or Interval. Period honours the Daylight Saving Time.

From a data-modelling perspective they are different and should both be usable correctly so the suggested fix would be beneficial.

Also Duration should ignore leap seconds whereas Period should honour them as well (not sure if this is implemented in Joda).

cowtowncoder commented 7 years ago

Implemented as suggested so should now accept wider range of values, both as values and as keys.