Report why an update action does not apply to a mixed index (#4930) - #4939
Open
batrived wants to merge 1 commit into
Open
Report why an update action does not apply to a mixed index (#4930)#4939batrived wants to merge 1 commit into
batrived wants to merge 1 commit into
Conversation
…ph#4930) A mixed index keeps a SchemaStatus per field instead of one status on the index, so updateIndex collects the fields whose status the action applies to. When no field matched, the empty set was passed on to setStatus, which routed to setStatusVertex, whose precondition can never hold for a mixed index. That precondition carries no message, so the caller received a bare IllegalArgumentException with nothing to act on. Re-running an idempotent schema script is enough to reach this. REGISTER_INDEX applies to INSTALLED and DISABLED, so it matches no field once the index is enabled. An index built on a property key created in the same management transaction is enabled immediately, so a script which creates an index and then registers it fails the second time it runs. The composite branch guards against this a few lines earlier with isApplicableStatus, which throws a message naming the action and the status. Give the mixed branch the equivalent check, and report the status of every field so it is clear which one blocked the action. This makes a mixed index behave as a composite index already does for an action which does not apply to it. It also means DISCARD_INDEX no longer clears the index backend before failing, and DROP_INDEX on a mixed index which is not DISCARDED is now rejected rather than dropping the schema vertex and orphaning the documents. The equivalent check that REMOVE_STALE_ENTRIES made for itself is now unreachable, so remove it. Signed-off-by: Balmukund Trivedi <btrivedipublic@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
batrived
force-pushed
the
fix/4930-mixed-index-update-status-guard
branch
from
August 13, 2026 17:10
e8f1099 to
7525f26
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.
Fixes a failure mode when applying updateIndex actions to mixed indexes where no field matches the action’s applicable statuses, by rejecting early with a descriptive error (and adding regression tests).
Changes:
- Add a precondition guard for mixed indexes to reject update actions that match no fields, including reporting per-field statuses.
- Remove a now-redundant/late guard in the mixed
REMOVE_STALE_ENTRIESpath. - Add
MixedIndexUpdateStatusTestcovering the rejected case and two success paths (registering INSTALLED, disabling ENABLED).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| janusgraph-test/src/test/java/org/janusgraph/graphdb/database/management/MixedIndexUpdateStatusTest.java | Adds regression tests ensuring a descriptive exception is thrown when an update action matches no mixed-index fields. |
| janusgraph-core/src/main/java/org/janusgraph/graphdb/database/management/ManagementSystem.java | Adds an early guard for mixed-index updates when no fields match, and includes per-field status in the rejection message. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+988
to
+995
| //A mixed index keeps a status per field instead of one status on the index, so this is the | ||
| //equivalent of the isApplicableStatus check the composite branch makes above. Without it an action | ||
| //which matches no field reaches setStatusVertex, whose precondition then fails with a message about | ||
| //RelationTypeVertex and composite indexes which says nothing about the real problem | ||
| Preconditions.checkArgument(!keySubset.isEmpty(), | ||
| "Update action [%s] cannot be invoked for index [%s] because none of its fields has one of the" + | ||
| " applicable statuses %s. The current status of each field is %s", updateAction, index.name(), | ||
| applicableStatus, fieldStatuses(mixedIndexType)); |
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 #4930.
A mixed index keeps a
SchemaStatusper field instead of one status on the index, soupdateIndexcollects the fields whose status the action applies to. When no field matched, that empty set was passed tosetStatus, which routed tosetStatusVertex, whose precondition can never hold for a mixed index.One correction to the issue: that precondition is
Preconditions.checkArgument(condition)with no message argument, so the exception's message isnull. The caller does not get a confusing message aboutRelationTypeVertex— it gets a bareIllegalArgumentExceptionwith no message at all. I found this while writing the test, which failed with aNullPointerExceptionone.getMessage()before the fix.Re-running an idempotent schema script is enough to reach it, exactly as the issue describes:
REGISTER_INDEXapplies toINSTALLEDandDISABLED, so it matches no field once the index is enabled. An index built on a property key created in the same management transaction is enabled immediately, so a script that creates an index and then registers it fails the second time it runs.Change
The composite branch guards this a few lines earlier via
isApplicableStatus. Worth noting that method never returnsfalse— it throws — so thereturn nullafter it in the composite branch is unreachable, and the composite behaviour is "throw a descriptive error". This gives the mixed branch the equivalent check, and additionally reports the status of every field so it is clear which one blocked the action:Two consequences worth your attention
The guard applies to every action, which is what makes a mixed index behave the way a composite index already does. Two of those are behaviour changes beyond the reported symptom, both of which I believe are improvements — but they are yours to judge:
DISCARD_INDEXno longer clears the index backend before failing. Previously it calledIndexSerializer.clearStoreand then threw fromsetStatusVertex, so the documents were already gone while the status change had not happened.DROP_INDEXon a mixed index which is notDISCARDEDis now rejected. This is the second problem you flagged in the issue as possibly deserving its own issue —schemaVertex.remove()used to run regardless, dropping the schema vertex and orphaning every document in the index backend. A composite index is already gated this way.I also removed the
!keySubset.isEmpty()check thatREMOVE_STALE_ENTRIESmade for itself, since the new guard runs earlier and makes it unreachable. If you would rather narrow the guard to only the status-changing actions, that check needs to come back.Testing
MixedIndexUpdateStatusTestruns against the in-memory backend with the existingRecordingIndexProvideras the index backend. It covers the rejected case and asserts the message names the action, the index, the blocking status and the applicable statuses; plus two paths that must keep working, registering anINSTALLEDindex and disabling anENABLEDone.I checked every existing caller of
DISCARD_INDEXandDROP_INDEXin the test tree. The mixed-index sequences inJanusGraphIndexTestandElasticsearchJanusGraphIndexTestall discard or mark-discarded before dropping, so they stay compatible; the sequences inJanusGraphTestare composite and relation indexes, which this does not touch.IndexSerializerTeststill passes. Docker was unavailable to me, so I could not run the container-backed suites locally.For all changes:
master)?For code changes: