BgpProcess: only sort maps for JSON APIs (#8907)

And make them immutable at conversion time to save space.

commit-id:280d2500

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/BgpProcess.java

@@ -19,12 +19,11 @@
 import com.google.common.collect.Streams;
 import java.io.Serializable;
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
-import java.util.SortedMap;
-import java.util.TreeMap;
 import java.util.function.Function;
 import java.util.stream.Stream;
 import javax.annotation.Nonnull;
@@ -229,7 +228,7 @@ public Set<Long> get() {
   private final int _ibgpAdminCost;
   private final int _localAdminCost;
   private final @Nonnull Supplier<Set<Long>> _clusterIds;
-  private @Nonnull SortedMap<String, BgpUnnumberedPeerConfig> _interfaceNeighbors;
+  private @Nonnull Map<String, BgpUnnumberedPeerConfig> _interfaceNeighbors;
   private boolean _multipathEbgp;
   private MultipathEquivalentAsPathMatchMode _multipathEquivalentAsPathMatchMode;
   private boolean _multipathIbgp;
@@ -241,13 +240,13 @@ public Set<Long> get() {
    * A map of all non-dynamic bgp neighbors with which the router owning this process is configured
    * to peer, keyed by unique ID.
    */
-  private @Nonnull SortedMap<Ip, BgpActivePeerConfig> _activeNeighbors;
+  private @Nonnull Map<Ip, BgpActivePeerConfig> _activeNeighbors;
 
   /**
    * A map of all dynamic bgp neighbors with which the router owning this process is configured to
    * peer, keyed by unique ID.
    */
-  private @Nonnull SortedMap<Prefix, BgpPassivePeerConfig> _passiveNeighbors;
+  private @Nonnull Map<Prefix, BgpPassivePeerConfig> _passiveNeighbors;
 
   /** Space of prefixes to be advertised using explicit network statements */
   private PrefixSpace _originationSpace;
@@ -288,18 +287,18 @@ private BgpProcess(
       @Nonnull LocalOriginationTypeTieBreaker localOriginationTypeTieBreaker,
       @Nonnull NextHopIpTieBreaker networkNextHopIpTieBreaker,
       @Nonnull NextHopIpTieBreaker redistributeNextHopIpTieBreaker) {
-    _activeNeighbors = new TreeMap<>();
+    _activeNeighbors = new HashMap<>();
     _aggregates = ImmutableMap.of();
     _clientToClientReflection = firstNonNull(clientToClientReflection, true);
     _confederation = confederation;
     _ebgpAdminCost = ebgpAdminCost;
     _ibgpAdminCost = ibgpAdminCost;
     _localAdminCost = localAdminCost;
-    _interfaceNeighbors = new TreeMap<>();
+    _interfaceNeighbors = new HashMap<>();
     _tieBreaker = BgpTieBreaker.ARRIVAL_ORDER;
     _clusterIds = Suppliers.memoize(new ClusterIdsSupplier());
     _originationSpace = new PrefixSpace();
-    _passiveNeighbors = new TreeMap<>();
+    _passiveNeighbors = new HashMap<>();
     _routerId = routerId;
     _clusterListAsIbgpCost = false;
     _independentNetworkPolicy = independentNetworkPolicy;
@@ -434,11 +433,16 @@ public Set<Long> getClusterIds() {
   }
 
   /** Neighbor relationships configured for this BGP process. */
-  @JsonProperty(PROP_ACTIVE_NEIGHBORS)
-  public @Nonnull SortedMap<Ip, BgpActivePeerConfig> getActiveNeighbors() {
+  @JsonIgnore
+  public @Nonnull Map<Ip, BgpActivePeerConfig> getActiveNeighbors() {
     return _activeNeighbors;
   }
 
+  @JsonProperty(PROP_ACTIVE_NEIGHBORS)
+  private @Nonnull Map<Ip, BgpActivePeerConfig> getActiveNeighborsJson() {
+    return ImmutableSortedMap.copyOf(_activeNeighbors);
+  }
+
   /** Returns the admin cost of the given BGP protocol */
   @JsonIgnore
   public int getAdminCost(RoutingProtocol protocol) {
@@ -491,11 +495,16 @@ public int getLocalAdminCost() {
   }
 
   /** Returns BGP unnumbered peer configurations keyed by peer-interface */
-  @JsonProperty(PROP_INTERFACE_NEIGHBORS)
-  public @Nonnull SortedMap<String, BgpUnnumberedPeerConfig> getInterfaceNeighbors() {
+  @JsonIgnore
+  public @Nonnull Map<String, BgpUnnumberedPeerConfig> getInterfaceNeighbors() {
     return _interfaceNeighbors;
   }
 
+  @JsonProperty(PROP_INTERFACE_NEIGHBORS)
+  private @Nonnull Map<String, BgpUnnumberedPeerConfig> getInterfaceNeighborsJson() {
+    return ImmutableSortedMap.copyOf(_interfaceNeighbors);
+  }
+
   @JsonProperty(PROP_MULTIPATH_EBGP)
   public boolean getMultipathEbgp() {
     return _multipathEbgp;
@@ -512,11 +521,16 @@ public boolean getMultipathIbgp() {
   }
 
   /** Neighbor relationships configured for this BGP process. */
-  @JsonProperty(PROP_PASSIVE_NEIGHBORS)
-  public @Nonnull SortedMap<Prefix, BgpPassivePeerConfig> getPassiveNeighbors() {
+  @JsonIgnore
+  public @Nonnull Map<Prefix, BgpPassivePeerConfig> getPassiveNeighbors() {
     return _passiveNeighbors;
   }
 
+  @JsonProperty(PROP_PASSIVE_NEIGHBORS)
+  private @Nonnull Map<Prefix, BgpPassivePeerConfig> getPassiveNeighborsJson() {
+    return ImmutableSortedMap.copyOf(_passiveNeighbors);
+  }
+
   @JsonIgnore
   public PrefixSpace getOriginationSpace() {
     return _originationSpace;
@@ -570,7 +584,7 @@ public Stream<BgpPeerConfig> allPeerConfigsStream() {
 
   @JsonProperty(PROP_INTERFACE_NEIGHBORS)
   public void setInterfaceNeighbors(
-      @Nonnull SortedMap<String, BgpUnnumberedPeerConfig> interfaceNeighbors) {
+      @Nonnull Map<String, BgpUnnumberedPeerConfig> interfaceNeighbors) {
     _interfaceNeighbors = interfaceNeighbors;
   }
 
@@ -591,17 +605,17 @@ public void setMultipathIbgp(boolean multipathIbgp) {
   }
 
   @JsonProperty(PROP_ACTIVE_NEIGHBORS)
-  public void setNeighbors(SortedMap<Ip, BgpActivePeerConfig> neighbors) {
-    _activeNeighbors = firstNonNull(neighbors, new TreeMap<>());
+  public void setNeighbors(Map<Ip, BgpActivePeerConfig> neighbors) {
+    _activeNeighbors = firstNonNull(neighbors, new HashMap<>());
   }
 
   public void setOriginationSpace(PrefixSpace originationSpace) {
     _originationSpace = originationSpace;
   }
 
   @JsonProperty(PROP_PASSIVE_NEIGHBORS)
-  public void setPassiveNeighbors(@Nullable SortedMap<Prefix, BgpPassivePeerConfig> neighbors) {
-    _passiveNeighbors = firstNonNull(neighbors, new TreeMap<>());
+  public void setPassiveNeighbors(@Nullable Map<Prefix, BgpPassivePeerConfig> neighbors) {
+    _passiveNeighbors = firstNonNull(neighbors, new HashMap<>());
   }
 
   @JsonProperty(PROP_TIE_BREAKER)

projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/matchers/BgpProcessMatchers.java

@@ -3,7 +3,6 @@
 import static org.hamcrest.Matchers.equalTo;
 
 import java.util.Map;
-import java.util.SortedMap;
 import javax.annotation.Nonnull;
 import org.batfish.datamodel.BgpActivePeerConfig;
 import org.batfish.datamodel.BgpPassivePeerConfig;
@@ -40,7 +39,7 @@ public static Matcher<BgpProcess> hasInterfaceNeighbor(
    * provided {@code subMatcher}.
    */
   public static Matcher<BgpProcess> hasInterfaceNeighbors(
-      @Nonnull Matcher<? super SortedMap<String, BgpUnnumberedPeerConfig>> subMatcher) {
+      @Nonnull Matcher<? super Map<String, BgpUnnumberedPeerConfig>> subMatcher) {
     return new HasInterfaceNeighbors(subMatcher);
   }
 

projects/batfish-common-protocol/src/test/java/org/batfish/datamodel/matchers/BgpProcessMatchersImpl.java

@@ -1,7 +1,6 @@
 package org.batfish.datamodel.matchers;
 
 import java.util.Map;
-import java.util.SortedMap;
 import javax.annotation.Nonnull;
 import org.batfish.datamodel.BgpActivePeerConfig;
 import org.batfish.datamodel.BgpPassivePeerConfig;
@@ -36,14 +35,14 @@ protected BgpUnnumberedPeerConfig featureValueOf(BgpProcess actual) {
   }
 
   static final class HasInterfaceNeighbors
-      extends FeatureMatcher<BgpProcess, SortedMap<String, BgpUnnumberedPeerConfig>> {
+      extends FeatureMatcher<BgpProcess, Map<String, BgpUnnumberedPeerConfig>> {
     HasInterfaceNeighbors(
-        @Nonnull Matcher<? super SortedMap<String, BgpUnnumberedPeerConfig>> subMatcher) {
+        @Nonnull Matcher<? super Map<String, BgpUnnumberedPeerConfig>> subMatcher) {
       super(subMatcher, "A BGP process with interfaceNeighbors:", "interfaceNeighbors:");
     }
 
     @Override
-    protected SortedMap<String, BgpUnnumberedPeerConfig> featureValueOf(BgpProcess actual) {
+    protected Map<String, BgpUnnumberedPeerConfig> featureValueOf(BgpProcess actual) {
       return actual.getInterfaceNeighbors();
     }
   }

projects/batfish/src/main/java/org/batfish/job/ConvertConfigurationJob.java

@@ -527,6 +527,16 @@ static void finalizeConfiguration(Configuration c, VendorConfiguration vc, Warni
     c.setTrackingGroups(toImmutableMap(c.getTrackingGroups()));
     c.setVrfs(verifyAndToImmutableMap(c.getVrfs(), Vrf::getName, w));
     c.setZones(toImmutableMap(c.getZones()));
+    for (Vrf v : c.getVrfs().values()) {
+      BgpProcess p = v.getBgpProcess();
+      if (p == null) {
+        continue;
+      }
+      p.setNeighbors(ImmutableMap.copyOf(p.getActiveNeighbors()));
+      p.setInterfaceNeighbors(ImmutableMap.copyOf(p.getInterfaceNeighbors()));
+      p.setPassiveNeighbors(ImmutableMap.copyOf(p.getPassiveNeighbors()));
+    }
+
     verifyAclInvariants(c, w);
     verifyAsPathStructures(c);
     verifyCommunityStructures(c);

projects/batfish/src/test/java/org/batfish/grammar/cisco/CiscoGrammarTest.java

@@ -1160,7 +1160,7 @@ public void testIosIbgpMissingUpdateSource() throws IOException {
     // Confirm that BGP peer on r1 is missing its local IP, as expected
     Ip r1NeighborPeerAddress = Ip.parse("2.2.2.2");
     Configuration r1 = batfish.loadConfigurations(batfish.getSnapshot()).get("r1");
-    SortedMap<Ip, BgpActivePeerConfig> r1Peers =
+    Map<Ip, BgpActivePeerConfig> r1Peers =
         r1.getVrfs().get(DEFAULT_VRF_NAME).getBgpProcess().getActiveNeighbors();
     assertTrue(r1Peers.containsKey(r1NeighborPeerAddress));
     assertThat(r1Peers.get(r1NeighborPeerAddress).getLocalIp(), nullValue());