Port HttpDate-related parsers to cats-parse #3958
Conversation
rossabaker
commented
Nov 28, 2020
rossabaker
commented
- Can't eliminate the parboiled2 version yet, because other rules reference it, and parboiled2 doesn't compose.
- Deserves more tests, but it's more than we had.
- Ported the test we had to MUnit.
| pure(org.http4s.HttpDate.unsafeFromZonedDateTime(dt)) | ||
| } catch { | ||
| case _: DateTimeException => | ||
| failWith(s"Invalid IMF-fixdate: $year-$month-$day $hour:$min:$sec") |
There was a problem hiding this comment.
This demonstrates a failure of our model: the RFC doesn't have anything to say about the date making sense, and being forced to flatMap is bad for parser optimization.
In 1.0, we should carry around the six fields, and have a Try[ZonedDateTime].
There was a problem hiding this comment.
Ah, nuts. RFC 5322:
A date-time specification MUST be semantically valid. That is, the
day-of-week (if included) MUST be the day implied by the date, the
numeric day-of-month MUST be between 1 and the number of days allowed
for the specified month (in the specified year), the time-of-day MUST
be in the range 00:00:00 through 23:59:60 (the number of seconds
allowing for a leap second; see [RFC1305]), and the last two digits
of the zone MUST be within the range 00 through 59.
| min: Int, | ||
| sec: Int): Parser[HttpDate] = | ||
| try { | ||
| val dt = ZonedDateTime.of(year, month, day, hour, min, sec, 0, ZoneOffset.UTC) |
There was a problem hiding this comment.
Note that the day of week is parsed, but never validated. Never has been. Probably never should be.
rossabaker
changed the title
|
I thought I could eliminate the parboiled date rules, but cookie has one, and the cookie spec is one of more complicated ones, and one whose strictness people have complained about. That one will require more thought. I think this one represents a nice step forward as is. |
| // A cache recipient MUST interpret invalid date formats, especially the | ||
| // value "0", as representing a time in the past (i.e., "already | ||
| // expired"). | ||
| def invalid = anyChar.rep.as(HttpDate.Epoch) |
There was a problem hiding this comment.
We used to handle "0" and "-1" special, but the spec likes them all.
A better model in 1.0 would preserve the invalid value for round tripping instead of forcing it to the epoch.
| } | ||
|
|
||
| /* `Expires = HTTP-date` */ | ||
| private[http4s] val parser: Parser[Expires] = { |
There was a problem hiding this comment.
As a consequence of treating invalid dates as special, this becomes a Parser instead of a Parser1. I don't think that will haunt us, but, maybe?
