Repository navigation
TINKERPOP-3294: AddPropertyStep does not capture the old property for EventStrategy when cardinality is not specified - #3715
paul-lartigau wants to merge 1 commit into
Conversation
paul-lartigau
changed the title
| // find property to remove. only need to capture the removedProperty if eventing is configured | ||
| final Property removedProperty = eventingIsConfigured ? | ||
| captureRemovedProperty(element, key, value, es) : | ||
| captureRemovedProperty(element, key, value, effectiveCardinality, es) : |
There was a problem hiding this comment.
On 3.7, this cardinality is used only for event capture. The actual write below still calls vertex.property(key, value) when cardinality is omitted, and providers may implement that differently from getCardinality(key), which is documented as potentially only a hint. This could cause the event to report a replacement when the provider actually appended, or miss a replacement when it wrote as single. I fully agree with your fix on 3.8-dev though.
Do you need this fix on 3.7.x or can we just apply it to 3.8.x?
There was a problem hiding this comment.
You're right, I missed that. I've patched it on my side in a fork for now, so applying it to 3.8.x only is fine, and I'll pick it up there later. I've retargeted this PR to 3.8-dev.
…t cardinality Assisted-by: Claude Code:claude-opus-5-5
0d4d528 to
863263a
Compare
| @Test | ||
| @FeatureRequirementSet(FeatureRequirementSet.Package.VERTICES_ONLY) | ||
| public void shouldCaptureOldVertexPropertyWhenChangedWithDefaultCardinality() { | ||
| assumeTrue(graph.features().vertex().getCardinality("to-change") == VertexProperty.Cardinality.single); |
There was a problem hiding this comment.
This fix also changes behavior when the graph’s default cardinality is set, but this test only runs for default single cardinality. Please add a second test for default set and ensure it executes with TinkerGraph by configuring TinkerGraphProvider#getBaseConfiguration() for that test method, following the existing pattern.
|
Thank you for reporting this issue and contributing a fix. It's very much appreciated. VOTE +1 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 3.8-dev #3715 +/- ##
==========================================
Coverage ? 76.97%
Complexity ? 15063
==========================================
Files ? 1162
Lines ? 72576
Branches ? 8131
==========================================
Hits ? 55867
Misses ? 13732
Partials ? 2977 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3 participants
https://issues.apache.org/jira/browse/TINKERPOP-3294
captureRemovedProperty() was checking the step's cardinality field, which is null when property() is called without an explicit cardinality. The old value passed to vertexPropertyChanged() was then always empty, even though the property itself was written with the graph's default cardinality.
The effective cardinality (explicit one, or the graph default for the key) was already computed in sideEffect() but not used. It is now passed to captureRemovedProperty().
Testing:
Merge notes: the code differs on 3.8-dev, the fallback cardinality is computed after the call to captureRemovedProperty(), so it needs to be moved above it when merging forward.