Skip to content

Adopt ip4s #4242

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

Merged
merged 7 commits into from
Feb 4, 2021
Merged

Adopt ip4s #4242

merged 7 commits into from
Feb 4, 2021

Conversation

rossabaker
Copy link
Member

fs2-3.x is taking on an ip4s dependency, so it will be on our classpath anyway in 1.0. It has a richer model of addresses and is a lot harder to accidentally trigger network lookups, so it's good for us to use.

In the spirit of making 0.22 look as much like 1.0 as possible without cats-effect, we begin the adoption here.

@rossabaker rossabaker added the breaking Change breaks compatibility (binary or source) label Jan 23, 2021
@rossabaker rossabaker marked this pull request as draft January 23, 2021 21:13
@rossabaker
Copy link
Member Author

Not being as cute as we were with folds in #4207. This will supersede it.

}

final case class Ipv4Address(a: Byte, b: Byte, c: Byte, d: Byte)
final case class Ipv4Address(address: ip4s.Ipv4Address)
Copy link
Member Author

Choose a reason for hiding this comment

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

Forwarded has wrappers just called Ipv4. We could alternatively reclaim that name here and not need this prefix. We should be consistent with Forwarded either way.


implicit val http4sTestingCogenForIpv4Address: Cogen[Uri.Ipv4Address] =
Cogen[(Byte, Byte, Byte, Byte)].contramap(ipv4 => (ipv4.a, ipv4.b, ipv4.c, ipv4.d))
Cogen[Array[Byte]].contramap(_.address.toBytes)
Copy link
Member Author

Choose a reason for hiding this comment

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

@mpilquist Do you want Cogen instances?

// TODO After release, replace with https://github.com/Comcast/ip4s/blob/4c799aa81d24f0d097daec8cd6f8fc029ed4ad3e/jvm/src/main/scala/com/comcast/ip4s/IpAddressPlatform.scala#L58
fromByteArray(address.getAddress).valueOr(throw _)

def fromShorts(a: Short, b: Short, c: Short, d: Short, e: Short, f: Short, g: Short, h: Short)
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this should be in ip4s, but maybe we're the only ones who ever modeled it as shorts?

Copy link
Member Author

@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.

I'm surprised to not hit connection info problems in the Ember backend.

Comment on lines +263 to +268
local = SocketAddress(
IpAddress.fromBytes(local.getAddress.getAddress).get,
Port(local.getPort).get),
remote = SocketAddress(
IpAddress.fromBytes(remote.getAddress.getAddress).get,
Port(remote.getPort).get),
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 is inartful. And maybe wasteful if unreferenced!

def remoteHost: Option[String] = remote.map(_.getHostName)
def remoteHost(implicit F: Sync[F]): F[Option[Hostname]] = {
val inetAddress = remote.map(_.ip.toInetAddress)
F.delay(inetAddress.map(_.getHostName)).map(_.flatMap(Hostname(_)))
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this should have been getHostAddress all along, and then it wouldn't need the F? Why are we doing reverse lookups here?

Comment on lines +105 to +106
local = SocketAddress(IpAddress(req.getLocalAddr).get, Port(req.getLocalPort).get),
remote = SocketAddress(IpAddress(req.getRemoteAddr).get, Port(req.getRemotePort).get),
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 is also inartful.

class ErrorActionSuite extends Http4sSuite {
val remote = "192.168.0.1"
val remote = IpAddress("192.168.0.1").get
Copy link
Member Author

Choose a reason for hiding this comment

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

We're running up against a problem here where our ipv4 and ipv6 interpolators conflict with ip4s', and ip4s' are imported at the root level, so they're almost always going to win. I think we should retire ours. Most of what @hamnis just did for ours could continue on: just remove the conflicting ones.

@rossabaker
Copy link
Member Author

An ip4s 1.4.2 release, with its fromInetAddress methods, would clean up a lot of .get.

@rossabaker rossabaker marked this pull request as ready for review January 24, 2021 05:46
@rossabaker rossabaker merged commit 2a567ef into http4s:series/0.22 Feb 4, 2021
@rossabaker rossabaker deleted the ip4s-take-2 branch February 4, 2021 14:30
rossabaker added a commit to http4s/http4s-servlet that referenced this pull request Apr 3, 2022
rossabaker added a commit to http4s/http4s-servlet that referenced this pull request Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Change breaks compatibility (binary or source)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants