Allow hostInterfaces to be specified as list in json

[arifogel]

• prevents mismatch between key and name
• retain support for hostInterfaces specified as object (map)
• remove unused serialization support

[batfish-bot]

This change is 

[codecov]

Codecov Report

Merging #5291 into master will decrease coverage by <.01%.
The diff coverage is 85%.

@@             Coverage Diff              @@
##             master    #5291      +/-   ##
============================================
- Coverage     73.39%   73.39%   -0.01%     
- Complexity    31956    31962       +6     
============================================
  Files          2624     2624              
  Lines        128893   128926      +33     
  Branches      15499    15503       +4     
============================================
+ Hits          94602    94625      +23     
- Misses        26802    26812      +10     
  Partials       7489     7489

Impacted Files	Coverage Δ	Complexity Δ
...batfish/representation/host/HostConfiguration.java	82.05% <85%> (-1.29%)	28 <5> (+5)
.../src/main/java/org/batfish/datamodel/flow/Hop.java	53.33% <0%> (-6.67%)	5% <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	89.54% <0%> (-1.37%)	50% <0%> (-1%)
...src/main/java/org/batfish/coordinator/PoolMgr.java	64.04% <0%> (-1.13%)	15% <0%> (-1%)
...batfish/src/main/java/org/batfish/main/Driver.java	38.25% <0%> (-0.25%)	28% <0%> (ø)
...a/org/batfish/dataplane/traceroute/FlowTracer.java	90% <0%> (+0.04%)	72% <0%> (+1%)	⬆️
...mmar/palo_alto_nested/PaloAltoNestedFlattener.java	97.1% <0%> (+0.17%)	20% <0%> (+2%)	⬆️
.../src/main/java/org/batfish/datamodel/FlowDiff.java	81.08% <0%> (+1.35%)	28% <0%> (+1%)	⬆️
... and 2 more

[dhalperi]

-reviewer:@progwriter

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @arifogel and @dhalperi)

---

projects/batfish/src/main/java/org/batfish/representation/host/HostConfiguration.java, line 129 at r2 (raw file):

Quoted 22 lines of code…

    if (hostInterfacesNode == null) {
      hostInterfaces = ImmutableMap.of();
    } else if (hostInterfacesNode instanceof ObjectNode) {
      hostInterfaces =
          BatfishObjectMapper.mapper()
              .convertValue(hostInterfacesNode, new TypeReference<Map<String, HostInterface>>() {});
      hostInterfaces.forEach(
          (name, hostInterface) ->
              checkArgument(
                  name.equals(hostInterface.getName()),
                  "Mismatch between hostInterface key '%s' and name '%s'",
                  name,
                  hostInterface.getName()));
    } else {
      assert hostInterfacesNode instanceof ArrayNode;
      List<HostInterface> hostInterfacesList =
          BatfishObjectMapper.mapper()
              .convertValue(hostInterfacesNode, new TypeReference<List<HostInterface>>() {});
      hostInterfaces =
          hostInterfacesList.stream()
              .collect(ImmutableMap.toImmutableMap(HostInterface::getName, Function.identity()));
    }

refactor into unit-testable function (JsonNode -> List<HI>), and test that function directly.

---

projects/batfish/src/main/java/org/batfish/representation/host/HostConfiguration.java, line 144 at r2 (raw file):

  }

  @JsonIgnore

defer not-directly-related changes to a separate PR with an explanation of why you're making them. May want to ship that one first, if you need to do some complicated reorganization here.

[arifogel]

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @dhalperi)

---

projects/batfish/src/main/java/org/batfish/representation/host/HostConfiguration.java, line 129 at r2 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

    if (hostInterfacesNode == null) {
      hostInterfaces = ImmutableMap.of();
    } else if (hostInterfacesNode instanceof ObjectNode) {
      hostInterfaces =
          BatfishObjectMapper.mapper()
              .convertValue(hostInterfacesNode, new TypeReference<Map<String, HostInterface>>() {});
      hostInterfaces.forEach(
          (name, hostInterface) ->
              checkArgument(
                  name.equals(hostInterface.getName()),
                  "Mismatch between hostInterface key '%s' and name '%s'",
                  name,
                  hostInterface.getName()));
    } else {
      assert hostInterfacesNode instanceof ArrayNode;
      List<HostInterface> hostInterfacesList =
          BatfishObjectMapper.mapper()
              .convertValue(hostInterfacesNode, new TypeReference<List<HostInterface>>() {});
      hostInterfaces =
          hostInterfacesList.stream()
              .collect(ImmutableMap.toImmutableMap(HostInterface::getName, Function.identity()));
    }

refactor into unit-testable function (JsonNode -> List<HI>), and test that function directly.

better now, or want more granularity?

---

projects/batfish/src/main/java/org/batfish/representation/host/HostConfiguration.java, line 144 at r2 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

defer not-directly-related changes to a separate PR with an explanation of why you're making them. May want to ship that one first, if you need to do some complicated reorganization here.

Went simpler route and skipped unrelated changes

[dhalperi]

Reviewed 1 of 1 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @arifogel)

---

projects/batfish/src/main/java/org/batfish/representation/host/HostConfiguration.java, line 304 at r3 (raw file):

      hostInterfaces =
          hostInterfacesList.stream()
              .collect(ImmutableMap.toImmutableMap(HostInterface::getName, Function.identity()));

I believe this will crash if the same interface name is given twice. Is that your intent?

[arifogel]

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @dhalperi)

---

projects/batfish/src/main/java/org/batfish/representation/host/HostConfiguration.java, line 304 at r3 (raw file):

Previously, dhalperi (Dan Halperin) wrote…

I believe this will crash if the same interface name is given twice. Is that your intent?

Good catch.
Now just using last definition.

[dhalperi]

Reviewed 2 of 2 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved