You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by sp...@apache.org on 2021/02/19 19:55:17 UTC
[tinkerpop] branch TINKERPOP-2513 created (now 52ce578)
This is an automated email from the ASF dual-hosted git repository.
spmallette pushed a change to branch TINKERPOP-2513
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git.
at 52ce578 TINKERPOP-2513 Improved error messaging in Gremln construction.
This branch includes the following new commits:
new 52ce578 TINKERPOP-2513 Improved error messaging in Gremln construction.
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
[tinkerpop] 01/01: TINKERPOP-2513 Improved error messaging in
Gremln construction.
Posted by sp...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
spmallette pushed a commit to branch TINKERPOP-2513
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 52ce57852d5df24076cf22a78a73f0b8ab052cab
Author: Stephen Mallette <st...@amazon.com>
AuthorDate: Fri Feb 19 14:50:23 2021 -0500
TINKERPOP-2513 Improved error messaging in Gremln construction.
Focused mostly on addE() with property() cardinality and improper from()/to() usage. This is a bit of a patch for the exact issues described in the JIRA, but probably needs to be implemented more generally as described in TINKERPOP-800.
---
CHANGELOG.asciidoc | 2 +
.../traversal/dsl/graph/GraphTraversal.java | 63 +++++++++++---
.../traversal/step/map/AddEdgeStartStep.java | 27 +++++-
.../process/traversal/step/map/AddEdgeStep.java | 19 ++++-
.../traversal/step/sideEffect/AddPropertyStep.java | 8 ++
.../tinkergraph/structure/TinkerGraphTest.java | 95 ++++++++++++++++++++++
6 files changed, 195 insertions(+), 19 deletions(-)
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index 0ab66ea..0156bc2 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -29,6 +29,8 @@ This release also includes changes from <<release-3-4-3, 3.4.3>>.
* Fixed a bug where spark-gremlin was not re-attaching properties when using `dedup()`.
* Ensured better consistency of the use of `null` as arguments to mutation steps.
* Prevented `TraversalStrategy` instances from being added more than once, where the new instance replaces the old.
+* Improved error message for `addE()` when the `from()` or `to()` does not resolve to a `Vertex`.
+* Improved error message for `addE()` when cardinality is specified on `property()` assignments.
* Allowed `property(T.label,Object)` to be used if no value was supplied to `addV(String)`.
* Allowed additional arguments to `Client.submit()` in Javascript driver to enable setting of parameters like `scriptEvaluationTimeout`.
* Gremlin.Net driver no longer supports skipping deserialization by default. Users can however create their own `IMessageSerializer` if they need this functionality.
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java
index a56f8a4..fcb6318 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java
@@ -172,6 +172,8 @@ import java.util.function.Consumer;
import java.util.function.Function;
import java.util.function.Predicate;
+import static org.apache.tinkerpop.gremlin.structure.VertexProperty.Cardinality.single;
+
/**
* @author Marko A. Rodriguez (http://markorodriguez.com)
* @author Stephen Mallette (http://stephen.genoprime.com)
@@ -1096,8 +1098,13 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> {
* @since 3.1.0-incubating
*/
public default GraphTraversal<S, E> to(final String toStepLabel) {
+ final Step<?,?> prev = this.asAdmin().getEndStep();
+ if (!(prev instanceof FromToModulating))
+ throw new IllegalArgumentException(String.format(
+ "The to() step cannot follow %s", prev.getClass().getSimpleName()));
+
this.asAdmin().getBytecode().addStep(Symbols.to, toStepLabel);
- ((FromToModulating) this.asAdmin().getEndStep()).addTo(toStepLabel);
+ ((FromToModulating) prev).addTo(toStepLabel);
return this;
}
@@ -1110,8 +1117,13 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> {
* @since 3.1.0-incubating
*/
public default GraphTraversal<S, E> from(final String fromStepLabel) {
+ final Step<?,?> prev = this.asAdmin().getEndStep();
+ if (!(prev instanceof FromToModulating))
+ throw new IllegalArgumentException(String.format(
+ "The from() step cannot follow %s", prev.getClass().getSimpleName()));
+
this.asAdmin().getBytecode().addStep(Symbols.from, fromStepLabel);
- ((FromToModulating) this.asAdmin().getEndStep()).addFrom(fromStepLabel);
+ ((FromToModulating) prev).addFrom(fromStepLabel);
return this;
}
@@ -1125,8 +1137,13 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> {
* @since 3.1.0-incubating
*/
public default GraphTraversal<S, E> to(final Traversal<?, Vertex> toVertex) {
+ final Step<?,?> prev = this.asAdmin().getEndStep();
+ if (!(prev instanceof FromToModulating))
+ throw new IllegalArgumentException(String.format(
+ "The to() step cannot follow %s", prev.getClass().getSimpleName()));
+
this.asAdmin().getBytecode().addStep(Symbols.to, toVertex);
- ((FromToModulating) this.asAdmin().getEndStep()).addTo(toVertex.asAdmin());
+ ((FromToModulating) prev).addTo(toVertex.asAdmin());
return this;
}
@@ -1140,8 +1157,13 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> {
* @since 3.1.0-incubating
*/
public default GraphTraversal<S, E> from(final Traversal<?, Vertex> fromVertex) {
+ final Step<?,?> prev = this.asAdmin().getEndStep();
+ if (!(prev instanceof FromToModulating))
+ throw new IllegalArgumentException(String.format(
+ "The from() step cannot follow %s", prev.getClass().getSimpleName()));
+
this.asAdmin().getBytecode().addStep(Symbols.from, fromVertex);
- ((FromToModulating) this.asAdmin().getEndStep()).addFrom(fromVertex.asAdmin());
+ ((FromToModulating) prev).addFrom(fromVertex.asAdmin());
return this;
}
@@ -1155,8 +1177,13 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> {
* @since 3.3.0
*/
public default GraphTraversal<S, E> to(final Vertex toVertex) {
+ final Step<?,?> prev = this.asAdmin().getEndStep();
+ if (!(prev instanceof FromToModulating))
+ throw new IllegalArgumentException(String.format(
+ "The to() step cannot follow %s", prev.getClass().getSimpleName()));
+
this.asAdmin().getBytecode().addStep(Symbols.to, toVertex);
- ((FromToModulating) this.asAdmin().getEndStep()).addTo(__.constant(toVertex).asAdmin());
+ ((FromToModulating) prev).addTo(__.constant(toVertex).asAdmin());
return this;
}
@@ -1170,8 +1197,13 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> {
* @since 3.3.0
*/
public default GraphTraversal<S, E> from(final Vertex fromVertex) {
+ final Step<?,?> prev = this.asAdmin().getEndStep();
+ if (!(prev instanceof FromToModulating))
+ throw new IllegalArgumentException(String.format(
+ "The from() step cannot follow %s", prev.getClass().getSimpleName()));
+
this.asAdmin().getBytecode().addStep(Symbols.from, fromVertex);
- ((FromToModulating) this.asAdmin().getEndStep()).addFrom(__.constant(fromVertex).asAdmin());
+ ((FromToModulating) prev).addFrom(__.constant(fromVertex).asAdmin());
return this;
}
@@ -2177,12 +2209,19 @@ public interface GraphTraversal<S, E> extends Traversal<S, E> {
endStep = endStep.getPreviousStep();
}
- // edge properties can always be folded as there is no cardinality/metaproperties. for a vertex mutation,
- // it's possible to fold the property() into the Mutating step if there are no metaproperties (i.e. keyValues)
- // and if (1) the key is an instance of T OR OR (3) the key is a string and the cardinality is not specifiied.
- // Note that checking for single cardinality of the argument doesn't work well because once folded we lose
- // the cardinality argument associated to the key/value pair and then it relies on the graph. that
- // means that if you do:
+ // edge properties can always be folded as there are no cardinality/metaproperties. of course, if the
+ // cardinality is specified as something other than single or null it would be confusing to simply allow it to
+ // execute and not throw an error.
+ if ((endStep instanceof AddEdgeStep || endStep instanceof AddEdgeStartStep) && (null != cardinality && cardinality != single))
+ throw new IllegalStateException(String.format(
+ "Multi-property cardinality of [%s] can only be set for a Vertex but is being used for addE() with key: %s",
+ cardinality.name(), key));
+
+ // for a vertex mutation, it's possible to fold the property() into the Mutating step if there are no
+ // metaproperties (i.e. keyValues) and if (1) the key is an instance of T OR OR (3) the key is a string and the
+ // cardinality is not specified. Note that checking for single cardinality of the argument doesn't work well
+ // because once folded we lose the cardinality argument associated to the key/value pair and then it relies on
+ // the graph. that means that if you do:
//
// g.addV().property(single, 'k',1).property(single,'k',2)
//
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStartStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStartStep.java
index 63f6776..d2b81e9 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStartStep.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStartStep.java
@@ -105,16 +105,35 @@ public class AddEdgeStartStep extends AbstractStep<Edge, Edge>
if (this.first) {
this.first = false;
final TraverserGenerator generator = this.getTraversal().getTraverserGenerator();
- final Traverser.Admin traverser = generator.generate(1, (Step) this, 1); // a dead traverser to trigger the traversal
- Vertex toVertex = (Vertex) this.parameters.get(traverser, TO, Collections::emptyList).get(0);
- Vertex fromVertex = (Vertex) this.parameters.get(traverser, FROM, Collections::emptyList).get(0);
+
+ // a dead traverser to trigger the traversal
+ final Traverser.Admin traverser = generator.generate(1, (Step) this, 1);
+
+ final String edgeLabel = (String) this.parameters.get(traverser, T.label, () -> Edge.DEFAULT_LABEL).get(0);
+
+ // FROM/TO must be set and must be vertices
+ final Object theTo = this.parameters.get(traverser, TO, () -> null).get(0);
+ if (!(theTo instanceof Vertex))
+ throw new IllegalStateException(String.format(
+ "addE(%s) could not find a Vertex for to() - encountered: %s", edgeLabel,
+ null == theTo ? "null" : theTo.getClass().getSimpleName()));
+
+ final Object theFrom = this.parameters.get(traverser, FROM, () -> null).get(0);
+ if (!(theFrom instanceof Vertex))
+ throw new IllegalStateException(String.format(
+ "addE(%s) could not find a Vertex for from() - encountered: %s", edgeLabel,
+ null == theFrom ? "null" : theFrom.getClass().getSimpleName()));
+
+ Vertex toVertex = (Vertex) theTo;
+ Vertex fromVertex = (Vertex) theFrom;
+
if (toVertex instanceof Attachable)
toVertex = ((Attachable<Vertex>) toVertex)
.attach(Attachable.Method.get(this.getTraversal().getGraph().orElse(EmptyGraph.instance())));
if (fromVertex instanceof Attachable)
fromVertex = ((Attachable<Vertex>) fromVertex)
.attach(Attachable.Method.get(this.getTraversal().getGraph().orElse(EmptyGraph.instance())));
- final String edgeLabel = (String) this.parameters.get(traverser, T.label, () -> Edge.DEFAULT_LABEL).get(0);
+
final Edge edge = fromVertex.addEdge(edgeLabel, toVertex, this.parameters.getKeyValues(traverser, TO, FROM, T.label));
if (callbackRegistry != null && !callbackRegistry.getCallbacks().isEmpty()) {
final EventStrategy eventStrategy = getTraversal().getStrategies().getStrategy(EventStrategy.class).get();
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStep.java
index 14faa2c..1bf5d4a 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStep.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddEdgeStep.java
@@ -96,15 +96,28 @@ public class AddEdgeStep<S> extends ScalarMapStep<S, Edge>
@Override
protected Edge map(final Traverser.Admin<S> traverser) {
- Vertex toVertex = this.parameters.get(traverser, TO, () -> (Vertex) traverser.get()).get(0);
- Vertex fromVertex = this.parameters.get(traverser, FROM, () -> (Vertex) traverser.get()).get(0);
+ final String edgeLabel = this.parameters.get(traverser, T.label, () -> Edge.DEFAULT_LABEL).get(0);
+ final Object theTo = this.parameters.get(traverser, TO, traverser::get).get(0);
+ if (!(theTo instanceof Vertex))
+ throw new IllegalStateException(String.format(
+ "addE(%s) could not find a Vertex for to() - encountered: %s", edgeLabel,
+ null == theTo ? "null" : theTo.getClass().getSimpleName()));
+
+ final Object theFrom = this.parameters.get(traverser, FROM, traverser::get).get(0);
+ if (!(theFrom instanceof Vertex))
+ throw new IllegalStateException(String.format(
+ "addE(%s) could not find a Vertex for from() - encountered: %s", edgeLabel,
+ null == theFrom ? "null" : theFrom.getClass().getSimpleName()));
+
+ Vertex toVertex = (Vertex) theTo;
+ Vertex fromVertex = (Vertex) theFrom;
+
if (toVertex instanceof Attachable)
toVertex = ((Attachable<Vertex>) toVertex)
.attach(Attachable.Method.get(this.getTraversal().getGraph().orElse(EmptyGraph.instance())));
if (fromVertex instanceof Attachable)
fromVertex = ((Attachable<Vertex>) fromVertex)
.attach(Attachable.Method.get(this.getTraversal().getGraph().orElse(EmptyGraph.instance())));
- final String edgeLabel = this.parameters.get(traverser, T.label, () -> Edge.DEFAULT_LABEL).get(0);
final Edge edge = fromVertex.addEdge(edgeLabel, toVertex, this.parameters.getKeyValues(traverser, TO, FROM, T.label));
if (callbackRegistry != null && !callbackRegistry.getCallbacks().isEmpty()) {
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java
index cee4346..7f1d42c 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java
@@ -98,6 +98,14 @@ public class AddPropertyStep<S extends Element> extends SideEffectStep<S>
final Element element = traverser.get();
+ // can't set cardinality if the element is something other than a vertex as only vertices can have
+ // a cardinality of properties. if we don't throw an error here we end up with a confusing cast exception
+ // which doesn't explain what went wrong
+ if (this.cardinality != null && !(element instanceof Vertex))
+ throw new IllegalStateException(String.format(
+ "Property cardinality can only be set for a Vertex but the traversal encountered %s for key: %s",
+ element.getClass().getSimpleName(), key));
+
if (this.callbackRegistry != null && !callbackRegistry.getCallbacks().isEmpty()) {
getTraversal().getStrategies().getStrategy(EventStrategy.class)
.ifPresent(eventStrategy -> {
diff --git a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java
index 442c779..9ccdef1 100644
--- a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java
+++ b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphTest.java
@@ -749,6 +749,101 @@ public class TinkerGraphTest {
}
@Test
+ public void shouldProvideClearErrorWhenTryingToMutateEdgeWithCardinality() {
+ final GraphTraversalSource g = TinkerFactory.createModern().traversal();
+
+ try {
+ g.E().property(VertexProperty.Cardinality.single, "k", 100).iterate();
+ fail("Should have thrown an error");
+ } catch (IllegalStateException ise) {
+ assertEquals("Property cardinality can only be set for a Vertex but the traversal encountered TinkerEdge for key: k", ise.getMessage());
+ }
+
+ try {
+ g.E().property(VertexProperty.Cardinality.list, "k", 100).iterate();
+ fail("Should have thrown an error");
+ } catch (IllegalStateException ise) {
+ assertEquals("Property cardinality can only be set for a Vertex but the traversal encountered TinkerEdge for key: k", ise.getMessage());
+ }
+
+ try {
+ g.addE("link").to(__.V(1)).from(__.V(1)).
+ property(VertexProperty.Cardinality.list, "k", 100).iterate();
+ fail("Should have thrown an error");
+ } catch (IllegalStateException ise) {
+ assertEquals("Multi-property cardinality of [list] can only be set for a Vertex but is being used for addE() with key: k", ise.getMessage());
+ }
+ }
+
+ @Test
+ public void shouldProvideClearErrorWhenPuttingFromToInWrongSpot() {
+ final GraphTraversalSource g = TinkerFactory.createModern().traversal();
+
+ try {
+ g.addE("link").property(VertexProperty.Cardinality.single, "k", 100).out().
+ to(__.V(1)).from(__.V(1)).iterate();
+ fail("Should have thrown an error");
+ } catch (IllegalArgumentException ise) {
+ assertEquals("The to() step cannot follow VertexStep", ise.getMessage());
+ }
+
+ try {
+ g.addE("link").property("k", 100).out().
+ from(__.V(1)).to(__.V(1)).iterate();
+ fail("Should have thrown an error");
+ } catch (IllegalArgumentException ise) {
+ assertEquals("The from() step cannot follow VertexStep", ise.getMessage());
+ }
+ }
+
+ @Test
+ public void shouldProvideClearErrorWhenFromOrToDoesNotResolveToVertex() {
+ final GraphTraversalSource g = TinkerFactory.createModern().traversal();
+
+ try {
+ g.addE("link").property(VertexProperty.Cardinality.single, "k", 100).to(__.V(1)).iterate();
+ fail("Should have thrown an error");
+ } catch (IllegalStateException ise) {
+ assertEquals("addE(link) could not find a Vertex for from() - encountered: null", ise.getMessage());
+ }
+
+ try {
+ g.addE("link").property(VertexProperty.Cardinality.single, "k", 100).from(__.V(1)).iterate();
+ fail("Should have thrown an error");
+ } catch (IllegalStateException ise) {
+ assertEquals("addE(link) could not find a Vertex for to() - encountered: null", ise.getMessage());
+ }
+
+ try {
+ g.addE("link").property("k", 100).from(__.V(1)).iterate();
+ fail("Should have thrown an error");
+ } catch (IllegalStateException ise) {
+ assertEquals("addE(link) could not find a Vertex for to() - encountered: null", ise.getMessage());
+ }
+
+ try {
+ g.V(1).values("name").as("a").addE("link").property(VertexProperty.Cardinality.single, "k", 100).from("a").iterate();
+ fail("Should have thrown an error");
+ } catch (IllegalStateException ise) {
+ assertEquals("addE(link) could not find a Vertex for to() - encountered: String", ise.getMessage());
+ }
+
+ try {
+ g.V(1).values("name").as("a").addE("link").property(VertexProperty.Cardinality.single, "k", 100).to("a").iterate();
+ fail("Should have thrown an error");
+ } catch (IllegalStateException ise) {
+ assertEquals("addE(link) could not find a Vertex for to() - encountered: String", ise.getMessage());
+ }
+
+ try {
+ g.V(1).as("v").values("name").as("a").addE("link").property(VertexProperty.Cardinality.single, "k", 100).to("v").from("a").iterate();
+ fail("Should have thrown an error");
+ } catch (IllegalStateException ise) {
+ assertEquals("addE(link) could not find a Vertex for from() - encountered: String", ise.getMessage());
+ }
+ }
+
+ @Test
public void shouldWorkWithoutIdentityStrategy() {
final Graph graph = TinkerFactory.createModern();
final GraphTraversalSource g = traversal().withEmbedded(graph).withoutStrategies(IdentityRemovalStrategy.class);