Skip to content

Commit 2958a30

Browse files
committed
Unify multi-label representation in addV step hierarchy
Collapse the separate Set<Object>/List<Traversal.Admin<?,?>> label representations into a single Collection<Object>-typed label field across AbstractAddElementStepPlaceholder, AbstractAddVertexStepPlaceholder, AddVertexStepPlaceholder, and AddVertexStartStepPlaceholder, with a private canonical constructor and setLabel() as the single source of truth for label normalization (constructor and post-construction calls now behave identically). Fix AddVertexStep/AddVertexStartStep: introduce a single 'label' field replacing the disconnected internalParameters T.label entry and the separate labelTraversals field, so getLabel()/setLabel() are no longer blind to multi-traversal labels. getLabel() resolves ConstantTraversal/GValueConstantTraversal elements to their String value where possible, and otherwise returns the unresolved Traversal itself (consistent with existing AddEdgeStep behavior), rather than silently dropping or throwing on unresolvable elements. Add/update javadoc on AddElementStepContract.getLabel()/setLabel() to document actual return/accepted types and drop the previously-implied 'set once' contract, which is not honored uniformly by all implementations (e.g. AddEdgeStep permits overwriting). Assisted-by: Kiro:claude-sonnet-5
1 parent 9ea3df1 commit 2958a30

9 files changed

Lines changed: 339 additions & 244 deletions

