Add Ipv6 version of IpSpace

[eterny13]

#8706
I try to implement minor ipv6 support.

[batfish-bot]

This change is 

[dhalperi]

Thanks for the contribution!

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @eterny13)

a discussion (no related file):
FYI I enabled the pre-commit analysis to run on this PR and many tests and other required checks failed.

---

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/AbstractIp6SpaceContainsIp.java line 5 at r1 (raw file):

import org.batfish.datamodel.visitors.GenericIp6SpaceVisitor;

public abstract class AbstractIp6SpaceContainsIp implements GenericIp6SpaceVisitor<Boolean> {

javadoc.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Ip6Space.java line 19 at r1 (raw file):

    /** Return the {@link Ip6Space} of all IPs not in {@code this}. */
    public Ip6Space complement() {

please add @Nonnull annotation to the return type: public @Nonnull Ip6Space

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Ip6Space.java line 20 at r1 (raw file):

    /** Return the {@link Ip6Space} of all IPs not in {@code this}. */
    public Ip6Space complement() {
        //return AclIp6Space.difference(UniverseIpSpace.INSTANCE, this);

This seems like a bug?

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Ip6Space.java line 37 at r1 (raw file):

    }

    protected abstract int compareSameClass(Ip6Space o);

please add javadoc

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Ip6Space.java line 59 at r1 (raw file):

    @Override
    public abstract String toString();

please add @Nonnull String.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Ip6SpaceContainsIp.java line 8 at r1 (raw file):

import java.util.concurrent.ConcurrentHashMap;

public final class Ip6SpaceContainsIp extends AbstractIp6SpaceContainsIp {

please add javadoc.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Ip6SpaceReference.java line 17 at r1 (raw file):

    private final @Nullable String _description;

    private final String _name;

Useful to add @Nonnull for things that are guaranteed to be populated.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Ip6SpaceReference.java line 24 at r1 (raw file):

    /** A reference to a named {@link IpSpace} */
    @JsonCreator

@JsonCreator functions are typically private static functions that have JSON-specific error handling (e.g., that any value could be null and that should be an IllegalArgumentException) and then call the developer-facing public constructor.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Ip6SpaceReference.java line 35 at r1 (raw file):

    public <R> R accept(GenericIp6SpaceVisitor<R> visitor) {
        // TODO:
        return null;

fill in?

[eterny13]

Thanks. I've fixed them.

[eterny13]

add PrefixIp6Space to pass the tests.

[codecov]

Codecov Report

Merging #8795 (25be613) into master (149ac6e) will increase coverage by 0.02%.
The diff coverage is 79.13%.

@@            Coverage Diff             @@
##           master    #8795      +/-   ##
==========================================
+ Coverage   72.43%   72.46%   +0.02%     
==========================================
  Files        3293     3300       +7     
  Lines      168928   169052     +124     
  Branches    19828    19844      +16     
==========================================
+ Hits       122364   122503     +139     
+ Misses      37454    37425      -29     
- Partials     9110     9124      +14     

Files	Coverage Δ
.../batfish/datamodel/AbstractIp6SpaceContainsIp.java	100.00% <100.00%> (ø)
...ain/java/org/batfish/datamodel/PrefixIp6Space.java	100.00% <100.00%> (ø)
...ish/datamodel/visitors/GenericIp6SpaceVisitor.java	100.00% <100.00%> (ø)
...mar/cisco_nxos/CiscoNxosControlPlaneExtractor.java	78.85% <100.00%> (-0.01%)	⬇️
.../java/org/batfish/datamodel/Ip6SpaceReference.java	94.11% <94.11%> (ø)
...l/src/main/java/org/batfish/datamodel/Prefix6.java	84.61% <80.00%> (+28.87%)	⬆️
...cisco_nxos/InterfaceIpv6AddressWithAttributes.java	88.46% <80.00%> (-2.02%)	⬇️
...java/org/batfish/datamodel/Ip6SpaceContainsIp.java	85.71% <85.71%> (ø)
...tocol/src/main/java/org/batfish/datamodel/Ip6.java	73.43% <40.00%> (+12.87%)	⬆️
.../src/main/java/org/batfish/datamodel/Ip6Space.java	56.25% <56.25%> (ø)
... and 1 more

... and 5 files with indirect coverage changes

[dhalperi]

Thanks! The tests now all pass, which is awesome. But, we're missing tests.

Here's what I'd do -- look at the IPv4 versions of the tests and copy them:

1. Java serialization tests for anything that can be stored on a Configuration
2. Jackson serialization tests for anything that we expect to serve over the API
3. Guava EqualsTester-based tests for anything that has a sensible notion of equality (e.g., Ip6Space)
4. Functional tests for anything with function (e.g., contains IP)

Reviewable status: 0 of 6 files reviewed, 9 unresolved discussions (waiting on @eterny13)

[dhalperi]

Reviewed 4 of 6 files at r3, 5 of 8 files at r4, all commit messages.
Reviewable status: 9 of 12 files reviewed, 12 unresolved discussions (waiting on @eterny13)

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Ip6SpaceReference.java line 24 at r1 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

@JsonCreator functions are typically private static functions that have JSON-specific error handling (e.g., that any value could be null and that should be an IllegalArgumentException) and then call the developer-facing public constructor.

So this still needs to be done... the function with @JsonProperty annotations needs to be private static, the public function needs to not have any JSON annotations and only take typed values.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Ip6SpaceReference.java line 23 at r4 (raw file):

  /** A reference to a named {@link Ip6Space} */
  public Ip6SpaceReference(

Please add tests for this class, in Ip6SpaceReferenceTest.java

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Ip6SpaceReference.java line 61 at r4 (raw file):

  }

  @Nonnull

the @Nonnull goes on the String, not on the @Override.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Prefix6.java line 47 at r4 (raw file):

  }

  public static Prefix6 create(Ip6 ip6, int prefixLength) {

Interning these is a nice improvement, but it only works if it's the only way to create them. Please make the constructor private then.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Prefix6.java line 153 at r4 (raw file):

  }

  @Nonnull

@Nonnull on the Ip6Space.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/PrefixIp6Space.java line 24 at r4 (raw file):

  @JsonCreator
  static PrefixIp6Space create(@JsonProperty(PROP_PREFIX) Prefix6 prefix6) {

This needs to be private, and is usually called something like jacksonCreator to prevent it from being confused with create that should be called by any user written code.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/PrefixIp6Space.java line 24 at r4 (raw file):

  @JsonCreator
  static PrefixIp6Space create(@JsonProperty(PROP_PREFIX) Prefix6 prefix6) {

All @JsonProperty parameters need to be @Nullable and the nullness needs to be checked (with IllegalArgumentException thrown if not present). There's nothing stopping a caller from omitting the field or putting null in.

This is why we have dedicated functions for Jackson, because its contract is so crap.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/PrefixIp6Space.java line 32 at r4 (raw file):

  }

  @Override

Please add tests for this class, in PrefixIpSpace6Test.java

---

projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/Ip6SpaceTest.java line 11 at r4 (raw file):

import org.junit.Test;

public class Ip6SpaceTest {

This class should only test code in IpSpace.java. Put tests of other classes in class-specific files.

---

projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/Ip6SpaceTest.java line 13 at r4 (raw file):

public class Ip6SpaceTest {
  @Test
  public void testIp6Space() {}

Delete empty?

---

projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/matchers/Ip6SpaceMatchers.java line 14 at r4 (raw file):

  /** Provides a matcher that matches if the {@link Ip6Space} contains the specified {@link Ip6}. */
  public static ContainsIp containsIp(@Nonnull Ip6 ip6) {

Should probably be named containsIp6, similar wisdom throughout

---

projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/matchers/Ip6SpaceMatchersImpl.java line 24 at r4 (raw file):

    @Override
    public void describeTo(Description description) {
      description.appendText(String.format("An Ip6Space containing IP: %s", _ip6));

Ip6, not IP

[dhalperi]

Reviewable status: 9 of 12 files reviewed, 12 unresolved discussions (waiting on @eterny13)

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Ip6SpaceReference.java line 61 at r4 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

the @Nonnull goes on the String, not on the @Override.

I added some new checkstyle configuration to help enforce this, so that it's not a thing to be addressed during review.

[dhalperi]

Reviewed 14 of 14 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @eterny13)

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Ip6WildcardSetIp6Space.java line 68 at r5 (raw file):

  private static final String PROP_BLACKLIST = "blacklist";
  private static final String PROP_WHITELIST = "whitelist";

We still need to make breaking changes to the IPv4 versions of these classes to remove outdated language. That's not a blocker for your PR.

But, as you're adding this class, can you please use the now-preferred terms "blocklist" and "allowlist" instead (throughout)?

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/Prefix6.java line 47 at r4 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

Interning these is a nice improvement, but it only works if it's the only way to create them. Please make the constructor private then.

Note this does mean updating all callers, which is why the build currently fails. If you prefer, reverting the Interning.

---

projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/Ip6SpaceReferenceTest.java line 27 at r5 (raw file):

  @Test
  public void TestExprEquals() {
    Ip6SpaceReference ip6SR1 = new Ip6SpaceReference(IP_SPACE_NAME1, DESCRIPTION1);

use Guava's EqualsTesters for this, will be much simpler. You'll see it all over the codebase, and the docs are here: https://www.javadoc.io/doc/com.google.guava/guava-testlib/latest/com/google/common/testing/EqualsTester.html

I guess this is a slightly newer pattern (texting exprEquals via equals, which calls it) but it makes so much more sense and will make your code easier to write and read.

---

projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/PrefixIp6SpaceTest.java line 24 at r5 (raw file):

  @Test
  public void testExprEquals() {

prefer to just use guava EqualsTester as described in the other location

[dhalperi]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @eterny13)

a discussion (no related file):
This is great! Please don't add more scope to this PR until we get it merged. That way follow-ons will be easier to write, review, and merge faster

---

[dhalperi]

Reviewed 8 of 8 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @eterny13)

[dhalperi]

Reviewed 6 of 6 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @eterny13)

[dhalperi]

Reviewed 1 of 1 files at r8.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @eterny13)

[dhalperi]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @eterny13)

[eterny13]

Thanks for accepting my suggestion !

[dhalperi]

Thanks for the contribution! Hope it is the first of many.