From 7525f2625ec96fa860b52b8b8e0ffd146c395732 Mon Sep 17 00:00:00 2001 From: Balmukund Trivedi Date: Tue, 11 Aug 2026 14:42:18 -0700 Subject: [PATCH] Report why an update action does not apply to a mixed index (#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 Co-Authored-By: Claude Opus 5 (1M context) --- .../database/management/ManagementSystem.java | 20 ++- .../MixedIndexUpdateStatusTest.java | 138 ++++++++++++++++++ 2 files changed, 153 insertions(+), 5 deletions(-) create mode 100644 janusgraph-test/src/test/java/org/janusgraph/graphdb/database/management/MixedIndexUpdateStatusTest.java diff --git a/janusgraph-core/src/main/java/org/janusgraph/graphdb/database/management/ManagementSystem.java b/janusgraph-core/src/main/java/org/janusgraph/graphdb/database/management/ManagementSystem.java index 97ac3bc84b..2fbb23bac8 100644 --- a/janusgraph-core/src/main/java/org/janusgraph/graphdb/database/management/ManagementSystem.java +++ b/janusgraph-core/src/main/java/org/janusgraph/graphdb/database/management/ManagementSystem.java @@ -985,6 +985,14 @@ private ScanJobFuture updateIndex(Index index, SchemaAction updateAction, List trigger) { updatedTypeTriggers.add(trigger); } + //Reports the status of every field of a mixed index, so that a rejected update action can say why it was rejected + private static String fieldStatuses(MixedIndexType index) { + return Arrays.stream(index.getFieldKeys()) + .map(field -> field.getFieldKey().name() + "=" + field.getStatus()) + .collect(Collectors.joining(", ", "[", "]")); + } + private void setStatus(JanusGraphSchemaVertex vertex, SchemaStatus status, Set keys) { if (keys.isEmpty()) setStatusVertex(vertex, status); else setStatusEdges(vertex, status, keys); diff --git a/janusgraph-test/src/test/java/org/janusgraph/graphdb/database/management/MixedIndexUpdateStatusTest.java b/janusgraph-test/src/test/java/org/janusgraph/graphdb/database/management/MixedIndexUpdateStatusTest.java new file mode 100644 index 0000000000..b90a30ef4e --- /dev/null +++ b/janusgraph-test/src/test/java/org/janusgraph/graphdb/database/management/MixedIndexUpdateStatusTest.java @@ -0,0 +1,138 @@ +// Copyright 2026 JanusGraph Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package org.janusgraph.graphdb.database.management; + +import org.apache.tinkerpop.gremlin.structure.Vertex; +import org.janusgraph.core.Cardinality; +import org.janusgraph.core.JanusGraph; +import org.janusgraph.core.JanusGraphFactory; +import org.janusgraph.core.PropertyKey; +import org.janusgraph.core.schema.JanusGraphManagement; +import org.janusgraph.core.schema.SchemaAction; +import org.janusgraph.core.schema.SchemaStatus; +import org.janusgraph.diskstorage.configuration.ModifiableConfiguration; +import org.janusgraph.graphdb.configuration.GraphDatabaseConfiguration; +import org.janusgraph.graphdb.database.IndexSerializerTest; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +//A mixed index keeps a SchemaStatus per field rather than one status on the index itself, so updateIndex selects the +//fields whose status the action applies to. When that selection is empty the action used to reach setStatusVertex, +//whose precondition is guaranteed to fail for a mixed index and carries no message, so the caller got a bare +//IllegalArgumentException. Re-running an idempotent schema script is enough to reach it. +public class MixedIndexUpdateStatusTest { + + private static final String INDEX_BACKEND_NAME = "search"; + private static final String INDEX_NAME = "nameMixed"; + private static final String PROPERTY_NAME = "name"; + + private JanusGraph graph; + + @BeforeEach + public void openGraph() { + final ModifiableConfiguration config = GraphDatabaseConfiguration.buildGraphConfiguration(); + config.set(GraphDatabaseConfiguration.STORAGE_BACKEND, "inmemory"); + config.set(GraphDatabaseConfiguration.INDEX_BACKEND, + IndexSerializerTest.RecordingIndexProvider.class.getName(), INDEX_BACKEND_NAME); + graph = JanusGraphFactory.open(config.getConfiguration()); + } + + @AfterEach + public void closeGraph() { + if (graph != null) { + graph.close(); + } + } + + //A key created in the same management transaction as the index cannot hold data yet, so no reindex is needed and + //the index is enabled straight away + private void createMixedIndexOnNewKey() { + final JanusGraphManagement management = graph.openManagement(); + final PropertyKey name = management.makePropertyKey(PROPERTY_NAME).dataType(String.class) + .cardinality(Cardinality.SINGLE).make(); + management.buildIndex(INDEX_NAME, Vertex.class).addKey(name).buildMixedIndex(INDEX_BACKEND_NAME); + management.commit(); + } + + //An index over a key which already exists starts as INSTALLED, because the existing data still has to be indexed + private void createMixedIndexOnExistingKey() { + JanusGraphManagement management = graph.openManagement(); + management.makePropertyKey(PROPERTY_NAME).dataType(String.class).cardinality(Cardinality.SINGLE).make(); + management.commit(); + + management = graph.openManagement(); + management.buildIndex(INDEX_NAME, Vertex.class).addKey(management.getPropertyKey(PROPERTY_NAME)) + .buildMixedIndex(INDEX_BACKEND_NAME); + management.commit(); + } + + private void updateIndex(SchemaAction action) { + final JanusGraphManagement management = graph.openManagement(); + try { + management.updateIndex(management.getGraphIndex(INDEX_NAME), action); + management.commit(); + } catch (RuntimeException e) { + management.rollback(); + throw e; + } + } + + private void awaitStatus(SchemaStatus status) throws Exception { + ManagementSystem.awaitGraphIndexStatus(graph, INDEX_NAME).status(status).call(); + } + + @Test + public void shouldRejectAnActionWhichMatchesNoFieldWithADescriptiveError() throws Exception { + createMixedIndexOnNewKey(); + awaitStatus(SchemaStatus.ENABLED); + + //REGISTER_INDEX applies to INSTALLED and DISABLED, so an already enabled index matches no field. This is the + //idempotent schema script case from the issue + final IllegalArgumentException e = assertThrows(IllegalArgumentException.class, + () -> updateIndex(SchemaAction.REGISTER_INDEX)); + + //Previously the action reached setStatusVertex, whose Preconditions.checkArgument carries no message at all, + //so the caller got a bare IllegalArgumentException with nothing to act on + final String message = e.getMessage(); + assertNotNull(message, "the rejection must say why it was rejected"); + assertTrue(message.contains("REGISTER_INDEX"), message); + assertTrue(message.contains(INDEX_NAME), message); + //The status which blocked the action, and the statuses which would have allowed it + assertTrue(message.contains(SchemaStatus.ENABLED.name()), message); + assertTrue(message.contains(SchemaStatus.INSTALLED.name()), message); + } + + @Test + public void shouldStillRegisterAnInstalledIndex() throws Exception { + //The fields are INSTALLED, which REGISTER_INDEX applies to, so the normal path is unaffected + createMixedIndexOnExistingKey(); + updateIndex(SchemaAction.REGISTER_INDEX); + awaitStatus(SchemaStatus.REGISTERED); + } + + @Test + public void shouldStillDisableAnEnabledIndex() throws Exception { + //DISABLE_INDEX applies to ENABLED, so it matches the field and is carried out + createMixedIndexOnNewKey(); + awaitStatus(SchemaStatus.ENABLED); + updateIndex(SchemaAction.DISABLE_INDEX); + awaitStatus(SchemaStatus.DISABLED); + } +}