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

Proposal for toString representation of AsnRange and AbstractIpRange #19

Open
dadepo opened this issue Nov 3, 2017 · 2 comments
Open

Comments

@dadepo
Copy link
Contributor

dadepo commented Nov 3, 2017

In practice, we have had to render a collection of different ip numbers (asn, ipv4, ipv6) as strings.

The current string representation is in the format startIp - endIp and since the library also supports constructing a range with both values as start and end, or converting a single ASN to a range, you run into the situation where the string representation becomes a little bit redundant.

For example new Asn(3333l).asRange(); gives AS3333-AS3333 which appears redundant, and IMHO, strictly speaking, is not even a range if it starts and end with the same value.

Will it be an acceptable proposal to have the toString representation return just a value in cases where a range contains just one value?

@jgonian
Copy link
Owner

jgonian commented Nov 4, 2017

Hi @dadepo,

the current implementation of toString aims to be symmetrical with parse(), that is, the string representation of a type should be possible to be parsed into the same type again. From that angle, your proposal breaks this contract. For example,

Asn.parse(Asn.of("AS3333").toString());  // good
Ipv4.parse(Ipv4.parse("192.188.1.1").toString()); // good

// however
AsnRange.parse("AS3333");       // throws IllegalArgumentException
Ipv4Range.parse("192.168.1.1");  // throws IllegalArgumentException

Moreover, for IP ranges it is not correct to have a toString() method that omits the prefix length even when you have a range of one address. For example, if Ipv4Range.parse("192.168.1.1-192.168.1.1").toString() returns 192.168.1.1 this is not correct as it not the same as 192.168.1.1/32. The first is a single IPv4 address while the second is an IP prefix (or subnet) of one IP address.

So, the only option we could consider is adding a new method to AsnRange (only) that does the check of the length for you e.g.

public String toStringShortenSingleResources() { ... }

But I think that if user-facing software is the main concern here, then the application should take care of the condensed representation as the library cannot cater for all cases.

@dadepo
Copy link
Contributor Author

dadepo commented Nov 11, 2017

Regarding the toString/parse repercussion, perhaps the implementation of the parse needs to be updated?

In a world where the Asn class which represents a single-valued asn has a asRange method, which allows construction of a range from an Asn, for completeness sake, shouldn't parse also be able to do same?

Asn.of("AS3333").asRange(); // range created from AS3333
AsnRange.parse("AS3333"); // should this also be possible?

if it matters, I observed IANA does use the [start] - [end] format to represent contiguous ASN numbers, but not when the contiguous ASN numbers contain just one value; then just that value is used.

As regards the other observation with IP ranges and prefix length, I feel this is a result of a fundamental tension between the design choice to conflate ranges that can be represented via CIDR and ranges that are just arbitrary contiguous IP numbers.

Moreover, for IP ranges it is not correct to have a toString() method that omits the prefix length even when you have a range of one address

Yes, but it is also not correct to have a range of 3 IP numbers...that is Ipv4Range.from("192.168.1.1").to("192.168.1.3")

or not? Well, maybe it depends? If you intend to work with CIDR then it is not correct, if you just want contiguous IP numbers, then maybe it is correct? And then, having Ipv4Range.parse("192.168.1.1-192.168.1.1") return 192.168.1.1 in a non-CIDR use case is valid

But that also leads to the ambiguity where you ask: when is 192.168.1.1 a single IP number, in one valued non-CIDR range, and when is it supposed to be 192.168.1.1/32?

For ip-num, which I am working on, I currently have support for only CIDR-able range, and still thinking of how to introduce another abstraction for contiguous IP numbers in such a way that the two work together as intuitively as possible.

I am quite certain there must have been discussions along these lines during the early development of commons-ip-math. Would be curious to know why the current design choice was made :)

But yes, with the same abstraction used for CIDR-able ranges and non CIDR ranges, I am not sure it is possible (or how) to resolve the conflict/ambiguity my proposal to the toString implementation would lead to.

So, the only option we could consider is adding a new method to AsnRange (only) that does the check of the length for you e.g.

This would not help that much with the use case I ran into at work that leads to making this proposition, which is having a collection of different ranges (Asn, IPv4, IPv6) treating them all as AbstractIpRange and calling toString on them.

But I think that if user-facing software is the main concern here, then the application should take care of the condensed representation as the library cannot cater for all cases.

Yeah. This is what we are currently doing. It might just be that there isn't a feasible way to make the library cater for these cases, then I guess there is not much that can be done.

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

No branches or pull requests

2 participants