From 76260d9b3c6acbabf9a8ddae11d4fff3985b6272 Mon Sep 17 00:00:00 2001 From: Google Java Core Libraries Date: Tue, 24 May 2022 19:58:48 -0700 Subject: [PATCH] Fix issue #5843 RELNOTES=Fix issue #5843 PiperOrigin-RevId: 450827341 --- .../AbstractStandardUndirectedGraphTest.java | 4 +- ...AbstractStandardUndirectedNetworkTest.java | 45 +++++++++++---- .../google/common/graph/EndpointPairTest.java | 4 +- .../google/common/graph/ValueGraphTest.java | 40 ++++++++++++-- .../common/graph/AbstractBaseGraph.java | 6 +- .../google/common/graph/AbstractNetwork.java | 2 +- .../google/common/graph/GraphConstants.java | 2 +- .../AbstractStandardUndirectedGraphTest.java | 4 +- ...AbstractStandardUndirectedNetworkTest.java | 52 ++++++++++++------ .../google/common/graph/EndpointPairTest.java | 4 +- .../google/common/graph/ValueGraphTest.java | 55 ++++++++++++++++--- .../common/graph/AbstractBaseGraph.java | 6 +- .../google/common/graph/AbstractNetwork.java | 2 +- .../google/common/graph/GraphConstants.java | 2 +- 14 files changed, 171 insertions(+), 57 deletions(-) diff --git a/android/guava-tests/test/com/google/common/graph/AbstractStandardUndirectedGraphTest.java b/android/guava-tests/test/com/google/common/graph/AbstractStandardUndirectedGraphTest.java index a483f42d1720..da9226c31418 100644 --- a/android/guava-tests/test/com/google/common/graph/AbstractStandardUndirectedGraphTest.java +++ b/android/guava-tests/test/com/google/common/graph/AbstractStandardUndirectedGraphTest.java @@ -166,8 +166,8 @@ public void hasEdgeConnecting_correct() { @Test public void hasEdgeConnecting_mismatch() { putEdge(N1, N2); - assertThat(graph.hasEdgeConnecting(EndpointPair.ordered(N1, N2))).isTrue(); - assertThat(graph.hasEdgeConnecting(EndpointPair.ordered(N2, N1))).isTrue(); + assertThat(graph.hasEdgeConnecting(EndpointPair.ordered(N1, N2))).isFalse(); + assertThat(graph.hasEdgeConnecting(EndpointPair.ordered(N2, N1))).isFalse(); } @Test diff --git a/android/guava-tests/test/com/google/common/graph/AbstractStandardUndirectedNetworkTest.java b/android/guava-tests/test/com/google/common/graph/AbstractStandardUndirectedNetworkTest.java index 5cda1c1bb767..beadaf92d65c 100644 --- a/android/guava-tests/test/com/google/common/graph/AbstractStandardUndirectedNetworkTest.java +++ b/android/guava-tests/test/com/google/common/graph/AbstractStandardUndirectedNetworkTest.java @@ -16,6 +16,7 @@ package com.google.common.graph; +import static com.google.common.graph.GraphConstants.ENDPOINTS_MISMATCH; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.TruthJUnit.assume; import static org.junit.Assert.assertTrue; @@ -195,15 +196,30 @@ public void successors_checkReturnedSetMutability() { @Test public void edges_containsOrderMismatch() { addEdge(N1, N2, E12); - assertThat(network.asGraph().edges()).contains(ENDPOINTS_N2N1); - assertThat(network.asGraph().edges()).contains(ENDPOINTS_N1N2); + assertThat(network.asGraph().edges()).doesNotContain(ENDPOINTS_N2N1); + assertThat(network.asGraph().edges()).doesNotContain(ENDPOINTS_N1N2); + } + + @Test + public void edgesConnecting_orderMismatch() { + addEdge(N1, N2, E12); + try { + Set unused = network.edgesConnecting(ENDPOINTS_N1N2); + fail("Expected IllegalArgumentException: " + ENDPOINTS_MISMATCH); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains(ENDPOINTS_MISMATCH); + } } @Test public void edgeConnectingOrNull_orderMismatch() { addEdge(N1, N2, E12); - assertThat(network.edgeConnectingOrNull(ENDPOINTS_N2N1)).isEqualTo(E12); - assertThat(network.edgeConnectingOrNull(ENDPOINTS_N1N2)).isEqualTo(E12); + try { + String unused = network.edgeConnectingOrNull(ENDPOINTS_N1N2); + fail("Expected IllegalArgumentException: " + ENDPOINTS_MISMATCH); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains(ENDPOINTS_MISMATCH); + } } @Test @@ -418,7 +434,7 @@ public void addEdge_existingEdgeBetweenDifferentNodes() { networkAsMutableNetwork.addEdge(N4, N5, E12); fail(ERROR_ADDED_EXISTING_EDGE); } catch (IllegalArgumentException e) { - assertThat(e.getMessage()).contains(ERROR_REUSE_EDGE); + assertThat(e).hasMessageThat().contains(ERROR_REUSE_EDGE); } } @@ -432,13 +448,13 @@ public void addEdge_parallelEdge_notAllowed() { networkAsMutableNetwork.addEdge(N1, N2, EDGE_NOT_IN_GRAPH); fail(ERROR_ADDED_PARALLEL_EDGE); } catch (IllegalArgumentException e) { - assertThat(e.getMessage()).contains(ERROR_PARALLEL_EDGE); + assertThat(e).hasMessageThat().contains(ERROR_PARALLEL_EDGE); } try { networkAsMutableNetwork.addEdge(N2, N1, EDGE_NOT_IN_GRAPH); fail(ERROR_ADDED_PARALLEL_EDGE); } catch (IllegalArgumentException e) { - assertThat(e.getMessage()).contains(ERROR_PARALLEL_EDGE); + assertThat(e).hasMessageThat().contains(ERROR_PARALLEL_EDGE); } } @@ -458,7 +474,12 @@ public void addEdge_orderMismatch() { assume().that(graphIsMutable()).isTrue(); EndpointPair endpoints = EndpointPair.ordered(N1, N2); - assertThat(networkAsMutableNetwork.addEdge(endpoints, E12)).isTrue(); + try { + networkAsMutableNetwork.addEdge(endpoints, E12); + fail("Expected IllegalArgumentException: " + ENDPOINTS_MISMATCH); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains(ENDPOINTS_MISMATCH); + } } @Test @@ -527,20 +548,20 @@ public void addEdge_existingEdgeBetweenDifferentNodes_selfLoops() { networkAsMutableNetwork.addEdge(N1, N2, E11); fail("Reusing an existing self-loop edge to connect different nodes succeeded"); } catch (IllegalArgumentException e) { - assertThat(e.getMessage()).contains(ERROR_REUSE_EDGE); + assertThat(e).hasMessageThat().contains(ERROR_REUSE_EDGE); } try { networkAsMutableNetwork.addEdge(N2, N2, E11); fail("Reusing an existing self-loop edge to make a different self-loop edge succeeded"); } catch (IllegalArgumentException e) { - assertThat(e.getMessage()).contains(ERROR_REUSE_EDGE); + assertThat(e).hasMessageThat().contains(ERROR_REUSE_EDGE); } addEdge(N1, N2, E12); try { networkAsMutableNetwork.addEdge(N1, N1, E12); fail("Reusing an existing edge to add a self-loop edge between different nodes succeeded"); } catch (IllegalArgumentException e) { - assertThat(e.getMessage()).contains(ERROR_REUSE_EDGE); + assertThat(e).hasMessageThat().contains(ERROR_REUSE_EDGE); } } @@ -555,7 +576,7 @@ public void addEdge_parallelSelfLoopEdge_notAllowed() { networkAsMutableNetwork.addEdge(N1, N1, EDGE_NOT_IN_GRAPH); fail("Adding a parallel self-loop edge succeeded"); } catch (IllegalArgumentException e) { - assertThat(e.getMessage()).contains(ERROR_PARALLEL_EDGE); + assertThat(e).hasMessageThat().contains(ERROR_PARALLEL_EDGE); } } diff --git a/android/guava-tests/test/com/google/common/graph/EndpointPairTest.java b/android/guava-tests/test/com/google/common/graph/EndpointPairTest.java index 099be1d22bd0..c67693a715f9 100644 --- a/android/guava-tests/test/com/google/common/graph/EndpointPairTest.java +++ b/android/guava-tests/test/com/google/common/graph/EndpointPairTest.java @@ -214,8 +214,8 @@ public void endpointPair_undirected_contains() { assertThat(edges).contains(EndpointPair.unordered(N1, N2)); assertThat(edges).contains(EndpointPair.unordered(N2, N1)); // equal to unordered(N1, N2) - // ordered endpoints OK for undirected graph (because ordering is irrelevant) - assertThat(edges).contains(EndpointPair.ordered(N1, N2)); + // ordered endpoints not compatible with undirected graph + assertThat(edges).doesNotContain(EndpointPair.ordered(N1, N2)); assertThat(edges).doesNotContain(EndpointPair.unordered(N2, N2)); // edge not present assertThat(edges).doesNotContain(EndpointPair.unordered(N3, N4)); // nodes not in graph diff --git a/android/guava-tests/test/com/google/common/graph/ValueGraphTest.java b/android/guava-tests/test/com/google/common/graph/ValueGraphTest.java index a3f48140f999..71b6439aa092 100644 --- a/android/guava-tests/test/com/google/common/graph/ValueGraphTest.java +++ b/android/guava-tests/test/com/google/common/graph/ValueGraphTest.java @@ -173,8 +173,8 @@ public void hasEdgeConnecting_undirected_backwards() { public void hasEdgeConnecting_undirected_mismatch() { graph = ValueGraphBuilder.undirected().build(); graph.putEdgeValue(1, 2, "A"); - assertThat(graph.hasEdgeConnecting(EndpointPair.ordered(1, 2))).isTrue(); - assertThat(graph.hasEdgeConnecting(EndpointPair.ordered(2, 1))).isTrue(); + assertThat(graph.hasEdgeConnecting(EndpointPair.ordered(1, 2))).isFalse(); + assertThat(graph.hasEdgeConnecting(EndpointPair.ordered(2, 1))).isFalse(); } @Test @@ -223,8 +223,19 @@ public void edgeValueOrDefault_undirected_backwards() { public void edgeValueOrDefault_undirected_mismatch() { graph = ValueGraphBuilder.undirected().build(); graph.putEdgeValue(1, 2, "A"); - assertThat(graph.edgeValueOrDefault(EndpointPair.ordered(2, 1), "default")).isEqualTo("A"); - assertThat(graph.edgeValueOrDefault(EndpointPair.ordered(2, 1), "default")).isEqualTo("A"); + // Check that edgeValueOrDefault() throws on each possible ordering of an ordered EndpointPair + try { + String unused = graph.edgeValueOrDefault(EndpointPair.ordered(1, 2), "default"); + fail("Expected IllegalArgumentException: " + ENDPOINTS_MISMATCH); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains(ENDPOINTS_MISMATCH); + } + try { + String unused = graph.edgeValueOrDefault(EndpointPair.ordered(2, 1), "default"); + fail("Expected IllegalArgumentException: " + ENDPOINTS_MISMATCH); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains(ENDPOINTS_MISMATCH); + } } @Test @@ -251,7 +262,12 @@ public void putEdgeValue_directed_orderMismatch() { @Test public void putEdgeValue_undirected_orderMismatch() { graph = ValueGraphBuilder.undirected().build(); - assertThat(graph.putEdgeValue(EndpointPair.ordered(1, 2), "irrelevant")).isNull(); + try { + graph.putEdgeValue(EndpointPair.ordered(1, 2), "irrelevant"); + fail("Expected IllegalArgumentException: " + ENDPOINTS_MISMATCH); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains(ENDPOINTS_MISMATCH); + } } @Test @@ -311,7 +327,19 @@ public void removeEdge_directed_orderMismatch() { public void removeEdge_undirected_orderMismatch() { graph = ValueGraphBuilder.undirected().build(); graph.putEdgeValue(1, 2, "1-2"); - assertThat(graph.removeEdge(EndpointPair.ordered(1, 2))).isEqualTo("1-2"); + // Check that removeEdge() throws on each possible ordering of an ordered EndpointPair + try { + graph.removeEdge(EndpointPair.ordered(1, 2)); + fail("Expected IllegalArgumentException: " + ENDPOINTS_MISMATCH); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains(ENDPOINTS_MISMATCH); + } + try { + graph.removeEdge(EndpointPair.ordered(2, 1)); + fail("Expected IllegalArgumentException: " + ENDPOINTS_MISMATCH); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains(ENDPOINTS_MISMATCH); + } } @Test diff --git a/android/guava/src/com/google/common/graph/AbstractBaseGraph.java b/android/guava/src/com/google/common/graph/AbstractBaseGraph.java index 797468b7aa92..994332299e75 100644 --- a/android/guava/src/com/google/common/graph/AbstractBaseGraph.java +++ b/android/guava/src/com/google/common/graph/AbstractBaseGraph.java @@ -177,7 +177,11 @@ protected final void validateEndpoints(EndpointPair endpoints) { checkArgument(isOrderingCompatible(endpoints), ENDPOINTS_MISMATCH); } + /** + * Returns {@code true} iff {@code endpoints}' ordering is compatible with the directionality of + * this graph. + */ protected final boolean isOrderingCompatible(EndpointPair endpoints) { - return endpoints.isOrdered() || !this.isDirected(); + return endpoints.isOrdered() == this.isDirected(); } } diff --git a/android/guava/src/com/google/common/graph/AbstractNetwork.java b/android/guava/src/com/google/common/graph/AbstractNetwork.java index 9afbb0377b81..5dfeaf2242dc 100644 --- a/android/guava/src/com/google/common/graph/AbstractNetwork.java +++ b/android/guava/src/com/google/common/graph/AbstractNetwork.java @@ -241,7 +241,7 @@ protected final void validateEndpoints(EndpointPair endpoints) { } protected final boolean isOrderingCompatible(EndpointPair endpoints) { - return endpoints.isOrdered() || !this.isDirected(); + return endpoints.isOrdered() == this.isDirected(); } @Override diff --git a/android/guava/src/com/google/common/graph/GraphConstants.java b/android/guava/src/com/google/common/graph/GraphConstants.java index ae224fdd2252..4e8510662cf1 100644 --- a/android/guava/src/com/google/common/graph/GraphConstants.java +++ b/android/guava/src/com/google/common/graph/GraphConstants.java @@ -52,7 +52,7 @@ private GraphConstants() {} + "adjacentNode(node) if you already have a node, or nodeU()/nodeV() if you don't."; static final String EDGE_ALREADY_EXISTS = "Edge %s already exists in the graph."; static final String ENDPOINTS_MISMATCH = - "Mismatch: unordered endpoints cannot be used with directed graphs"; + "Mismatch: endpoints' ordering is not compatible with directionality of the graph"; /** Singleton edge value for {@link Graph} implementations backed by {@link ValueGraph}s. */ enum Presence { diff --git a/guava-tests/test/com/google/common/graph/AbstractStandardUndirectedGraphTest.java b/guava-tests/test/com/google/common/graph/AbstractStandardUndirectedGraphTest.java index a483f42d1720..da9226c31418 100644 --- a/guava-tests/test/com/google/common/graph/AbstractStandardUndirectedGraphTest.java +++ b/guava-tests/test/com/google/common/graph/AbstractStandardUndirectedGraphTest.java @@ -166,8 +166,8 @@ public void hasEdgeConnecting_correct() { @Test public void hasEdgeConnecting_mismatch() { putEdge(N1, N2); - assertThat(graph.hasEdgeConnecting(EndpointPair.ordered(N1, N2))).isTrue(); - assertThat(graph.hasEdgeConnecting(EndpointPair.ordered(N2, N1))).isTrue(); + assertThat(graph.hasEdgeConnecting(EndpointPair.ordered(N1, N2))).isFalse(); + assertThat(graph.hasEdgeConnecting(EndpointPair.ordered(N2, N1))).isFalse(); } @Test diff --git a/guava-tests/test/com/google/common/graph/AbstractStandardUndirectedNetworkTest.java b/guava-tests/test/com/google/common/graph/AbstractStandardUndirectedNetworkTest.java index 7939ad8358b6..de6fee22e64b 100644 --- a/guava-tests/test/com/google/common/graph/AbstractStandardUndirectedNetworkTest.java +++ b/guava-tests/test/com/google/common/graph/AbstractStandardUndirectedNetworkTest.java @@ -16,14 +16,15 @@ package com.google.common.graph; +import static com.google.common.graph.GraphConstants.ENDPOINTS_MISMATCH; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.Truth8.assertThat; import static com.google.common.truth.TruthJUnit.assume; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import com.google.common.collect.ImmutableSet; import com.google.common.testing.EqualsTester; +import java.util.Optional; import java.util.Set; import org.junit.After; import org.junit.Test; @@ -196,29 +197,41 @@ public void successors_checkReturnedSetMutability() { @Test public void edges_containsOrderMismatch() { addEdge(N1, N2, E12); - assertThat(network.asGraph().edges()).contains(ENDPOINTS_N2N1); - assertThat(network.asGraph().edges()).contains(ENDPOINTS_N1N2); + assertThat(network.asGraph().edges()).doesNotContain(ENDPOINTS_N2N1); + assertThat(network.asGraph().edges()).doesNotContain(ENDPOINTS_N1N2); } @Test public void edgesConnecting_orderMismatch() { addEdge(N1, N2, E12); - assertThat(network.edgesConnecting(ENDPOINTS_N2N1)).containsExactly(E12); - assertThat(network.edgesConnecting(ENDPOINTS_N1N2)).containsExactly(E12); + try { + Set unused = network.edgesConnecting(ENDPOINTS_N1N2); + fail("Expected IllegalArgumentException: " + ENDPOINTS_MISMATCH); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains(ENDPOINTS_MISMATCH); + } } @Test public void edgeConnecting_orderMismatch() { addEdge(N1, N2, E12); - assertThat(network.edgeConnecting(ENDPOINTS_N2N1)).hasValue(E12); - assertThat(network.edgeConnecting(ENDPOINTS_N1N2)).hasValue(E12); + try { + Optional unused = network.edgeConnecting(ENDPOINTS_N1N2); + fail("Expected IllegalArgumentException: " + ENDPOINTS_MISMATCH); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains(ENDPOINTS_MISMATCH); + } } @Test public void edgeConnectingOrNull_orderMismatch() { addEdge(N1, N2, E12); - assertThat(network.edgeConnectingOrNull(ENDPOINTS_N2N1)).isEqualTo(E12); - assertThat(network.edgeConnectingOrNull(ENDPOINTS_N1N2)).isEqualTo(E12); + try { + String unused = network.edgeConnectingOrNull(ENDPOINTS_N1N2); + fail("Expected IllegalArgumentException: " + ENDPOINTS_MISMATCH); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains(ENDPOINTS_MISMATCH); + } } @Test @@ -433,7 +446,7 @@ public void addEdge_existingEdgeBetweenDifferentNodes() { networkAsMutableNetwork.addEdge(N4, N5, E12); fail(ERROR_ADDED_EXISTING_EDGE); } catch (IllegalArgumentException e) { - assertThat(e.getMessage()).contains(ERROR_REUSE_EDGE); + assertThat(e).hasMessageThat().contains(ERROR_REUSE_EDGE); } } @@ -447,13 +460,13 @@ public void addEdge_parallelEdge_notAllowed() { networkAsMutableNetwork.addEdge(N1, N2, EDGE_NOT_IN_GRAPH); fail(ERROR_ADDED_PARALLEL_EDGE); } catch (IllegalArgumentException e) { - assertThat(e.getMessage()).contains(ERROR_PARALLEL_EDGE); + assertThat(e).hasMessageThat().contains(ERROR_PARALLEL_EDGE); } try { networkAsMutableNetwork.addEdge(N2, N1, EDGE_NOT_IN_GRAPH); fail(ERROR_ADDED_PARALLEL_EDGE); } catch (IllegalArgumentException e) { - assertThat(e.getMessage()).contains(ERROR_PARALLEL_EDGE); + assertThat(e).hasMessageThat().contains(ERROR_PARALLEL_EDGE); } } @@ -473,7 +486,12 @@ public void addEdge_orderMismatch() { assume().that(graphIsMutable()).isTrue(); EndpointPair endpoints = EndpointPair.ordered(N1, N2); - assertThat(networkAsMutableNetwork.addEdge(endpoints, E12)).isTrue(); + try { + networkAsMutableNetwork.addEdge(endpoints, E12); + fail("Expected IllegalArgumentException: " + ENDPOINTS_MISMATCH); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains(ENDPOINTS_MISMATCH); + } } @Test @@ -542,20 +560,20 @@ public void addEdge_existingEdgeBetweenDifferentNodes_selfLoops() { networkAsMutableNetwork.addEdge(N1, N2, E11); fail("Reusing an existing self-loop edge to connect different nodes succeeded"); } catch (IllegalArgumentException e) { - assertThat(e.getMessage()).contains(ERROR_REUSE_EDGE); + assertThat(e).hasMessageThat().contains(ERROR_REUSE_EDGE); } try { networkAsMutableNetwork.addEdge(N2, N2, E11); fail("Reusing an existing self-loop edge to make a different self-loop edge succeeded"); } catch (IllegalArgumentException e) { - assertThat(e.getMessage()).contains(ERROR_REUSE_EDGE); + assertThat(e).hasMessageThat().contains(ERROR_REUSE_EDGE); } addEdge(N1, N2, E12); try { networkAsMutableNetwork.addEdge(N1, N1, E12); fail("Reusing an existing edge to add a self-loop edge between different nodes succeeded"); } catch (IllegalArgumentException e) { - assertThat(e.getMessage()).contains(ERROR_REUSE_EDGE); + assertThat(e).hasMessageThat().contains(ERROR_REUSE_EDGE); } } @@ -570,7 +588,7 @@ public void addEdge_parallelSelfLoopEdge_notAllowed() { networkAsMutableNetwork.addEdge(N1, N1, EDGE_NOT_IN_GRAPH); fail("Adding a parallel self-loop edge succeeded"); } catch (IllegalArgumentException e) { - assertThat(e.getMessage()).contains(ERROR_PARALLEL_EDGE); + assertThat(e).hasMessageThat().contains(ERROR_PARALLEL_EDGE); } } diff --git a/guava-tests/test/com/google/common/graph/EndpointPairTest.java b/guava-tests/test/com/google/common/graph/EndpointPairTest.java index 099be1d22bd0..c67693a715f9 100644 --- a/guava-tests/test/com/google/common/graph/EndpointPairTest.java +++ b/guava-tests/test/com/google/common/graph/EndpointPairTest.java @@ -214,8 +214,8 @@ public void endpointPair_undirected_contains() { assertThat(edges).contains(EndpointPair.unordered(N1, N2)); assertThat(edges).contains(EndpointPair.unordered(N2, N1)); // equal to unordered(N1, N2) - // ordered endpoints OK for undirected graph (because ordering is irrelevant) - assertThat(edges).contains(EndpointPair.ordered(N1, N2)); + // ordered endpoints not compatible with undirected graph + assertThat(edges).doesNotContain(EndpointPair.ordered(N1, N2)); assertThat(edges).doesNotContain(EndpointPair.unordered(N2, N2)); // edge not present assertThat(edges).doesNotContain(EndpointPair.unordered(N3, N4)); // nodes not in graph diff --git a/guava-tests/test/com/google/common/graph/ValueGraphTest.java b/guava-tests/test/com/google/common/graph/ValueGraphTest.java index edc2631219ae..f74680cef73e 100644 --- a/guava-tests/test/com/google/common/graph/ValueGraphTest.java +++ b/guava-tests/test/com/google/common/graph/ValueGraphTest.java @@ -175,8 +175,8 @@ public void hasEdgeConnecting_undirected_backwards() { public void hasEdgeConnecting_undirected_mismatch() { graph = ValueGraphBuilder.undirected().build(); graph.putEdgeValue(1, 2, "A"); - assertThat(graph.hasEdgeConnecting(EndpointPair.ordered(1, 2))).isTrue(); - assertThat(graph.hasEdgeConnecting(EndpointPair.ordered(2, 1))).isTrue(); + assertThat(graph.hasEdgeConnecting(EndpointPair.ordered(1, 2))).isFalse(); + assertThat(graph.hasEdgeConnecting(EndpointPair.ordered(2, 1))).isFalse(); } @Test @@ -224,8 +224,19 @@ public void edgeValue_undirected_backwards() { public void edgeValue_undirected_mismatch() { graph = ValueGraphBuilder.undirected().build(); graph.putEdgeValue(1, 2, "A"); - assertThat(graph.edgeValue(EndpointPair.ordered(1, 2))).hasValue("A"); - assertThat(graph.edgeValue(EndpointPair.ordered(2, 1))).hasValue("A"); + // Check that edgeValue() throws on each possible ordering of an ordered EndpointPair + try { + Optional unused = graph.edgeValue(EndpointPair.ordered(1, 2)); + fail("Expected IllegalArgumentException: " + ENDPOINTS_MISMATCH); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains(ENDPOINTS_MISMATCH); + } + try { + Optional unused = graph.edgeValue(EndpointPair.ordered(2, 1)); + fail("Expected IllegalArgumentException: " + ENDPOINTS_MISMATCH); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains(ENDPOINTS_MISMATCH); + } } @Test @@ -274,8 +285,19 @@ public void edgeValueOrDefault_undirected_backwards() { public void edgeValueOrDefault_undirected_mismatch() { graph = ValueGraphBuilder.undirected().build(); graph.putEdgeValue(1, 2, "A"); - assertThat(graph.edgeValueOrDefault(EndpointPair.ordered(2, 1), "default")).isEqualTo("A"); - assertThat(graph.edgeValueOrDefault(EndpointPair.ordered(2, 1), "default")).isEqualTo("A"); + // Check that edgeValueOrDefault() throws on each possible ordering of an ordered EndpointPair + try { + String unused = graph.edgeValueOrDefault(EndpointPair.ordered(1, 2), "default"); + fail("Expected IllegalArgumentException: " + ENDPOINTS_MISMATCH); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains(ENDPOINTS_MISMATCH); + } + try { + String unused = graph.edgeValueOrDefault(EndpointPair.ordered(2, 1), "default"); + fail("Expected IllegalArgumentException: " + ENDPOINTS_MISMATCH); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains(ENDPOINTS_MISMATCH); + } } @Test @@ -302,7 +324,12 @@ public void putEdgeValue_directed_orderMismatch() { @Test public void putEdgeValue_undirected_orderMismatch() { graph = ValueGraphBuilder.undirected().build(); - assertThat(graph.putEdgeValue(EndpointPair.ordered(1, 2), "irrelevant")).isNull(); + try { + graph.putEdgeValue(EndpointPair.ordered(1, 2), "irrelevant"); + fail("Expected IllegalArgumentException: " + ENDPOINTS_MISMATCH); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains(ENDPOINTS_MISMATCH); + } } @Test @@ -362,7 +389,19 @@ public void removeEdge_directed_orderMismatch() { public void removeEdge_undirected_orderMismatch() { graph = ValueGraphBuilder.undirected().build(); graph.putEdgeValue(1, 2, "1-2"); - assertThat(graph.removeEdge(EndpointPair.ordered(1, 2))).isEqualTo("1-2"); + // Check that removeEdge() throws on each possible ordering of an ordered EndpointPair + try { + graph.removeEdge(EndpointPair.ordered(1, 2)); + fail("Expected IllegalArgumentException: " + ENDPOINTS_MISMATCH); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains(ENDPOINTS_MISMATCH); + } + try { + graph.removeEdge(EndpointPair.ordered(2, 1)); + fail("Expected IllegalArgumentException: " + ENDPOINTS_MISMATCH); + } catch (IllegalArgumentException e) { + assertThat(e).hasMessageThat().contains(ENDPOINTS_MISMATCH); + } } @Test diff --git a/guava/src/com/google/common/graph/AbstractBaseGraph.java b/guava/src/com/google/common/graph/AbstractBaseGraph.java index 797468b7aa92..994332299e75 100644 --- a/guava/src/com/google/common/graph/AbstractBaseGraph.java +++ b/guava/src/com/google/common/graph/AbstractBaseGraph.java @@ -177,7 +177,11 @@ protected final void validateEndpoints(EndpointPair endpoints) { checkArgument(isOrderingCompatible(endpoints), ENDPOINTS_MISMATCH); } + /** + * Returns {@code true} iff {@code endpoints}' ordering is compatible with the directionality of + * this graph. + */ protected final boolean isOrderingCompatible(EndpointPair endpoints) { - return endpoints.isOrdered() || !this.isDirected(); + return endpoints.isOrdered() == this.isDirected(); } } diff --git a/guava/src/com/google/common/graph/AbstractNetwork.java b/guava/src/com/google/common/graph/AbstractNetwork.java index d6bf4c36c6fe..633ea0bfa773 100644 --- a/guava/src/com/google/common/graph/AbstractNetwork.java +++ b/guava/src/com/google/common/graph/AbstractNetwork.java @@ -253,7 +253,7 @@ protected final void validateEndpoints(EndpointPair endpoints) { } protected final boolean isOrderingCompatible(EndpointPair endpoints) { - return endpoints.isOrdered() || !this.isDirected(); + return endpoints.isOrdered() == this.isDirected(); } @Override diff --git a/guava/src/com/google/common/graph/GraphConstants.java b/guava/src/com/google/common/graph/GraphConstants.java index ae224fdd2252..4e8510662cf1 100644 --- a/guava/src/com/google/common/graph/GraphConstants.java +++ b/guava/src/com/google/common/graph/GraphConstants.java @@ -52,7 +52,7 @@ private GraphConstants() {} + "adjacentNode(node) if you already have a node, or nodeU()/nodeV() if you don't."; static final String EDGE_ALREADY_EXISTS = "Edge %s already exists in the graph."; static final String ENDPOINTS_MISMATCH = - "Mismatch: unordered endpoints cannot be used with directed graphs"; + "Mismatch: endpoints' ordering is not compatible with directionality of the graph"; /** Singleton edge value for {@link Graph} implementations backed by {@link ValueGraph}s. */ enum Presence {