This is an automated email from the ASF dual-hosted git repository.
fortino 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 97adb4653b OAK-12071: fix property removal for externally modifiable
indexes (#2700)
97adb4653b is described below
commit 97adb4653bc5cf9c0bf967bb5276ffe7d35bee9b
Author: Fabrizio Fortino <[email protected]>
AuthorDate: Tue Jan 27 20:07:30 2026 +0100
OAK-12071: fix property removal for externally modifiable indexes (#2700)
* OAK-12071: fix property removal for externally modifiable indexes
* OAK-12071: fix flaky test increasing eventual assertion time
* OAK-12071: test improvements
* OAK-12071: ensure index name uniqueness in tests
* OAK-12071: fix case where properties are not correctly removed
* OAK-12071: minor code improvement
---
.../elastic/index/ElasticBulkProcessorHandler.java | 2 +-
.../index/elastic/index/ElasticDocumentMaker.java | 22 ++++----
.../plugins/index/elastic/ElasticContentTest.java | 37 +++++++++++++
.../search/spi/editor/FulltextDocumentMaker.java | 63 ++++++++++++++--------
.../oak/plugins/index/IndexQueryCommonTest.java | 26 ++++-----
5 files changed, 98 insertions(+), 52 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 0e64503003..2498c1409f 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 class ElasticBulkProcessorHandler {
@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()) {
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 eee77bcd2c..bc22980771 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 @@ public class ElasticDocumentMaker extends
FulltextDocumentMaker<ElasticDocument>
|| 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 @@ public class ElasticDocumentMaker extends
FulltextDocumentMaker<ElasticDocument>
}
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 4aa959f5c8..7c571f43c0 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,41 @@ public class ElasticContentTest extends
ElasticAbstractQueryTest {
});
}
+ @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));
+ });
+ }
+
}
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 a1ab08536d..6965403ea0 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 javax.jcr.PropertyType;
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 @@ public abstract class FulltextDocumentMaker<D> implements
DocumentMaker<D> {
}
}
+ /**
+ * 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 abstract class FulltextDocumentMaker<D> implements
DocumentMaker<D> {
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 abstract class FulltextDocumentMaker<D> implements
DocumentMaker<D> {
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 @@ public abstract class FulltextDocumentMaker<D> implements
DocumentMaker<D> {
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 @@ public abstract class FulltextDocumentMaker<D> implements
DocumentMaker<D> {
}
}
- 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.
*
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 0063dd8c8c..7d48f41df3 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.text.ParseException;
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 abstract class IndexQueryCommonTest extends
AbstractQueryTest {
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 abstract class IndexQueryCommonTest extends
AbstractQueryTest {
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 abstract class IndexQueryCommonTest extends
AbstractQueryTest {
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 abstract class IndexQueryCommonTest extends
AbstractQueryTest {
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 abstract class IndexQueryCommonTest extends
AbstractQueryTest {
setTraversalEnabled(true);
}
-
@Test
public void fullTextQueryTestAllowLeadingWildcards() throws Exception {
@@ -562,7 +557,6 @@ public abstract class IndexQueryCommonTest extends
AbstractQueryTest {
assertEventually(() -> assertQuery(query, XPATH, List.of("/test/e")));
}
-
@Test
public void fullTextQueryTestAllowLeadingWildcards2() throws Exception {
@@ -701,7 +695,6 @@ public abstract class IndexQueryCommonTest extends
AbstractQueryTest {
assertEventually(() -> assertQuery(query2, XPATH,
List.of("/test/test1")));
}
-
@Test
public void testEqualityQuery_native() throws Exception {
@@ -832,7 +825,8 @@ public abstract class IndexQueryCommonTest extends
AbstractQueryTest {
};
}
- 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);
}
}
