-
Notifications
You must be signed in to change notification settings - Fork 787
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Origin header #5082
Update Origin header #5082
Conversation
// If the Origin is not "null", it is a non-empty list of Hosts: | ||
// http://tools.ietf.org/html/rfc6454#section-7 | ||
final case class HostList(hosts: NonEmptyList[Host]) extends Origin | ||
case object `null` extends Origin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of another case that we modeled the header as a sum type instead of a unary case class of a sum type, but I think I like it. It sidesteps the org.http4s.Origin
vs. org.http4s.header.Origin
debate. And it slipped through review the first time without me noticing!
test("OriginHeader parser should Parse an empty origin") { | ||
val text = "" | ||
val origin = Origin.Null | ||
test("OriginHeader should fail on a list of multiple hosts") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth a comment referring to the obsolete RFC here.
test("OriginHeader parser should Parse a 'null' origin") { | ||
val text = "null" | ||
val origin = Origin.Null | ||
test("OriginHeader should fail on an empty string") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about whether this a bug we should backport, or a breaking change we shouldn't backport. But it's certainly right for this version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say the former, but I don't think it's super important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I was cursing that HostList
over the weekend on another thing I'm working on.
Backport empty Origin failure from #5082
Resolves #5009.
I've also taken the opportunity to backtify the
null
value for consistency with the rest of the library, to stop parsing empty values asnull
(it's not allowed by the spec), and generally to simplify the parsing implementation.