OAK-12071: fix property removal for externally modifiable indexes

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<OperationCont
 
         @Override
         public void afterBulk(long executionId, BulkRequest request, List<OperationContext> 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()) {

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);
+    }
 }

oak-search-elastic/src/test/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticContentTest.java

@@ -372,4 +372,41 @@ public void fulltextFieldValuesCleanup() throws Exception {
         });
     }
 
+    @Test
+    public void propertyRemoval() throws Exception {
+        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);
+        root.commit();
+
+        Tree content = root.getTree("/").addChild("content");
+        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));
+        });
+    }
+
 }

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<PropertyState> propertiesModified) throws IOException {
+    public D makeDocument(NodeState state, boolean isUpdate, @NotNull List<PropertyState> 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<String, PropertyState> 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
@@ -176,7 +193,12 @@ public D makeDocument(NodeState state, boolean isUpdate, List<PropertyState> pro
                 dirty |= addTypedOrderedFields(document, property, pname, pd);
             }
 
-            dirty |= indexProperty(path, document, state, property, pname, pd);
+            var indexed = indexProperty(path, document, state, property, pname, pd);
+            if (indexed) {
+                dirty = true;
+                // property was indexed, so remove from the removed list
+                propertiesToRemove.remove(pname);
+            }
 
             facet |= pd.facet;
         }
@@ -189,9 +211,8 @@ public D makeDocument(NodeState state, boolean isUpdate, List<PropertyState> 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 +351,21 @@ private boolean indexProperty(String path,
         return dirty;
     }
 
+    private boolean removeProperties(D doc, Map<String, PropertyState> 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 +539,6 @@ private PropertyState calculateValue(String path, NodeState state, String[] func
         }
     }
 
-    private boolean indexIfSinglePropertyRemoved(List<PropertyState> 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.
      *

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;
 
@@ -75,11 +76,10 @@ public void closeLogger(){
         logCustomizer.finished();
     }
 
-
     @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");
@@ -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<String> result = executeQuery(query, Query.JCR_SQL2).iterator();
-            List<String> paths = new ArrayList<>();
-            result.forEachRemaining(paths::add);
+            List<String> 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<String> result = executeQuery(query, Query.JCR_SQL2).iterator();
-            List<String> paths = new ArrayList<>();
-            result.forEachRemaining(paths::add);
+            List<String> 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 + ", 'bar')]", "xpath", List.of()));
     }
 
     @SuppressWarnings("unused")
@@ -544,7 +540,6 @@ public void oak3371() throws Exception {
         setTraversalEnabled(true);
     }
 
-
     @Test
     public void fullTextQueryTestAllowLeadingWildcards() throws Exception {
 
@@ -562,7 +557,6 @@ public void fullTextQueryTestAllowLeadingWildcards() throws Exception {
         assertEventually(() -> assertQuery(query, XPATH, List.of("/test/e")));
     }
 
-
     @Test
     public void fullTextQueryTestAllowLeadingWildcards2() throws Exception {
 
@@ -701,7 +695,6 @@ public void testEqualityInequalityCombined_native() throws Exception {
         assertEventually(() -> assertQuery(query2, XPATH, List.of("/test/test1")));
     }
 
-
     @Test
     public void testEqualityQuery_native() throws Exception {
 
@@ -832,7 +825,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);
     }
 }