Syncleus / Ferma

An ORM / OGM for the TinkerPop graph stack.
http://syncleus.com/Ferma
Apache License 2.0
137 stars 26 forks source link

Required argument type in AbstractVertexFrame.traverse() does not allow for anonymous traversals #16

Closed alvanson closed 7 years ago

alvanson commented 7 years ago

In the Node.java reproduced below, using Ferma 3.0.2 and tinkergraph-gremlin 3.2.3 on OpenJDK 8u121-b13, attempting to use Node.traverse() with an anonymous traversal (__.out("parent")) results in the following compile-time error (change the call to traverse1() back to traverse() first):

Node.java:[56,46] error: incompatible types: no instance(s) of type variable(s) A exist so that GraphTraversal<A,Vertex> conforms to Traversal<?,CAP#1>

Changing the argument type of traverse() from Function<GraphTraversal<? extends Vertex, ? extends Vertex>, GraphTraversal<?, ?>> to Function<GraphTraversal<Vertex, Vertex>, GraphTraversal<?, ?>> seems to correct the issue. Following the types through the current TinkerPop API, input.V(getElement().id()) returns type GraphTraversal<Vertex, Vertex> so I believe it is sufficient to require traverser to have type Function<GraphTraversal<Vertex, Vertex>, GraphTraversal<?, ?>>.

I imagine a similar issue exists with AbstractEdgeFrame.

Node.java:

package com.syncleus.ferma.issues;

import com.syncleus.ferma.AbstractVertexFrame;
import com.syncleus.ferma.DelegatingFramedGraph;
import com.syncleus.ferma.FramedGraph;
import com.syncleus.ferma.Traversable;
import com.syncleus.ferma.annotations.Adjacency;
import com.syncleus.ferma.annotations.Property;
import javax.annotation.Nullable;
import java.util.function.Function;
import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversal;
import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__;
import org.apache.tinkerpop.gremlin.structure.Direction;
import org.apache.tinkerpop.gremlin.structure.Graph;
import org.apache.tinkerpop.gremlin.structure.Vertex;
import org.apache.tinkerpop.gremlin.tinkergraph.structure.TinkerGraph;

public abstract class Node extends AbstractVertexFrame {

    public static void main(String[] args) {
        // create new graph
        Graph graph = TinkerGraph.open();
        FramedGraph fg = new DelegatingFramedGraph(graph, true, true);
        // add five nodes (vertices)
        Node[] nodes = new Node[5];
        for (int i = 0; i < nodes.length; i++) {
            nodes[i] = fg.addFramedVertex(Node.class);
            nodes[i].setName("Node" + i);
        }
        // Node0 <- Node1 <- ... <- Node4
        for (int i = 1; i < nodes.length; i++) {
            nodes[i].setParent(nodes[i - 1]);
        }
        // ask Node3 for its grandparent (Node1)
        System.out.println(nodes[3].getGrandParent());
    }

    @Property("name")
    public abstract String getName();

    @Property("name")
    public abstract void setName(String name);

    @Adjacency(label = "parent", direction = Direction.OUT)
    public abstract Node getParent();

    public void setParent(Node parent) {
        // node can only have one parent
        setLinkOut(parent, "parent");
    }

    public Node getGrandParent() {
        // with traverse(): Node.java:[55,46] error: incompatible types: no instance(s) of type variable(s) A exist so that GraphTraversal<A,Vertex> conforms to Traversal<?,CAP#1>
        return traverse1((v) -> v.repeat(__.out("parent")).times(2)).next(Node.class);
    }

    public <T extends Traversable<?, ?>> T traverse1(final Function<GraphTraversal<Vertex, Vertex>, GraphTraversal<?, ?>> traverser) {
        return this.getGraph().traverse(new Function<GraphTraversalSource, GraphTraversal<?, ?>>() {
            @Nullable
            @Override
            public GraphTraversal<?, ?> apply(@Nullable final GraphTraversalSource input) {
                return traverser.apply(input.V(getElement().id()));
            }
        });
    }

}
freemo commented 7 years ago

@alvanson I will be able to try out this code and take a closer look at it tomorrow. Hope it isnt blocking you at the moment.

alvanson commented 7 years ago

@freemo No problem. I'm working around the issue right now by calling getGraph().traverse((g) -> g.V(getElement().id())...) instead of traverse(...), so I'm not currently blocked.

freemo commented 7 years ago

@alvanson Sorry this ticket seemed to slip through the cracks until now. Is it still an issue for you or has it been fixed? I can hopefully investigate it before the next release.

freemo commented 7 years ago

@alvanson would you like to submit a pull request so we can add you in the contributors file for credit? If not its an easy enough change I can attempt to make it myself, assuming it doesnt break backwards compatibility.

alvanson commented 7 years ago

@freemo I haven't been able to fix this issue without breaking backwards compatibility.

Explicitly casting v to GraphTraversal<Vertex,Vertex> is another possible workaround:

    public Node getGrandParent() {
        return traverse((v) -> ((GraphTraversal<Vertex,Vertex>) v).repeat(__.out("parent")).times(2)).next(Node.class);
    }
freemo commented 7 years ago

We could break backwards compatibility if we bump major version. But may be better to create a new method entirely with the new signature. Sadly due to erasure we can't overload it.

On Sep 25, 2017 4:35 PM, "Evan Thompson" notifications@github.com wrote:

@freemo https://github.com/freemo I haven't been able to fix this issue without breaking backwards compatibility.

Explicitly casting v to GraphTraversal<Vertex,Vertex> is another possible workaround:

public Node getGrandParent() {
    return traverse((v) -> ((GraphTraversal<Vertex,Vertex>) v).repeat(__.out("parent")).times(2)).next(Node.class);
}

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Syncleus/Ferma/issues/16#issuecomment-332004314, or mute the thread https://github.com/notifications/unsubscribe-auth/AC5JAtBJV5uzy96v4I7ZJoiooCaCwAuPks5smA6ogaJpZM4Mh67s .

alvanson commented 7 years ago

It seems like there may be some other underlying issues between TinkerPop and Java generics. For example, apparently one needs to type repeat() when used as the head of an anonymous traversal. Explicitly casting to GraphTraversal<Vertex,Vertex> by the calling code where required is probably cleaner overall than adding a traverse1() to the library.

freemo commented 7 years ago

@alvanson if we broke backwards compatability (and bumped the major version) to use the new form so you wouldnt have to cast would that cause any problems you can foresee other than the broken backwards compatibility?

Generics are erased at compile time so it shouldnt have any functional effects, i suspect the call to repeat just transforms an argument to one of the generics somehow.

freemo commented 7 years ago

@alvanson So i just made this change to test it out in Ferma and it appears if i make this change all the unit tests continue to work and nothing really breaks. So while strictly speaking it could break backwards compatibility i think for the vast majority of users it would not.

I'm starting to think it might be a change worth making but id like to hear your thoughts.

freemo commented 7 years ago

@alvanson So I just pushed this fix to master for evaluation. Let me know if it breaks anything for you. If so we can always revert it before the next release.

https://github.com/Syncleus/Ferma/commit/6b3a372dd6f0ca898c6f899088c5b234b27fc8eb