guardrail-dev / guardrail-dev/guardrail

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

Open
#113 2 comments 1 reaction 0 assignees View on GitHub
bug help wanted scala
Dominant language
Scala
Stars
541
Forks
138
PR merge metrics
No merged PRs in 30d

Description

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:
```scala
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:

```scala
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?

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.