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 2020/03/31 17:18:26 UTC
[tinkerpop] 01/01: TINKERPOP-2345 Improved error message for bad
value for math()
This is an automated email from the ASF dual-hosted git repository.
spmallette pushed a commit to branch TINKERPOP-2345
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit aad8583e1245e75b59267d4f03b4233aa8951f03
Author: Stephen Mallette <sp...@genoprime.com>
AuthorDate: Tue Mar 31 13:13:25 2020 -0400
TINKERPOP-2345 Improved error message for bad value for math()
This problem really only applies to 3.4.x since by() didn't modulate Map traversers in 3.3.x. This change does also however fix the error message for non-Number objects trying to be passed into a math() expression, but the error is at least more understandable than the NullPointerException - on 3.3.x we get "java.lang.String cannot be cast to java.lang.Number" which while not perfect is probably good enough for that older version. It didn't seem like a big enough problem to add the en [...]
---
CHANGELOG.asciidoc | 1 +
.../gremlin/process/traversal/step/Scoping.java | 22 ++++++------------
.../process/traversal/step/map/MathStep.java | 18 +++++++++++----
.../process/traversal/CoreTraversalTest.java | 26 +++++++++++++++++++++-
4 files changed, 47 insertions(+), 20 deletions(-)
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index fee7d66..ceef345 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -26,6 +26,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
This release also includes changes from <<release-3-3-11, 3.3.11>>.
* Gremlin.NET driver: Fixed a `NullReferenceException` and throw clear exception if received message is empty.
+* Improved error message for `math()` when the selected key in a `Map` is `null` or not a `Number`.
[[release-3-4-6]]
=== TinkerPop 3.4.6 (Release Date: February 20, 2020)
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java
index da7b0eb..36e7493 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/Scoping.java
@@ -110,32 +110,24 @@ public interface Scoping {
public enum Variable {START, END}
public default <S> S getScopeValue(final Pop pop, final String key, final Traverser.Admin<?> traverser) throws IllegalArgumentException {
- final Object object = traverser.get();
- if (object instanceof Map && ((Map<String, S>) object).containsKey(key))
- return ((Map<String, S>) object).get(key);
- ///
- if (traverser.getSideEffects().exists(key))
- return traverser.getSideEffects().get(key);
- ///
- final Path path = traverser.path();
- if (path.hasLabel(key))
- return path.get(pop, key);
- ///
- throw new IllegalArgumentException("Neither the sideEffects, map, nor path has a " + key + "-key: " + this);
+ final S o = getNullableScopeValue(pop, key, traverser);
+ if (null == o)
+ throw new IllegalArgumentException("Neither the sideEffects, map, nor path has a " + key + "-key: " + this);
+ return o;
}
public default <S> S getNullableScopeValue(final Pop pop, final String key, final Traverser.Admin<?> traverser) {
final Object object = traverser.get();
if (object instanceof Map && ((Map<String, S>) object).containsKey(key))
return ((Map<String, S>) object).get(key);
- ///
+
if (traverser.getSideEffects().exists(key))
return traverser.getSideEffects().get(key);
- ///
+
final Path path = traverser.path();
if (path.hasLabel(key))
return path.get(pop, key);
- ///
+
return null;
}
diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MathStep.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MathStep.java
index 0994411..7a483a1 100644
--- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MathStep.java
+++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/MathStep.java
@@ -37,6 +37,7 @@ import java.io.Serializable;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.List;
+import java.util.Objects;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
@@ -67,10 +68,19 @@ public final class MathStep<S> extends MapStep<S, Double> implements ByModulatin
protected Double map(final Traverser.Admin<S> traverser) {
final Expression localExpression = new Expression(this.expression.getExpression());
for (final String var : this.expression.getVariables()) {
- localExpression.setVariable(var,
- var.equals(CURRENT) ?
- TraversalUtil.applyNullable(traverser, this.traversalRing.next()).doubleValue() :
- TraversalUtil.applyNullable((S) this.getNullableScopeValue(Pop.last, var, traverser), this.traversalRing.next()).doubleValue());
+ final Object o = var.equals(CURRENT) ?
+ TraversalUtil.applyNullable(traverser, this.traversalRing.next()) :
+ TraversalUtil.applyNullable((S) this.getNullableScopeValue(Pop.last, var, traverser), this.traversalRing.next());
+
+ // it's possible for ElementValueTraversal to return null or something that is possibly not a Number.
+ // worth a check to try to return a nice error message. The TraversalRing<S, Number> is a bit optimistic
+ // given type erasure. It could easily end up otherwise.
+ if (!(o instanceof Number))
+ throw new IllegalStateException(String.format(
+ "The variable %s for math() step must resolve to a Number - it is instead of type %s with value %s",
+ var, Objects.isNull(o) ? "null" : o.getClass().getName(), o));
+
+ localExpression.setVariable(var, ((Number) o).doubleValue());
}
this.traversalRing.reset();
return localExpression.evaluate();
diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/CoreTraversalTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/CoreTraversalTest.java
index 04417a0..cf34ee2 100644
--- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/CoreTraversalTest.java
+++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/CoreTraversalTest.java
@@ -48,8 +48,10 @@ import static org.apache.tinkerpop.gremlin.LoadGraphWith.GraphData.MODERN;
import static org.apache.tinkerpop.gremlin.process.traversal.AnonymousTraversalSource.traversal;
import static org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__.inject;
import static org.apache.tinkerpop.gremlin.structure.Graph.Features.GraphFeatures.FEATURE_TRANSACTIONS;
+import static org.hamcrest.core.StringStartsWith.startsWith;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
@@ -267,6 +269,7 @@ public class CoreTraversalTest extends AbstractGremlinProcessTest {
//They should be converted to regular exceptions before returning to the the user.
try {
g.V().has("foo").next();
+ fail("Expected a user facing NoSuchElementException");
} catch (NoSuchElementException e) {
assertEquals(NoSuchElementException.class, e.getClass());
}
@@ -289,7 +292,6 @@ public class CoreTraversalTest extends AbstractGremlinProcessTest {
} catch (NoSuchElementException e) {
assertEquals(FastNoSuchElementException.class, e.getClass());
}
-
}
@Test
@@ -299,4 +301,26 @@ public class CoreTraversalTest extends AbstractGremlinProcessTest {
assertEquals(6, simulatedRemoteG.V().count().next().intValue());
assertEquals("marko", simulatedRemoteG.V().has("name", "marko").values("name").next());
}
+
+ @Test
+ @LoadGraphWith(MODERN)
+ public void shouldThrowNiceExceptionWhenMapKeyNotFoundInMathStep() {
+ try {
+ g.V().hasLabel("person").project("age").by("age").as("x").math("x").by("aged").iterate();
+ fail("Traversal should no have succeeded since the 'aged' key does not exist");
+ } catch (IllegalStateException ise) {
+ assertThat(ise.getMessage(), startsWith("The variable x for math() step must resolve to a Number"));
+ }
+ }
+
+ @Test
+ @LoadGraphWith(MODERN)
+ public void shouldThrowNiceExceptionWhenMapKeyIsNotResolvingToNumberInMathStep() {
+ try {
+ g.V().hasLabel("person").project("age").by("age").as("x").math("x").by("name").iterate();
+ fail("Traversal should no have succeeded since the 'name' key does not resolve to Number");
+ } catch (IllegalStateException ise) {
+ assertThat(ise.getMessage(), startsWith("The variable x for math() step must resolve to a Number"));
+ }
+ }
}