File tree

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversal.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1476,7 +1476,7 @@ public default GraphTraversal<S, Vertex> addV(final Traversal<?, ?> first, final
14761476
return this.asAdmin().addStep(new AddVertexStepPlaceholder<>(this.asAdmin(), first.asAdmin()));
14771477
} else {
14781478
this.asAdmin().getGremlinLang().addStep(Symbols.addV, first, more);
1479-
final List<Traversal.Admin<?, ?>> allTraversals = new ArrayList<>(more.length + 1);
1479+
final Collection<Object> allTraversals = new LinkedHashSet<>(more.length + 1);
14801480
allTraversals.add(first.asAdmin());
14811481
for (final Traversal<?, ?> t : more) {
14821482
allTraversals.add(t.asAdmin());
@@ -1515,7 +1515,7 @@ public default GraphTraversal<S, Vertex> addV(final GValue<String> label, final
15151515
return this.asAdmin().addStep(new AddVertexStepPlaceholder<>(this.asAdmin(), label));
15161516
} else {
15171517
this.asAdmin().getGremlinLang().addStep(Symbols.addV, label, additionalLabels);
1518-
final LinkedHashSet<Object> allLabels = new LinkedHashSet<>();
1518+
final Collection<Object> allLabels = new LinkedHashSet<>();
15191519
allLabels.add(label);
15201520
Collections.addAll(allLabels, additionalLabels);
15211521
return this.asAdmin().addStep(new AddVertexStepPlaceholder<>(this.asAdmin(), allLabels));

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/dsl/graph/GraphTraversalSource.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -376,7 +376,7 @@ public GraphTraversal<Vertex, Vertex> addV(final Traversal<?, ?> label, final Tr
376376
} else {
377377
clone.gremlinLang.addStep(GraphTraversal.Symbols.addV, label, additionalLabels);
378378
final GraphTraversal.Admin<Vertex, Vertex> traversal = new DefaultGraphTraversal<>(clone);
379-
final List<Traversal.Admin<?, ?>> allTraversals = new ArrayList<>(additionalLabels.length + 1);
379+
final Collection<Object> allTraversals = new LinkedHashSet<>(additionalLabels.length + 1);
380380
allTraversals.add(label.asAdmin());
381381
for (final Traversal<?, ?> t : additionalLabels) {
382382
allTraversals.add(t.asAdmin());

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AbstractAddElementStepPlaceholder.java

Lines changed: 80 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
2222
import org.apache.tinkerpop.gremlin.process.traversal.Traverser;
23+
import org.apache.tinkerpop.gremlin.process.traversal.lambda.ConstantTraversal;
2324
import org.apache.tinkerpop.gremlin.process.traversal.lambda.GValueConstantTraversal;
2425
import org.apache.tinkerpop.gremlin.process.traversal.step.GValue;
2526
import org.apache.tinkerpop.gremlin.process.traversal.step.GValueHolder;
@@ -55,58 +56,32 @@ public abstract class AbstractAddElementStepPlaceholder<S, E extends Element, X
5556
protected Parameters withConfiguration = new Parameters();
5657

5758
protected AbstractAddElementStepPlaceholder(final Traversal.Admin traversal, final String label) {
58-
super(traversal);
59-
this.label = label == null ? this.getDefaultLabel() : label;
59+
this(traversal, (Object) label);
6060
}
6161

6262
protected AbstractAddElementStepPlaceholder(final Traversal.Admin traversal, final GValue<String> label) {
63-
super(traversal);
64-
this.label = label == null ? this.getDefaultLabel() : label;
65-
if (label != null && label.isVariable()) {
66-
traversal.getGValueManager().register(label);
67-
}
63+
this(traversal, (Object) label);
6864
}
6965

7066
protected AbstractAddElementStepPlaceholder(final Traversal.Admin traversal, final Traversal.Admin<S,?> labelTraversal) {
71-
super(traversal);
72-
this.label = labelTraversal == null ? this.getDefaultLabel() : labelTraversal;
73-
if (labelTraversal instanceof GValueConstantTraversal) {
74-
traversal.getGValueManager().register(((GValueConstantTraversal<S, String>) labelTraversal).getGValue());
75-
}
76-
addTraversal(labelTraversal);
77-
}
78-
79-
protected AbstractAddElementStepPlaceholder(final Traversal.Admin traversal, final Set<Object> labels) {
80-
super(traversal);
81-
if (labels == null || labels.isEmpty()) {
82-
this.label = this.getDefaultLabel();
83-
} else {
84-
this.label = labels;
85-
for (final Object l : labels) {
86-
if (l instanceof GValue && ((GValue<?>) l).isVariable()) {
87-
traversal.getGValueManager().register((GValue<?>) l);
88-
}
89-
}
90-
}
67+
this(traversal, (Object) labelTraversal);
9168
}
9269

9370
/**
94-
* Constructor for multiple label traversals. Each traversal resolves to a single String label
95-
* at execution time.
71+
* Constructor for multiple labels, each of which may be a {@code String}, a {@link GValue}, or a
72+
* {@link Traversal.Admin} that resolves to a single {@code String} label at execution time.
9673
*
9774
* @param traversal the parent traversal
98-
* @param labelTraversals the list of label-producing traversals (must have 2+ elements)
75+
* @param labels the labels to add (must have 2+ elements; {@code null} or empty results in the default label)
9976
*/
100-
protected AbstractAddElementStepPlaceholder(final Traversal.Admin traversal, final List<Traversal.Admin<?, ?>> labelTraversals) {
77+
protected AbstractAddElementStepPlaceholder(final Traversal.Admin traversal, final Collection<Object> labels) {
78+
this(traversal, (Object) labels);
79+
}
80+
81+
private AbstractAddElementStepPlaceholder(final Traversal.Admin traversal, final Object label) {
10182
super(traversal);
102-
if (labelTraversals == null || labelTraversals.isEmpty()) {
103-
this.label = this.getDefaultLabel();
104-
} else {
105-
this.label = labelTraversals;
106-
for (final Traversal.Admin<?, ?> t : labelTraversals) {
107-
addTraversal(t);
108-
}
109-
}
83+
this.label = this.getDefaultLabel();
84+
if (label != null) setLabel(label);
11085
}
11186

11287
protected abstract String getDefaultLabel();
@@ -136,8 +111,8 @@ protected void addTraversal(final Traversal.Admin<?, ?> traversal) {
136111
}
137112
if (label != null && label instanceof Traversal) {
138113
childTraversals.add(((Traversal<?, ?>) label).asAdmin());
139-
} else if (label instanceof List) {
140-
for (final Object item : (List<?>) label) {
114+
} else if (label instanceof Collection) {
115+
for (final Object item : (Collection<?>) label) {
141116
if (item instanceof Traversal) {
142117
childTraversals.add(((Traversal<?, ?>) item).asAdmin());
143118
}
@@ -206,8 +181,8 @@ public boolean isParameterized() {
206181
(elementId instanceof GValue && ((GValue<?>) elementId).isVariable())) {
207182
return true;
208183
}
209-
if (label instanceof Set) {
210-
for (final Object l : (Set<?>) label) {
184+
if (label instanceof Collection) {
185+
for (final Object l : (Collection<?>) label) {
211186
if (l instanceof GValue && ((GValue<?>) l).isVariable()) {
212187
return true;
213188
}
@@ -227,41 +202,76 @@ public Object getLabel() {
227202
traversal.getGValueManager().pinVariable(((GValue<?>) label).getName());
228203
return ((GValue<?>) label).get();
229204
}
230-
if (label instanceof Set) {
231-
final Set<String> resolved = new LinkedHashSet<>();
232-
for (final Object l : (Set<?>) label) {
233-
if (l instanceof GValue) {
234-
traversal.getGValueManager().pinVariable(((GValue<?>) l).getName());
235-
resolved.add((String) ((GValue<?>) l).get());
236-
} else {
237-
resolved.add((String) l);
238-
}
205+
if (label instanceof Collection) {
206+
final Set<Object> resolved = new LinkedHashSet<>();
207+
for (final Object l : (Collection<?>) label) {
208+
resolved.add(resolveLabelElement(l));
239209
}
240210
return resolved;
241211
}
212+
if (label instanceof Traversal) {
213+
return resolveLabelElement(label);
214+
}
242215
return label;
243216
}
244217

218+
/**
219+
* Resolves a single label element. Handles {@code GValue}, {@code ConstantTraversal}, and
220+
* {@code GValueConstantTraversal}, which can be resolved to a {@code String} without a live
221+
* {@code Traverser}. Any other {@code Traversal} cannot be resolved statically and is returned as-is.
222+
*/
223+
private Object resolveLabelElement(final Object l) {
224+
if (l instanceof GValue) {
225+
traversal.getGValueManager().pinVariable(((GValue<?>) l).getName());
226+
return ((GValue<?>) l).get();
227+
}
228+
if (l instanceof GValueConstantTraversal) {
229+
traversal.getGValueManager().pinVariable(((GValueConstantTraversal<?, ?>) l).getGValue().getName());
230+
return ((GValueConstantTraversal<?, ?>) l).next();
231+
}
232+
if (l instanceof ConstantTraversal) {
233+
return ((ConstantTraversal<?, ?>) l).next();
234+
}
235+
return l;
236+
}
237+
245238
@Override
246239
public Object getLabelWithGValue() {
247240
return label;
248241
}
249242

250243
@Override
251244
public void setLabel(Object label) {
252-
if (this.label.equals(this.getDefaultLabel())) {
253-
if (label instanceof Traversal) {
254-
this.label = ((Traversal<S, String>) label).asAdmin();
255-
this.integrateChild((Traversal.Admin<?, ?>) this.label);
256-
} else if (label instanceof GValue) {
257-
this.label = label;
245+
if (!this.label.equals(this.getDefaultLabel())) {
246+
throw new IllegalArgumentException(String.format("Element T.label has already been set to [%s] and cannot be overridden with [%s]",
247+
this.label, label));
248+
}
249+
if (label instanceof Traversal) {
250+
this.label = ((Traversal<S, String>) label).asAdmin();
251+
if (label instanceof GValueConstantTraversal) {
252+
traversal.getGValueManager().register(((GValueConstantTraversal<S, String>) label).getGValue());
253+
}
254+
addTraversal((Traversal.Admin<?, ?>) this.label);
255+
} else if (label instanceof GValue) {
256+
this.label = label;
257+
if (((GValue<?>) label).isVariable()) {
258258
traversal.getGValueManager().register((GValue<?>) label);
259-
} else {
260-
this.label = label;
259+
}
260+
} else if (label instanceof Collection) {
261+
final Collection<?> labels = (Collection<?>) label;
262+
if (labels.isEmpty()) {
263+
return;
264+
}
265+
this.label = new LinkedHashSet<>(labels);
266+
for (final Object l : labels) {
267+
if (l instanceof Traversal) {
268+
addTraversal(((Traversal<?, ?>) l).asAdmin());
269+
} else if (l instanceof GValue && ((GValue<?>) l).isVariable()) {
270+
traversal.getGValueManager().register((GValue<?>) l);
271+
}
261272
}
262273
} else {
263-
throw new IllegalArgumentException(String.format("Element T.label has already been set to [%s] and cannot be overridden with [%s]",
264-
this.label, label));
274+
this.label = label;
265275
}
266276
}
267277

@@ -364,10 +374,10 @@ public void setElementId(Object elementId) {
364374
public void updateVariable(String name, Object value) {
365375
if (label instanceof GValue && name.equals(((GValue<?>) label).getName())) {
366376
label = GValue.of(name, value);
367-
} else if (label instanceof Set) {
377+
} else if (label instanceof Collection) {
368378
final LinkedHashSet<Object> updated = new LinkedHashSet<>();
369379
boolean changed = false;
370-
for (final Object l : (Set<?>) label) {
380+
for (final Object l : (Collection<?>) label) {
371381
if (l instanceof GValue && name.equals(((GValue<?>) l).getName())) {
372382
updated.add(GValue.of(name, value));
373383
changed = true;
@@ -396,8 +406,8 @@ public Collection<GValue<?>> getGValues() {
396406
Set<GValue<?>> gValues = GValueHelper.getGValuesFromProperties(properties);
397407
if (label instanceof GValue && ((GValue<?>) label).isVariable()) {
398408
gValues.add((GValue<?>) label);
399-
} else if (label instanceof Set) {
400-
for (final Object l : (Set<?>) label) {
409+
} else if (label instanceof Collection) {
410+
for (final Object l : (Collection<?>) label) {
401411
if (l instanceof GValue && ((GValue<?>) l).isVariable()) {
402412
gValues.add((GValue<?>) l);
403413
}
@@ -423,11 +433,13 @@ public AbstractAddElementStepPlaceholder<S, E, X> clone() {
423433
clone.label = ((Traversal<?, ?>) this.label).asAdmin().clone();
424434
} else if (this.label instanceof GValue) {
425435
clone.label = ((GValue<?>) this.label).clone();
426-
} else if (this.label instanceof Set) {
436+
} else if (this.label instanceof Collection) {
427437
final LinkedHashSet<Object> clonedSet = new LinkedHashSet<>();
428-
for (final Object l : (Set<?>) this.label) {
438+
for (final Object l : (Collection<?>) this.label) {
429439
if (l instanceof GValue) {
430440
clonedSet.add(((GValue<?>) l).clone());
441+
} else if (l instanceof Traversal) {
442+
clonedSet.add(((Traversal<?, ?>) l).asAdmin().clone());
431443
} else {
432444
clonedSet.add(l);
433445
}

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AbstractAddVertexStepPlaceholder.java

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,8 @@
2424
import org.apache.tinkerpop.gremlin.process.traversal.step.util.event.Event;
2525
import org.apache.tinkerpop.gremlin.structure.Vertex;
2626

27-
import java.util.List;
27+
import java.util.Collection;
2828
import java.util.Objects;
29-
import java.util.Set;
3029

3130
public abstract class AbstractAddVertexStepPlaceholder<S> extends AbstractAddElementStepPlaceholder<S, Vertex, Event.VertexAddedEvent>
3231
implements AddVertexStepContract<S>, GValueHolder<S, Vertex> {
@@ -35,27 +34,18 @@ public abstract class AbstractAddVertexStepPlaceholder<S> extends AbstractAddEle
3534

3635
protected AbstractAddVertexStepPlaceholder(final Traversal.Admin traversal, final String label) {
3736
super(traversal, label);
38-
userProvidedLabel = label != null;
3937
}
4038

4139
protected AbstractAddVertexStepPlaceholder(final Traversal.Admin traversal, final GValue<String> label) {
4240
super(traversal, label);
43-
userProvidedLabel = label != null;
4441
}
4542

4643
protected AbstractAddVertexStepPlaceholder(final Traversal.Admin traversal, final Traversal.Admin<S,?> vertexLabelTraversal) {
4744
super(traversal, vertexLabelTraversal);
48-
userProvidedLabel = vertexLabelTraversal != null;
4945
}
5046

51-
protected AbstractAddVertexStepPlaceholder(final Traversal.Admin traversal, final Set<Object> labels) {
47+
protected AbstractAddVertexStepPlaceholder(final Traversal.Admin traversal, final Collection<Object> labels) {
5248
super(traversal, labels);
53-
userProvidedLabel = labels != null && !labels.isEmpty();
54-
}
55-
56-
protected AbstractAddVertexStepPlaceholder(final Traversal.Admin traversal, final List<Traversal.Admin<?, ?>> labelTraversals) {
57-
super(traversal, labelTraversals);
58-
userProvidedLabel = labelTraversals != null && !labelTraversals.isEmpty();
5949
}
6050

6151
@Override

gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/map/AddElementStepContract.java

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
package org.apache.tinkerpop.gremlin.process.traversal.step.map;
2020

2121
import org.apache.tinkerpop.gremlin.process.traversal.Step;
22+
import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
2223
import org.apache.tinkerpop.gremlin.process.traversal.step.Configuring;
2324
import org.apache.tinkerpop.gremlin.process.traversal.step.PropertiesHolder;
2425
import org.apache.tinkerpop.gremlin.process.traversal.step.Scoping;
@@ -37,12 +38,28 @@ public interface AddElementStepContract<S, E> extends Step<S, E>, PropertiesHold
3738
List<Class<? extends Step>> CONCRETE_STEPS = Stream.of(AddVertexStepContract.CONCRETE_STEPS, AddEdgeStepContract.CONCRETE_STEPS)
3839
.flatMap(List::stream).collect(Collectors.toList());
3940

41+
/**
42+
* Gets the label(s) assigned to the element produced by this step. Returns a single value, or a
43+
* {@code Set} when multiple labels were specified (e.g. {@code addV("a", "b")}). Each individual label
44+
* value will either be a {@code String} or {@link Traversal} depending on how it was initially set. Callers
45+
* should not assume the type is {@code String} and should check with {@code instanceof} before casting.
46+
*
47+
* @return a {@code String}, {@code Traversal}, or a {@code Set} containing either
48+
*/
4049
Object getLabel();
4150

4251
default Object getLabelWithGValue() {
4352
return getLabel();
4453
}
4554

55+
/**
56+
* Sets the label(s) to assign to the element produced by this step. Accepts a single {@code String},
57+
* {@code GValue<String>}, or {@code Traversal} resolving to a {@code String}, or a {@code Collection<Object>}
58+
* of two or more such values. Should behave identically to specifying the same value at construction time.
59+
* Implementations may restrict calling this method more than once at their own discretion.
60+
*
61+
* @param label a {@code String}, {@code GValue<String>}, {@code Traversal}, or {@code Collection<Object>}
62+
*/
4663
void setLabel(Object label);
4764

4865
Object getElementId();

0 commit comments

Comments
 (0)