From 950799691c4efdbf9f1b6d6c384b613c22beef65 Mon Sep 17 00:00:00 2001 From: ineuwirth Date: Fri, 6 Oct 2023 11:30:41 -0700 Subject: [PATCH] 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 --- .../test/com/google/common/graph/AbstractGraphTest.java | 9 ++++++--- .../com/google/common/graph/AbstractNetworkTest.java | 9 +++++---- .../google/common/graph/StandardMutableValueGraph.java | 9 +++++++-- .../test/com/google/common/graph/AbstractGraphTest.java | 9 ++++++--- .../com/google/common/graph/AbstractNetworkTest.java | 9 +++++---- .../google/common/graph/StandardMutableValueGraph.java | 9 +++++++-- 6 files changed, 36 insertions(+), 18 deletions(-) diff --git a/android/guava-tests/test/com/google/common/graph/AbstractGraphTest.java b/android/guava-tests/test/com/google/common/graph/AbstractGraphTest.java index 756a50c68276..a8209244c037 100644 --- a/android/guava-tests/test/com/google/common/graph/AbstractGraphTest.java +++ b/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 unused = graph.adjacentNodes(N1); // ensure cache (if any) is populated + putEdge(N1, N2); + putEdge(N2, N1); + Set n1AdjacentNodes = graph.adjacentNodes(N1); + Set 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); diff --git a/android/guava-tests/test/com/google/common/graph/AbstractNetworkTest.java b/android/guava-tests/test/com/google/common/graph/AbstractNetworkTest.java index a5fc1b254bc6..b9558f706e86 100644 --- a/android/guava-tests/test/com/google/common/graph/AbstractNetworkTest.java +++ b/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 unused = - networkAsMutableNetwork.adjacentNodes(N1); // ensure cache (if any) is populated + addEdge(N1, N2, E12); + Set n1AdjacentNodes = networkAsMutableNetwork.adjacentNodes(N1); + Set n2AdjacentNodes = networkAsMutableNetwork.adjacentNodes(N2); assertTrue(networkAsMutableNetwork.removeNode(N1)); + assertThat(n1AdjacentNodes).isEmpty(); + assertThat(n2AdjacentNodes).isEmpty(); IllegalArgumentException e = assertThrows( IllegalArgumentException.class, () -> networkAsMutableNetwork.adjacentNodes(N1)); diff --git a/android/guava/src/com/google/common/graph/StandardMutableValueGraph.java b/android/guava/src/com/google/common/graph/StandardMutableValueGraph.java index 0ea641a5b1cd..1ad474083610 100644 --- a/android/guava/src/com/google/common/graph/StandardMutableValueGraph.java +++ b/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; } } diff --git a/guava-tests/test/com/google/common/graph/AbstractGraphTest.java b/guava-tests/test/com/google/common/graph/AbstractGraphTest.java index 756a50c68276..a8209244c037 100644 --- a/guava-tests/test/com/google/common/graph/AbstractGraphTest.java +++ b/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 unused = graph.adjacentNodes(N1); // ensure cache (if any) is populated + putEdge(N1, N2); + putEdge(N2, N1); + Set n1AdjacentNodes = graph.adjacentNodes(N1); + Set 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); diff --git a/guava-tests/test/com/google/common/graph/AbstractNetworkTest.java b/guava-tests/test/com/google/common/graph/AbstractNetworkTest.java index 5d3f53505870..a29ffc5ae42b 100644 --- a/guava-tests/test/com/google/common/graph/AbstractNetworkTest.java +++ b/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 unused = - networkAsMutableNetwork.adjacentNodes(N1); // ensure cache (if any) is populated + addEdge(N1, N2, E12); + Set n1AdjacentNodes = networkAsMutableNetwork.adjacentNodes(N1); + Set n2AdjacentNodes = networkAsMutableNetwork.adjacentNodes(N2); assertTrue(networkAsMutableNetwork.removeNode(N1)); + assertThat(n1AdjacentNodes).isEmpty(); + assertThat(n2AdjacentNodes).isEmpty(); IllegalArgumentException e = assertThrows( IllegalArgumentException.class, () -> networkAsMutableNetwork.adjacentNodes(N1)); diff --git a/guava/src/com/google/common/graph/StandardMutableValueGraph.java b/guava/src/com/google/common/graph/StandardMutableValueGraph.java index 0ea641a5b1cd..1ad474083610 100644 --- a/guava/src/com/google/common/graph/StandardMutableValueGraph.java +++ b/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; } }