-
Notifications
You must be signed in to change notification settings - Fork 19
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
Common API design for IPv4 and IPv6 #9
Comments
Hi Ribesg, Although I understand your use-case, the objective of the library was to have very simple API with strongly-typed guarantees and as such it was a deliberate design choice to use more specific type representations as opposed to have one generic type that can either represent IPv4 or IPv6. Moreover, compared to the API of Having said that, it is left to the clients of the library to find ways of sharing code and one way I've seen in practice is to introduce a parameterized abstract class to keep the shared code (like shown below), although it is not the cleanest solution because of generics. class MyClass<SingleIp extends AbstractIp<SingleIp, IpRange>, IpRange extends AbstractIpRange<SingleIp, IpRange>> {
private final IpRange ipRange;
public MyClass(IpRange ipRange) {
this.ipRange = ipRange;
}
public String getAddressFamily() {
if (ipRange instanceof Ipv4Range) {
return "IPv4";
} else {
return "IPv6";
}
}
} I'm keeping this issue open for now as something to consider for a future version (pull requests welcome) |
This is exactly what I expected from such library, but I understand your point of view. Another little flaw I found is that this lib is oriented a bit much to IP ranges, and does not really take the concept of subnets into account. I would expect such library to offer ways to get subnet address, broadcast address and some way to iterate over only assignable IPs in the subnet. Something like I don't think that I'll contribute to this project, my ideas are too different, I would rewrite mostly everything but the "final" classes 👅 Thanks again for this lib anyway! |
We were also trying to mix ipv4 and ipv6 addresses together. After doing some research, there is an RFC which describes an official way to encode ipv4 destinations as ipv6 addresses. Following this we are able to do comparisons and range checks treating both ipv4 and ipv6 addresses the same. List<Ipv6Range> list = new ArrayList<>();
for (String ipset : license.valid_ip_ranges.split(",")) {
if (ipset.contains(":")){
if (ipset.contains("/") || ipset.contains("-")) {
list.add(Ipv6Range.parse(ipset));
} else {
list.add(Ipv6Range.parse(ipset + "-" + ipset));
}
continue;
}
if (ipset.contains("/") || ipset.contains("-")) {
Ipv4Range ipv4Range = Ipv4Range.parse(ipset);
list.add(Ipv6Range.parse("::FFFF:"+ipv4Range.start()+"-"+"::FFFF:"+ipv4Range.end()));
} else {
list.add(Ipv6Range.parse("::FFFF:"+ipset + "-::FFFF:" + ipset));
}
}
Collections.sort(list, (a, b) -> {
if (a.start().equals(b.start())) {
return a.end().compareTo(b.end());
}
return a.start().compareTo(b.start());
}); |
Thanks for the input @michael-newsrx. Indeed, this is something you can also do as the library supports parsing IPv6 address with embedded IPv4 addressed according to RFC 4291. However it is not only limited to IPv4-Compatible IPv6 Address (§2.5.5.1) and IPv4-Mapped IPv6 Address (§2.5.5.2) but can parse any valid address that is composed by an IPv6 and an IPv4 address e.g. Thinking of it, it does make sense to add a helper method to support this use case (e.g. Finally, you can simplify your code by using one of the predefined Comparators for sorting the IPv6 addressed: Collections.sort(list, StartAndSizeComparator.<Ipv6, Ipv6Range>get());
// or
Collections.sort(list, SizeComparator.<Ipv6Range>get()); |
As we chose FFFF variant based on what information I could find at the time (using 0000 as not-recommended) and there exists the other 0000 variant, you might want to set the first utility function to use FFFF and add a second copy of the function where you can specify the 0000 or FFFF being applied. I can see where people might need access to both forms. But I think it would be more useful to have casting methods (not util functions) on ipv4 and ipv6 addresses and ranges so that you can do: ... = ipv4address.asIpv6Legacy();
... = ipv4address.asIpv6Mapped();
... = ipv4range.asIpv6Legacy();
... = ipv4range.asIpv6Mapped();
boolean ... = ipv6address.isIpv6Legacy();
boolean ... = ipv6address.isIpv6Mapped();
boolean ... = ipv6range.isIpv6Legacy();
boolean ... = ipv6range.isIpv6Mapped();
... = ipv6address.asIpv4Address(); //null or ip4address, no exceptions - they can be expensive
... = ipv6range.asIpv4Range(); // null or ip4range, no exceptions - they can be expensive |
Hi, thanks for this library! I'm using it in Kotlin.
My main problem with this library is that the IPv4 and IPv6 parts are entirely separated, from an API point of view. There is no
IpRange
for example. It would be far easier to use, especially as I have to mostly write the same code twice.I understand that it's a huge change, but it would nice if you could consider this for a v2.
Thanks,
Ribesg
The text was updated successfully, but these errors were encountered: