[
https://issues.apache.org/jira/browse/TINKERPOP-3193?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18022895#comment-18022895
]
ASF GitHub Bot commented on TINKERPOP-3193:
-------------------------------------------
andreachild commented on code in PR #3215:
URL: https://github.com/apache/tinkerpop/pull/3215#discussion_r2380240055
##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/step/sideEffect/AddPropertyStep.java:
##########
@@ -48,41 +48,46 @@
* @author Marko A. Rodriguez (http://markorodriguez.com)
*/
public class AddPropertyStep<S extends Element> extends SideEffectStep<S>
- implements AddPropertyStepContract<S>,
Writing<Event.ElementPropertyChangedEvent>,
Deleting<Event.ElementPropertyChangedEvent>, Configuring {
+ implements AddPropertyStepContract<S>,
Writing<Event.ElementPropertyChangedEvent>,
Deleting<Event.ElementPropertyChangedEvent> {
- private Parameters parameters = new Parameters();
+ private Parameters internalParameters = new Parameters();
+ private Parameters withConfiguration = new Parameters();
private final VertexProperty.Cardinality cardinality;
private CallbackRegistry<Event.ElementPropertyChangedEvent>
callbackRegistry;
public AddPropertyStep(final Traversal.Admin traversal, final
VertexProperty.Cardinality cardinality, final Object keyObject, final Object
valueObject) {
super(traversal);
- this.parameters.set(this, T.key, keyObject, T.value, valueObject);
+ this.internalParameters.set(this, T.key, keyObject, T.value,
valueObject);
this.cardinality = cardinality;
}
@Override
public Parameters getParameters() {
- return this.parameters;
+ return this.withConfiguration;
}
@Override
public Set<String> getScopeKeys() {
- return this.parameters.getReferencedLabels();
+ return this.internalParameters.getReferencedLabels();
}
@Override
public <S, E> List<Traversal.Admin<S, E>> getLocalChildren() {
- return this.parameters.getTraversals();
+ return this.internalParameters.getTraversals();
}
@Override
public void configure(final Object... keyValues) {
- this.parameters.set(this, keyValues);
+ this.withConfiguration.set(this, keyValues);
+ }
+
+ private void configureInternalParams(final Object... keyValues) {
Review Comment:
Nit: this method is just one line and called once so not very useful
> Decouple non with()-related configs from Configuring/Parameterizing interfaces
> ------------------------------------------------------------------------------
>
> Key: TINKERPOP-3193
> URL: https://issues.apache.org/jira/browse/TINKERPOP-3193
> Project: TinkerPop
> Issue Type: Improvement
> Components: process
> Affects Versions: 3.7.4
> Reporter: Cole Greer
> Priority: Major
> Labels: breaking
>
> Currently, the Configuring and Parameterizing interfaces serve two distinct
> purposes. The most notable purpose is that these interfaces enable
> with()-modulation for a step. Additionally, there are a handful of steps such
> as AddVertexStep, AddEdgeStep, and AddPropertyStep which utilize Parameters
> to store their internal state. The dual purpose of these interfaces leads to
> strange behaviours such as the ability to set properties via with modulators:
> {code:java}
> gremlin> g.addV().with("name", "cole").valueMap()
> ==>[name:[cole]]
> {code}
> As part of the ongoing work leading to 3.8.0, we’ve extracted StepContracts
> (interfaces) from many steps including the ones listed above.
> These contracts expose new methods to interact with a step’s contents in a
> controlled manner instead of by directly accessing the
> Parameters (getElementId(), getLabel(), addProperty()…). The presence of
> these new interfaces creates an opportunity to escape the
> internal-state management role of Parameterizing, and change
> Configuring/Parameterizing to be purely focused on with()-modulation.
> We should isolate any state not related to with()-modulation from
> Configuring/Parameterizing interfaces. TinkerPop should never call the
> configure() method on a step except in response to a with() modulator, and
> that the Parameters returned by getParameters() should only contain
> configurations originating from with() modulators. Classes such as
> AddVertexStep may still leverage a second private Parameters objects to store
> its internal state, however this should not be publicly exposed at all. Any
> accesses and mutations of these steps should be forced to use the new
> contract interfaces instead.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
