This is an automated email from the ASF dual-hosted git repository. spmallette pushed a commit to branch TINKERPOP-2507 in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit e288984a38935a92283e73d38d647156c3845624 Author: Stephen Mallette <[email protected]> AuthorDate: Thu Jun 3 10:46:38 2021 -0400 TINKERPOP-2507 Allowed graph filters with mixed id types Upgrade docs in this commit explain this change fairly well. There doesn't seem to be a need to be so prescriptive about the types that can be filtered on and it should be up to each graph to decide how they handle such input. Removed the general exception, tests and updated the behavior of graphs to support the change. --- CHANGELOG.asciidoc | 2 + docs/src/upgrade/release-3.6.x.asciidoc | 43 ++++++++ .../apache/tinkerpop/gremlin/structure/Graph.java | 4 - .../gremlin/structure/util/ElementHelper.java | 22 +--- .../util/iterator/AbortiveMultiIterator.java | 120 +++++++++++++++++++++ ...torTest.java => AbortiveMultiIteratorTest.java} | 83 +++++++++++--- .../gremlin/util/iterator/MultiIteratorTest.java | 11 +- .../process/traversal/CoreTraversalTest.java | 9 +- .../tinkerpop/gremlin/structure/GraphTest.java | 68 ------------ .../gremlin/hadoop/structure/HadoopGraph.java | 62 ++++++----- .../gremlin/neo4j/structure/Neo4jGraph.java | 2 - .../gremlin/tinkergraph/structure/TinkerGraph.java | 31 ++---- .../structure/TinkerGraphIdManagerTest.java | 6 +- .../tinkergraph/structure/TinkerGraphTest.java | 19 ++++ 14 files changed, 305 insertions(+), 177 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 48438e9..2015e76 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -24,6 +24,7 @@ limitations under the License. === TinkerPop 3.6.0 (Release Date: NOT OFFICIALLY RELEASED YET) * `TraversalOpProcessor` no longer accepts a `String` representation of `Bytecode` for the "gremlin" argument which was left to support older versions of the drivers. +* Removed requirement that "ids" used to filter vertices and edges need to be all of a single type. == TinkerPop 3.5.0 (The Sleeping Gremlin: No. 18 Entr'acte Symphonique) @@ -364,6 +365,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima [[release-3-4-12]] === TinkerPop 3.4.12 (Release Date: NOT OFFICIALLY RELEASED YET) +* Changed TinkerGraph to allow identifiers to be heterogeneous when filtering. * Fixed bug in the `vertexLabelKey` validation for `GraphMLWriter` which was inadvertently validating the `edgeLabelKey`. * Changed `IndexStep` to make it easier for providers to determine the type of indexer being used. diff --git a/docs/src/upgrade/release-3.6.x.asciidoc b/docs/src/upgrade/release-3.6.x.asciidoc new file mode 100644 index 0000000..712c4f4 --- /dev/null +++ b/docs/src/upgrade/release-3.6.x.asciidoc @@ -0,0 +1,43 @@ +//// +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +//// + += TinkerPop 3.6.0 + +*NOT NAMED YET* + +== TinkerPop 3.6.0 + +*Release Date: NOT OFFICIALLY RELEASED YET* + +Please see the link:https://github.com/apache/tinkerpop/blob/3.6.0/CHANGELOG.asciidoc#release-3-6-0[changelog] for a complete list of all the modifications that are part of this release. + +=== Upgrading for Users + + +=== Upgrading for Providers + +==== Graph System Providers + +===== Filters with Mixed Id Types + +The requirement that "ids" passed to `Graph.vertices` and `Graph.edges` all be of a single type has been removed. This +requirement was a bit to prescriptive when there really wasn't a need to enforce such a validation. It even conflicted +with TinkerGraph operations where mixed `T.id` types is a feature. Graph providers may continue to support this +requirement if they wish, but it is no longer enforced by TinkerPop and the `Graph.idArgsMustBeEitherIdOrElement` has +been removed so providers will need to construct their own exception. + +See: link:https://issues.apache.org/jira/browse/TINKERPOP-2507[TINKERPOP-2507] \ No newline at end of file diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Graph.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Graph.java index da1c998..a8131c1 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Graph.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/Graph.java @@ -1228,10 +1228,6 @@ public interface Graph extends AutoCloseable, Host { return new IllegalArgumentException(String.format("Edge with id already exists: %s", id)); } - public static IllegalArgumentException idArgsMustBeEitherIdOrElement() { - return new IllegalArgumentException("id arguments must be either ids or Elements"); - } - public static IllegalArgumentException argumentCanNotBeNull(final String argument) { return new IllegalArgumentException(String.format("The provided argument can not be null: %s", argument)); } diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/ElementHelper.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/ElementHelper.java index 6ba5b29..9f852ef 100644 --- a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/ElementHelper.java +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/util/ElementHelper.java @@ -67,23 +67,6 @@ public final class ElementHelper { } /** - * Determine whether an array of ids are either all elements or ids of elements. This is typically used as a pre-condition check. - * - * @param clazz the class of the element for which the ids will bind - * @param ids the ids that must be either elements or id objects, else - * {@link org.apache.tinkerpop.gremlin.structure.Graph.Exceptions#idArgsMustBeEitherIdOrElement()} is thrown. - */ - public static void validateMixedElementIds(final Class<? extends Element> clazz, final Object... ids) throws IllegalArgumentException { - if (ids.length > 1) { - final boolean element = clazz.isAssignableFrom(ids[0].getClass()); - for (int i = 1; i < ids.length; i++) { - if (clazz.isAssignableFrom(ids[i].getClass()) != element) - throw Graph.Exceptions.idArgsMustBeEitherIdOrElement(); - } - } - } - - /** * Determines whether the property key/value for the specified thing can be legally set. This is typically used as * a pre-condition check prior to setting a property. * @@ -557,8 +540,9 @@ public final class ElementHelper { // it is OK to evaluate equality of ids via toString() now given that the toString() the test suite // enforces the value of id.()toString() to be a first class representation of the identifier - if (1 == providedIds.length) return id.toString().equals(providedIds[0].toString()); - else { + if (1 == providedIds.length) { + return id.toString().equals(providedIds[0].toString()); + } else { for (final Object temp : providedIds) { if (temp.toString().equals(id.toString())) return true; diff --git a/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/util/iterator/AbortiveMultiIterator.java b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/util/iterator/AbortiveMultiIterator.java new file mode 100644 index 0000000..2235822 --- /dev/null +++ b/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/util/iterator/AbortiveMultiIterator.java @@ -0,0 +1,120 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.util.iterator; + +import org.apache.tinkerpop.gremlin.process.traversal.util.FastNoSuchElementException; +import org.apache.tinkerpop.gremlin.structure.util.CloseableIterator; + +import java.io.Serializable; +import java.util.ArrayList; +import java.util.Iterator; +import java.util.List; +import java.util.function.Predicate; + +/** + * An {@code Iterator} that checks a {@code Predicate} prior to processing the specified child {@code Iterator} + * instances. + */ +public final class AbortiveMultiIterator<T> implements Iterator<T>, Serializable, AutoCloseable { + + private final List<Iterator<T>> iterators = new ArrayList<>(); + private final List<Predicate<Long>> checks = new ArrayList<>(); + private long counter = 0; + private int current = 0; + + /** + * Adds an {@code Iterator} that will always be iterated. + */ + public void addIterator(final Iterator<T> iterator) { + addIterator(iterator, c -> true); + } + + /** + * Adds an {@code Iterator} that will iterate only if the {@code check} passes. + * + * @param check when returning {@code true} processing of the associated {@code Iterator} will proceed + */ + public void addIterator(final Iterator<T> iterator, final Predicate<Long> check) { + this.iterators.add(iterator); + this.checks.add(check); + } + + @Override + public boolean hasNext() { + if (this.current >= this.iterators.size()) + return false; + + Iterator<T> currentIterator = this.iterators.get(this.current); + + while (true) { + if (currentIterator.hasNext()) { + return true; + } else { + this.current++; + if (this.current >= iterators.size() || !checks.get(current).test(counter)) + break; + currentIterator = iterators.get(this.current); + } + } + return false; + } + + @Override + public void remove() { + this.iterators.get(this.current).remove(); + } + + @Override + public T next() { + if (this.iterators.isEmpty()) throw FastNoSuchElementException.instance(); + + Iterator<T> currentIterator = iterators.get(this.current); + + while (true) { + if (currentIterator.hasNext()) { + this.counter++; + return currentIterator.next(); + } else { + this.current++; + if (this.current >= iterators.size() || !checks.get(current).test(counter)) + break; + currentIterator = iterators.get(current); + } + } + throw FastNoSuchElementException.instance(); + } + + public void clear() { + this.iterators.clear(); + this.checks.clear(); + this.counter = 0; + this.current = 0; + } + + /** + * Close the underlying iterators if auto-closeable. Note that when Exception is thrown from any iterator + * in the for loop on closing, remaining iterators possibly left unclosed. + */ + @Override + public void close() { + for (Iterator<T> iterator : this.iterators) { + CloseableIterator.closeIterator(iterator); + } + } +} diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/util/iterator/MultiIteratorTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/util/iterator/AbortiveMultiIteratorTest.java similarity index 61% copy from gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/util/iterator/MultiIteratorTest.java copy to gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/util/iterator/AbortiveMultiIteratorTest.java index 4b97299..36637da 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/util/iterator/MultiIteratorTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/util/iterator/AbortiveMultiIteratorTest.java @@ -20,6 +20,7 @@ package org.apache.tinkerpop.gremlin.util.iterator; import org.apache.tinkerpop.gremlin.process.traversal.util.FastNoSuchElementException; import org.junit.Test; + import java.util.ArrayList; import java.util.Iterator; import java.util.List; @@ -27,52 +28,51 @@ import java.util.List; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; /** * @author Stephen Mallette (http://stephen.genoprime.com) */ -public class MultiIteratorTest { +public class AbortiveMultiIteratorTest { @Test public void shouldNotHaveNextIfNoIteratorsAreAdded() { - final Iterator<String> itty = new MultiIterator<>(); - assertFalse(itty.hasNext()); + final Iterator<String> itty = new AbortiveMultiIterator<>(); + assertThat(itty.hasNext(), is(false)); } @Test(expected = FastNoSuchElementException.class) public void shouldThrowFastNoSuchElementExceptionIfNoIteratorsAreAdded() { - final Iterator<String> itty = new MultiIterator<>(); + final Iterator<String> itty = new AbortiveMultiIterator<>(); itty.next(); } @Test public void shouldNotHaveNextIfEmptyIteratorIsAdded() { - final MultiIterator<String> itty = new MultiIterator<>(); + final AbortiveMultiIterator<String> itty = new AbortiveMultiIterator<>(); itty.addIterator(EmptyIterator.instance()); - assertFalse(itty.hasNext()); + assertThat(itty.hasNext(), is(false)); } @Test(expected = FastNoSuchElementException.class) public void shouldThrowFastNoSuchElementExceptionIfEmptyIteratorIsAdded() { - final MultiIterator<String> itty = new MultiIterator<>(); + final AbortiveMultiIterator<String> itty = new AbortiveMultiIterator<>(); itty.addIterator(EmptyIterator.instance()); itty.next(); } @Test public void shouldNotHaveNextIfEmptyIteratorsAreAdded() { - final MultiIterator<String> itty = new MultiIterator<>(); + final AbortiveMultiIterator<String> itty = new AbortiveMultiIterator<>(); itty.addIterator(EmptyIterator.instance()); itty.addIterator(EmptyIterator.instance()); itty.addIterator(EmptyIterator.instance()); itty.addIterator(EmptyIterator.instance()); - assertFalse(itty.hasNext()); + assertThat(itty.hasNext(), is(false)); } @Test(expected = FastNoSuchElementException.class) public void shouldThrowFastNoSuchElementExceptionIfEmptyIteratorsAreAdded() { - final MultiIterator<String> itty = new MultiIterator<>(); + final AbortiveMultiIterator<String> itty = new AbortiveMultiIterator<>(); itty.addIterator(EmptyIterator.instance()); itty.addIterator(EmptyIterator.instance()); itty.addIterator(EmptyIterator.instance()); @@ -87,7 +87,7 @@ public class MultiIteratorTest { list.add("test2"); list.add("test3"); - final MultiIterator<String> itty = new MultiIterator<>(); + final AbortiveMultiIterator<String> itty = new AbortiveMultiIterator<>(); itty.addIterator(EmptyIterator.instance()); itty.addIterator(list.iterator()); @@ -95,7 +95,58 @@ public class MultiIteratorTest { assertEquals("test1", itty.next()); assertEquals("test2", itty.next()); assertEquals("test3", itty.next()); - assertFalse(itty.hasNext()); + assertThat(itty.hasNext(), is(false)); + } + + @Test + public void shouldAbortIteration() { + final List<String> list1 = new ArrayList<>(); + list1.add("test1"); + list1.add("test2"); + list1.add("test3"); + + final List<String> list2 = new ArrayList<>(); + list2.add("test4"); + list2.add("test5"); + list2.add("test6"); + + final AbortiveMultiIterator<String> itty = new AbortiveMultiIterator<>(); + itty.addIterator(EmptyIterator.instance()); + itty.addIterator(list1.iterator()); + itty.addIterator(list2.iterator(), c -> c != 3); + + assertThat(itty.hasNext(), is(true)); + assertEquals("test1", itty.next()); + assertEquals("test2", itty.next()); + assertEquals("test3", itty.next()); + assertThat(itty.hasNext(), is(false)); + } + + @Test + public void shouldAllowFullIteration() { + final List<String> list1 = new ArrayList<>(); + list1.add("test1"); + list1.add("test2"); + list1.add("test3"); + + final List<String> list2 = new ArrayList<>(); + list2.add("test4"); + list2.add("test5"); + list2.add("test6"); + + final AbortiveMultiIterator<String> itty = new AbortiveMultiIterator<>(); + itty.addIterator(EmptyIterator.instance()); + itty.addIterator(list1.iterator()); + itty.addIterator(list2.iterator(), c -> c == 3); + + assertThat(itty.hasNext(), is(true)); + assertEquals("test1", itty.next()); + assertEquals("test2", itty.next()); + assertEquals("test3", itty.next()); + assertEquals("test4", itty.next()); + assertEquals("test5", itty.next()); + assertEquals("test6", itty.next()); + assertThat(itty.hasNext(), is(false)); } @Test @@ -105,18 +156,18 @@ public class MultiIteratorTest { list.add("test2"); list.add("test3"); - final MultiIterator<String> itty = new MultiIterator<>(); + final AbortiveMultiIterator<String> itty = new AbortiveMultiIterator<>(); itty.addIterator(list.iterator()); itty.clear(); - assertFalse(itty.hasNext()); + assertThat(itty.hasNext(), is(false)); } @Test public void shouldCloseIterators() { - final MultiIterator<String> itty = new MultiIterator<>(); + final AbortiveMultiIterator<String> itty = new AbortiveMultiIterator<>(); final DummyAutoCloseableIterator<String> inner1 = new DummyAutoCloseableIterator<>(); final DummyAutoCloseableIterator<String> inner2 = new DummyAutoCloseableIterator<>(); itty.addIterator(inner1); diff --git a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/util/iterator/MultiIteratorTest.java b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/util/iterator/MultiIteratorTest.java index 4b97299..ab3ba01 100644 --- a/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/util/iterator/MultiIteratorTest.java +++ b/gremlin-core/src/test/java/org/apache/tinkerpop/gremlin/util/iterator/MultiIteratorTest.java @@ -27,7 +27,6 @@ import java.util.List; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; /** @@ -37,7 +36,7 @@ public class MultiIteratorTest { @Test public void shouldNotHaveNextIfNoIteratorsAreAdded() { final Iterator<String> itty = new MultiIterator<>(); - assertFalse(itty.hasNext()); + assertThat(itty.hasNext(), is(false)); } @Test(expected = FastNoSuchElementException.class) @@ -50,7 +49,7 @@ public class MultiIteratorTest { public void shouldNotHaveNextIfEmptyIteratorIsAdded() { final MultiIterator<String> itty = new MultiIterator<>(); itty.addIterator(EmptyIterator.instance()); - assertFalse(itty.hasNext()); + assertThat(itty.hasNext(), is(false)); } @Test(expected = FastNoSuchElementException.class) @@ -67,7 +66,7 @@ public class MultiIteratorTest { itty.addIterator(EmptyIterator.instance()); itty.addIterator(EmptyIterator.instance()); itty.addIterator(EmptyIterator.instance()); - assertFalse(itty.hasNext()); + assertThat(itty.hasNext(), is(false)); } @Test(expected = FastNoSuchElementException.class) @@ -95,7 +94,7 @@ public class MultiIteratorTest { assertEquals("test1", itty.next()); assertEquals("test2", itty.next()); assertEquals("test3", itty.next()); - assertFalse(itty.hasNext()); + assertThat(itty.hasNext(), is(false)); } @Test @@ -110,7 +109,7 @@ public class MultiIteratorTest { itty.clear(); - assertFalse(itty.hasNext()); + assertThat(itty.hasNext(), is(false)); } @Test 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 7fbc8b5..356677c 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 @@ -169,13 +169,10 @@ public class CoreTraversalTest extends AbstractGremlinProcessTest { @Test @LoadGraphWith(MODERN) - public void shouldThrowExceptionWhenIdsMixed() { + public void shouldAllowIdsOfMixedTypes() { final List<Vertex> vertices = g.V().toList(); - try { - g.V(vertices.get(0), vertices.get(1).id()).toList(); - } catch (Exception ex) { - validateException(Graph.Exceptions.idArgsMustBeEitherIdOrElement(), ex); - } + assertEquals(2, g.V(vertices.get(0), vertices.get(1).id()).count().next().intValue()); + assertEquals(2, g.V(vertices.get(0).id(), vertices.get(1)).count().next().intValue()); } @Test diff --git a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/GraphTest.java b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/GraphTest.java index 95d5adc..6d2cb35 100644 --- a/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/GraphTest.java +++ b/gremlin-test/src/main/java/org/apache/tinkerpop/gremlin/structure/GraphTest.java @@ -701,38 +701,6 @@ public class GraphTest extends AbstractGremlinTest { @Test @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_ADD_VERTICES) - @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_ANY_IDS) - @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_STRING_IDS) - public void shouldNotMixTypesForGettingSpecificVerticesWithVertexFirst() { - final Vertex v1 = graph.addVertex(); - try { - graph.vertices(v1, graphProvider.convertId("1", Vertex.class)); - fail("Should have thrown an exception because id arguments were mixed."); - } catch (Exception ex) { - final Exception expected = Graph.Exceptions.idArgsMustBeEitherIdOrElement(); - assertEquals(expected.getClass(), ex.getClass()); - assertEquals(expected.getMessage(), ex.getMessage()); - } - } - - @Test - @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_ADD_VERTICES) - @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_ANY_IDS) - @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_STRING_IDS) - public void shouldNotMixTypesForGettingSpecificVerticesWithStringFirst() { - final Vertex v1 = graph.addVertex(); - try { - graph.vertices(graphProvider.convertId("1", Vertex.class), v1); - fail("Should have thrown an exception because id arguments were mixed."); - } catch (Exception ex) { - final Exception expected = Graph.Exceptions.idArgsMustBeEitherIdOrElement(); - assertEquals(expected.getClass(), ex.getClass()); - assertEquals(expected.getMessage(), ex.getMessage()); - } - } - - @Test - @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_ADD_VERTICES) @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_STRING_IDS) public void shouldIterateVerticesWithStringIdSupportUsingVertex() { // if the graph supports id assigned, it should allow it. if the graph does not, it will generate one @@ -1645,42 +1613,6 @@ public class GraphTest extends AbstractGremlinTest { @Test @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_ADD_VERTICES) @FeatureRequirement(featureClass = Graph.Features.EdgeFeatures.class, feature = Graph.Features.EdgeFeatures.FEATURE_ADD_EDGES) - @FeatureRequirement(featureClass = Graph.Features.EdgeFeatures.class, feature = Graph.Features.EdgeFeatures.FEATURE_ANY_IDS) - @FeatureRequirement(featureClass = Graph.Features.EdgeFeatures.class, feature = Graph.Features.EdgeFeatures.FEATURE_STRING_IDS) - public void shouldNotMixTypesForGettingSpecificEdgesWithEdgeFirst() { - final Vertex v = graph.addVertex(); - final Edge e1 = v.addEdge("self", v); - try { - graph.edges(e1, graphProvider.convertId("1", Edge.class)); - fail("Should have thrown an exception because id arguments were mixed."); - } catch (Exception ex) { - final Exception expected = Graph.Exceptions.idArgsMustBeEitherIdOrElement(); - assertEquals(expected.getClass(), ex.getClass()); - assertEquals(expected.getMessage(), ex.getMessage()); - } - } - - @Test - @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_ADD_VERTICES) - @FeatureRequirement(featureClass = Graph.Features.EdgeFeatures.class, feature = Graph.Features.EdgeFeatures.FEATURE_ADD_EDGES) - @FeatureRequirement(featureClass = Graph.Features.EdgeFeatures.class, feature = Graph.Features.EdgeFeatures.FEATURE_ANY_IDS) - @FeatureRequirement(featureClass = Graph.Features.EdgeFeatures.class, feature = Graph.Features.EdgeFeatures.FEATURE_STRING_IDS) - public void shouldNotMixTypesForGettingSpecificEdgesWithStringFirst() { - final Vertex v = graph.addVertex(); - final Edge e1 = v.addEdge("self", v); - try { - graph.edges(graphProvider.convertId("1", Edge.class), e1); - fail("Should have thrown an exception because id arguments were mixed."); - } catch (Exception ex) { - final Exception expected = Graph.Exceptions.idArgsMustBeEitherIdOrElement(); - assertEquals(expected.getClass(), ex.getClass()); - assertEquals(expected.getMessage(), ex.getMessage()); - } - } - - @Test - @FeatureRequirement(featureClass = Graph.Features.VertexFeatures.class, feature = Graph.Features.VertexFeatures.FEATURE_ADD_VERTICES) - @FeatureRequirement(featureClass = Graph.Features.EdgeFeatures.class, feature = Graph.Features.EdgeFeatures.FEATURE_ADD_EDGES) @FeatureRequirement(featureClass = Graph.Features.EdgeFeatures.class, feature = Graph.Features.EdgeFeatures.FEATURE_STRING_IDS) public void shouldIterateEdgesWithStringIdSupportUsingEdge() { // if the graph supports id assigned, it should allow it. if the graph does not, it will generate one diff --git a/hadoop-gremlin/src/main/java/org/apache/tinkerpop/gremlin/hadoop/structure/HadoopGraph.java b/hadoop-gremlin/src/main/java/org/apache/tinkerpop/gremlin/hadoop/structure/HadoopGraph.java index f4722ea..5a2578b 100644 --- a/hadoop-gremlin/src/main/java/org/apache/tinkerpop/gremlin/hadoop/structure/HadoopGraph.java +++ b/hadoop-gremlin/src/main/java/org/apache/tinkerpop/gremlin/hadoop/structure/HadoopGraph.java @@ -35,14 +35,20 @@ import org.apache.tinkerpop.gremlin.structure.Transaction; import org.apache.tinkerpop.gremlin.structure.Vertex; import org.apache.tinkerpop.gremlin.structure.util.ElementHelper; import org.apache.tinkerpop.gremlin.structure.util.StringFactory; +import org.apache.tinkerpop.gremlin.util.iterator.AbortiveMultiIterator; import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.IOException; +import java.util.ArrayList; import java.util.Iterator; +import java.util.List; import java.util.Optional; +import java.util.Spliterator; +import java.util.Spliterators; import java.util.stream.Stream; +import java.util.stream.StreamSupport; /** * @author Marko A. Rodriguez (http://markorodriguez.com) @@ -257,22 +263,18 @@ public final class HadoopGraph implements Graph { if (0 == vertexIds.length) { return new HadoopVertexIterator(this); } else { - // base the conversion function on the first item in the id list as the expectation is that these - // id values will be a uniform list - if (vertexIds[0] instanceof Vertex) { - // based on the first item assume all vertices in the argument list - if (!Stream.of(vertexIds).allMatch(id -> id instanceof Vertex)) - throw Graph.Exceptions.idArgsMustBeEitherIdOrElement(); - - // no need to get the vertices again, so just flip it back - some implementation may want to treat this - // as a refresh operation. that's not necessary for hadoopgraph. - return Stream.of(vertexIds).map(id -> (Vertex) id).iterator(); - } else { - final Class<?> firstClass = vertexIds[0].getClass(); - if (!Stream.of(vertexIds).map(Object::getClass).allMatch(firstClass::equals)) - throw Graph.Exceptions.idArgsMustBeEitherIdOrElement(); // todo: change exception to be ids of the same type - return IteratorUtils.filter(new HadoopVertexIterator(this), vertex -> ElementHelper.idExists(vertex.id(), vertexIds)); - } + final Stream<Vertex> idsThatWereVertices = Stream.of(vertexIds). + filter(id -> id instanceof Vertex).map(id -> (Vertex) id); + final Stream<Vertex> verticesFromHadoopGraph = StreamSupport.stream(Spliterators.spliteratorUnknownSize( + IteratorUtils.filter(new HadoopVertexIterator(this), + vertex -> ElementHelper.idExists(vertex.id(), vertexIds)), Spliterator.ORDERED), false); + + // if the vertexIds are all Vertex objects then abort the iteration of the full HadoopGraph as there + // is no need to refresh the data in this use case as other graphs might + final AbortiveMultiIterator<Vertex> iterator = new AbortiveMultiIterator<>(); + iterator.addIterator(idsThatWereVertices.iterator()); + iterator.addIterator(verticesFromHadoopGraph.iterator(), c -> c != vertexIds.length); + return iterator; } } catch (final IOException e) { throw new IllegalStateException(e.getMessage(), e); @@ -285,22 +287,18 @@ public final class HadoopGraph implements Graph { if (0 == edgeIds.length) { return new HadoopEdgeIterator(this); } else { - // base the conversion function on the first item in the id list as the expectation is that these - // id values will be a uniform list - if (edgeIds[0] instanceof Edge) { - // based on the first item assume all Edges in the argument list - if (!Stream.of(edgeIds).allMatch(id -> id instanceof Edge)) - throw Graph.Exceptions.idArgsMustBeEitherIdOrElement(); - - // no need to get the vertices again, so just flip it back - some implementation may want to treat this - // as a refresh operation. that's not necessary for hadoopgraph. - return Stream.of(edgeIds).map(id -> (Edge) id).iterator(); - } else { - final Class<?> firstClass = edgeIds[0].getClass(); - if (!Stream.of(edgeIds).map(Object::getClass).allMatch(firstClass::equals)) - throw Graph.Exceptions.idArgsMustBeEitherIdOrElement(); // todo: change exception to be ids of the same type - return IteratorUtils.filter(new HadoopEdgeIterator(this), vertex -> ElementHelper.idExists(vertex.id(), edgeIds)); - } + final Stream<Edge> idsThatWereEdges = Stream.of(edgeIds). + filter(id -> id instanceof Edge).map(id -> (Edge) id); + final Stream<Edge> edgesFromHadoopGraph = StreamSupport.stream(Spliterators.spliteratorUnknownSize( + IteratorUtils.filter(new HadoopEdgeIterator(this), + edge -> ElementHelper.idExists(edge.id(), edgeIds)), Spliterator.ORDERED), false); + + // if the edgeIds are all Edge objects then abort the iteration of the full HadoopGraph as there + // is no need to refresh the data in this use case as other graphs might + final AbortiveMultiIterator<Edge> iterator = new AbortiveMultiIterator<>(); + iterator.addIterator(idsThatWereEdges.iterator()); + iterator.addIterator(edgesFromHadoopGraph.iterator(), c -> c != edgeIds.length); + return iterator; } } catch (final IOException e) { throw new IllegalStateException(e.getMessage(), e); diff --git a/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/structure/Neo4jGraph.java b/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/structure/Neo4jGraph.java index 55be5f6..d62a81e 100644 --- a/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/structure/Neo4jGraph.java +++ b/neo4j-gremlin/src/main/java/org/apache/tinkerpop/gremlin/neo4j/structure/Neo4jGraph.java @@ -143,7 +143,6 @@ public final class Neo4jGraph implements Graph, WrappedGraph<Neo4jGraphAPI> { return IteratorUtils.stream(this.getBaseGraph().allNodes()) .map(node -> (Vertex) new Neo4jVertex(node, this)).iterator(); } else { - ElementHelper.validateMixedElementIds(Vertex.class, vertexIds); return Stream.of(vertexIds) .map(id -> { if (id instanceof Number) @@ -174,7 +173,6 @@ public final class Neo4jGraph implements Graph, WrappedGraph<Neo4jGraphAPI> { return IteratorUtils.stream(this.getBaseGraph().allRelationships()) .map(relationship -> (Edge) new Neo4jEdge(relationship, this)).iterator(); } else { - ElementHelper.validateMixedElementIds(Edge.class, edgeIds); return Stream.of(edgeIds) .map(id -> { if (id instanceof Number) diff --git a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java index 6f4f821..2795ae0 100644 --- a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java +++ b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraph.java @@ -305,18 +305,18 @@ public final class TinkerGraph implements Graph { final Object... ids) { final Iterator<T> iterator; if (0 == ids.length) { - iterator = new TinkerGraphIterator<T>(elements.values().iterator()); + iterator = new TinkerGraphIterator<>(elements.values().iterator()); } else { final List<Object> idList = Arrays.asList(ids); - validateHomogenousIds(idList); - // if the type is of Element - have to look each up because it might be an Attachable instance or - // other implementation. the assumption is that id conversion is not required for detached - // stuff - doesn't seem likely someone would detach a Titan vertex then try to expect that - // vertex to be findable in OrientDB - return clazz.isAssignableFrom(ids[0].getClass()) ? - new TinkerGraphIterator<T>(IteratorUtils.filter(IteratorUtils.map(idList, id -> elements.get(clazz.cast(id).id())).iterator(), Objects::nonNull)) - : new TinkerGraphIterator<T>(IteratorUtils.filter(IteratorUtils.map(idList, id -> elements.get(idManager.convert(id))).iterator(), Objects::nonNull)); + // TinkerGraph can take a Vertex/Edge or any object as an "id". If it is an Element then we just cast + // to that type and pop off the identifier. there is no need to pass that through the IdManager since + // the assumption is that if it's already an Element, its identifier must be valid to the Graph and to + // its associated IdManager. All other objects are passed to the IdManager for conversion. + return new TinkerGraphIterator<>(IteratorUtils.filter(IteratorUtils.map(idList, id -> { + final Object iid = clazz.isAssignableFrom(id.getClass()) ? clazz.cast(id).id() : idManager.convert(id); + return elements.get(idManager.convert(iid)); + }).iterator(), Objects::nonNull)); } return TinkerHelper.inComputerMode(this) ? (Iterator<T>) (clazz.equals(Vertex.class) ? @@ -336,19 +336,6 @@ public final class TinkerGraph implements Graph { return features; } - private void validateHomogenousIds(final List<Object> ids) { - final Iterator<Object> iterator = ids.iterator(); - Object id = iterator.next(); - if (id == null) - throw Graph.Exceptions.idArgsMustBeEitherIdOrElement(); - final Class firstClass = id.getClass(); - while (iterator.hasNext()) { - id = iterator.next(); - if (id == null || !id.getClass().equals(firstClass)) - throw Graph.Exceptions.idArgsMustBeEitherIdOrElement(); - } - } - public class TinkerGraphFeatures implements Features { private final TinkerGraphGraphFeatures graphFeatures = new TinkerGraphGraphFeatures(); diff --git a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphIdManagerTest.java b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphIdManagerTest.java index e589c5c..8dd2e12 100644 --- a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphIdManagerTest.java +++ b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerGraphIdManagerTest.java @@ -54,7 +54,8 @@ public class TinkerGraphIdManagerTest { {"coerceInt", 100, 200, 300}, {"coerceDouble", 100d, 200d, 300d}, {"coerceFloat", 100f, 200f, 300f}, - {"coerceString", "100", "200", "300"}}); + {"coerceString", "100", "200", "300"}, + {"coerceMixed", 100d, 200f, "300"}}); } @Parameterized.Parameter(value = 0) @@ -120,7 +121,8 @@ public class TinkerGraphIdManagerTest { public static Iterable<Object[]> data() { return Arrays.asList(new Object[][]{ {"coerceUuid", vertexId, edgeId, vertexPropertyId}, - {"coerceString", vertexId.toString(), edgeId.toString(), vertexPropertyId.toString()}}); + {"coerceString", vertexId.toString(), edgeId.toString(), vertexPropertyId.toString()}, + {"coerceMixed", vertexId, edgeId, vertexPropertyId.toString()}}); } @Parameterized.Parameter(value = 0) 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 b78dfc1..7ecc134 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 @@ -76,6 +76,7 @@ import java.util.List; import java.util.Map; import java.util.Random; import java.util.Set; +import java.util.UUID; import java.util.concurrent.TimeUnit; import java.util.function.Consumer; import java.util.function.Supplier; @@ -879,6 +880,24 @@ public class TinkerGraphTest { assertEquals(3, traversal().withEmbedded(graph).V().hasLabel("person").count().next().intValue()); } + @Test + public void shouldAllowHeterogeneousIdsWithAnyManager() { + final Configuration anyManagerConfig = new BaseConfiguration(); + anyManagerConfig.addProperty(TinkerGraph.GREMLIN_TINKERGRAPH_EDGE_ID_MANAGER, TinkerGraph.DefaultIdManager.ANY.name()); + anyManagerConfig.addProperty(TinkerGraph.GREMLIN_TINKERGRAPH_VERTEX_ID_MANAGER, TinkerGraph.DefaultIdManager.ANY.name()); + anyManagerConfig.addProperty(TinkerGraph.GREMLIN_TINKERGRAPH_VERTEX_PROPERTY_ID_MANAGER, TinkerGraph.DefaultIdManager.ANY.name()); + final Graph graph = TinkerGraph.open(anyManagerConfig); + final GraphTraversalSource g = traversal().withEmbedded(graph); + + final UUID uuid = UUID.fromString("0E939658-ADD2-4598-A722-2FC178E9B741"); + g.addV("person").property(T.id, 100). + addV("person").property(T.id, "1000"). + addV("person").property(T.id, "1001"). + addV("person").property(T.id, uuid).iterate(); + + assertEquals(3, g.V(100, "1000", uuid).count().next().intValue()); + } + /** * Coerces a {@code Color} to a {@link TinkerGraph} during serialization. Demonstrates how custom serializers * can be developed that can coerce one value to another during serialization.
