Undesirable Implicit fallback to defaults in case of failure during Time parsing

[sledorze]

This issue is a followup of this comment: https://github.com/twilio/guardrail/issues/103#issuecomment-426459346

Here's a copy/pasted recap;

Looking at these implicits do:

implicit def decodeInstant: Decoder[Instant] = j8time.decodeInstant.or(decodeLong.map(Instant.ofEpochMilli))
implicit def decodeLocalDate: Decoder[LocalDate] = j8time.decodeLocalDateDefault.or(decodeInstant.map(_.atZone(ZoneOffset.UTC).toLocalDate))
implicit def decodeOffsetDateTime: Decoder[OffsetDateTime] = j8time.decodeOffsetDateTimeDefault.or(decodeInstant.map(_.atZone(ZoneOffset.UTC).toOffsetDateTime))

// Mirror
implicit val encodeInstant = j8time.encodeInstant
implicit val encodeLocalDateDefault = j8time.encodeLocalDateDefault
implicit val encodeLocalDateTimeDefault = j8time.encodeLocalDateTimeDefault
implicit val encodeLocalTimeDefault = j8time.encodeLocalTimeDefault
implicit val encodeOffsetDateTimeDefault = j8time.encodeOffsetDateTimeDefault
implicit val encodeZonedDateTimeDefault = j8time.encodeZonedDateTimeDefault

decodeInstant:

Attempts to decode an Instant using whatever format is default to circe's java8 time formatter. If that fails, we attempt to parse a long and treat it as Epoch Millis (❗️) decodeLocalDate:

Attempt to decode a LocalDate using whatever format is default to circe's java8 time formatter. If that fails, we attempt to decode an instant and assume it's UTC (❗️) decodeOffsetDateTime:

Attempts to decode an OffsetDateTime using whatever format is default to circe's java8 time formatter. If that fails, we attempt to decode an instant and assume it's UTC (❗️) It would be good to break the fallback handlers out into explicit vendor extensions if these are actually desirable. Instead of type: string and format: date-time secretly handling milliseconds, it would be better to have type: integer and format: int64 with a vendor extension that specifies that the values are actually UTC milliseconds (or nanoseconds, or seconds).

Once an answer is determined for how to handle these fallback scenarios, describing a new type that represents the transition between Long and OffsetDateTime could look like this:

case class EpochMillis(value: OffsetDateTime)
implicit val encodeMillis: Encoder[EpochMillis] =
  Encoder[Long]
    .contramap[OffsetDateTime](_.toInstant.toEpochMilli)
    .contramap[EpochMillis](_.value)
implicit val decodeMillis: Decoder[EpochMillis] =
  Decoder[Long]
    .map(Instant.ofEpochMilli _)
    .map(_.atZone(ZoneOffset.UTC).toOffsetDateTime)
    .map(EpochMillis.apply _)

This would not be exposed to the user directly, similar to how respond members or file upload parts work.

I don't have answers about exactly how this should work, from a user perspective though, or from what already exists in the broader OpenAPI community though. What do you think?

[blast-hardcheese]

This definitely seems like the kind of thing that could be solved with oneOf, and should be moved to an opt-in.

[blast-hardcheese]

calling this out as a bug as it's an undocumented feature