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

Draft: proof-of-concept for Java 17 API improvements #19

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

erikrozendaal
Copy link
Contributor

New classes are in the net.ripe.ipresource.scratch package.

Major changes are:

  1. Single resources (ASN, IPv4 address, IPv6 address) no longer extends the range classes.

  2. Additional implementation classes for IP prefixes (both v4 and v6) so you can use static types when a prefix is required.

  3. IPv6 address now uses two longs to represent the address, instead of a BigInteger.

  4. ASN ranges and IPv4 blocks use primitive types internally to reduce memory usage. This does result in allocations when converting the start and end points into ASN/IPv4Address instances.

  5. Internal code tries to avoid allocations in common paths, prefering to use primitive types instead.

  6. Old RIPE prefix notation without trailing zeroes (e.g. 10.0/16) is not supported.

Still many things to do:

  1. Documentation and tests.

  2. Code makes use of the preview pattern matching feature, for production release this should be replaced with if-else chains.

  3. Parsing code needs to be ported from old classes.

  4. API is not complete yet.

  • I have updated the changelog in README.md

New classes are in the net.ripe.ipresource.scratch package.

Major changes are:

1. Single resources (ASN, IPv4 address, IPv6 address) no longer
   extends the range classes.

2. Additional implementation classes for IP prefixes (both v4 and v6)
   so you can use static types when a prefix is required.

3. IPv6 address now uses two `long`s to represent the address, instead
   of a `BigInteger`.

4. ASN ranges and IPv4 blocks use primitive types internally to reduce
   memory usage. This does result in allocations when converting the
   start and end points into ASN/IPv4Address instances.

5. Internal code tries to avoid allocations in common paths, prefering
   to use primitive types instead.

6. Old RIPE prefix notation without trailing zeroes (e.g. 10.0/16) is
   not supported.

Still many things to do:

1. Documentation and tests.

2. Code makes use of the preview pattern matching feature, for
   production release this should be replaced with if-else chains.

3. Parsing code needs to be ported from old classes.

4. API is not complete yet.
@erikrozendaal
Copy link
Contributor Author

Also, there are no separate classes for AsnRangeSet, IpBlockSet, etc. So currently you cannot statically enforce that only certain resource types are present in a set of resources.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@erikrozendaal
Copy link
Contributor Author

Should we rename AsnRange to AsnBlock for consistency with IpBlock (parent of IpRange and IpPrefix) and the RIPE DB naming?

private static Ipv4Block of(long start, long end) {
if (isLegalPrefix(start, end)) {
long temp = start ^ end;
int length = Integer.numberOfLeadingZeros((int) temp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand correctly that it's irreversible? So if the (being, end) range is not a prefix, it will build the biggest prefix fitting into the range?

import static net.ripe.ipresource.scratch.Ipv6Address.parse;
import static org.junit.Assert.*;

public class Ipv6AddressTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have a property test for parse(toString(i)) == i?

@lolepezy
Copy link
Contributor

It would be nice to fix the CodeQL-found issues


public final class Ipv6Address implements IpAddress {
public static final int NUMBER_OF_BITS = 128;

public static final Ipv6Address LOWEST = new Ipv6Address(0, 0);
public static final Ipv6Address HIGHEST = new Ipv6Address(-1, -1);

/* Pattern to match IPv6 addresses in forms defined in http://www.ietf.org/rfc/rfc4291.txt */
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks quite intimidating and complex.
Don't we want to use some library for parsing?
https://github.com/AdoptOpenJDK/openjdk-jdk11/blob/master/src/java.base/share/classes/sun/net/util/IPAddressUtil.java
Or Guava or something

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

Successfully merging this pull request may close these issues.

3 participants