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

Improved ipv6 support in Uri #2700

Merged
merged 2 commits into from Jul 9, 2019
Merged

Improved ipv6 support in Uri #2700

merged 2 commits into from Jul 9, 2019

Conversation

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Jul 7, 2019

  • Deprecate IPv6 in favor of Ipv6Address
  • Ipv6Address is a case class of eight hextets
  • Implements RFC5952 for normalized output
  • Adds string interpolator
  • Requires that ipv6 addresses be bracketed in authority form of request target

override def render(writer: Writer): writer.type = this match {
case RegName(n) => writer << n
case a: Ipv4Address => writer << a.value
case IPv6(a) => writer << '[' << a << ']'
case a: Ipv6Address => writer << '[' << a << ']'
Copy link
Member Author

@rossabaker rossabaker Jul 7, 2019

Choose a reason for hiding this comment

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

We need to have a long navel-gazing over the differences between render, value, toString, and Show. I'm only making it worse.

Loading

Choose a reason for hiding this comment

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

I strongly dislike the mutable renderers.

Loading

Copy link
Member Author

@rossabaker rossabaker Jul 8, 2019

Choose a reason for hiding this comment

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

I was just thinking about this mutability last night, and came to the conclusion that it's a side effect I can live with. I think it's necessary to have something like it, because rendering message headers is String concatenation: I don't think there's a practical way around having a StringBuilder under the hood.

  • We could just target a ByteVector, but encoding to bytes on the fly has proven to be slower than building a String and encoding that. That's why, unlike akka-http, we don't have a byte-based writer.
  • We might accumulate a Chain[String] and flatten it for encoding, but I think that would increase GC pressure. It would also not support the phantom writing of headers we're starting to do for multipart.
  • Another answer is to suspend Writer operations into an effect, but these writers don't get shared. It's the functional answer, but I'm not sure it's an appropriate engineering tradeoff here.

Maybe this merits a quick proof-of-concept benchmark.

Loading

(IpLiteral | capture(IpV6Address)) ~> { s: String =>
org.http4s.Uri.IPv6(s.ci)
} |
"[" ~ ipv6Address ~ "]" |
Copy link
Member Author

@rossabaker rossabaker Jul 7, 2019

Choose a reason for hiding this comment

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

I think we temporarily lost support for IPvFuture here. There wasn't a test, and I highly doubt anyone cares, but it needs to be brought back for compliance sake.

Loading

).map(Uri.IPv6.apply)
implicit val http4sTestingArbitraryForIpv6Address: Arbitrary[Uri.Ipv6Address] = Arbitrary {
for {
a <- getArbitrary[Short]
Copy link
Member Author

@rossabaker rossabaker Jul 7, 2019

Choose a reason for hiding this comment

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

I chose not to take on the cats-scalacheck dependency to get mapN. Since we publish the arbitraries, I thought it better to stay light.

Talk me out of it.

Loading

Choose a reason for hiding this comment

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

These are equivalent, so I really don't care enough to argue.

It does the job right, but its not breaking. That being said. cats-scalacheck is one of those libraries that I think shouldn't be a microlibrary but instead in cats or at least recommended as these instances should be available everywhere.

Loading

Copy link
Member Author

@rossabaker rossabaker Jul 8, 2019

Choose a reason for hiding this comment

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

I was worried about the circular dependency and wary of relying on it, but I wasn't thinking clearly.

  1. Putting it into scalacheck would be a circular dependency. That would be bad.
  2. Putting it into cats-laws would not add any new dependencies.
  3. It can live on its own, with the only constraint that it not upgrade scalacheck higher than what's in cats-laws for the corresponding cats. It's just a chore, not an impossibility, to make a compatible release.

We shouldn't be so scared of the status quo (3), but I see no reason not to implement (2).

Loading

Copy link
Member

@ChristopherDavenport ChristopherDavenport left a comment

No bikeshedding needed here

Loading

@rossabaker rossabaker merged commit 462ff33 into http4s:master Jul 9, 2019
1 check passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants