This is an automated email from the ASF dual-hosted git repository. spmallette pushed a commit to branch TINKERPOP-3214 in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 6290b41905ae6349fc5bde1aa6c509ef69389c1e Author: Stephen Mallette <[email protected]> AuthorDate: Mon Mar 16 11:08:32 2026 -0400 TINKERPOP-3214 Fixed bug in VertexProperty id generation Prevented duplicate identifiers by using the same pattern for tracking vertex properties as is used for vertices and edges. --- CHANGELOG.asciidoc | 1 + .../tinkergraph/structure/AbstractTinkerGraph.java | 25 +++-- .../tinkergraph/structure/TinkerVertex.java | 3 + .../structure/TinkerVertexProperty.java | 1 + .../structure/TinkerGraphIdManagerTest.java | 116 +++++++++++++++++++++ 5 files changed, 138 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 555e70a604..dd9c116ab6 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -26,6 +26,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima === TinkerPop 3.7.6 (NOT OFFICIALLY RELEASED YET) * Integrated Python driver examples into automated build process to ensure examples remain functional. +* Fixed bug in `IdManager` for `VertexProperty` id creation to better ensure identifier uniqueness. * Fixed bug in `SubgraphStrategy` where specifying `edges` and `vertices` filters that had `map`-type steps could generate an error. * Fixed bug in `ReservedKeysVerificationStrategy` where `AddPropertyStep` was not triggering proper validations. * Fixed bug in `mergeE` where `onCreate` validation of invalid static argument overrides did not trigger until traversal runtime. diff --git a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/AbstractTinkerGraph.java b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/AbstractTinkerGraph.java index 318c8ae6f7..b0f4921973 100644 --- a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/AbstractTinkerGraph.java +++ b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/AbstractTinkerGraph.java @@ -39,6 +39,7 @@ import java.io.File; import java.lang.reflect.InvocationTargetException; import java.util.Collections; import java.util.Iterator; +import java.util.Map; import java.util.Set; import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; @@ -63,8 +64,8 @@ public abstract class AbstractTinkerGraph implements Graph { public static final String GREMLIN_TINKERGRAPH_ALLOW_NULL_PROPERTY_VALUES = "gremlin.tinkergraph.allowNullPropertyValues"; public static final String GREMLIN_TINKERGRAPH_SERVICE = "gremlin.tinkergraph.service"; - protected AtomicLong currentId = new AtomicLong(-1L); + protected Map<Object, VertexProperty> vertexProperties = new ConcurrentHashMap<>(); protected TinkerGraphVariables variables = null; protected TinkerGraphComputerView graphComputerView = null; @@ -170,6 +171,13 @@ public abstract class AbstractTinkerGraph implements Graph { */ public abstract boolean hasEdge(final Object id); + /** + * Returns true if a {@link VertexProperty} with the given identifier exists in this graph. + */ + public boolean hasVertexProperty(final Object id) { + return vertexProperties.containsKey(id); + } + protected void loadGraph() { final File f = new File(graphLocation); if (f.exists() && f.isFile()) { @@ -257,6 +265,7 @@ public abstract class AbstractTinkerGraph implements Graph { this.vertexIndex = null; this.edgeIndex = null; this.graphComputerView = null; + this.vertexProperties.clear(); } /** @@ -398,14 +407,14 @@ public abstract class AbstractTinkerGraph implements Graph { * Construct an {@link IdManager} from the TinkerGraph {@code Configuration}. */ protected static <T extends Element> IdManager<T> selectIdManager(final Configuration config, final String configKey, final Class<T> clazz) { - final String vertexIdManagerConfigValue = config.getString(configKey, DefaultIdManager.ANY.name()); + final String idManagerConfigValue = config.getString(configKey, DefaultIdManager.ANY.name()); try { - return DefaultIdManager.valueOf(vertexIdManagerConfigValue); + return DefaultIdManager.valueOf(idManagerConfigValue); } catch (IllegalArgumentException iae) { try { - return (IdManager) Class.forName(vertexIdManagerConfigValue).newInstance(); + return (IdManager) Class.forName(idManagerConfigValue).newInstance(); } catch (Exception ex) { - throw new IllegalStateException(String.format("Could not configure TinkerGraph %s id manager with %s", clazz.getSimpleName(), vertexIdManagerConfigValue)); + throw new IllegalStateException(String.format("Could not configure TinkerGraph %s id manager with %s", clazz.getSimpleName(), idManagerConfigValue)); } } } @@ -447,7 +456,7 @@ public abstract class AbstractTinkerGraph implements Graph { LONG { @Override public Long getNextId(final AbstractTinkerGraph graph) { - return Stream.generate(() -> (graph.currentId.incrementAndGet())).filter(id -> !graph.hasVertex(id) && !graph.hasEdge(id)).findAny().get(); + return Stream.generate(() -> (graph.currentId.incrementAndGet())).filter(id -> !graph.hasVertex(id) && !graph.hasEdge(id) && !graph.hasVertexProperty(id)).findAny().get(); } @Override @@ -482,7 +491,7 @@ public abstract class AbstractTinkerGraph implements Graph { INTEGER { @Override public Integer getNextId(final AbstractTinkerGraph graph) { - return Stream.generate(() -> (graph.currentId.incrementAndGet())).map(Long::intValue).filter(id -> !graph.hasVertex(id) && !graph.hasEdge(id)).findAny().get(); + return Stream.generate(() -> (graph.currentId.incrementAndGet())).map(Long::intValue).filter(id -> !graph.hasVertex(id) && !graph.hasEdge(id) && !graph.hasVertexProperty(id)).findAny().get(); } @Override @@ -551,7 +560,7 @@ public abstract class AbstractTinkerGraph implements Graph { ANY { @Override public Long getNextId(final AbstractTinkerGraph graph) { - return Stream.generate(() -> (graph.currentId.incrementAndGet())).filter(id -> !graph.hasVertex(id) && !graph.hasEdge(id)).findAny().get(); + return Stream.generate(() -> (graph.currentId.incrementAndGet())).filter(id -> !graph.hasVertex(id) && !graph.hasEdge(id) && !graph.hasVertexProperty(id)).findAny().get(); } @Override diff --git a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerVertex.java b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerVertex.java index 8abb94e06c..c8498b9434 100644 --- a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerVertex.java +++ b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerVertex.java @@ -166,6 +166,7 @@ public class TinkerVertex extends TinkerElement implements Vertex { final List<VertexProperty> list = this.properties.getOrDefault(key, new ArrayList<>()); list.add(vertexProperty); this.properties.put(key, list); + graph.vertexProperties.put(vertexProperty.id(), vertexProperty); TinkerIndexHelper.autoUpdateIndex(this, key, value, null); ElementHelper.attachProperties(vertexProperty, keyValues); return vertexProperty; @@ -196,6 +197,8 @@ public class TinkerVertex extends TinkerElement implements Vertex { this.edges(Direction.BOTH).forEachRemaining(edge -> edges.add(edge)); edges.stream().filter(edge -> !((TinkerEdge) edge).removed).forEach(Edge::remove); TinkerIndexHelper.removeElementIndex(this); + if (null != this.properties) + this.properties.values().forEach(vpList -> vpList.forEach(vp -> graph.vertexProperties.remove(vp.id()))); this.properties = null; this.graph.removeVertex(this.id); this.removed = true; diff --git a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerVertexProperty.java b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerVertexProperty.java index 63d85bc491..f1061672a0 100644 --- a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerVertexProperty.java +++ b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerVertexProperty.java @@ -174,6 +174,7 @@ public class TinkerVertexProperty<V> extends TinkerElement implements VertexProp delete.set(false); }); if (delete.get()) TinkerIndexHelper.removeIndex(this.vertex, this.key, this.value); + ((AbstractTinkerGraph) vertex.graph()).vertexProperties.remove(this.id); this.properties = null; this.removed = true; } 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 fade7fbb5a..1ac3550fb2 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 @@ -32,9 +32,13 @@ import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; import java.util.UUID; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; /** * @author Stephen Mallette (http://stephen.genoprime.com) @@ -157,4 +161,116 @@ public class TinkerGraphIdManagerTest { assertEquals(vertexPropertyId, vp.id()); } } + + /** + * TINKERPOP-3214 - Tests that auto-generated vertex property IDs do not collide with explicitly-assigned vertex + * property IDs. TinkerFactory.createModern() assigns vertex property IDs 0L-11L explicitly while the shared counter + * stays at -1, so the first call to getNextId() would naively return 0L without the global vertex property map fix. + * Covers LONG, INTEGER, and ANY managers (the three sequential counter-based implementations). + */ + @RunWith(Parameterized.class) + public static class VertexPropertyIdUniquenessTest { + + @Parameterized.Parameters(name = "{0}") + public static Iterable<Object[]> data() { + return Arrays.asList(new Object[][]{ + {TinkerGraph.DefaultIdManager.LONG}, + {TinkerGraph.DefaultIdManager.INTEGER}, + {TinkerGraph.DefaultIdManager.ANY}}); + } + + @Parameterized.Parameter(value = 0) + public TinkerGraph.DefaultIdManager idManager; + + private TinkerGraph openGraph() { + final Configuration config = new BaseConfiguration(); + config.addProperty(TinkerGraph.GREMLIN_TINKERGRAPH_VERTEX_ID_MANAGER, idManager.name()); + config.addProperty(TinkerGraph.GREMLIN_TINKERGRAPH_EDGE_ID_MANAGER, idManager.name()); + config.addProperty(TinkerGraph.GREMLIN_TINKERGRAPH_VERTEX_PROPERTY_ID_MANAGER, idManager.name()); + return TinkerGraph.open(config); + } + + /** + * Regression test for TINKERPOP-3214: when VP IDs are assigned explicitly (leaving the shared counter + * untouched at -1), the first auto-generated VP must not reuse any of those IDs. + */ + @Test + public void shouldNotGenerateDuplicateVertexPropertyIdAfterExplicitAssignment() { + final TinkerGraph graph = openGraph(); + final Vertex v = graph.addVertex(T.id, 100); + + // Explicitly assign VP IDs 0-11, mirroring what TinkerFactory.createModern() does. + for (int i = 0; i < 12; i++) { + v.property(VertexProperty.Cardinality.list, "tag", "v" + i, T.id, i); + } + + final Set<Object> existingVpIds = new HashSet<>(); + v.properties().forEachRemaining(vp -> existingVpIds.add(vp.id())); + assertEquals(12, existingVpIds.size()); + + // Auto-generate a new VP ID — must not collide with any of 0-11. + final VertexProperty<?> newVp = v.property(VertexProperty.Cardinality.list, "tag", "auto"); + assertNotEquals("auto-generated VP id must not duplicate an explicitly-set VP id", + true, existingVpIds.contains(newVp.id())); + } + + /** + * All vertex property IDs across the graph must be globally unique regardless of whether they were + * explicitly assigned or auto-generated. + */ + @Test + public void shouldMaintainGloballyUniqueVertexPropertyIds() { + final TinkerGraph graph = openGraph(); + final Vertex v1 = graph.addVertex(T.id, 1); + final Vertex v2 = graph.addVertex(T.id, 2); + final Vertex v3 = graph.addVertex(T.id, 3); + + // Seed some explicit VP IDs, then add auto-generated ones on different vertices. + v1.property(VertexProperty.Cardinality.single, "name", "alice", T.id, 10); + v2.property(VertexProperty.Cardinality.single, "name", "bob", T.id, 11); + v1.property(VertexProperty.Cardinality.single, "city", "seattle"); + v2.property(VertexProperty.Cardinality.single, "city", "portland"); + v3.property(VertexProperty.Cardinality.single, "city", "vancouver"); + + final Set<Object> allVpIds = new HashSet<>(); + graph.vertices().forEachRemaining(v -> v.properties().forEachRemaining(vp -> + assertTrue("duplicate vertex property id: " + vp.id(), allVpIds.add(vp.id())))); + } + + /** + * Auto-generated vertex property IDs must be unique even when no explicit IDs are used. + */ + @Test + public void shouldGenerateUniqueAutoVertexPropertyIds() { + final TinkerGraph graph = openGraph(); + final Vertex v = graph.addVertex(); + + v.property(VertexProperty.Cardinality.list, "tag", "a"); + v.property(VertexProperty.Cardinality.list, "tag", "b"); + v.property(VertexProperty.Cardinality.list, "tag", "c"); + + final Set<Object> ids = new HashSet<>(); + v.properties("tag").forEachRemaining(vp -> + assertTrue("duplicate auto-generated VP id: " + vp.id(), ids.add(vp.id()))); + assertEquals(3, ids.size()); + } + + /** + * Auto-generated vertex property IDs must not collide with vertex or edge IDs, preserving the + * existing cross-element-type uniqueness guarantee. + */ + @Test + public void shouldNotCollideWithVertexOrEdgeIds() { + final TinkerGraph graph = openGraph(); + final Vertex v1 = graph.addVertex(T.id, 1); + final Vertex v2 = graph.addVertex(T.id, 2); + v1.addEdge("knows", v2, T.id, 3); + + final VertexProperty<?> vp = v1.property(VertexProperty.Cardinality.single, "name", "alice"); + + assertNotEquals(idManager.convert(1), vp.id()); + assertNotEquals(idManager.convert(2), vp.id()); + assertNotEquals(idManager.convert(3), vp.id()); + } + } }
