Batfish: continue work on canonical lowercase hostnames

[dhalperi]

• Batfish: enforce canonical lowercase name everywhere

  Main thing I did was fix tests that built structures incorrectly.

  Added lcase-enforcement in a very few user APIs, typically those ones that are
  called on unsanitized data. For other user APIs, added assertions instead.

• Delete InferRolesMixedCaseTest

  Since hostnames are now properly canonicalized to lowercase.

[batfish-bot]

This change is 

[corinaminer]

Reviewed 32 of 32 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @arifogel and @dhalperi)

---

projects/batfish/src/test/java/org/batfish/bddreachability/SessionInstrumentationTest.java, line 69 at r1 (raw file):

  private static final String FW_I1 = "FW:I1";
  private static final String SOURCE1_IFACE = "SOURCE1:IFACE";
  private static final String SOURCE2_IFACE = "SOURCE2:IFACE";

These are weird interface names anyway -- look like stringified NodeInterfacePairs. Can we swap colons for underscores (or otherwise change the names)?

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/collections/NodeInterfacePair.java, line 56 at r1 (raw file):

  @Deprecated
  public NodeInterfacePair(Interface iface) {

Since you're already switching the other constructor to private, can you also delete this constructor? Looks to be unused.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/pojo/Node.java, line 23 at r1 (raw file):

  @VisibleForTesting
  Node(@Nonnull String name, @Nullable String id, @Nullable DeviceType type) {
    super(firstNonNull(id, makeId(name.toLowerCase())));

toLowerCase() isn't necessary here (since makeId() lowercases it). But maybe useful for clarity

---

projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/EdgeTest.java, line 50 at r1 (raw file):

        .addEqualityGroup(
            new Edge(
                NodeInterfacePair.of("tail", "tailint"), NodeInterfacePair.of("head", "headInt")))

this seems like a test of NodeInterfacePair equality, not Edge equality. I would limit equality testing here to:

• one pair that are equal
• one where tail is different
• one where head is different

Meanwhile, NodeInterfacePairTest should be updated to test case sensitivity.

[codecov]

Codecov Report

Merging #4811 into master will decrease coverage by 0.01%.
The diff coverage is 65%.

@@             Coverage Diff              @@
##             master    #4811      +/-   ##
============================================
- Coverage     75.48%   75.46%   -0.02%     
- Complexity    28647    28649       +2     
============================================
  Files          2283     2283              
  Lines        112552   112573      +21     
  Branches      13518    13526       +8     
============================================
+ Hits          84956    84957       +1     
- Misses        21105    21115      +10     
- Partials       6491     6501      +10

Impacted Files	Coverage Δ	Complexity Δ
.../java/org/batfish/datamodel/IpsecPeerConfigId.java	87.5% <0%> (-12.5%)	10 <0> (+1)
...main/java/org/batfish/datamodel/isis/IsisEdge.java	71.62% <0%> (-7.49%)	24 <0> (+1)
...src/main/java/org/batfish/datamodel/pojo/Node.java	100% <100%> (ø)	8 <5> (+1)	⬆️
...main/java/org/batfish/datamodel/Configuration.java	96.06% <100%> (+0.03%)	111 <0> (ø)	⬇️
...tfish/datamodel/collections/NodeInterfacePair.java	91.66% <100%> (+7.05%)	16 <1> (ø)	⬇️
.../src/main/java/org/batfish/datamodel/flow/Hop.java	53.33% <0%> (-6.67%)	5% <0%> (-1%)
...f5_bigip_imish/parsing/F5BigipImishBaseParser.java	62.5% <0%> (-4.17%)	4% <0%> (+1%)
...rc/main/java/org/batfish/datamodel/flow/Trace.java	87.09% <0%> (-3.23%)	12% <0%> (-1%)
...col/src/main/java/org/batfish/role/InferRoles.java	90.53% <0%> (-1.9%)	64% <0%> (-3%)
... and 5 more

[dhalperi]

Reviewable status: 29 of 33 files reviewed, all discussions resolved (waiting on @arifogel and @corinaminer)

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/collections/NodeInterfacePair.java, line 56 at r1 (raw file):

Previously, corinaminer (Corina Miner) wrote…

Since you're already switching the other constructor to private, can you also delete this constructor? Looks to be unused.

Done.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/pojo/Node.java, line 23 at r1 (raw file):

Previously, corinaminer (Corina Miner) wrote…

toLowerCase() isn't necessary here (since makeId() lowercases it). But maybe useful for clarity

Done.

---

projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/EdgeTest.java, line 50 at r1 (raw file):

Previously, corinaminer (Corina Miner) wrote…

this seems like a test of NodeInterfacePair equality, not Edge equality. I would limit equality testing here to:

• one pair that are equal
• one where tail is different
• one where head is different

Meanwhile, NodeInterfacePairTest should be updated to test case sensitivity.

Done both.

[corinaminer]

Reviewed 4 of 4 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @arifogel)