This is an automated email from the ASF dual-hosted git repository.
xiazcy pushed a commit to branch 3.6-dev
in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
The following commit(s) were added to refs/heads/3.6-dev by this push:
new de4ac90dda [TINKERPOP-2423] Do not use XOR in hashCode computation
when redundant parameters are used (#2254)
de4ac90dda is described below
commit de4ac90dda0f256c0747cf832e534d3052760e1b
Author: Norio Akagi <[email protected]>
AuthorDate: Wed Oct 18 16:41:41 2023 -0700
[TINKERPOP-2423] Do not use XOR in hashCode computation when redundant
parameters are used (#2254)
Co-authored-by: Yang Xia <[email protected]>
---
CHANGELOG.asciidoc | 1 +
.../process/traversal/step/map/GraphStep.java | 8 ++---
.../process/traversal/step/map/PropertiesStep.java | 15 ++++++--
.../process/traversal/step/map/VertexStep.java | 15 ++++++--
.../step/sideEffect/SideEffectCapStep.java | 9 +++--
.../process/traversal/step/map/GraphStepTest.java | 27 ++++++++++++++
.../traversal/step/map/PropertiesStepTest.java | 42 ++++++++++++++++++++++
.../process/traversal/step/map/VertexStepTest.java | 28 +++++++++++++++
.../step/sideEffect/SideEffectCapStepTest.java | 28 +++++++++++++++
9 files changed, 158 insertions(+), 15 deletions(-)
diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc
index 8f77255c99..6e53144486 100644
--- a/CHANGELOG.asciidoc
+++ b/CHANGELOG.asciidoc
@@ -37,6 +37,7 @@ This release also includes changes from <<release-3-5-8,
3.5.8>>.
* Fixed bug in `PythonTranslator` where `Set` syntax was not being generated
properly.
* Fixed bug in configuration object given to `PartitionStrategy` for Go that
prevented `readPartitions` from behing set properly.
* Fixed bug where the `partitionKey` was not being written when using
`PartitionStrategy` with `mergeV()` and `mergeE()`
+* Do not use `XOR` for hashCode computation of Step when only simple keys are
used and duplicate keys are allowed.
[[release-3-6-5]]
=== TinkerPop 3.6.5 (Release Date: July 31, 2023)
diff --git
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java
index afb27abf40..3b1720ff9a 100644
---
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java
+++
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStep.java
@@ -175,10 +175,10 @@ public class GraphStep<S, E extends Element> extends
AbstractStep<S, E> implemen
@Override
public int hashCode() {
- int result = super.hashCode() ^ this.returnClass.hashCode();
- if (null != this.ids) {
- for (final Object id : this.ids) {
- result ^= Objects.hashCode(id);
+ int result = Objects.hash(super.hashCode(), returnClass);
+ if (ids != null) {
+ for (Object id : ids) {
+ result = 31 * result + Objects.hashCode(id);
}
}
return result;
diff --git
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertiesStep.java
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertiesStep.java
index dce4fedbce..2c3bc2f503 100644
---
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertiesStep.java
+++
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertiesStep.java
@@ -29,9 +29,12 @@ import
org.apache.tinkerpop.gremlin.structure.util.StringFactory;
import java.util.Arrays;
import java.util.Collections;
+import java.util.Comparator;
import java.util.Iterator;
+import java.util.List;
import java.util.Objects;
import java.util.Set;
+import java.util.stream.Collectors;
/**
* @author Marko A. Rodriguez (http://markorodriguez.com)
@@ -80,9 +83,15 @@ public class PropertiesStep<E> extends FlatMapStep<Element,
E> implements AutoCl
@Override
public int hashCode() {
- int result = super.hashCode() ^ this.returnType.hashCode();
- for (final String propertyKey : this.propertyKeys) {
- result ^= Objects.hashCode(propertyKey);
+ int result = Objects.hash(super.hashCode(), returnType);
+ // property keys' order does not matter because properties("x", "y"),
and properties("y", "x") should be
+ // considered identical.
+ if (propertyKeys != null) {
+ final List<String> sortedPropertyKeys = Arrays.stream(propertyKeys)
+
.sorted(Comparator.nullsLast(Comparator.naturalOrder())).collect(Collectors.toList());
+ for (final String propertyKey : sortedPropertyKeys) {
+ result = 31 * result + Objects.hashCode(propertyKey);
+ }
}
return result;
}
diff --git
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/VertexStep.java
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/VertexStep.java
index 6825736dc5..bd429f049c 100644
---
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/VertexStep.java
+++
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/VertexStep.java
@@ -31,8 +31,12 @@ import
org.apache.tinkerpop.gremlin.structure.util.StringFactory;
import java.util.Arrays;
import java.util.Collections;
+import java.util.Comparator;
import java.util.Iterator;
+import java.util.List;
+import java.util.Objects;
import java.util.Set;
+import java.util.stream.Collectors;
/**
* @author Marko A. Rodriguez (http://markorodriguez.com)
@@ -99,9 +103,14 @@ public class VertexStep<E extends Element> extends
FlatMapStep<Vertex, E> implem
@Override
public int hashCode() {
- int result = super.hashCode() ^ this.direction.hashCode() ^
this.returnClass.hashCode();
- for (final String edgeLabel : this.edgeLabels) {
- result ^= edgeLabel.hashCode();
+ int result = Objects.hash(super.hashCode(), direction, returnClass);
+ // edgeLabel's order does not matter because in("x", "y") and in("y",
"x") must be considered equal.
+ if (edgeLabels != null && edgeLabels.length > 0) {
+ final List<String> sortedEdgeLabels = Arrays.stream(edgeLabels)
+
.sorted(Comparator.nullsLast(Comparator.naturalOrder())).collect(Collectors.toList());
+ for (final String edgeLabel : sortedEdgeLabels) {
+ result = 31 * result + Objects.hashCode(edgeLabel);
+ }
}
return result;
}
diff --git
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/SideEffectCapStep.java
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/SideEffectCapStep.java
index 337dbe4ce4..3e9d1c704e 100644
---
a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/SideEffectCapStep.java
+++
b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/SideEffectCapStep.java
@@ -28,10 +28,12 @@ import
org.apache.tinkerpop.gremlin.process.traversal.util.TraversalHelper;
import org.apache.tinkerpop.gremlin.structure.util.StringFactory;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.Set;
/**
@@ -56,11 +58,8 @@ public final class SideEffectCapStep<S, E> extends
SupplyingBarrierStep<S, E> {
@Override
public int hashCode() {
- int result = super.hashCode();
- for (final String sideEffectKey : this.sideEffectKeys) {
- result ^= sideEffectKey.hashCode();
- }
- return result;
+ // keys' order does not matter because cap("x", "y") and cap("y", "x")
should be considered equal.
+ return Objects.hash(super.hashCode(),
Arrays.hashCode(sideEffectKeys.stream().sorted().toArray()));
}
public List<String> getSideEffectKeys() {
diff --git
a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStepTest.java
b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStepTest.java
index 6ea38bc223..721cb5fd70 100644
---
a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStepTest.java
+++
b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/GraphStepTest.java
@@ -21,10 +21,17 @@ package
org.apache.tinkerpop.gremlin.process.traversal.step.map;
import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__;
import org.apache.tinkerpop.gremlin.process.traversal.step.StepTest;
+import org.junit.Test;
import java.util.Arrays;
import java.util.List;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasSize;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.not;
+
/**
* @author Daniel Kuppitz (http://gremlin.guru)
*/
@@ -38,4 +45,24 @@ public class GraphStepTest extends StepTest {
__.V(2,3)
);
}
+
+ /**
+ * We used to have an issue that because we use XOR of vertex ids, V() and
V("x", "x") are considered equal.
+ * The same 2 vertex ids "x" are reset as a result of XOR. See: <a
href="https://issues.apache.org/jira/browse/TINKERPOP-2423";>TINKERPOP-2423</a>
+ * This test verifies that the issue was fixed.
+ */
+ @Test
+ public void testCheckEqualityWithRedundantIds() {
+ final Traversal<?, ?> t0 = __.V();
+ final Traversal<?, ?> t1 = __.V(1, 1);
+
+ assertThat(t0.asAdmin().getSteps(), hasSize(1));
+ assertThat(t1.asAdmin().getSteps(), hasSize(1));
+ assertThat(t0.asAdmin().getSteps().get(0),
instanceOf(GraphStep.class));
+ assertThat(t1.asAdmin().getSteps().get(0),
instanceOf(GraphStep.class));
+
+ final GraphStep<?, ?> graphStep0 = (GraphStep<?, ?>)
t0.asAdmin().getSteps().get(0);
+ final GraphStep<?, ?> graphStep1 = (GraphStep<?, ?>)
t1.asAdmin().getSteps().get(0);
+ assertThat("V() and V(1,1) must not be considered equal", graphStep0,
not(equalTo(graphStep1)));
+ }
}
diff --git
a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertiesStepTest.java
b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertiesStepTest.java
index 61fea7607c..95e7f46d20 100644
---
a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertiesStepTest.java
+++
b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/PropertiesStepTest.java
@@ -21,10 +21,18 @@ package
org.apache.tinkerpop.gremlin.process.traversal.step.map;
import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__;
import org.apache.tinkerpop.gremlin.process.traversal.step.StepTest;
+import
org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.SideEffectCapStep;
+import org.junit.Test;
import java.util.Arrays;
import java.util.List;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasSize;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.not;
+
/**
* @author Daniel Kuppitz (http://gremlin.guru)
*/
@@ -43,4 +51,38 @@ public class PropertiesStepTest extends StepTest {
__.properties("name", "age")
);
}
+
+ /**
+ * We used to have an issue that because we use XOR of vertex ids,
properties("x") and properties("x", "y", "y") are considered equal.
+ * The same 2 property key "y" are reset as a result of XOR. See: <a
href="https://issues.apache.org/jira/browse/TINKERPOP-2423";>TINKERPOP-2423</a>
+ * This test verifies that the issue was fixed.
+ */
+ @Test
+ public void testCheckEqualityWithRedundantKeys() {
+ final Traversal<?, ?> t0 = __.properties("x");
+ final Traversal<?, ?> t1 = __.properties("x", "y", "y");
+
+ final Traversal<?, ?> t2 = __.values("x");
+ final Traversal<?, ?> t3 = __.values("x", "y", "y");
+
+ assertThat(t0.asAdmin().getSteps(), hasSize(1));
+ assertThat(t1.asAdmin().getSteps(), hasSize(1));
+ assertThat(t2.asAdmin().getSteps(), hasSize(1));
+ assertThat(t3.asAdmin().getSteps(), hasSize(1));
+
+ assertThat(t0.asAdmin().getSteps().get(0),
instanceOf(PropertiesStep.class));
+ assertThat(t1.asAdmin().getSteps().get(0),
instanceOf(PropertiesStep.class));
+ assertThat(t2.asAdmin().getSteps().get(0),
instanceOf(PropertiesStep.class));
+ assertThat(t3.asAdmin().getSteps().get(0),
instanceOf(PropertiesStep.class));
+
+ final PropertiesStep<?> propertiesStep0 = (PropertiesStep<?>)
t0.asAdmin().getSteps().get(0);
+ final PropertiesStep<?> propertiesStep1 = (PropertiesStep<?>)
t1.asAdmin().getSteps().get(0);
+ assertThat("properties(\"x\") and properties(\"x\",\"y\",\"y\") must
not be considered equal",
+ propertiesStep0, not(equalTo(propertiesStep1)));
+
+ final PropertiesStep<?> propertiesStep2 = (PropertiesStep<?>)
t0.asAdmin().getSteps().get(0);
+ final PropertiesStep<?> propertiesStep3 = (PropertiesStep<?>)
t1.asAdmin().getSteps().get(0);
+ assertThat("values(\"x\") and values(\"x\",\"y\",\"y\") must not be
considered equal",
+ propertiesStep2, not(equalTo(propertiesStep3)));
+ }
}
diff --git
a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/VertexStepTest.java
b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/VertexStepTest.java
index 6d8f6d225c..c83177cf81 100644
---
a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/VertexStepTest.java
+++
b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/VertexStepTest.java
@@ -21,10 +21,18 @@ package
org.apache.tinkerpop.gremlin.process.traversal.step.map;
import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__;
import org.apache.tinkerpop.gremlin.process.traversal.step.StepTest;
+import
org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect.SideEffectCapStep;
+import org.junit.Test;
import java.util.Arrays;
import java.util.List;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasSize;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.not;
+
/**
* @author Daniel Kuppitz (http://gremlin.guru)
*/
@@ -41,4 +49,24 @@ public class VertexStepTest extends StepTest {
__.out("knows", "created")
);
}
+
+ /**
+ * We used to have an issue that because we use XOR of edge labels, for
example out("x") and out("x", "y", "y") are considered equal.
+ * The same 2 sideEffectKey "y" are reset as a result of XOR. See: <a
href="https://issues.apache.org/jira/browse/TINKERPOP-2423";>TINKERPOP-2423</a>
+ * This test verifies that the issue was fixed.
+ */
+ @Test
+ public void testCheckEqualityWithRedundantEdgeLabels() {
+ final Traversal<?, ?> t0 = __.both("x");
+ final Traversal<?, ?> t1 = __.both("x", "y", "y");
+
+ assertThat(t0.asAdmin().getSteps(), hasSize(1));
+ assertThat(t1.asAdmin().getSteps(), hasSize(1));
+ assertThat(t0.asAdmin().getSteps().get(0),
instanceOf(VertexStep.class));
+ assertThat(t1.asAdmin().getSteps().get(0),
instanceOf(VertexStep.class));
+
+ final VertexStep<?> vertexStep0 = (VertexStep<?>)
t0.asAdmin().getSteps().get(0);
+ final VertexStep<?> vertexStep1 = (VertexStep<?>)
t1.asAdmin().getSteps().get(0);
+ assertThat("both(\"x\") and both(\"x\",\"y\",\"y\") must not be
considered equal", vertexStep0, not(equalTo(vertexStep1)));
+ }
}
diff --git
a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/SideEffectCapStepTest.java
b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/SideEffectCapStepTest.java
index 356459b2e0..c10b58d2e6 100644
---
a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/SideEffectCapStepTest.java
+++
b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/SideEffectCapStepTest.java
@@ -21,10 +21,18 @@ package
org.apache.tinkerpop.gremlin.process.traversal.step.sideEffect;
import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.__;
import org.apache.tinkerpop.gremlin.process.traversal.step.StepTest;
+import org.apache.tinkerpop.gremlin.process.traversal.step.map.GraphStep;
+import org.junit.Test;
import java.util.Arrays;
import java.util.List;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasSize;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.not;
+
/**
* @author Daniel Kuppitz (http://gremlin.guru)
*/
@@ -38,4 +46,24 @@ public class SideEffectCapStepTest extends StepTest {
__.cap("x", "y")
);
}
+
+ /**
+ * We used to have an issue that because we use XOR of vertex ids,
cap("x") and V("x", "y", "y") are considered equal.
+ * The same 2 sideEffectKey "y" are reset as a result of XOR. See: <a
href="https://issues.apache.org/jira/browse/TINKERPOP-2423";>TINKERPOP-2423</a>
+ * This test verifies that the issue was fixed.
+ */
+ @Test
+ public void testCheckEqualityWithRedundantKeys() {
+ final Traversal<?, ?> t0 = __.cap("x");
+ final Traversal<?, ?> t1 = __.cap("x", "y", "y");
+
+ assertThat(t0.asAdmin().getSteps(), hasSize(1));
+ assertThat(t1.asAdmin().getSteps(), hasSize(1));
+ assertThat(t0.asAdmin().getSteps().get(0),
instanceOf(SideEffectCapStep.class));
+ assertThat(t1.asAdmin().getSteps().get(0),
instanceOf(SideEffectCapStep.class));
+
+ final SideEffectCapStep<?, ?> sideEffectCapStep0 =
(SideEffectCapStep<?, ?>) t0.asAdmin().getSteps().get(0);
+ final SideEffectCapStep<?, ?> sideEffectCapStep1 =
(SideEffectCapStep<?, ?>) t1.asAdmin().getSteps().get(0);
+ assertThat("cap(\"x\") and cap(\"x\",\"y\",\"y\") must not be
considered equal", sideEffectCapStep0, not(equalTo(sideEffectCapStep1)));
+ }
}
