From 2c06238115dae88b77ae84f1283cac6e05416643 Mon Sep 17 00:00:00 2001 From: Fabrizio Fortino Date: Mon, 26 Jan 2026 14:15:04 +0100 Subject: [PATCH 1/6] OAK-12071: fix property removal for externally modifiable indexes --- .../elastic/index/ElasticDocumentMaker.java | 22 +++---- .../index/elastic/ElasticContentTest.java | 32 ++++++++++ .../spi/editor/FulltextDocumentMaker.java | 59 ++++++++++++------- 3 files changed, 79 insertions(+), 34 deletions(-) diff --git a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticDocumentMaker.java b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticDocumentMaker.java index eee77bcd2c6..bc22980771b 100644 --- a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticDocumentMaker.java +++ b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticDocumentMaker.java @@ -165,19 +165,6 @@ protected boolean isFulltextValuePersistedAtNode(PropertyDefinition pd) { || pd.getType() == Type.DOUBLE.tag(); } - /** - * ElasticDocument can be updated. If a property gets deleted from the node, we need to add it to the list of properties to remove. - * This is needed to remove the property from the index. See @{link ElasticBulkProcessorHandler#updateDocument} for more details. - */ - @Override - protected boolean addTypedFields(ElasticDocument doc, PropertyState property, String pname, PropertyDefinition pd) { - boolean added = super.addTypedFields(doc, property, pname, pd); - if (!added) { - doc.removeProperty(pname); - } - return added; - } - @Override protected void indexTypedProperty(ElasticDocument doc, PropertyState property, String propertyName, PropertyDefinition pd, int i) { // Get the Type tag from the defined index definition here - and not from the actual persisted property state - this way in case @@ -298,4 +285,13 @@ protected boolean indexDynamicBoost(ElasticDocument doc, String parent, String n } return false; } + + /** + * ElasticDocument can be updated. If a property gets deleted from the node, we need to add it to the list of properties to remove. + * This is needed to remove the property from the index. See @{link ElasticBulkProcessorHandler#updateDocument} for more details. + */ + @Override + protected void removeProperty(ElasticDocument doc, String propertyName) { + doc.removeProperty(propertyName); + } } \ No newline at end of file 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 4aa959f5c88..bd5cb974308 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,4 +372,36 @@ public void fulltextFieldValuesCleanup() throws Exception { }); } + @Test + public void propertyRemoval() throws Exception { + IndexDefinitionBuilder builder = createIndex("a", "b").noAsync(); + builder.includedPaths("/content"); + builder.indexRule("nt:base").property("a").propertyIndex(); + builder.indexRule("nt:base").property("b").propertyIndex(); + // partial updates only happen when the index is externally modifiable, aka inference config is enabled + builder.getBuilderTree().addChild(ElasticIndexDefinition.INFERENCE_CONFIG); + Tree index = setIndex(UUID.randomUUID().toString(), builder); + root.commit(); + + Tree content = root.getTree("/").addChild("content"); + Tree node = content.addChild("node"); + node.setProperty("a", "foo"); + node.setProperty("b", "foo"); + root.commit(); + assertEventually(() -> { + ObjectNode doc = getDocument(index, "/content/node"); + assertThat(doc.get(ElasticIndexUtils.fieldName("a")).asText(), equalTo("foo")); + assertThat(doc.get(ElasticIndexUtils.fieldName("b")).asText(), equalTo("foo")); + }); + + node.removeProperty(ElasticIndexUtils.fieldName("a")); + node.setProperty("b", "bar"); + root.commit(); + assertEventually(() -> { + ObjectNode doc = getDocument(index, "/content/node"); + assertThat(doc.get(ElasticIndexUtils.fieldName("a")), equalTo(null)); + assertThat(doc.get(ElasticIndexUtils.fieldName("b")).asText(), equalTo("bar")); + }); + } + } diff --git a/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/editor/FulltextDocumentMaker.java b/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/editor/FulltextDocumentMaker.java index a1ab08536de..52a627c52d4 100644 --- a/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/editor/FulltextDocumentMaker.java +++ b/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/editor/FulltextDocumentMaker.java @@ -41,10 +41,12 @@ import java.io.IOException; import java.util.Arrays; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicInteger; import java.util.regex.Pattern; +import java.util.stream.Collectors; import static java.util.Objects.requireNonNull; @@ -143,18 +145,33 @@ protected void logLargeStringProperties(String propertyName, String value) { } } + /** + * Remove properties from the document. Default implementation is a no-op since the entire document is rebuilt + * on update. For implementations that support partial updates, this method should be overridden. + * @param doc the document + * @param propertyName the property name to be removed + */ + protected void removeProperty(D doc, String propertyName) { + // Default no-op + } + @Nullable public D makeDocument(NodeState state) throws IOException { return makeDocument(state, false, List.of()); } @Nullable - public D makeDocument(NodeState state, boolean isUpdate, List propertiesModified) throws IOException { + public D makeDocument(NodeState state, boolean isUpdate, @NotNull List propertiesModified) throws IOException { boolean facet = false; D document = initDoc(); boolean dirty = false; + // we make a copy of the modified properties names. These will be removed while iterating over all properties. + // The remaining properties are the ones that were removed. + Map propertiesToRemove = propertiesModified.stream() + .collect(Collectors.toMap(PropertyState::getName, ps -> ps)); + String nodeName = PathUtils.getName(path); //We 'intentionally' are indexing node names only on root state as we don't support indexing relative or //regex for node name indexing @@ -162,6 +179,9 @@ public D makeDocument(NodeState state, boolean isUpdate, List pro for (PropertyState property : IterableUtils.chainedIterable(state.getProperties(), List.of(nodeNamePS))) { String pname = property.getName(); + // remove from properties to remove list + propertiesToRemove.remove(pname); + if (!isVisible(pname) && !FieldNames.NODE_NAME.equals(pname)) { continue; } @@ -189,9 +209,8 @@ public D makeDocument(NodeState state, boolean isUpdate, List pro dirty |= indexNotNullCheckEnabledProps(path, document, state); dirty |= augmentCustomFields(path, document, state); - // Check if a node having a single property was modified/deleted - if (!dirty) { - dirty = indexIfSinglePropertyRemoved(propertiesModified); + if (!propertiesToRemove.isEmpty()) { + dirty |= removeProperties(document, propertiesToRemove); } if (isUpdate && !dirty) { @@ -330,6 +349,21 @@ private boolean indexProperty(String path, return dirty; } + private boolean removeProperties(D doc, Map properties) { + boolean dirty = false; + for (PropertyState ps : properties.values()) { + PropertyDefinition pd = indexingRule.getConfig(ps.getName()); + if (pd != null + && pd.index + && (pd.includePropertyType(ps.getType().tag()) + || indexingRule.includePropertyType(ps.getType().tag()))) { + removeProperty(doc, ps.getName()); + dirty = true; + } + } + return dirty; + } + /** * In elastic we don't add analyzed data in :fulltext if index has both analyzed * and nodescope property. Instead we fire a multiMatch with cross_fields. @@ -503,23 +537,6 @@ private PropertyState calculateValue(String path, NodeState state, String[] func } } - private boolean indexIfSinglePropertyRemoved(List propertiesModified) { - boolean dirty = false; - // Performance critical code: using indexed traversal to avoid creating an iterator instance. - for (int i = 0; i < propertiesModified.size(); i++) { - PropertyState ps = propertiesModified.get(i); - PropertyDefinition pd = indexingRule.getConfig(ps.getName()); - if (pd != null - && pd.index - && (pd.includePropertyType(ps.getType().tag()) - || indexingRule.includePropertyType(ps.getType().tag()))) { - dirty = true; - break; - } - } - return dirty; - } - /* * Determine if the property as defined by PropertyDefinition exists or not. * From 89abe0860e92d1b74eb139891214bc242e8d27ed Mon Sep 17 00:00:00 2001 From: fabriziofortino Date: Mon, 26 Jan 2026 16:41:01 +0100 Subject: [PATCH 2/6] OAK-12071: fix flaky test increasing eventual assertion time --- .../index/ElasticBulkProcessorHandler.java | 2 +- .../plugins/index/IndexQueryCommonTest.java | 19 ++++++++----------- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticBulkProcessorHandler.java b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticBulkProcessorHandler.java index 0e645030039..2498c1409f0 100644 --- a/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticBulkProcessorHandler.java +++ b/oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticBulkProcessorHandler.java @@ -449,7 +449,7 @@ public void beforeBulk(long executionId, BulkRequest request, List contexts, BulkResponse response) { - // Bullk request has been processed successfully. Some operations may have failed, but the request itself was successful. + // Bulk request has been processed successfully. Some operations may have failed, but the request itself was successful. try { LOG.debug("Bulk with id {} processed in {} ms", executionId, response.took()); if (LOG.isTraceEnabled()) { diff --git a/oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexQueryCommonTest.java b/oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexQueryCommonTest.java index 0063dd8c8c6..03dce935ed2 100644 --- a/oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexQueryCommonTest.java +++ b/oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexQueryCommonTest.java @@ -142,9 +142,7 @@ public void testValueRegex() throws Exception { final String query = "select [jcr:path] from [nt:base] where isdescendantnode('/test') and contains(*, 'hello')"; assertEventually(() -> { - Iterator result = executeQuery(query, Query.JCR_SQL2).iterator(); - List paths = new ArrayList<>(); - result.forEachRemaining(paths::add); + List paths = new ArrayList<>(executeQuery(query, Query.JCR_SQL2)); assertEquals(2, paths.size()); assertEquals(paths.get(0), a.getPath()); assertEquals(paths.get(1), b.getPath()); @@ -155,9 +153,7 @@ public void testValueRegex() throws Exception { root.commit(); assertEventually(() -> { - Iterator result = executeQuery(query, Query.JCR_SQL2).iterator(); - List paths = new ArrayList<>(); - result.forEachRemaining(paths::add); + List paths = new ArrayList<>(executeQuery(query, Query.JCR_SQL2)); assertEquals(1, paths.size()); assertEquals(paths.get(0), b.getPath()); }); @@ -485,15 +481,15 @@ public void testMultiValuedPropUpdate() throws Exception { test.getChild(child).setProperty(mulValuedProp, List.of(), STRINGS); root.commit(); - assertEventually(() -> assertQuery("/jcr:root//*[jcr:contains(@" + mulValuedProp + ", 'foo')]", "xpath", new ArrayList<>())); + assertEventually(() -> assertQuery("/jcr:root//*[jcr:contains(@" + mulValuedProp + ", 'foo')]", "xpath", List.of())); test.getChild(child).setProperty(mulValuedProp, List.of("bar"), STRINGS); root.commit(); - assertEventually(() -> assertQuery("/jcr:root//*[jcr:contains(@" + mulValuedProp + ", 'foo')]", "xpath", new ArrayList<>())); + assertEventually(() -> assertQuery("/jcr:root//*[jcr:contains(@" + mulValuedProp + ", 'foo')]", "xpath", List.of())); test.getChild(child).removeProperty(mulValuedProp); root.commit(); - assertEventually(() -> assertQuery("/jcr:root//*[jcr:contains(@" + mulValuedProp + ", 'foo')]", "xpath", new ArrayList<>())); + assertEventually(() -> assertQuery("/jcr:root//*[jcr:contains(@" + mulValuedProp + ", 'foo')]", "xpath", List.of())); } @SuppressWarnings("unused") @@ -832,7 +828,8 @@ protected Runnable getAssertionForExplain(String query, String language, String }; } - protected static void assertEventually(Runnable r) { - TestUtil.assertEventually(r, 3000 * 3); + protected void assertEventually(Runnable r) { + TestUtil.assertEventually(r, + ((repositoryOptionsUtil.isAsync() ? repositoryOptionsUtil.defaultAsyncIndexingTimeInSeconds * 1000 : 0) + 3000) * 5); } } From dd581829e5f8735fd85fcb07843465d8c27a0c0d Mon Sep 17 00:00:00 2001 From: fabriziofortino Date: Tue, 27 Jan 2026 10:05:01 +0100 Subject: [PATCH 3/6] OAK-12071: test improvements --- .../jackrabbit/oak/plugins/index/IndexQueryCommonTest.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexQueryCommonTest.java b/oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexQueryCommonTest.java index 03dce935ed2..a4b6bfd1269 100644 --- a/oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexQueryCommonTest.java +++ b/oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexQueryCommonTest.java @@ -75,7 +75,6 @@ public void closeLogger(){ logCustomizer.finished(); } - @Override protected void createTestIndexNode() throws Exception { Tree index = root.getTree("/"); @@ -489,7 +488,7 @@ public void testMultiValuedPropUpdate() throws Exception { test.getChild(child).removeProperty(mulValuedProp); root.commit(); - assertEventually(() -> assertQuery("/jcr:root//*[jcr:contains(@" + mulValuedProp + ", 'foo')]", "xpath", List.of())); + assertEventually(() -> assertQuery("/jcr:root//*[jcr:contains(@" + mulValuedProp + ", 'bar')]", "xpath", List.of())); } @SuppressWarnings("unused") @@ -540,7 +539,6 @@ public void oak3371() throws Exception { setTraversalEnabled(true); } - @Test public void fullTextQueryTestAllowLeadingWildcards() throws Exception { @@ -558,7 +556,6 @@ public void fullTextQueryTestAllowLeadingWildcards() throws Exception { assertEventually(() -> assertQuery(query, XPATH, List.of("/test/e"))); } - @Test public void fullTextQueryTestAllowLeadingWildcards2() throws Exception { @@ -697,7 +694,6 @@ public void testEqualityInequalityCombined_native() throws Exception { assertEventually(() -> assertQuery(query2, XPATH, List.of("/test/test1"))); } - @Test public void testEqualityQuery_native() throws Exception { From 5a7860701100d4b3e3c4e5bb12f87e871dea1eaa Mon Sep 17 00:00:00 2001 From: fabriziofortino Date: Tue, 27 Jan 2026 12:22:19 +0100 Subject: [PATCH 4/6] OAK-12071: ensure index name uniqueness in tests --- .../jackrabbit/oak/plugins/index/IndexQueryCommonTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexQueryCommonTest.java b/oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexQueryCommonTest.java index a4b6bfd1269..7d48f41df32 100644 --- a/oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexQueryCommonTest.java +++ b/oak-search/src/test/java/org/apache/jackrabbit/oak/plugins/index/IndexQueryCommonTest.java @@ -35,6 +35,7 @@ import java.util.ArrayList; import java.util.Iterator; import java.util.List; +import java.util.UUID; import javax.jcr.query.Query; @@ -78,7 +79,7 @@ public void closeLogger(){ @Override protected void createTestIndexNode() throws Exception { Tree index = root.getTree("/"); - indexDefn = createTestIndexNode(index, indexOptions.getIndexType()); + indexDefn = createTestIndexNode(UUID.randomUUID().toString(), index, indexOptions.getIndexType()); TestUtil.useV2(indexDefn); indexDefn.setProperty(FulltextIndexConstants.EVALUATE_PATH_RESTRICTION, true); indexDefn.setProperty("tags", "x"); From b1faaba2796a034b12dd71c3dd3207124f547e7e Mon Sep 17 00:00:00 2001 From: fabriziofortino Date: Tue, 27 Jan 2026 12:55:53 +0100 Subject: [PATCH 5/6] OAK-12071: fix case where properties are not correctly removed --- .../oak/plugins/index/elastic/ElasticContentTest.java | 7 ++++++- .../index/search/spi/editor/FulltextDocumentMaker.java | 10 ++++++---- 2 files changed, 12 insertions(+), 5 deletions(-) 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 bd5cb974308..7c571f43c01 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 @@ -374,10 +374,11 @@ public void fulltextFieldValuesCleanup() throws Exception { @Test public void propertyRemoval() throws Exception { - IndexDefinitionBuilder builder = createIndex("a", "b").noAsync(); + IndexDefinitionBuilder builder = createIndex("a", "b", "c").noAsync(); builder.includedPaths("/content"); builder.indexRule("nt:base").property("a").propertyIndex(); builder.indexRule("nt:base").property("b").propertyIndex(); + builder.indexRule("nt:base").property("c").propertyIndex(); // partial updates only happen when the index is externally modifiable, aka inference config is enabled builder.getBuilderTree().addChild(ElasticIndexDefinition.INFERENCE_CONFIG); Tree index = setIndex(UUID.randomUUID().toString(), builder); @@ -387,20 +388,24 @@ public void propertyRemoval() throws Exception { Tree node = content.addChild("node"); node.setProperty("a", "foo"); node.setProperty("b", "foo"); + node.setProperty("c", List.of("foo", "bar"), Type.STRINGS); root.commit(); assertEventually(() -> { ObjectNode doc = getDocument(index, "/content/node"); assertThat(doc.get(ElasticIndexUtils.fieldName("a")).asText(), equalTo("foo")); assertThat(doc.get(ElasticIndexUtils.fieldName("b")).asText(), equalTo("foo")); + assertThat(doc.get(ElasticIndexUtils.fieldName("c")).size(), equalTo(2)); }); node.removeProperty(ElasticIndexUtils.fieldName("a")); node.setProperty("b", "bar"); + node.setProperty("c", List.of(), Type.STRINGS); root.commit(); assertEventually(() -> { ObjectNode doc = getDocument(index, "/content/node"); assertThat(doc.get(ElasticIndexUtils.fieldName("a")), equalTo(null)); assertThat(doc.get(ElasticIndexUtils.fieldName("b")).asText(), equalTo("bar")); + assertThat(doc.get(ElasticIndexUtils.fieldName("c")), equalTo(null)); }); } diff --git a/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/editor/FulltextDocumentMaker.java b/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/editor/FulltextDocumentMaker.java index 52a627c52d4..928def294f3 100644 --- a/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/editor/FulltextDocumentMaker.java +++ b/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/editor/FulltextDocumentMaker.java @@ -179,9 +179,6 @@ public D makeDocument(NodeState state, boolean isUpdate, @NotNull List Date: Tue, 27 Jan 2026 12:59:23 +0100 Subject: [PATCH 6/6] OAK-12071: minor code improvement --- .../plugins/index/search/spi/editor/FulltextDocumentMaker.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/editor/FulltextDocumentMaker.java b/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/editor/FulltextDocumentMaker.java index 928def294f3..6965403ea0f 100644 --- a/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/editor/FulltextDocumentMaker.java +++ b/oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/editor/FulltextDocumentMaker.java @@ -194,8 +194,8 @@ public D makeDocument(NodeState state, boolean isUpdate, @NotNull List