You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by fo...@apache.org on 2026/05/11 12:07:27 UTC
(jackrabbit-oak) branch trunk updated: OAK-12206 Fix subtree deletion not cascading to child documents in Elastic index (#2892)
This is an automated email from the ASF dual-hosted git repository.
fabriziofortino pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/jackrabbit-oak.git
The following commit(s) were added to refs/heads/trunk by this push:
new 39fc8b122d OAK-12206 Fix subtree deletion not cascading to child documents in Elastic index (#2892)
39fc8b122d is described below
commit 39fc8b122d5c8c2d9aef0d5c713b866c95f3df38
Author: Fabrizio Fortino <fa...@gmail.com>
AuthorDate: Mon May 11 14:07:21 2026 +0200
OAK-12206 Fix subtree deletion not cascading to child documents in Elastic index (#2892)
* OAK-12206 Fix subtree deletion not cascading to child documents in Elastic index
* OAK-12206 add feature toggle + time bombed test
* OAK-12206 (minor) improved test
* OAK-12206 add more logs
* OAK-12206 fix test: missing mock clause
* OAK-12206 verify deleteByQuery gets called in tests
---
.../index/elastic/ElasticIndexProviderService.java | 3 ++
.../elastic/index/ElasticIndexEditorProvider.java | 9 ++++++
.../index/elastic/index/ElasticIndexWriter.java | 18 ++++++++++++
.../plugins/index/elastic/ElasticContentTest.java | 28 +++++++++++++++++++
.../elastic/index/ElasticIndexWriterTest.java | 32 +++++++++++++++++++++-
5 files changed, 89 insertions(+), 1 deletion(-)
diff --git a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexProviderService.java b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexProviderService.java
index 4d15ea8c1f..b784a9445e 100644
--- a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexProviderService.java
+++ b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexProviderService.java
@@ -218,6 +218,9 @@ public class ElasticIndexProviderService {
oakRegs.add(whiteboard.register(FeatureToggle.class,
new FeatureToggle(FulltextIndexPlanner.FT_OAK_12171, FulltextIndexPlanner.FT_OAK_12171_DISABLE),
emptyMap()));
+ oakRegs.add(whiteboard.register(FeatureToggle.class,
+ new FeatureToggle(ElasticIndexEditorProvider.FT_OAK_12206, ElasticIndexEditorProvider.FT_OAK_12206_DISABLE),
+ emptyMap()));
if (System.getProperty(QueryEngineSettings.OAK_INFERENCE_ENABLED) != null) {
this.isInferenceEnabled = Boolean.parseBoolean(System.getProperty(QueryEngineSettings.OAK_INFERENCE_ENABLED));
} else {
diff --git a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexEditorProvider.java b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexEditorProvider.java
index b00eada028..d116495c74 100644
--- a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexEditorProvider.java
+++ b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexEditorProvider.java
@@ -34,6 +34,7 @@ import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import java.io.IOException;
+import java.util.concurrent.atomic.AtomicBoolean;
import static org.apache.jackrabbit.oak.commons.PathUtils.ROOT_PATH;
import static org.apache.jackrabbit.oak.plugins.index.elastic.ElasticIndexDefinition.TYPE_ELASTICSEARCH;
@@ -48,6 +49,14 @@ public class ElasticIndexEditorProvider implements IndexEditorProvider {
public final static String OAK_INDEX_ELASTIC_WRITER_DISABLE_KEY = "oak.index.elastic.writer.disable";
+ public static final String FT_OAK_12206 = "FT_OAK-12206";
+ /**
+ * When {@code true}, the subtree deleteByQuery in {@code deleteDocuments} is skipped,
+ * reverting to the pre-fix behaviour of deleting only the exact-path document.
+ * The fix is active by default; set to {@code true} via the feature toggle to disable it.
+ */
+ public static final AtomicBoolean FT_OAK_12206_DISABLE = new AtomicBoolean(false);
+
private final boolean OAK_INDEX_ELASTIC_WRITER_DISABLE = Boolean.getBoolean(OAK_INDEX_ELASTIC_WRITER_DISABLE_KEY);
public ElasticIndexEditorProvider(@NotNull ElasticIndexTracker indexTracker,
diff --git a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexWriter.java b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexWriter.java
index 775e63896d..893dd792f7 100644
--- a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexWriter.java
+++ b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexWriter.java
@@ -41,6 +41,7 @@ import org.apache.jackrabbit.oak.plugins.index.elastic.query.inference.Inference
import org.apache.jackrabbit.oak.plugins.index.elastic.query.inference.InferenceIndexConfig;
import org.apache.jackrabbit.oak.plugins.index.elastic.util.ElasticIndexUtils;
import org.apache.jackrabbit.oak.plugins.index.importer.AsyncLaneSwitcher;
+import org.apache.jackrabbit.oak.plugins.index.search.FieldNames;
import org.apache.jackrabbit.oak.plugins.index.search.spi.editor.FulltextIndexWriter;
import org.apache.jackrabbit.oak.spi.commit.CommitInfo;
import org.apache.jackrabbit.oak.spi.state.NodeBuilder;
@@ -167,6 +168,23 @@ class ElasticIndexWriter implements FulltextIndexWriter<ElasticDocument> {
@Override
public void deleteDocuments(String path) throws IOException {
retryPolicy.withRetries(() -> bulkProcessorHandler.delete(indexName, ElasticIndexUtils.idFromPath(path)));
+ if (!ElasticIndexEditorProvider.FT_OAK_12206_DISABLE.get()) {
+ // Delete all descendants: mirrors Lucene's PrefixQuery on the path term.
+ // The :ancestors field is indexed with path_hierarchy, so a term query on `path`
+ // matches every document whose ancestor chain includes that path.
+ // The ES Bulk API does not support delete by query, so we need to issue a separate request.
+ // This is not ideal but should be ok since deletes are expected to be less frequent than updates.
+ // The alternative would be to get the list of affected documents and issue a bulk delete by id,
+ // but that would be more complex and potentially more expensive (if there are many descendants).
+ retryPolicy.withRetries(() -> {
+ var response = elasticConnection.getClient().deleteByQuery(
+ d -> d.index(indexName).query(q -> q.term(t -> t.field(FieldNames.ANCESTORS).value(path))));
+ response.failures().forEach(f -> LOG.warn("Failed to delete descendants of {}: shard {} reason {}", path, f.id(), f.cause()));
+ if (response.deleted() != null && response.deleted() > 0) {
+ LOG.info("Deleted {} descendants of {} in {} ms", response.deleted(), path, response.took());
+ }
+ });
+ }
}
@Override
diff --git a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticContentTest.java b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticContentTest.java
index 7c571f43c0..f76dd73f9d 100644
--- a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticContentTest.java
+++ b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticContentTest.java
@@ -372,6 +372,34 @@ public class ElasticContentTest extends ElasticAbstractQueryTest {
});
}
+ @Test
+ public void subtreeDeletion() throws Exception {
+ IndexDefinitionBuilder builder = createIndex("a").noAsync();
+ builder.includedPaths("/content");
+ builder.indexRule("nt:base").property("a").propertyIndex();
+ Tree index = setIndex(UUID.randomUUID().toString(), builder);
+ root.commit();
+
+ Tree content = root.getTree("/").addChild("content");
+ Tree parent = content.addChild("parent");
+ parent.setProperty("a", "foo");
+ parent.addChild("child1").setProperty("a", "foo");
+ parent.addChild("child2").setProperty("a", "foo");
+ parent.addChild("child3").setProperty("a", "foo");
+ // this node is added to check that only the subtree of the removed node gets deleted from the index
+ Tree otherParent = content.addChild("otherParent");
+ otherParent.setProperty("a", "foo");
+ otherParent.addChild("child4").setProperty("a", "foo");
+ root.commit();
+
+ assertEventually(() -> assertThat(countDocuments(index), equalTo(6L)));
+
+ content.getChild("parent").remove();
+ root.commit();
+
+ assertEventually(() -> assertThat(countDocuments(index), equalTo(2L)));
+ }
+
@Test
public void propertyRemoval() throws Exception {
IndexDefinitionBuilder builder = createIndex("a", "b", "c").noAsync();
diff --git a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexWriterTest.java b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexWriterTest.java
index 97f94a147f..e644f2a456 100644
--- a/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexWriterTest.java
+++ b/oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexWriterTest.java
@@ -16,6 +16,10 @@
*/
package org.apache.jackrabbit.oak.plugins.index.elastic.index;
+import co.elastic.clients.elasticsearch.ElasticsearchClient;
+import co.elastic.clients.elasticsearch.core.DeleteByQueryRequest;
+import co.elastic.clients.elasticsearch.core.DeleteByQueryResponse;
+import co.elastic.clients.util.ObjectBuilder;
import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticConnection;
import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticIndexDefinition;
import org.apache.jackrabbit.oak.plugins.index.elastic.ElasticIndexTracker;
@@ -25,17 +29,22 @@ import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.mockito.ArgumentCaptor;
+import org.mockito.ArgumentMatchers;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
import java.io.IOException;
+import java.time.LocalDate;
import java.util.Arrays;
+import java.util.Collections;
+import java.util.function.Function;
import static org.apache.jackrabbit.oak.plugins.index.elastic.ElasticTestUtils.randomString;
import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.number.OrderingComparison.lessThan;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.eq;
@@ -52,6 +61,9 @@ public class ElasticIndexWriterTest {
@Mock
private ElasticConnection elasticConnectionMock;
+ @Mock
+ private ElasticsearchClient elasticsearchClientMock;
+
@Mock
private ElasticIndexDefinition indexDefinitionMock;
@@ -65,10 +77,14 @@ public class ElasticIndexWriterTest {
private AutoCloseable closeable;
@Before
- public void setUp() {
+ public void setUp() throws IOException {
closeable = MockitoAnnotations.openMocks(this);
when(indexDefinitionMock.getIndexAlias()).thenReturn("test-index");
when(indexDefinitionMock.getIndexName()).thenReturn("test-index-name");
+ when(elasticConnectionMock.getClient()).thenReturn(elasticsearchClientMock);
+ when(elasticConnectionMock.getClient()
+ .deleteByQuery(ArgumentMatchers.<Function<DeleteByQueryRequest.Builder, ObjectBuilder<DeleteByQueryRequest>>>any()))
+ .thenReturn(DeleteByQueryResponse.of(d -> d.deleted(1L).failures(Collections.emptyList())));
// In this test we are explicitly disabling inference as bulkprocessor
// is called with update document if inference is enabled.
InferenceConfig.reInitialize(new MemoryNodeStore(), "/oak:index/:inferenceConfig", false);
@@ -99,6 +115,9 @@ public class ElasticIndexWriterTest {
ArgumentCaptor<String> idCaptor = ArgumentCaptor.forClass(String.class);
verify(bulkProcessorHandlerMock).delete(eq(indexAlias), idCaptor.capture());
+ verify(elasticsearchClientMock).deleteByQuery(
+ ArgumentMatchers.<Function<DeleteByQueryRequest.Builder, ObjectBuilder<DeleteByQueryRequest>>>any()
+ );
String id = idCaptor.getValue();
assertEquals("/bar", id);
@@ -113,6 +132,9 @@ public class ElasticIndexWriterTest {
verify(bulkProcessorHandlerMock, times(2)).index(eq(indexAlias), anyString(), any(ElasticDocument.class));
verify(bulkProcessorHandlerMock, times(2)).delete(eq(indexAlias), anyString());
+ verify(elasticsearchClientMock, times(2)).deleteByQuery(
+ ArgumentMatchers.<Function<DeleteByQueryRequest.Builder, ObjectBuilder<DeleteByQueryRequest>>>any()
+ );
}
@Test
@@ -157,4 +179,12 @@ public class ElasticIndexWriterTest {
"hello world", 2)));
}
+ @Test
+ public void ft_oak_12206_toggleShouldBeRemoved() {
+ // Time-bombed: if this test fails, the feature toggle FT_OAK-12206 and its guard in
+ // ElasticIndexWriter#deleteDocuments should be removed — the fix has been in production long enough.
+ assertTrue("Feature toggle " + ElasticIndexEditorProvider.FT_OAK_12206 + " is overdue for removal",
+ LocalDate.now().isBefore(LocalDate.of(2027, 5, 6)));
+ }
+
}