Skip to content
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

Cats parse uri #4132

Merged
merged 24 commits into from Jan 4, 2021
Merged

Cats parse uri #4132

merged 24 commits into from Jan 4, 2021

Conversation

RaasAhsan
Copy link
Member

Opening up another PR (original at #4095) so I can run builds here

* "conservative in our sending behavior and liberal in our
* receiving behavior", and encode them.
*/
private[http4s] def parser: Parser[Query] = {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a val in the original PR, there were some initialization ordering issues that caused a NPE

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of the dangerous of putting the parsers on the companions. We don't get compiler help with the ordering of eager vals. I'm not sure how heavy creating a new Parser instance every time is. We possibly want a lazy val here, or to understand the cycle that caused this. But this gets us moving forward.

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for picking this up!

core/src/main/scala/org/http4s/headers/Link.scala Outdated Show resolved Hide resolved
val bracketedIpv6 = char('[') *> Rfc3986.ipv6 <* char(']')
val host = List(bracketedIpv6, Rfc3986.ipv4, stringHost).reduceLeft(_ orElse1 _)
val bracketedIpv6 = char('[') *> Uri.Ipv6Address.parser <* char(']')
val host = List(bracketedIpv6, Uri.Ipv4Address.parser, stringHost).reduceLeft(_ orElse1 _)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should unnest these types from the Uri, but that can wait until we merge this to main.

"""Invalid input ' ', expected Pchar, '/', '?', '#' or 'EOI' (line 1, column 21):
http://example.org/a file
^""".replace("\r", "")
"Error(20,NonEmptyList(EndOfString(20,25)))"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Future note: this is something we'll want to improve in ParseResult.parseFrom. There's a LocationMap that will help with the arrow. And bosatsu allegedly has a nicer formatter already.

@rossabaker
Copy link
Member

Interesting: a few of these builds got canceled for hanging.

@rossabaker
Copy link
Member

The build hang is a pre-existing condition, and in trying to stabilize that, I hit the warning on the Link parser that this fixes. I think we should merge this to dotty now, and I'll beat on the hanging before we merge dotty to main.

}

"only parse part of an invalid ipv6 address where a single section is shortened (must be 2 or more)" in {
val invalidIp = "2001:db8::1:1:1:1:1"
Rfc3986.ipv6.parse(invalidIp).map(_._2.value) must beRight("2001:db8::1:1:1:1")
Ipv6Address.fromString(invalidIp).map(_.value) must beLeft
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarified with @lewisjkl on #4004: they were Rights because the tests are using parse, and don't consume all input. They flipped from left to right when the Parser.ends were removed.

Ipv6Address.fromString uses parseAll, which is a "clientside end", which correctly flips these back to lefts. Jeff was right, and now so is this.

@rossabaker rossabaker merged commit 4b14561 into http4s:dotty Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants