apicollective / apibuilder-generator

MIT License
54 stars 36 forks source link

java.time.Instant is not good fit for date-time-iso8601 representation #520

Open mkows opened 5 years ago

mkows commented 5 years ago

Currently java.time.Instant is used to represent date time (date-time-iso8601) for Play 2.6 (Gen 2) and http4s generators.

The problem is that java.time.Instant represents local time only and is not aware of timezone and hence can only support time in UTC. Therefore the only date time format it supports is the one ending with Z (e.g. 2019-04-04T14:16:23Z). While other valid ISO 8601 date times formats are not supported e.g.:

~ ❯ scala
scala> import java.time._
import java.time._

scala> Instant.parse("2019-04-04T14:16:23Z")
res0: java.time.Instant = 2019-04-04T14:16:23Z

scala> Instant.parse("2019-04-04T14:16:23+00:00")
java.time.format.DateTimeParseException: Text '2019-04-04T14:16:23+00:00' could not be parsed at index 19
  at java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:1949)
  at java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1851)
  at java.time.Instant.parse(Instant.java:395)
  ... 28 elided

scala> Instant.parse("2019-04-04T14:16:23+01:00")
java.time.format.DateTimeParseException: Text '2019-04-04T14:16:23+01:00' could not be parsed at index 19
  at java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:1949)
  at java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1851)
  at java.time.Instant.parse(Instant.java:395)
  ... 28 elided

ZonedDateTime seems to be a better fit:


scala> ZonedDateTime.parse("2019-04-04T14:16:23Z")
res1: java.time.ZonedDateTime = 2019-04-04T14:16:23Z

scala> ZonedDateTime.parse("2019-04-04T14:16:23+00:00")
res2: java.time.ZonedDateTime = 2019-04-04T14:16:23Z

scala> ZonedDateTime.parse("2019-04-04T14:16:23+01:00")
res3: java.time.ZonedDateTime = 2019-04-04T14:16:23+01:00

So when currently Play 2.6 (Gen 1) service exposes date time in non-UTC (as "2019-04-04T14:16:23-05:00") then Http4s / Play 2.6 Gen 2 client is unable to deserialize json payload (expecting Instant as <datetime>Z).

Reference: https://en.wikipedia.org/wiki/ISO_8601 https://en.wikipedia.org/wiki/ISO_8601#Time_zone_designators

Please share your thoughts. CC: @gheine @plippe

plippe commented 5 years ago

Hey,

What you explain makes 100% sense. If there is currently an issue where all readers aren't compatible with all writers, we should fix that as soon as possible.

I am not sure about the plumbing, stepped away from API Builder recently, but Gregor should have all the context related to using Instant (sorry for passing the ball).

gheine commented 5 years ago

Yeah, that's a good point and was an oversight when we implemented the http4s generator. I guess the assumption was that datetime values are always serialized in Zulu time (eg 2019-04-08T09:03:48.589Z) rather than with a time offset. Not sure ZonedDateTime is the right datatype either through, as it serializes timezone Ids in addition to time offsets, which may not be supported by all languages and frameworks either. The correct datatype in java.time I think would be OffsetDateTime.

Changing the generators to de/serialize OffsetDateTime instead of Instant would obviously be a breaking change wrt the generated code, so we should be careful making this change and only enable it via a generator attribute, but we can certainly fix the deserialization of date times with offsets into Instant via ZonedDateTime.parse(str).toInstant

gheine commented 5 years ago

Fixed offset date time parsing in #525

gheine commented 5 years ago

In preparation to a fix that allows generating OffsetDateTime instead of Instant via a generator attribute, all de/serialization had to be consolidated in one place. See #526