Support parsing ISO 8601 date time with offset in circe decoder

[gheine]

No description provided.

[mkows]

@gheine, any reason why you did not decide to go with OffsetDateTime that you mentioned #520. I think it's actually better suited (as more direct) to what we're trying to achieve here. Example:

scala> ZonedDateTime.parse("2016-08-22T14:30+02:00[Europe/Paris]")
res5: java.time.ZonedDateTime = 2016-08-22T14:30+02:00[Europe/Paris]

scala> OffsetDateTime.parse("2016-08-22T14:30+02:00[Europe/Paris]")
java.time.format.DateTimeParseException: Text '2016-08-22T14:30+02:00[Europe/Paris]' could not be parsed, unparsed text found at index 22
  at java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:1952)
  at java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1851)
  at java.time.OffsetDateTime.parse(OffsetDateTime.java:402)
  at java.time.OffsetDateTime.parse(OffsetDateTime.java:387)
  ... 28 elided

As currently suggested, 2016-08-22T14:30+02:00[Europe/Paris] would also be supported, while it does not seem to be a valid ISO 8601 date time (as per https://en.wikipedia.org/wiki/ISO_8601#Time_zone_designators).

Also, while this approach will address the (more severe) issue of parsing (huge improvement), still, the ability to output (encode) time as ISO 8601 will be missing (as flatten to UTC -- so time zone info gets lost).

[gheine]

My thought was that the scala/java data type for fields models and operation parameters for the apibuilder date_time type should/could be OffsetDateTime so that when the type is serialized over the wire, no information is lost and the serialization is in accordance with ISO 8601 (which ZonedDateTime seemingly isn't). But on the way in, ie when a json string that represents a date_time type is parsed into a scala/java type, using ZonedDateTime is the most flexible as it would even be able to deserialize date times with zone id. The date time formats that ZonedDateTime is able to parse is a superset of the ones that OffsetDateTime and Instant are able to parse. But again, on the way out we should be more strict and not serialize ZonedDateTime.

And correct, this fix only addresses the issue of parsing any ISO 8601 date time string, not the problem that timezone offset information is lost by converting to Instant. But since that would be a breaking change (code that relies on generated code that uses Instant would break if the spec is regenerated with OffsetDateTime). I'm working on a fix that will use a generator attribute to make that switch explicit.

[mkows]

@gheine nice!

I did refer to just a fact of using OffsetDateTime instead of ZonedDateTime for the purpose of parsing here. As the (long-term) plan is to go with OffsetDateTime, I don't think it really makes sense to broaden even further the input type (so that it's narrowed down to OffsetDateTime on the later stage).

[gheine]

Even though hypothetical, I don't think there's harm in parsing beyond what the ISO 8601 spec mandates.

[mkows]

I don't think there's harm in parsing beyond what the ISO 8601 spec mandates.

Still, if the plan is to represent date time as OffsetDateTime then at that point we should only stick to this both on input and output. Therefore I feel that introducing ZonedDateTime is not justified given that plan (as it will be removed on the later stage).

[gheine]

We're not "introducing" ZonedDateTime we're just using it under the hood to parse as many date time strings as possible. It'll then be converted to either an Instant or an OffsetDateTime, which are the data types that are/will be exposed in the API, models, etc.

[mkows]

@gheine by using ZonedDateTime for parsing we are supporting all ZonedDateTime-compatible values on the input (as valid iso-8601 date times). If the (most likely?) plan is to switch over to OffsetDateTime then support for ZonedDateTime-specific (i.e. that are not OffsetDateTime) values will be dropped soon -- so why not stick to OffsetDateTime from now on.

Now I played with it some more, and it looks like joda time would also reject value in ZonedDateTime-specific format e.g. 2016-08-22T14:30+02:00[Europe/Paris] as in:

scala> import org.joda.time._
import org.joda.time._

scala> DateTime.parse("2016-08-22T14:30+02:00")
res0: org.joda.time.DateTime = 2016-08-22T14:30:00.000+02:00

scala> DateTime.parse("2016-08-22T14:30+02:00[Europe/Paris]")
java.lang.IllegalArgumentException: Invalid format: "2016-08-22T14:30+02:00[Europe/Paris]" is malformed at "[Europe/Paris]"
  at org.joda.time.format.DateTimeFormatter.parseDateTime(DateTimeFormatter.java:945)
  at org.joda.time.DateTime.parse(DateTime.java:160)
  at org.joda.time.DateTime.parse(DateTime.java:149)
  ... 36 elided

and also

scala> val str = "2016-08-22T14:30+02:00[Europe/Paris]"
str: String = 2016-08-22T14:30+02:00[Europe/Paris]

scala> import org.joda.time.format.ISODateTimeFormat.dateTimeParser
import org.joda.time.format.ISODateTimeFormat.dateTimeParser

scala> dateTimeParser.parseDateTime(str)
java.lang.IllegalArgumentException: Invalid format: "2016-08-22T14:30+02:00[Europe/Paris]" is malformed at "[Europe/Paris]"
  at org.joda.time.format.DateTimeFormatter.parseDateTime(DateTimeFormatter.java:945)
  ... 36 elided

Given the above, I don't see a reason to widen the accepted values outside of strict iso-8601 date time. (Just for the record: at the same time I am ok with the currently suggested changes but my preference would be to be more strict as stick to OffsetDateTime).

[gheine]

My proposal was to offer the choice between ZonedDateTime and Instant on the API level (ie. model case classes, client operations etc.) but in either case use ZonedDateTime to parse incoming json strings and then convert this to the appropriate API type (ZonedDateTime or Instant). So really the fact that we're using ZonedDateTime under the hood would just be an implementation detail. It might as well be "MyGreatDateTimeParser" that spits out the input String as either an Instant or a ZonedDateTime.

But I see your point, the joda doesn't support zone id suffixes like [Europe/Paris] nor are they part of the ISO 8601 spec - https://docs.oracle.com/javase/8/docs/api/java/time/format/DateTimeFormatter.html#ISO_ZONED_DATE_TIME explicitly states The section in square brackets is not part of the ISO-8601 standard.
So I'm willing to concede. Thanks for challenging my viewpoint and assumptions. :-)

[mkows]

@gheine regarding

My proposal was to offer the choice between ZonedDateTime and Instant on the API level (ie. model case classes, client operations etc.)

I would be against allowing Instant in any case. IMO Instant is not an option here as it's not ISO-8601 date time compliant (which is required by https://app.apibuilder.io/doc/types). Too narrow. OffsetDateTime which you initially suggested seems to be the right (and only one really) fit here to model date time going forward.

[mkows]

🙃

[gheine]

@mkows Instant is the type that is being generated right now, so we can't remove it or change it without p*ssing people off.