fix behavior of Graph/ValueGraph views for a node when that node is removed from the graph

RELNOTES=fix behavior of Graph/ValueGraph views for a node when that node is removed from the graph
PiperOrigin-RevId: 571390607

Author: ineuwirth

android/guava-tests/test/com/google/common/graph/AbstractGraphTest.java

@@ -385,10 +385,13 @@ public void removeNode_nodeNotPresent() {
   public void removeNode_queryAfterRemoval() {
     assume().that(graphIsMutable()).isTrue();
 
-    addNode(N1);
-    @SuppressWarnings("unused")
-    Set<Integer> unused = graph.adjacentNodes(N1); // ensure cache (if any) is populated
+    putEdge(N1, N2);
+    putEdge(N2, N1);
+    Set<Integer> n1AdjacentNodes = graph.adjacentNodes(N1);
+    Set<Integer> n2AdjacentNodes = graph.adjacentNodes(N2);
     assertThat(graphAsMutableGraph.removeNode(N1)).isTrue();
+    assertThat(n1AdjacentNodes).isEmpty();
+    assertThat(n2AdjacentNodes).isEmpty();
     IllegalArgumentException e =
         assertThrows(IllegalArgumentException.class, () -> graph.adjacentNodes(N1));
     assertNodeNotInGraphErrorMessage(e);

android/guava-tests/test/com/google/common/graph/AbstractNetworkTest.java

@@ -670,11 +670,12 @@ public void removeNode_nodeNotPresent() {
   public void removeNode_queryAfterRemoval() {
     assume().that(graphIsMutable()).isTrue();
 
-    addNode(N1);
-    @SuppressWarnings("unused")
-    Set<Integer> unused =
-        networkAsMutableNetwork.adjacentNodes(N1); // ensure cache (if any) is populated
+    addEdge(N1, N2, E12);
+    Set<Integer> n1AdjacentNodes = networkAsMutableNetwork.adjacentNodes(N1);
+    Set<Integer> n2AdjacentNodes = networkAsMutableNetwork.adjacentNodes(N2);
     assertTrue(networkAsMutableNetwork.removeNode(N1));
+    assertThat(n1AdjacentNodes).isEmpty();
+    assertThat(n2AdjacentNodes).isEmpty();
     IllegalArgumentException e =
         assertThrows(
             IllegalArgumentException.class, () -> networkAsMutableNetwork.adjacentNodes(N1));

android/guava/src/com/google/common/graph/StandardMutableValueGraph.java

@@ -24,6 +24,7 @@
 import static com.google.common.graph.Graphs.checkPositive;
 import static java.util.Objects.requireNonNull;
 
+import com.google.common.collect.ImmutableList;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import javax.annotation.CheckForNull;
 
@@ -136,17 +137,21 @@ public boolean removeNode(N node) {
       }
     }
 
-    for (N successor : connections.successors()) {
+    for (N successor : ImmutableList.copyOf(connections.successors())) {
       // requireNonNull is safe because the node is a successor.
       requireNonNull(nodeConnections.getWithoutCaching(successor)).removePredecessor(node);
+      requireNonNull(connections.removeSuccessor(successor));
       --edgeCount;
     }
     if (isDirected()) { // In undirected graphs, the successor and predecessor sets are equal.
-      for (N predecessor : connections.predecessors()) {
+      // Since views are returned, we need to copy the predecessors that will be removed.
+      // Thus we avoid modifying the underlying view while iterating over it.
+      for (N predecessor : ImmutableList.copyOf(connections.predecessors())) {
         // requireNonNull is safe because the node is a predecessor.
         checkState(
             requireNonNull(nodeConnections.getWithoutCaching(predecessor)).removeSuccessor(node)
                 != null);
+        connections.removePredecessor(predecessor);
         --edgeCount;
       }
     }

guava-tests/test/com/google/common/graph/AbstractGraphTest.java

@@ -385,10 +385,13 @@ public void removeNode_nodeNotPresent() {
   public void removeNode_queryAfterRemoval() {
     assume().that(graphIsMutable()).isTrue();
 
-    addNode(N1);
-    @SuppressWarnings("unused")
-    Set<Integer> unused = graph.adjacentNodes(N1); // ensure cache (if any) is populated
+    putEdge(N1, N2);
+    putEdge(N2, N1);
+    Set<Integer> n1AdjacentNodes = graph.adjacentNodes(N1);
+    Set<Integer> n2AdjacentNodes = graph.adjacentNodes(N2);
     assertThat(graphAsMutableGraph.removeNode(N1)).isTrue();
+    assertThat(n1AdjacentNodes).isEmpty();
+    assertThat(n2AdjacentNodes).isEmpty();
     IllegalArgumentException e =
         assertThrows(IllegalArgumentException.class, () -> graph.adjacentNodes(N1));
     assertNodeNotInGraphErrorMessage(e);

guava-tests/test/com/google/common/graph/AbstractNetworkTest.java

@@ -677,11 +677,12 @@ public void removeNode_nodeNotPresent() {
   public void removeNode_queryAfterRemoval() {
     assume().that(graphIsMutable()).isTrue();
 
-    addNode(N1);
-    @SuppressWarnings("unused")
-    Set<Integer> unused =
-        networkAsMutableNetwork.adjacentNodes(N1); // ensure cache (if any) is populated
+    addEdge(N1, N2, E12);
+    Set<Integer> n1AdjacentNodes = networkAsMutableNetwork.adjacentNodes(N1);
+    Set<Integer> n2AdjacentNodes = networkAsMutableNetwork.adjacentNodes(N2);
     assertTrue(networkAsMutableNetwork.removeNode(N1));
+    assertThat(n1AdjacentNodes).isEmpty();
+    assertThat(n2AdjacentNodes).isEmpty();
     IllegalArgumentException e =
         assertThrows(
             IllegalArgumentException.class, () -> networkAsMutableNetwork.adjacentNodes(N1));

guava/src/com/google/common/graph/StandardMutableValueGraph.java

@@ -24,6 +24,7 @@
 import static com.google.common.graph.Graphs.checkPositive;
 import static java.util.Objects.requireNonNull;
 
+import com.google.common.collect.ImmutableList;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import javax.annotation.CheckForNull;
 
@@ -136,17 +137,21 @@ public boolean removeNode(N node) {
       }
     }
 
-    for (N successor : connections.successors()) {
+    for (N successor : ImmutableList.copyOf(connections.successors())) {
       // requireNonNull is safe because the node is a successor.
       requireNonNull(nodeConnections.getWithoutCaching(successor)).removePredecessor(node);
+      requireNonNull(connections.removeSuccessor(successor));
       --edgeCount;
     }
     if (isDirected()) { // In undirected graphs, the successor and predecessor sets are equal.
-      for (N predecessor : connections.predecessors()) {
+      // Since views are returned, we need to copy the predecessors that will be removed.
+      // Thus we avoid modifying the underlying view while iterating over it.
+      for (N predecessor : ImmutableList.copyOf(connections.predecessors())) {
         // requireNonNull is safe because the node is a predecessor.
         checkState(
             requireNonNull(nodeConnections.getWithoutCaching(predecessor)).removeSuccessor(node)
                 != null);
+        connections.removePredecessor(predecessor);
         --edgeCount;
       }
     }