Open dare2try opened 3 years ago
d-michail
commented
3 years ago Make sure you read and understand the following: https://jgrapht.org/guide/VertexAndEdgeTypes If you adjust vertex attributes during import, you should make sure that equals/hashCode does not depend on these attributes.
dare2try
commented
3 years ago Thanks for the link, I will read it over (again); Given what I have read so far though, I don't see how it really helps since the problem here is that the Item provided by the Supplier does not have those attributes assigned yet before it is added to the vertexMap. This is why the default constructor exists in the example, solely for the purpose of the Supplier
If I were to instead construct the Item ahead of time this bug does not occur because I can leverage the secondary constructor, or assign the field, before adding the vertex to the graph.
dare2try
commented
3 years ago Consider the following updated example... If I force object equality during the import everything works, but to get back to being able to use my own equality methods I need to set a flag and clone the graph (which will reuse the existing nodes and vertices and add them using the add* methods).
Also note, I have now hidden the default constructor since it is only available to the supplier.
package com.example.jgrapht;
import org.jgrapht.Graph;
import org.jgrapht.alg.util.Pair;
import org.jgrapht.graph.DefaultEdge;
import org.jgrapht.graph.DirectedAcyclicGraph;
import org.jgrapht.nio.Attribute;
import org.jgrapht.nio.json.JSONImporter;
import org.jgrapht.util.SupplierUtil;
import org.junit.jupiter.api.Test;
import java.io.StringReader;
import java.util.function.Supplier;
public class importTests {
@Test
public void bugExample() {
// @formatter:off
String input = "{\n"
+ " \"nodes\": [\n"
+ " { \"id\":\"1\", \"name\": \"item1\" },\n"
+ " { \"id\":\"2\", \"name\": \"item2\" },\n"
+ " { \"id\":\"3\", \"name\": \"item3\" }\n"
+ " ],\n"
+ " \"edges\": [\n"
+ " { \"source\":\"1\", \"target\": \"3\" },\n"
+ " { \"source\":\"2\", \"target\": \"3\" }\n"
+ " ]\n"
+ "}";
// @formatter:on
DirectedAcyclicGraph<Item, DefaultEdge> graph = new DirectedAcyclicGraph<>(
new Item.ItemSupplier(),
SupplierUtil.createDefaultEdgeSupplier(),
false);
JSONImporter<Item, DefaultEdge> importer = new JSONImporter<>();
importer.addVertexAttributeConsumer((p, a) -> {
Pair pair = (Pair)p;
Attribute attribute = (Attribute)a;
Item item = (Item) pair.getFirst();
String attributeName = (String)pair.getSecond();
switch (attributeName) {
case "name":
item.setName(attribute.getValue());
break;
default:
throw new RuntimeException("Unknown attribute '" + attributeName + "'");
}
});
importer.importGraph(graph, new StringReader(input));
// workaround for bug in library where import stores the item
// with the hashCode of the item before it has it's values assigned,
// and later has it's values assigned which will result in a different
// hashCode
graph.vertexSet().forEach(v -> v.setUseObjectEquality(false));
Graph<Item, DefaultEdge> clone = (Graph<Item, DefaultEdge>) graph.clone();
// test the workaround
clone.addEdge(new Item("item1"), new Item("item2"));
clone.removeEdge(new Item("item1"), new Item("item2"));
}
}
class Item {
private String name;
private boolean useObjectEquality = false;
private Item() {
// used by the import
}
public Item(String name) {
this.name = name;
}
public String getName() {
return name;
}
public void setName(String name) {
this.name = name;
}
public void setUseObjectEquality(boolean useObjectEquality) {
this.useObjectEquality = useObjectEquality;
}
@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
if (this.useObjectEquality) return super.equals(o);
Item item = (Item) o;
return name != null ? name.equals(item.name) : item.name == null;
}
@Override
public int hashCode() {
if (this.useObjectEquality) return super.hashCode();
return name != null ? name.hashCode() : 0;
}
@Override
public String toString() {
return "Item{" +
"name='" + name + '\'' +
'}';
}
/*
This supplier explicitly provides objects that will use object equality
*/
static class ItemSupplier implements Supplier<Item> {
@Override
public Item get() {
Item i = new Item();
i.setUseObjectEquality(true);
return i;
}
}
}As a follow-up, it would appear to me that this bug could be avoided by notifying the vertex consumers after the vertex attributes have been assigned (e.g. vertex attribute consumers have been invoked). https://github.com/jgrapht/jgrapht/blob/master/jgrapht-io/src/main/java/org/jgrapht/nio/json/JSONEventDrivenImporter.java#L219-L222
Something like this... From:
notifyVertex(nodeId);
for (String key : attributes.keySet()) {
notifyVertexAttribute(nodeId, key, attributes.get(key));
}for (String key : attributes.keySet()) {
notifyVertexAttribute(nodeId, key, attributes.get(key));
}
notifyVertex(nodeId);This would imply that the consumer code would need to be altered to take into account that the node may not exist by the time the attributes are iterated, something like this...
public final Consumer<String> vertexConsumer = (t) -> {
if (map.containsKey(t)) {
//throw new ImportException("Node " + t + " already exists");
graph.addVertex(map.get(t));
} else {
map.put(t, graph.addVertex());
}
};
public final BiConsumer<Pair<String, String>, Attribute> vertexAttributeConsumer =
(p, a) -> {
String vertexId = p.getFirst();
V vertex;
if (!map.containsKey(vertexId)) {
//throw new ImportException("Node " + vertex + " does not exist");
// create a new one and add it to the map
vertex = graph.getVertexSupplier().get();
map.put(vertexId, vertex);
} else {
vertex = map.get(vertexId);
}
notifyVertexAttribute(vertex, p.getSecond(), a);
};Let me know if you're ok with these changes and I can provide a PR for the 1.4.0 version. 👍
d-michail
commented
3 years ago Could you try our latest version and see if you still have this problem? We have a VertexFactory which you can provide in the importer and completely bypass the vertex suppliers of the graph. In that case you get the identifier of the vertex from file and you can construct your object.
I suspect you have a Java 8 dependency, which might be a problem, as we do not support old versions. If that is the case, you will probably need to backport the code and publish your own artifact in a private repository.
I also recall doing some changes in the DotImporter which also helps to avoid the "read first and clone the graph" problem that some users experience when they want their vertices to depend on the vertex labels/attributes. I could do something similar in the DotImporter, but this will still require you to update to our latest version.
dare2try
commented
3 years ago Yes, this is Java 8, with JGraphT 1.4.0. I was not aware that there is only support one version, otherwise I would not have created the issue. I will see if I have some time to try this out with Java 11+ and let you know.
d-michail
commented
3 years ago No problem. We fix bugs from old versions but we do not backport the fixes.
Issue Given a vertex that is defined as a custom object, and the class defines equality (e.g. equals(...), and hashCode()), when the graph is imported using the JsonImporter, a failure occurs when the importer attempts to assign the edges.
Steps to reproduce (small coding example)
Expected behaviour When using a class that defines its own equality methods, the import should succeed as expected
Other information After debugging this issue, I have determined that failure occurs when the import attempts to add the edge and this code is encountered
The root of the problem seems to be that during the import process the vertex (provided by the supplier) is added to the vertexMap (implemented as a
LinkedHashMap) before the attributes are assigned by the vertex attribute consumers; as a result thehashCodemethod is called on the object as a result of being added to the map, but this value changes after the attributes have been assigned to the object.If you were to remove the
equals(...)andhashCodemethods of theItemclass, the import succeeds as expected.