Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MutableNetwork.hasEdgeConnecting throw IllegalArgumentException when node are missing #3721

Closed
fathzer opened this issue Nov 23, 2019 · 4 comments
Assignees
Labels
P3 package=graph type=defect Bug, not working as expected

Comments

@fathzer
Copy link

fathzer commented Nov 23, 2019

According to the javadoc, the hasEdgeConnecting method is equivalent to nodes().contains(nodeU) && successors(nodeU).contains(nodeV).
But, unlike this expression, it throws an exception if the nodes are unknown.
Here is a simple demonstration

import com.google.common.graph.MutableNetwork;
import com.google.common.graph.NetworkBuilder;

public class Bug {
	public static void main(String[] args) {
		final MutableNetwork<String, String> graph = NetworkBuilder.undirected().allowsParallelEdges(false).build();
		// Foolowing line prints false
		System.out.println(graph.nodes().contains("1") && graph.successors("1").contains("2"));
		// Next should print false but throws an IllegalArgumentException
		System.out.println(graph.hasEdgeConnecting("1", "2"));
	}
}

version guava-28.1-jre

@nick-someone nick-someone added P3 package=graph type=defect Bug, not working as expected and removed type=defect Bug, not working as expected labels Nov 25, 2019
@nick-someone
Copy link
Member

It looks like a bit subtle, but successors(nodeU).contains(nodeV) throws an IllegalArgumentException when nodeU is not in the network (which is the actual behavior you're seeing here).

However, you're right that the short-circuiting behavior of && isn't replicating here. That seems like an important part of the documented behavior :). FWIW, it appears to also throw an IllegalArgumentException even if "1" is in the network but "2" isn't.

@jrtom
Copy link
Member

jrtom commented Nov 25, 2019

This is just a straight-up bug on our parts; I'm coding up a fix now.

The problem originates in the fact that hasEdgeConnecting() is actually (in effect) calling edgesConnecting().isEmpty(). This seems reasonable on the face of it, but edgesConnecting(u, v) has the contract that it throws if either u or v are not in the graph; this is consistent with the contracts for the other Set-returning accessors, but it's not what we want for boolean-returning queries like hasEdgeConnecting().

Sorry about that; thanks for the catch. :)

@jrtom
Copy link
Member

jrtom commented Nov 28, 2019

I've submitted an internal fix; this should be propagating externally soon.

nick-someone pushed a commit that referenced this issue Dec 1, 2019
…ng it to throw if either endpoint was not in the graph.

RELNOTES=Fix bug in AbstractNetwork.hasEdgeConnecting() causing it to throw if either endpoint was not in the graph.  Originally reported as GitHub issue #3721.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=282846559
nick-someone pushed a commit that referenced this issue Dec 2, 2019
…ng it to throw if either endpoint was not in the graph.

RELNOTES=Fix bug in AbstractNetwork.hasEdgeConnecting() causing it to throw if either endpoint was not in the graph.  Originally reported as GitHub issue #3721.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=282846559
@jrtom
Copy link
Member

jrtom commented Jan 12, 2020

This is fixed in v28.2: https://github.com/google/guava/releases/tag/v28.2

@jrtom jrtom closed this as completed Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 package=graph type=defect Bug, not working as expected
Projects
None yet
Development

No branches or pull requests

4 participants
@nick-someone @fathzer @jrtom and others