Accept a String vertex id in IndexSerializer.removeElement (#4928) - #4937
Open
batrived wants to merge 1 commit into
Open
Accept a String vertex id in IndexSerializer.removeElement (#4928)#4937batrived wants to merge 1 commit into
batrived wants to merge 1 commit into
Conversation
…h#4928) removeElement rejected every vertex id which is not a Long, but the very next line calls element2String, which encodes a Long, a RelationIdentifier or a String. The two methods disagreed about a valid element id, and the stricter of them ran first. A graph configured with graph.allow-custom-vid-types assigns String vertex ids. StandardTransactionLogProcessor calls removeElement when the element of a restored transaction no longer exists, so recovering a transaction which deleted an indexed vertex threw IllegalArgumentException. The stale index document was never removed, and the recovery which exists to repair a diverged index could not complete. Accept the same vertex ids that element2String accepts. A relation index still requires a RelationIdentifier. Signed-off-by: Balmukund Trivedi <btrivedipublic@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
batrived
force-pushed
the
fix/4928-remove-element-string-id
branch
from
August 13, 2026 17:10
6576b76 to
cabd792
Compare
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Updates IndexSerializer.removeElement to accept String vertex IDs (in addition to Long) to support graphs configured with custom vertex ID types, and adds unit tests to validate accepted and rejected ID/category combinations.
Changes:
- Allow
StringIDs when removing vertex documents from mixed indexes. - Add unit tests covering
Stringvertex IDs,Longvertex IDs, relation IDs, and invalid ID/category combinations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| janusgraph-core/src/main/java/org/janusgraph/graphdb/database/IndexSerializer.java | Expands vertex ID validation in removeElement to accept String IDs. |
| janusgraph-core/src/test/java/org/janusgraph/graphdb/database/IndexSerializerRemoveElementTest.java | Adds regression/unit tests for removeElement ID handling and document-id roundtrip behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4928.
IndexSerializer.removeElementrejected every vertex id which is not aLong, but the very next line callselement2String, which accepts aLong, aRelationIdentifieror aString. The two methods disagreed about what a valid element id is, and the stricter of them ran first.A graph configured with
graph.allow-custom-vid-typesassigns String vertex ids. The only caller isStandardTransactionLogProcessor, which callsremoveElementwhen the element of a restored transaction no longer exists. So recovering a transaction that deleted an indexed vertex threwIllegalArgumentException, the stale index document was never removed, and the recovery mechanism that exists to repair a diverged index could not complete.This applies the fix suggested in the issue: accept the same vertex ids
element2Stringaccepts. A relation index still requires aRelationIdentifier.Testing
IndexSerializerRemoveElementTestcovers a String vertex id, a Long vertex id, a relation id, and the ids that must still be rejected. I confirmed the String case reproduces the reported failure without the fix:Each test also asserts the resulting document id round-trips back to the original element id through
string2ElementId, so the document that gets removed is the right one, and that the entry list is empty, which is how a document removal is expressed.For all changes:
master)?For code changes: