search

achanda / ipnetwork

A library to work with CIDRs in rust
Apache License 2.0
121 stars 38 forks source link

Consider removing From<IpXXAddr> for IpXXNetwork #190

Open sunshowers opened 6 months ago

sunshowers commented 6 months ago

Hi there, thanks for maintaining this crate!

At our workplace we ran into an issue where we were accidentally converting an Ipv6Addr to an Ipv6Network. The conversion seems a bit risky because an address and a network represent different things.

In contrast, From<Ipv4Network> for IpNetwork and From<Ipv6Network> for IpNetwork are reasonable to have.

Would you consider removing these impls and maybe introducing single constructors to represent the single IP use case? That would have made this bug much easier to spot.

Thanks!

achanda commented 6 months ago

Hi 👋 sounds like you want a conversion from Ipv6Addr to Ipv6Network to error out, but that does not happen due to this impl https://github.com/achanda/ipnetwork/blob/master/src/ipv6.rs#L315 so you want to remove those impls. Is that correct? What would the type signature of the single constructor be?

sunshowers commented 6 months ago

Thanks for the quick response! Yes, I'm suggesting removing those impls. It'll be a breaking change, but I think one that's good from an API misuse perspective.

How about just an Ipv6Network::single(addr: Ipv6Addr) constructor? I'm not saying don't provide this, I'm just saying that it would be nice for that to be more explicit.

faern commented 1 month ago

I agree with this suggestion. A impl From<X> for Y should only exist if what the conversion does is obvious. Here I'd say it's not so obvious that IP -> IPnet sets the prefix to the maximum and the result represents a single IP.

A dedicated constructor is more explicit and harder to accidentally misuse.

In contrast, From<Ipv4Network> for IpNetwork and From<Ipv6Network> for IpNetwork are reasonable to have.

I don't agree. They are vague in the same sense IMHO. Converting a network into a single IP can probably be misunderstood what it does. And there is an ip() method already that is more clear.